launchpad-reviewers team mailing list archive

[Ioana Lasc <mp+429643@xxxxxxxxxxxxxxxxxx>]

Ioana Lasc has proposed merging ~ilasc/launchpad:expose-send-notifications into launchpad:master.

Commit message:
expose send_notifications over the API for Bug.newMessage

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/429643
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:expose-send-notifications into launchpad:master.

diff --git a/lib/lp/bugs/interfaces/bug.py b/lib/lp/bugs/interfaces/bug.py
index 60a74c3..ab4e2fc 100644
--- a/lib/lp/bugs/interfaces/bug.py
+++ b/lib/lp/bugs/interfaces/bug.py
@@ -1033,11 +1033,16 @@ class IBugAppend(Interface):
     @operation_parameters(
         subject=optional_message_subject_field(),
         content=copy_field(IMessage["content"]),
+        send_notifications=Bool(
+            title=_("Enable email notifications for adding this comment."),
+            required=False,
+            default=True,
+        ),
     )
     @call_with(owner=REQUEST_USER)
     @export_factory_operation(IMessage, [])
     @operation_for_version("beta")
-    def newMessage(owner, subject, content):
+    def newMessage(owner, subject, content, send_notifications=True):
         """Create a new message, and link it to this object."""
 
     @operation_parameters(
diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
index 8b481ae..ac16f81 100644
--- a/lib/lp/bugs/model/bug.py
+++ b/lib/lp/bugs/model/bug.py
@@ -215,6 +215,13 @@ class CannotSetLockReason(Exception):
 
 
 @error_status(http.client.BAD_REQUEST)
+class CannotDisableNotifications(Exception):
+    """Raised when someone tries to disable notifications for adding comments
+    on a bug and the person calling the API is not the bugsupervisor.
+    """
+
+
+@error_status(http.client.BAD_REQUEST)
 class CannotLockBug(Exception):
     """
     Raised when someone tries to lock a bug already locked
@@ -1496,6 +1503,21 @@ class Bug(SQLBase, InformationTypeMixin):
         if not bugmsg:
             return
 
+        # If the user posting the new message is the bug supervisor for any of
+        # the affected pillars they should be able
+        # to disable email notifications
+        can_disable = False
+        roles = IPersonRoles(owner)
+        if send_notifications is False:
+            for pillar in self.affected_pillars:
+                if roles.isBugSupervisor(pillar):
+                    can_disable = True
+            if not can_disable:
+                raise CannotDisableNotifications(
+                    "Email notifications for this bug can only "
+                    "be disabled by the bug supervisor."
+                )
+
         if send_notifications:
             notify(ObjectCreatedEvent(bugmsg, user=owner))
 
diff --git a/lib/lp/bugs/tests/test_bug_messages_webservice.py b/lib/lp/bugs/tests/test_bug_messages_webservice.py
index f67f2ef..6651918 100644
--- a/lib/lp/bugs/tests/test_bug_messages_webservice.py
+++ b/lib/lp/bugs/tests/test_bug_messages_webservice.py
@@ -2,15 +2,17 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Webservice unit tests related to Launchpad Bug messages."""
-
+import transaction
 from testtools.matchers import HasLength
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
 from lp.bugs.interfaces.bugmessage import IBugMessageSet
+from lp.bugs.model.bugnotification import BugNotification
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import IPersonSet
+from lp.services.database.interfaces import IStore
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.testing import (
     TestCaseWithFactory,
@@ -118,6 +120,126 @@ class TestBugMessage(TestCaseWithFactory):
         }
         self.assertContentEqual(bug_attachment_ids, message_attachment_ids)
 
+    def test_disable_email_on_bug_comment_with_admin(self):
+        # Although admin has Append privileges, it is not
+        # a bug supervisor and should not be able to disable notifs
+        self.person_set = getUtility(IPersonSet)
+        admins = self.person_set.getByName("admins")
+        self.admin = admins.teamowner
+        with admin_logged_in():
+            bug = self.factory.makeBug()
+            bug_url = api_url(bug)
+        webservice = webservice_for_person(
+            self.admin, permission=OAuthPermission.WRITE_PUBLIC
+        )
+
+        response = webservice.named_post(
+            bug_url,
+            "newMessage",
+            content="Test comment on bug",
+            send_notifications=False,
+        )
+
+        self.assertEqual(400, response.status)
+        self.assertEqual(
+            b"Email notifications for this bug can only "
+            b"be disabled by the bug supervisor.",
+            response.body,
+        )
+        latest_notification = (
+            IStore(BugNotification)
+            .find(BugNotification)
+            .order_by(BugNotification.id)
+            .last()
+        )
+        self.assertNotIn(
+            "Test comment on bug", latest_notification.message.text_contents
+        )
+
+    def test_disable_email_on_bug_comment_with_supervisor(self):
+        # A bug supervisor on at least one affected_pillar of the bug should
+        # be able to disable email notifications
+        self.person_set = getUtility(IPersonSet)
+        admins = self.person_set.getByName("admins")
+        self.admin = admins.teamowner
+
+        with admin_logged_in():
+            product = self.factory.makeProduct(
+                owner=self.admin,
+                official_malone=True,
+                bug_supervisor=self.admin,
+            )
+            bug = self.factory.makeBug(target=product, owner=self.admin)
+            transaction.commit()
+            bug_url = api_url(bug)
+        webservice = webservice_for_person(
+            self.admin, permission=OAuthPermission.WRITE_PUBLIC
+        )
+
+        response = webservice.named_post(
+            bug_url,
+            "newMessage",
+            content="Test comment on bug",
+            send_notifications=False,
+        )
+
+        self.assertEqual(201, response.status)
+        message_url = response.getHeader("Location")
+        added_message = webservice.get(message_url)
+        self.assertEqual(
+            "Test comment on bug", added_message.jsonBody()["content"]
+        )
+
+        latest_notification = (
+            IStore(BugNotification)
+            .find(BugNotification)
+            .order_by(BugNotification.id)
+            .last()
+        )
+        self.assertNotIn(
+            "Test comment on bug", latest_notification.message.text_contents
+        )
+
+    def test_disable_email_on_bug_comment_regression(self):
+        # When send_notifications is not passed in it defaults to True
+        # and doesn't require bug supervisor privliges to create the comment
+        self.person_set = getUtility(IPersonSet)
+        admins = self.person_set.getByName("admins")
+        self.admin = admins.teamowner
+        with admin_logged_in():
+            bug = self.factory.makeBug()
+            bug_url = api_url(bug)
+        webservice = webservice_for_person(
+            self.admin, permission=OAuthPermission.WRITE_PUBLIC
+        )
+
+        response = webservice.named_post(
+            bug_url,
+            "newMessage",
+            content="Test comment on bug",
+        )
+
+        # the endpoint is still callable and if send_notifications is not
+        # passed in. The comment will be created and a notification will be
+        # created as it did before introducing the silencing of emails through
+        # exposure of send_notifications on the API
+        self.assertEqual(201, response.status)
+        message_url = response.getHeader("Location")
+        added_message = webservice.get(message_url)
+        self.assertEqual(
+            "Test comment on bug", added_message.jsonBody()["content"]
+        )
+
+        latest_notification = (
+            IStore(BugNotification)
+            .find(BugNotification)
+            .order_by(BugNotification.id)
+            .last()
+        )
+        self.assertIn(
+            "Test comment on bug", latest_notification.message.text_contents
+        )
+
 
 class TestSetCommentVisibility(TestCaseWithFactory):
     """Tests who can successfully set comment visibility."""