Merge lp:~dev-nigelj/launchpad/bug-697157 into lp:launchpad

[Nigel Jones]

[r=jtv][bug=697157] Special-case bug assignment mail for self-assignment.

Summary

If you assign yourself to a bug, under certain circumstances an e-mail will be sent saying "Nigel Jones (dev-nigelj) has assigned this bug to you for Launchpad itself:" when a better/more logical message would be "You have assigned this bug to yourself for Launchpad itself:"

Proposed Fix/Implementation Details

The Proposed and Implemented fix is to include an if statement, to verify if that the event creator is the same as the bug task assignee and generate the new message, otherwise to use the existing

Tests

All tests related to this change pass, and I couldn't see any new failures on a full test natty run.

The change also introduces a new test in lib/lp/bugs/mail/tests/test_bug_task_assignment.py (test_self_assignee_notification_message()) which tests for this new message in the situations that it should be generated.

Demo/QA

w/ AppServer mail setup in a way that you can read all e-mails sent
Create a new bug for "Ubuntu" under one username
Logout & login as a second username (not one that is a member of Ubuntu Team)
Assign the bug to yourself, verify the correct statement regarding bug assignment is used in the [NEW] e-mail.

Lint

make lint is now clean after r13865 in this branch

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/mail/newbug.py
  lib/lp/bugs/mail/tests/test_bug_task_assignment.py

[Jeroen T. Vermeulen (jtv)]

Thanks for the fix. The basic change looks good to me, but I have a few cosmetic changes to ask for in the test.

Please don't be disparaged by these; I can make the changes for you if you like but it helps to be aware of them.

21 === modified file 'lib/lp/bugs/mail/tests/test_bug_task_assignment.py'
22 --- lib/lp/bugs/mail/tests/test_bug_task_assignment.py 2011-08-12 11:37:08 +0000
23 +++ lib/lp/bugs/mail/tests/test_bug_task_assignment.py 2011-09-05 00:43:26 +0000
24 @@ -65,6 +65,22 @@
25 self.assertTrue(rationale in msg,
26 '%s not in\n%s\n' % (rationale, msg))
27
28 + def test_self_assignee_notification_message(self):
29 + """Test notification string when a person is assigned a task by
30 + someone else."""

This looks incorrect: the test is for the other case, where a person assigns a bug to themselves, no?

31 + self.assertEqual(len(stub.test_emails), 0, 'emails in queue')

This isn't really a test assertion. It's better just to make the precondition happen:

    stub.test_emails = []

Not only is this easier to read, it also makes the test insensitive to the previous contents of stub.test_emails.

(Yes, it seems naughty to poke stub.test_emails like this. But plenty of other tests seem to do it and, since this is a test helper, we shouldn't be too worried about protecting the abstraction.)

32 + self.bug_task.transitionToAssignee(self.user)
33 + notify(ObjectModifiedEvent(
34 + self.bug_task, self.bug_task_before_modification,
35 + edited_fields=['assignee']))
36 + transaction.commit()

Is the commit needed in this case? I know emails are special, but it's always worth trying because commits can be costly in tests.

37 + self.assertEqual(len(stub.test_emails), 1, 'email not sent')

In self.assertEqual and friends, state the expected outcome as the first argument to the comparison, with the actual outcome second.

The "email not sent" string doesn't add much value, so I'd leave it out. The explanation is obvious if len(stub.test_emails) is zero, or plainly wrong if it has some other unexpected value. We require explanatory strings for assertions in production code, but not for assertions in tests.

Here's a handy trick for dealing with short lists in tests:

    [email] = stub.test_emails

This expects stub.test_emails to contain 1 item, and assigns that item to “email.” If stub.test_emails is empty or contains more than 1 email, it will break with a relatively helpful message. Once you're familiar with it it's easier to read than an assertion, and you don't need the “[0]” (or “[-1]”) to access the initial element later.

(As a personal preference, I also don't recommend single-quoting free-form strings where it would be perfectly reasonable to use an apostrophe.)

38 + rationale = (
39 + 'You have assigned this bug to yourself for Rebirth')
40 + msg = stub.test_emails[-1][2]
41 + self.assertTrue(rationale in msg,
42 + '%s not in\n%s\n' % (rationale, msg))

For this we have self.assertIn(needle, haystack). It will produce a helpful message when it fails.

J...

review: Approve

[Nigel Jones (dev-nigelj)]

> Please don't be disparaged by these; I can make the changes for you if you
> like but it helps to be aware of them.
Not disparaged at all, in fact, thank you very much for the comments.

> 28 + def test_self_assignee_notification_message(self):
> 29 + """Test notification string when a person is assigned a task
> by
> 30 + someone else."""
>
> This looks incorrect: the test is for the other case, where a person assigns a
> bug to themselves, no?
Correct, this was a blooper, I was trying to keep the structure of this test the same as the others.

>
>
> 31 + self.assertEqual(len(stub.test_emails), 0, 'emails in queue')
>
> This isn't really a test assertion. It's better just to make the precondition
> happen:
>
> stub.test_emails = []
>
> Not only is this easier to read, it also makes the test insensitive to the
> previous contents of stub.test_emails.
Agreed, I've made the change

> 36 + transaction.commit()
>
> Is the commit needed in this case? I know emails are special, but it's always
> worth trying because commits can be costly in tests.
Unfortunately, it appears so, without the transaction.commit() I got an assertion error:
  File "/home/njones/launchpad/lp-branches/bug-697157/lib/lp/bugs/mail/tests/test_bug_task_assignment.py", line 77, in test_self_assignee_notification_message
    self.assertEqual(1, len(stub.test_emails))
AssertionError: 1 != 0

> 37 + self.assertEqual(len(stub.test_emails), 1, 'email not sent')
>
> In self.assertEqual and friends, state the expected outcome as the first
> argument to the comparison, with the actual outcome second.
Okay, done.

> Here's a handy trick for dealing with short lists in tests:
>
> [email] = stub.test_emails
>
> This expects stub.test_emails to contain 1 item, and assigns that item to
> “email.” If stub.test_emails is empty or contains more than 1 email, it will
> break with a relatively helpful message. Once you're familiar with it it's
> easier to read than an assertion, and you don't need the “[0]” (or “[-1]”) to
> access the initial element later.
I'd seen that done before, but hadn't really looked into the magic, so thank you.

> 41 + self.assertTrue(rationale in msg,
> 42 + '%s not in\n%s\n' % (rationale, msg))
>
> For this we have self.assertIn(needle, haystack). It will produce a helpful
> message when it fails.

Yep, self.assertIn definitely seems to be much more readable in the test itself.

Thank you for the review, and the notes, very valuable.

[Jeroen T. Vermeulen (jtv)]

Thanks for the changes. I see you also added a comment for the enigmatic “[2]” which is also nice: programming is explaining a solution in a form that computers and humans can both understand!

Want me to land the branch?

[Nigel Jones (dev-nigelj)]

Yes please, feel free to land the branch.

=== modified file 'lib/lp/bugs/mail/newbug.py'
--- lib/lp/bugs/mail/newbug.py	2010-12-21 23:49:34 +0000
+++ lib/lp/bugs/mail/newbug.py	2011-09-05 13:17:21 +0000
@@ -62,8 +62,13 @@
 
     if new_recipients:
         if "assignee" in reason and event_creator is not None:
-            contents += (
-                "%(assigner)s has assigned this bug to you for %(target)s")
+            if event_creator == bugtask.assignee:
+                contents += (
+                    "You have assigned this bug to yourself for %(target)s")
+            else:
+                contents += (
+                    "%(assigner)s has assigned this bug to you for " +
+                    "%(target)s")
             content_substitutions['assigner'] = (
                 event_creator.unique_displayname)
             content_substitutions['target'] = bugtask.target.displayname

=== modified file 'lib/lp/bugs/mail/tests/test_bug_task_assignment.py'
--- lib/lp/bugs/mail/tests/test_bug_task_assignment.py	2011-08-12 11:37:08 +0000
+++ lib/lp/bugs/mail/tests/test_bug_task_assignment.py	2011-09-05 13:17:21 +0000
@@ -65,6 +65,23 @@
         self.assertTrue(rationale in msg,
                         '%s not in\n%s\n' % (rationale, msg))
 
+    def test_self_assignee_notification_message(self):
+        """Test notification string when a person is assigned a task by
+           themselves."""
+        stub.test_emails = []
+        self.bug_task.transitionToAssignee(self.user)
+        notify(ObjectModifiedEvent(
+            self.bug_task, self.bug_task_before_modification,
+            edited_fields=['assignee']))
+        transaction.commit()
+        self.assertEqual(1, len(stub.test_emails))
+        rationale = (
+            'You have assigned this bug to yourself for Rebirth')
+        [email] = stub.test_emails
+        # Actual message is part 2 of the e-mail.
+        msg = email[2]
+        self.assertIn(rationale, msg)
+
     def test_assignee_not_a_subscriber(self):
         """Test that a new recipient being assigned a bug task does send
            a NEW message."""