Code review comment for lp:~wgrant/launchpad/team-verbose-bugnotifications-bug-253788

Hi William,

Thanks for submitting this branch. I think you're the first community
contributer to Launchpad - which I honestly kinda expected anyway.

This is a really good branch. As we discussed on IRC, you're going to
drop the sampledata changes and rewrite the tests to use the factory.
I'm going to mark the branch needs-fixing for the sake of taking a look
at the new tests.

The code is fine. There are a couple of stylistic changes I want you to
make but nothing major. This is a really nice patch for an annoying
problem (which, incidentally, I've just noticed is starting to crop up
on the launchpad-dev list too, so this might be worth cherrypicking;
we'll see what Kiko says).

 review needs-fixing

> === modified file 'database/sampledata/current.sql'

We've agreed on IRC that we can drop this change for using the object
factory and all that new hotness.

> === modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
> --- lib/lp/bugs/doc/bugnotification-sending.txt 2009-06-12 16:36:02 +0000
> +++ lib/lp/bugs/doc/bugnotification-sending.txt 2009-07-23 04:51:44 +0000
> @@ -1272,8 +1272,21 @@
> >>> bug_15.subscribe(ddaa, sample_person)
> <...>
>
> -If we then add a comment to the bug, ddaa will receieve a notification
> -containing that comment.
> +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
> +notifications containing that comment.
>
> >>> comment = getUtility(IMessageSet).fromText(
> ... 'subject', 'a really simple comment.', sample_person,
> @@ -1286,10 +1299,11 @@
> 1
>
> If we pass this notifcation to get_email_notifications we can see that
> -ddaa 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 function.
> +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
> +function.
>
> >>> def collate_messages_by_recipient(messages):
> ... messages_by_recipient = {}
> @@ -1350,6 +1364,31 @@
> <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.
> + <BLANKLINE>
> + ----------------------------------------------------------------------
> +
> == Notification Recipients ==
>
> Bug notifications are sent to direct subscribers of a bug as well as to
[SNIP]
> === modified file 'lib/lp/bugs/scripts/bugnotification.py'
> --- lib/lp/bugs/scripts/bugnotification.py 2009-06-25 00:40:31 +0000
> +++ lib/lp/bugs/scripts/bugnotification.py 2009-07-23 13:02:15 +0000
> @@ -11,8 +11,7 @@
>
> from canonical.config import config
> from canonical.database.sqlbase import rollback, begin
> -from canonical.launchpad.helpers import (
> - get_contact_email_addresses, get_email_template)
> +from canonical.launchpad.helpers import emailPeople, get_email_template
> from lp.bugs.interfaces.bugmessage import IBugMessageSet
> from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
> from lp.registry.interfaces.person import IPersonSet
> @@ -41,8 +40,8 @@
> recipients = {}
> for notification in bug_notifications:
> for recipient in notification.recipients:
> - for address in get_contact_email_addresses(recipient.person):
> - recipients[address] = recipient
> + for emailperson in emailPeople(recipient.person):
> + recipients[emailperson] = recipient

Nitpick: s/emailperson/email_person/ here for the sake of PEP8.

>
> for notification in bug_notifications:
> assert notification.bug == bug, bug.id
> @@ -105,14 +104,18 @@
> 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?

We should stick with the recipient's membership. You can take this XXX
out; we only tend to use XXXs where there's a filed bug, and you've
already made the right choice here.

> if (is_initial_import_notification or
> (bug_message is not None and bug_message.bugwatch is not None)):
> recipients = dict(
> - (address, recipient)
> - for address, recipient in recipients.items()
> + (emailperson, recipient)
> + for emailperson, recipient in recipients.items()
> if recipient.person.inTeam(comment_syncing_team))

s/emailperson/email_person here, too.

> bug_notification_builder = BugNotificationBuilder(bug)
> - for address, recipient in recipients.items():
> + for emailperson, recipient in sorted(recipients.items(),
> + key=lambda t: t[0].preferredemail.email):

This would be a bit easier to read as:

    sorted_recipients = sorted(
        recipients.items(), key=lambda t: t[0].preferredemail.email)
    for email_person, recipient in sorted_recipients:
        ...

(also, s/emailperson/email_person again ;))

> + address = str(emailperson.preferredemail.email)
> reason = recipient.reason_body
> rationale = recipient.reason_header
>
> @@ -125,8 +128,7 @@
> # If the person we're sending to receives verbose notifications
> # we include the description and status of the bug in the email
> # footer.
> - person = recipient.person
> - if person.verbose_bugnotifications:
> + if emailperson.verbose_bugnotifications:

s/emailperson/email_person here, too.

> email_template = 'bug-notification-verbose.txt'
> body_data['bug_description'] = bug.description
>
>

--
Graham Binns | PGP Key: EC66FA7D