launchpad-reviewers team mailing list archive

[Ioana Lasc <mp+429828@xxxxxxxxxxxxxxxxxx>]

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

Commit message:
Separate Bug.newMessage into internal and API methods

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

diff --git a/lib/lp/bugs/browser/bugtarget.py b/lib/lp/bugs/browser/bugtarget.py
index e59af36..e7b3dc6 100644
--- a/lib/lp/bugs/browser/bugtarget.py
+++ b/lib/lp/bugs/browser/bugtarget.py
@@ -648,7 +648,7 @@ class FileBugViewBase(LaunchpadFormView):
         attachment = self.request.form.get(self.widgets["filecontent"].name)
         if attachment or extra_data.attachments:
             # Attach all the comments to a single empty comment.
-            attachment_comment = bug.newMessage(
+            attachment_comment = bug.newMessage_internal(
                 owner=self.user,
                 subject=bug.followup_subject(),
                 content=None,
diff --git a/lib/lp/bugs/interfaces/bug.py b/lib/lp/bugs/interfaces/bug.py
index ab4e2fc..9697660 100644
--- a/lib/lp/bugs/interfaces/bug.py
+++ b/lib/lp/bugs/interfaces/bug.py
@@ -1045,6 +1045,11 @@ class IBugAppend(Interface):
     def newMessage(owner, subject, content, send_notifications=True):
         """Create a new message, and link it to this object."""
 
+    def newMessage_internal(owner, subject, content, send_notifications=True):
+        """Create a new message, and link it to this object.
+        This method is used internally mostly by the UI.
+        """
+
     @operation_parameters(
         person=Reference(IPerson, title=_("Person"), required=True),
         level=Choice(
diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
index 7685c10..aa83473 100644
--- a/lib/lp/bugs/model/bug.py
+++ b/lib/lp/bugs/model/bug.py
@@ -1487,6 +1487,38 @@ class Bug(SQLBase, InformationTypeMixin):
         remote_comment_id=None,
         send_notifications=True,
     ):
+        # Admins, commercial admins, registry experts, pillar owners,
+        # pillar drivers, and pillar bug supervisors should be able
+        # to disable email notifications
+        if not send_notifications:
+            authz = getAdapter(self, IAuthorization, "launchpad.Moderate")
+            if not authz.checkAuthenticated(IPersonRoles(owner)):
+                raise CannotDisableNotifications(
+                    "Email notifications can only "
+                    "be disabled by admins, commercial admins, "
+                    "registry experts, pillar owners, pillar drivers "
+                    "or pillar bug supervisors."
+                )
+        return self.newMessage_internal(
+            owner,
+            subject,
+            content,
+            parent,
+            bugwatch,
+            remote_comment_id,
+            send_notifications,
+        )
+
+    def newMessage_internal(
+        self,
+        owner=None,
+        subject=None,
+        content=None,
+        parent=None,
+        bugwatch=None,
+        remote_comment_id=None,
+        send_notifications=True,
+    ):
         """Create a new Message and link it to this bug."""
         if subject is None:
             subject = self.followup_subject()
@@ -1504,18 +1536,6 @@ class Bug(SQLBase, InformationTypeMixin):
         if not bugmsg:
             return
 
-        # Admins, commercial admins, registry experts, pillar owners,
-        # pillar drivers, and pillar bug supervisors should be able
-        # to disable email notifications
-        authz = getAdapter(self, IAuthorization, "launchpad.Moderate")
-        if not authz.checkAuthenticated(IPersonRoles(owner)):
-            raise CannotDisableNotifications(
-                "Email notifications can only "
-                "be disabled by admins, commercial admins, "
-                "registry experts, pillar owners, pillar drivers "
-                "or pillar bug supervisors."
-            )
-
         if send_notifications:
             notify(ObjectCreatedEvent(bugmsg, user=owner))
 
diff --git a/lib/lp/bugs/model/tests/test_bug.py b/lib/lp/bugs/model/tests/test_bug.py
index 1ce81f9..a2452ff 100644
--- a/lib/lp/bugs/model/tests/test_bug.py
+++ b/lib/lp/bugs/model/tests/test_bug.py
@@ -25,7 +25,7 @@ from lp.registry.interfaces.accesspolicy import (
     IAccessPolicyArtifactSource,
     IAccessPolicySource,
 )
-from lp.registry.interfaces.person import PersonVisibility
+from lp.registry.interfaces.person import IPersonSet, PersonVisibility
 from lp.registry.tests.test_accesspolicy import get_policies_for_artifact
 from lp.testing import (
     EventRecorder,
@@ -641,11 +641,17 @@ class TestBug(TestCaseWithFactory):
             self.assertIsInstance(recorder.events[0], ObjectCreatedEvent)
 
     def test_newMessage_send_notification_false(self):
-        # Notifications about new messages can be suppressed.
+        # After exposing this over the API extra permissions are required.
+        # Notifications about new messages can be suppressed
+        # by admins, commercial admins, registry experts,
+        # pillar owners, pillar drivers or pillar bug supervisors.
+        person_set = getUtility(IPersonSet)
+        admins = person_set.getByName("admins")
+        admin = admins.teamowner
         bug = self.factory.makeBug()
-        login_person(bug.owner)
+        login_person(admins.teamowner)
         with EventRecorder() as recorder:
-            bug.newMessage(owner=bug.owner, send_notifications=False)
+            bug.newMessage(owner=admin, send_notifications=False)
             self.assertEqual(0, len(recorder.events))
 
     def test_vulnerabilities_property(self):