launchpad-reviewers team mailing list archive

[Guruprasad <mp+411682@xxxxxxxxxxxxxxxxxx>]

Guruprasad has proposed merging ~lgp171188/launchpad:fix-has-assigned-this-bug-to-you into launchpad:master.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1894210 in Launchpad itself: ""has assigned this bug to you" could be "has assigned this bug to your team""
  https://bugs.launchpad.net/launchpad/+bug/1894210

For more details, see:
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/411682

This merge proposal tweaks the bug task assignment notification email that is sent to a team to explicitly mention that it is sent to a team.

-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:fix-has-assigned-this-bug-to-you into launchpad:master.

diff --git a/lib/lp/bugs/mail/newbug.py b/lib/lp/bugs/mail/newbug.py
index 5d921d6..9000cde 100644
--- a/lib/lp/bugs/mail/newbug.py
+++ b/lib/lp/bugs/mail/newbug.py
@@ -12,7 +12,8 @@ from lp.services.webapp.publisher import canonical_url
 
 
 def generate_bug_add_email(bug, new_recipients=False, reason=None,
-                           subscribed_by=None, event_creator=None):
+                           subscribed_by=None, event_creator=None,
+                           modified_bugtask=None):
     """Generate a new bug notification from the given IBug.
 
     If new_recipients is supplied we generate a notification explaining
@@ -60,17 +61,27 @@ def generate_bug_add_email(bug, new_recipients=False, reason=None,
         }
 
     if new_recipients:
-        if "assignee" in reason and event_creator is not None:
-            if event_creator == bugtask.assignee:
+        if ("assignee" in reason and
+                event_creator is not None and
+                modified_bugtask is not None):
+            if event_creator == modified_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")
+                contents += "%(assigner)s has assigned this bug to "
+
+                if modified_bugtask.assignee.is_team:
+                    contents += 'your team "%(team_name)s" '
+                    content_substitutions['team_name'] = (
+                        modified_bugtask.assignee.displayname)
+                else:
+                    contents += "you "
+
+            contents += "for %(target)s"
             content_substitutions['assigner'] = (
                 event_creator.unique_displayname)
-            content_substitutions['target'] = bugtask.target.displayname
+            content_substitutions['target'] = (
+                modified_bugtask.target.displayname)
         else:
             contents += "You have been subscribed to a %(visibility)s bug"
         if subscribed_by is not None:
diff --git a/lib/lp/bugs/subscribers/bug.py b/lib/lp/bugs/subscribers/bug.py
index b6539bd..7dee900 100644
--- a/lib/lp/bugs/subscribers/bug.py
+++ b/lib/lp/bugs/subscribers/bug.py
@@ -188,7 +188,7 @@ def add_bug_change_notifications(bug_delta, old_bugtask=None,
 
 def send_bug_details_to_new_bug_subscribers(
     bug, previous_subscribers, current_subscribers, subscribed_by=None,
-    event_creator=None):
+    event_creator=None, modified_bugtask=None):
     """Send an email containing full bug details to new bug subscribers.
 
     This function is designed to handle situations where bugtasks get
@@ -232,7 +232,8 @@ def send_bug_details_to_new_bug_subscribers(
             str(removeSecurityProxy(to_person).preferredemail.email))
         subject, contents = generate_bug_add_email(
             bug, new_recipients=True, subscribed_by=subscribed_by,
-            reason=reason, event_creator=event_creator)
+            reason=reason, event_creator=event_creator,
+            modified_bugtask=modified_bugtask)
         msg = bug_notification_builder.build(
             from_addr, to_person, contents, subject, email_date,
             rationale=rationale, references=references)
diff --git a/lib/lp/bugs/subscribers/bugtask.py b/lib/lp/bugs/subscribers/bugtask.py
index b702491..72600c1 100644
--- a/lib/lp/bugs/subscribers/bugtask.py
+++ b/lib/lp/bugs/subscribers/bugtask.py
@@ -44,4 +44,4 @@ def notify_bugtask_edited(modified_bugtask, event):
 
     send_bug_details_to_new_bug_subscribers(
         event.object.bug, previous_subscribers, current_subscribers,
-        event_creator=event_creator)
+        event_creator=event_creator, modified_bugtask=modified_bugtask)
diff --git a/lib/lp/bugs/tests/test_bugchanges.py b/lib/lp/bugs/tests/test_bugchanges.py
index 148e329..b2999af 100644
--- a/lib/lp/bugs/tests/test_bugchanges.py
+++ b/lib/lp/bugs/tests/test_bugchanges.py
@@ -2,6 +2,8 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for recording changes done to a bug."""
+import quopri
+from unittest.mock import patch
 
 from lazr.lifecycle.event import ObjectCreatedEvent
 from testtools.matchers import (
@@ -190,6 +192,10 @@ class TestBugChanges(TestCaseWithFactory):
                 for recipient in expected_recipients),
             recipients)
 
+    def getEmailBodyFromMessage(self, message):
+        body = message.get_payload()
+        return quopri.decodestring(body.encode('utf-8')).decode('utf-8')
+
     def test_subscribe(self):
         # Subscribing someone to a bug adds an item to the activity log,
         # but doesn't send an email notification.
@@ -1842,3 +1848,48 @@ class TestBugChanges(TestCaseWithFactory):
         # self.product_metadata_subscriber is not included among the
         # recipients.
         self.assertRecipients([self.user])
+
+    @patch('lp.bugs.subscribers.bug.sendmail')
+    def test_bugtask_subscription_email_mentions_the_user(
+            self,
+            mocked_sendmail):
+        # When a bugtask is assigned to a user, verify that the email
+        # notifications to the user mentions that the bug was assigned
+        # to the them directly.
+        user = self.factory.makePerson(
+            displayname='New user', selfgenerated_bugnotifications=True)
+
+        with notify_modified(self.bug_task, ['assignee'], user=self.user):
+            self.bug_task.transitionToAssignee(user)
+        expected_message = (
+            '{} ({}) has assigned this bug to you'.format(
+                self.user.displayname, self.user.name
+            )
+        )
+        email_body = self.getEmailBodyFromMessage(
+            mocked_sendmail.call_args[0][0])
+        self.assertThat(email_body, StartsWith(expected_message))
+
+    @patch('lp.bugs.subscribers.bug.sendmail')
+    def test_team_membership_subscription_email_mentions_the_team(
+            self,
+            mocked_sendmail):
+        # When a bugtask is assigned to a team, verify that the email
+        # notifications to the team members mentions that the bug was assigned
+        # to the team instead of them directly.
+        user = self.factory.makePerson(
+            displayname='New user', selfgenerated_bugnotifications=True)
+        team = self.factory.makeTeam()
+        team.addMember(user, team.teamowner)
+
+        with notify_modified(self.bug_task, ['assignee'], user=user):
+            self.bug_task.transitionToAssignee(team)
+        expected_message = (
+            '{} ({}) has assigned this bug to your team "{}" for {}'.format(
+                user.displayname, user.name, team.displayname,
+                self.bug_task.target.displayname
+            )
+        )
+        email_body = self.getEmailBodyFromMessage(
+            mocked_sendmail.call_args[0][0])
+        self.assertThat(email_body, StartsWith(expected_message))