Hi William,

The changes look good. You've agreed to add a test for the case where
Person.verbose_bugnotifications=False and
Team.verbose_bugnotifications=True.

There are a couple of stylistic changes I'd like you to make, otherwise
this is ready to go once that test has been added.

On Thu, Jul 23, 2009 at 03:00:39PM -0000, William Grant wrote:
> Thanks Graham, I believe I've fixed all those issues.
> 
> === modified file 'database/sampledata/current.sql'
> --- database/sampledata/current.sql	2009-07-23 04:49:04 +0000
> +++ database/sampledata/current.sql	2009-07-23 14:49:39 +0000
> @@ -4687,7 +4687,7 @@
> [snip reversion of change]
> 
> === modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
> --- lib/lp/bugs/doc/bugnotification-sending.txt	2009-07-23 04:51:44 +0000
> +++ lib/lp/bugs/doc/bugnotification-sending.txt	2009-07-23 14:45:59 +0000
> @@ -1247,9 +1247,40 @@
>  cases where the user doesn't save bug notifications, which can make
>  subsequent notifications seem somewhat obscure.
>  
> +To demonstrate verbose notifications, we'll create a bug, and subscribe
> +some very picky users to it. Verbose Person wants verbose emails, while
> +Concise Person does not. We'll also create a team with a member that
> +wants verbose notifications.
> +
> +    >>> switch_db_to_launchpad()
> +    >>> bug = factory.makeBug(product=factory.makeProduct(title='Foo'),
> +    ...                       title='Foo is broken', description='desc')

We prefer to do wrapping of method calls thus:

    >>> bug = factory.makeBug(
    ...     product=factory.makeProduct(title='Foo'), title='Foo is broken',
    ...     description='desc')

See http://dev.launchpad.net/PythonStyleGuide for details. The way
you've done it is okay, but we've pretty much standardised on the
style above, so we should stick with that (in fact I don't know why the
other version is in the style guide any more; this one is the one that
gets used).

> +
> +    >>> vperson = factory.makePerson(displayname='Verbose Person',
> +    ...                              <email address hidden>')
> +    >>> vperson.verbose_bugnotifications = True
> +    >>> bug.subscribe(vperson, vperson)
> +    <BugSubscription...>
> +

We should be verbose in tests where possible. So,
s/vperson/verbose_person/.

> +    >>> cperson = factory.makePerson(displayname='Concise Person',
> +    ...                              <email address hidden>')
> +    >>> cperson.verbose_bugnotifications = False
> +    >>> bug.subscribe(cperson, cperson)
> +    <BugSubscription...>
> +

And s/cperson/concise_person/

> +    >>> team = factory.makeTeam(name='bugteam', displayname='Bug Team')
> +    >>> team.verbose_bugnotifications = False
> +    >>> tperson = factory.makePerson(displayname='Team Person',
> +    ...                              <email address hidden>')

And s/tperson/team_person/

> +    >>> tperson.verbose_bugnotifications = True
> +    >>> team.addMember(tperson, tperson)
> +    >>> bug.subscribe(team, tperson)
> +    <BugSubscription...>
> +
>  We'll expire all existing notifications since we're not interested in
>  them:
>  
> +    >>> switch_db_to_bugnotification()
>      >>> notifications = getUtility(
>      ...     IBugNotificationSet).getNotificationsToSend()
>      >>> len(notifications)
> @@ -1261,37 +1292,13 @@
>      ...     syncUpdate(notification)
>  
>  
> -To demonstrate verbose notifications, we'll subscribe David Allouche,
> -who receives verbose bug notifications, to bug 15.
> -
> -    >>> bug_15 = getUtility(IBugSet).get(15)
> -    >>> ddaa = getUtility(IPersonSet).getByName('ddaa')
> -    >>> ddaa.verbose_bugnotifications
> -    True
> -
> -    >>> bug_15.subscribe(ddaa, sample_person)
> -    <...>
> -
> -Notifications sent through a team membership respect the member's
> -verbose_bugnotifications, not the team's. salgado gets notifications
> -through his membership in landscape-developers. salgado decides that
> -he wants verbose notifications.
> -    
> -    >>> salgado = getUtility(IPersonSet).getByName('salgado')
> -    >>> salgado.verbose_bugnotifications
> -    True
> -
> -    >>> ls = getUtility(IPersonSet).getByName('landscape-developers')
> -    >>> ls.verbose_bugnotifications
> -    False
> -
> -If we then add a comment to the bug, ddaa and salgado will receieve
> +If we then add a comment to the bug, the subscribers will receive
>  notifications containing that comment.
>  
>      >>> comment = getUtility(IMessageSet).fromText(
> -    ...     'subject', 'a really simple comment.', sample_person,
> +    ...     'subject', 'a really simple comment.', vperson,
>      ...     datecreated=ten_minutes_ago)
> -    >>> bug_15.addCommentNotification(comment)
> +    >>> bug.addCommentNotification(comment)
>  
>      >>> notifications = getUtility(
>      ...     IBugNotificationSet).getNotificationsToSend()
> @@ -1299,10 +1306,10 @@
>      1
>  
>  If we pass this notifcation to get_email_notifications we can see that
> -ddaa and salgado will receieve a notification which contains the bug
> -description and its status in all of its targets. All other subscribers
> -will receive standard notifications that don't include the bug
> -description. To help with demonstrating this, we'll define a helper
> +Verbose Person and Team Person will receive notifications which contain
> +the bug description and the status in all of its targets. All other
> +subscribers will receive standard notifications that don't include the
> +bug description. To help with demonstrating this, we'll define a helper
>  function.
>  
>      >>> def collate_messages_by_recipient(messages):
> @@ -1320,72 +1327,67 @@
>      ...     chain(*(messages for bug_notifications, messages in
>      ...             get_email_notifications(notifications))))
>  
> -We can see that <email address hidden> doesn't receive verbose
> -notifications:
> -
> -    >>> print_notification(collated_messages['<email address hidden>'][0])
> -    To: <email address hidden>
> -    From: Sample Person <email address hidden>
> -    Subject: [Bug 15] subject
> -    X-Launchpad-Message-Rationale: Subscriber
> -    <BLANKLINE>
> -    a really simple comment.
> -    <BLANKLINE>
> -    --
> -    Nonsensical bugs are useless
> -    http://bugs.launchpad.dev/bugs/15
> -    You received this bug notification because you are a direct subscriber
> -    of the bug.
> -    <BLANKLINE>
> -    ----------------------------------------------------------------------
> -
> -Whereas ddaa does:
> -
> -    >>> print_notification(
> -    ...     collated_messages['<email address hidden>'][0])
> -    To: <email address hidden>
> -    From: Sample Person <email address hidden>
> -    Subject: [Bug 15] subject
> -    X-Launchpad-Message-Rationale: Subscriber
> -    <BLANKLINE>
> -    a really simple comment.
> -    <BLANKLINE>
> -    --
> -    Nonsensical bugs are useless
> -    http://bugs.launchpad.dev/bugs/15
> -    You received this bug notification because you are a direct subscriber
> -    of the bug.
> -    <BLANKLINE>
> -    Status in Redfish: New
> -    Status in Mozilla Thunderbird: New
> -    <BLANKLINE>
> -    Bug description:
> -    Like this one, natch.
> -    <BLANKLINE>
> -    ----------------------------------------------------------------------
> -
> -And salgado does too: 
> -
> -    >>> print_notification(
> -    ...     collated_messages['<email address hidden>'][0])
> -    To: <email address hidden>
> -    From: Sample Person <email address hidden>
> -    Subject: [Bug 15] subject
> -    X-Launchpad-Message-Rationale: Subscriber (Redfish) @landscape-developers
> -    <BLANKLINE>
> -    a really simple comment.
> -    <BLANKLINE>
> -    --
> -    Nonsensical bugs are useless
> -    http://bugs.launchpad.dev/bugs/15
> -    You received this bug notification because you are a member of Landscape
> -    Developers, which is subscribed to Redfish.
> -    <BLANKLINE>
> -    Status in Redfish: New
> -    Status in Mozilla Thunderbird: New
> -    <BLANKLINE>
> -    Bug description:
> -    Like this one, natch.
> +We can see that Concise Person doesn't receive verbose notifications:
> +
> +    >>> print_notification(collated_messages['<email address hidden>'][0])
> +    To: <email address hidden>
> +    From: Verbose Person <email address hidden>
> +    Subject: [Bug ...] subject
> +    X-Launchpad-Message-Rationale: Subscriber
> +    <BLANKLINE>
> +    a really simple comment.
> +    <BLANKLINE>
> +    --
> +    Foo is broken
> +    http://bugs.launchpad.dev/bugs/...
> +    You received this bug notification because you are a direct subscriber
> +    of the bug.
> +    <BLANKLINE>
> +    ----------------------------------------------------------------------
> +
> +Whereas Verbose Person does:
> +
> +    >>> print_notification(collated_messages['<email address hidden>'][0])
> +    To: <email address hidden>
> +    From: Verbose Person <email address hidden>
> +    Subject: [Bug ...] subject
> +    X-Launchpad-Message-Rationale: Subscriber
> +    <BLANKLINE>
> +    a really simple comment.
> +    <BLANKLINE>
> +    --
> +    Foo is broken
> +    http://bugs.launchpad.dev/bugs/...
> +    You received this bug notification because you are a direct subscriber
> +    of the bug.
> +    <BLANKLINE>
> +    Status in Foo: New
> +    <BLANKLINE>
> +    Bug description:
> +    desc
> +    <BLANKLINE>
> +    ----------------------------------------------------------------------
> +
> +And Team Person does too, even though his team doesn't want them: 
> +
> +    >>> print_notification(collated_messages['<email address hidden>'][0])
> +    To: <email address hidden>
> +    From: Verbose Person <email address hidden>
> +    Subject: [Bug ...] subject
> +    X-Launchpad-Message-Rationale: Subscriber @bugteam
> +    <BLANKLINE>
> +    a really simple comment.
> +    <BLANKLINE>
> +    --
> +    Foo is broken
> +    http://bugs.launchpad.dev/bugs/...
> +    You received this bug notification because you are a member of Bug Team,
> +    which is a direct subscriber.
> +    <BLANKLINE>
> +    Status in Foo: New
> +    <BLANKLINE>
> +    Bug description:
> +    desc
>      <BLANKLINE>
>      ----------------------------------------------------------------------
>  
> 
> === modified file 'lib/lp/bugs/scripts/bugnotification.py'
> --- lib/lp/bugs/scripts/bugnotification.py	2009-07-23 13:02:15 +0000
> +++ lib/lp/bugs/scripts/bugnotification.py	2009-07-23 14:51:10 +0000
> @@ -40,8 +40,8 @@
>      recipients = {}
>      for notification in bug_notifications:
>          for recipient in notification.recipients:
> -            for emailperson in emailPeople(recipient.person):
> -                recipients[emailperson] = recipient
> +            for email_person in emailPeople(recipient.person):
> +                recipients[email_person] = recipient
>  
>      for notification in bug_notifications:
>          assert notification.bug == bug, bug.id
> @@ -104,18 +104,17 @@
>              config.malone.comment_syncing_team)
>          # Only members of the comment syncing team should get comment
>          # notifications related to bug watches or initial comment imports.
> -        # XXX wgrant 2009-07-23: Do we want to take the emailperson's or
> -        # recipient's membership?
>          if (is_initial_import_notification or
>              (bug_message is not None and bug_message.bugwatch is not None)):
>              recipients = dict(
> -                (emailperson, recipient)
> -                for emailperson, recipient in recipients.items()
> +                (email_person, recipient)
> +                for email_person, recipient in recipients.items()
>                  if recipient.person.inTeam(comment_syncing_team))
>      bug_notification_builder = BugNotificationBuilder(bug)
> -    for emailperson, recipient in sorted(recipients.items(),
> -                                    key=lambda t: t[0].preferredemail.email):
> -        address = str(emailperson.preferredemail.email)
> +    sorted_recipients = sorted(
> +        recipients.items(), key=lambda t: t[0].preferredemail.email)
> +    for email_person, recipient in sorted_recipients:
> +        address = str(email_person.preferredemail.email)
>          reason = recipient.reason_body
>          rationale = recipient.reason_header
>  
> @@ -128,7 +127,7 @@
>          # If the person we're sending to receives verbose notifications
>          # we include the description and status of the bug in the email
>          # footer.
> -        if emailperson.verbose_bugnotifications:
> +        if email_person.verbose_bugnotifications:
>              email_template = 'bug-notification-verbose.txt'
>              body_data['bug_description'] = bug.description
>