Merge lp:~wgrant/launchpad/team-verbose-bugnotifications-bug-253788 into lp:launchpad/db-devel

This branch fixes bug #253788, by making bugmail to a team member use the member's verbose_bugnotifications, not the team's.

bugnotification-sending.txt relied on dict ordering in construct_email_notifications, and I needed to rekey that dict with Persons. As they don't hash reliably, the order changed between test runs, so I decided to sort the returned messages by emailaddress. The majority of the changes in bugnotification-sending.txt are reorderings based on this; only the hunks mentioning salgado are new tests.

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 1...

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')
+
+ >>> vperson = factory.makePerson(displayname='Verbose Person',
+ ... <email address hidden>')
+ >>> vperson.verbose_bugnotifications = True
+ >>> bug.subscribe(vperson, vperson)
+ <BugSubscription...>
+
+ >>> cperson = factory.makePerson(displayname='Concise Person',
+ ... <email address hidden>')
+ >>> cperson.verbose_bugnotifications = False
+ >>> bug.subscribe(cperson, cperson)
+ <BugSubscription...>
+
+ >>> team = factory.makeTeam(name='bugteam', displayname='Bug Team')
+ >>> team.verbose_bugnotifications = False
+ >>> tperson = factory.makePerson(displayname='Team Person',
+ ... <email address hidden>')
+ >>> 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 ...

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 = getUti...

I've added the other case and renamed the variables.

=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt 2009-07-23 14:49:07 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt 2009-07-23 15:40:14 +0000
@@ -1249,32 +1249,53 @@

 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.
+Concise Person does not. We'll also create teams and give them members
+with different verbose_bugnotifications settings.

     >>> switch_db_to_launchpad()
- >>> bug = factory.makeBug(product=factory.makeProduct(title='Foo'),
- ... title='Foo is broken', description='desc')
-
- >>> vperson = factory.makePerson(displayname='Verbose Person',
- ... <email address hidden>')
- >>> vperson.verbose_bugnotifications = True
- >>> bug.subscribe(vperson, vperson)
- <BugSubscription...>
-
- >>> cperson = factory.makePerson(displayname='Concise Person',
- ... <email address hidden>')
- >>> cperson.verbose_bugnotifications = False
- >>> bug.subscribe(cperson, cperson)
- <BugSubscription...>
-
- >>> team = factory.makeTeam(name='bugteam', displayname='Bug Team')
- >>> team.verbose_bugnotifications = False
- >>> tperson = factory.makePerson(displayname='Team Person',
- ... <email address hidden>')
- >>> tperson.verbose_bugnotifications = True
- >>> team.addMember(tperson, tperson)
- >>> bug.subscribe(team, tperson)
+ >>> bug = factory.makeBug(
+ ... product=factory.makeProduct(title='Foo'), title='Foo is broken',
+ ... description='desc')
+
+ >>> verbose_person = factory.makePerson(
+ ... displayname='Verbose Person', <email address hidden>')
+ >>> verbose_person.verbose_bugnotifications = True
+ >>> bug.subscribe(verbose_person, verbose_person)
+ <BugSubscription...>
+
+ >>> concise_person = factory.makePerson(
+ ... displayname='Concise Person', <email address hidden>')
+ >>> concise_person.verbose_bugnotifications = False
+ >>> bug.subscribe(concise_person, concise_person)
+ <BugSubscription...>
+
+
+Concise Team doesn't want verbose notifications, while Concise Team
+Person, a member, does.
+
+ >>> concise_team = factory.makeTeam(
+ ... name='conciseteam', displayname='Concise Team')
+ >>> concise_team.verbose_bugnotifications = False
+ >>> concise_team_person = factory.makePerson(
+ ... displayname='Concise Team Person',
+ ... <email address hidden>')
+ >>> concise_team_person.verbose_bugnotifications = True
+ >>> concise_team.addMember(concise_team_person, concise_team_person)
+ >>> bug.subscribe(concise_team, concise_team_person)
+ <BugSubscription...>
+
+Verbose Team wants verbose notifications, while Verbose Team Person, a
+member, does not.
+
+ >>> verbose_team = factory.makeTeam(
+ ... name='verboseteam', displayname='Verbos...

Hi William,

This looks awesome. Merge approved!

 review approve code
 status approved

On Jul 23, 2009, at 11:18 AM, Graham Binns wrote:

>>
>> + >>> 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).

TOOWTDI. I've updated the style guide.

-Barry