launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/nomination-investigation-2 into lp:launchpad.

Commit message:
Remove unused arguments from Bug.getBugNotificationRecipients().

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/nomination-investigation-2/+merge/129761

This branch cleans up the method that I am investigating in
regards to timeouts getting the subscribers to an approved
bug nomination.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Remove unused args Bug.getBugNotificationRecipients().
      * The duplicate_of and old_bug args have not been used in
        a few years.
    * The old args are superseded by level=None, which should
      be mentioned in the IBug interface.
    * The default level is BugNotificationLevel.LIFECYCLE, but this
      is not obvious when calling the method without args.
      Make this explicit in the method signature.
    * Maybe this simplified method could cache the recipients using
      level as a key.


QA

    * None.


LINT

    lib/lp/bugs/interfaces/bug.py
    lib/lp/bugs/model/bug.py
    lib/lp/bugs/scripts/tests/test_bugnotification.py
    lib/lp/bugs/subscribers/bug.py


LoC

    This branch deletes obsolete lines of code.


TEST

    ./bin/test -vvc \
        -t doc/bugnotificationrecipients.txt -t doc/bugsubscription.txt \
        -t test_bug_notification_recipients -t bugs-emailinterface.txt \
        -t test_bugchanges lp.bugs


IMPLEMENTATION

Removed two args and documented the only arg that is used. No test changes...
the two unused are args are not exercised in the test suite.
    lib/lp/bugs/interfaces/bug.py
    lib/lp/bugs/model/bug.py
    lib/lp/bugs/scripts/tests/test_bugnotification.py
    lib/lp/bugs/subscribers/bug.py
-- 
https://code.launchpad.net/~sinzui/launchpad/nomination-investigation-2/+merge/129761
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/nomination-investigation-2 into lp:launchpad.

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2012-09-17 16:13:40 +0000
+++ lib/lp/bugs/interfaces/bug.py	2012-10-15 21:02:21 +0000
@@ -71,6 +71,7 @@
 from lp.app.interfaces.launchpad import IPrivacy
 from lp.app.validators.attachment import attachment_size_constraint
 from lp.app.validators.name import bug_name_validator
+from lp.bugs.enums import BugNotificationLevel
 from lp.bugs.interfaces.bugactivity import IBugActivity
 from lp.bugs.interfaces.bugattachment import IBugAttachment
 from lp.bugs.interfaces.bugbranch import IBugBranch
@@ -477,7 +478,7 @@
             `BugSubscriptionLevel.LIFECYCLE` if unspecified.
         """
 
-    def getBugNotificationRecipients(duplicateof=None, old_bug=None):
+    def getBugNotificationRecipients(level=BugNotificationLevel.LIFECYCLE):
         """Return a complete INotificationRecipientSet instance.
 
         The INotificationRecipientSet instance will contain details of

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-10-12 11:31:52 +0000
+++ lib/lp/bugs/model/bug.py	2012-10-15 21:02:21 +0000
@@ -1085,20 +1085,13 @@
         """
         return get_also_notified_subscribers(self, recipients, level)
 
-    def getBugNotificationRecipients(self, duplicateof=None, old_bug=None,
-                                     level=None):
+    def getBugNotificationRecipients(self,
+                                     level=BugNotificationLevel.LIFECYCLE):
         """See `IBug`."""
-        recipients = BugNotificationRecipients(duplicateof=duplicateof)
+        recipients = BugNotificationRecipients()
         self.getDirectSubscribers(
             recipients, level=level, filter_visible=True)
         self.getIndirectSubscribers(recipients, level=level)
-
-        # XXX Tom Berger 2008-03-18:
-        # We want to look up the recipients for `old_bug` too,
-        # but for this to work, this code has to move out of the
-        # class and into a free function, since `old_bug` is a
-        # `Snapshot`, and doesn't have any of the methods of the
-        # original `Bug`.
         return recipients
 
     def addCommentNotification(self, message, recipients=None, activity=None):

=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py	2012-10-15 03:31:26 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2012-10-15 21:02:21 +0000
@@ -111,7 +111,7 @@
     def title(self):
         return "Mock Bug #%s" % self.id
 
-    def getBugNotificationRecipients(self, duplicateof=None):
+    def getBugNotificationRecipients(self, level=None):
         recipients = BugNotificationRecipients()
         no_priv = getUtility(IPersonSet).getByEmail(
             'no-priv@xxxxxxxxxxxxx')
@@ -126,14 +126,14 @@
 class ExceptionBug(MockBug):
     """A bug which causes an exception to be raised."""
 
-    def getBugNotificationRecipients(self, duplicateof=None):
+    def getBugNotificationRecipients(self, level=None):
         raise Exception('FUBAR')
 
 
 class DBExceptionBug(MockBug):
     """A bug which causes a DB constraint to be triggered."""
 
-    def getBugNotificationRecipients(self, duplicateof=None):
+    def getBugNotificationRecipients(self, level=None):
         # Trigger a DB constraint, resulting in the transaction being
         # unusable.
         firefox = getUtility(IProductSet).getByName('firefox')

=== modified file 'lib/lp/bugs/subscribers/bug.py'
--- lib/lp/bugs/subscribers/bug.py	2012-10-15 03:31:26 +0000
+++ lib/lp/bugs/subscribers/bug.py	2012-10-15 21:02:21 +0000
@@ -135,7 +135,6 @@
     """Generate bug notifications and add them to the bug."""
     changes = get_bug_changes(bug_delta)
     recipients = bug_delta.bug.getBugNotificationRecipients(
-        old_bug=bug_delta.bug_before_modification,
         level=BugNotificationLevel.METADATA)
     if old_bugtask is not None:
         old_bugtask_recipients = BugNotificationRecipients()
@@ -147,7 +146,6 @@
         bug = bug_delta.bug
         if isinstance(change, BugDuplicateChange):
             no_dupe_master_recipients = bug.getBugNotificationRecipients(
-                old_bug=bug_delta.bug_before_modification,
                 level=change.change_level)
             bug_delta.bug.addChange(
                 change, recipients=no_dupe_master_recipients)
@@ -169,7 +167,6 @@
         else:
             if change.change_level == BugNotificationLevel.LIFECYCLE:
                 change_recipients = bug.getBugNotificationRecipients(
-                    old_bug=bug_delta.bug_before_modification,
                     level=change.change_level)
                 recipients.update(change_recipients)
             bug_delta.bug.addChange(change, recipients=recipients)