Merge lp:~gmb/launchpad/refactor-bugnotification-recipient-rationales-bug-594208 into lp:launchpad

[Graham Binns]

BugNotificationRecipients has been moved to lp.bugs.mail.bugnotificationrecipients.

This branch moves BugNotificationRecipients into its own module in the lp.bugs.mail package.

This turned out to be a much easier branch than I anticipated. I thought that some refactoring would be needed to make BugNotificationRecipients play nice with lp.services.mail, but it turned out that it already extends lp.services.mail.notificationrecipientset.NotificationRecipientSet.

I've moved the class out of mailnotification.py and updated all the import sites I could find. I've added an XXX to mailnotification.py for the import of NotificationRecipientSet, since there's a lot of things that import it indirectly but fixing those import sites is out-of-scope for this branch.

[Jeroen T. Vermeulen (jtv)]

Fine with me. As discussed on IRC, there's plenty more work to be done on the code that's moving (dealing with supervisors of duplicate bugs, factoring out the repetition in text composition, replacing reason = reason + x with reason += x) but as you point out that's best left to later branches after first moving the code out of conflicts' way.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/mailnotification.py'
--- lib/canonical/launchpad/mailnotification.py	2010-06-10 10:54:51 +0000
+++ lib/canonical/launchpad/mailnotification.py	2010-06-16 08:38:25 +0000
@@ -32,9 +32,8 @@
     get_contact_email_addresses, get_email_template, shortlist)
 from canonical.launchpad.interfaces import (
     IEmailAddressSet, IHeldMessageDetails, ILaunchpadCelebrities,
-    INotificationRecipientSet, IPerson, IPersonSet, ISpecification,
-    IStructuralSubscriptionTarget, ITeamMembershipSet, IUpstreamBugTask,
-    TeamMembershipStatus)
+    IPerson, IPersonSet, ISpecification, IStructuralSubscriptionTarget,
+    ITeamMembershipSet, IUpstreamBugTask, TeamMembershipStatus)
 from lp.bugs.interfaces.bugchange import IBugChange
 from canonical.launchpad.interfaces.launchpad import ILaunchpadRoot
 from canonical.launchpad.interfaces.message import (
@@ -44,154 +43,20 @@
 from canonical.launchpad.mail import (
     sendmail, simple_sendmail, simple_sendmail_from_person, format_address)
 from lp.services.mail.mailwrapper import MailWrapper
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.launchpad.webapp.url import urlappend
+
+# XXX 2010-06-16 gmb bug=594985
+#     This shouldn't be here, but if we take it out lots of things cry,
+#     which is sad.
 from lp.services.mail.notificationrecipientset import (
     NotificationRecipientSet)
-from canonical.launchpad.webapp.publisher import canonical_url
-from canonical.launchpad.webapp.url import urlappend
 
+from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
 
 CC = "CC"
 
 
-class BugNotificationRecipients(NotificationRecipientSet):
-    """A set of emails and rationales notified for a bug change.
-
-    Each email address registered in a BugNotificationRecipients is
-    associated to a string and a header that explain why the address is
-    being emailed. For instance, if the email address is that of a
-    distribution bug supervisor for a bug, the string and header will make
-    that fact clear.
-
-    The string is meant to be rendered in the email footer. The header
-    is meant to be used in an X-Launchpad-Message-Rationale header.
-
-    The first rationale registered for an email address is the one
-    which will be used, regardless of other rationales being added
-    for it later. This gives us a predictable policy of preserving
-    the first reason added to the registry; the callsite should
-    ensure that the manipulation of the BugNotificationRecipients
-    instance is done in preferential order.
-
-    Instances of this class are meant to be returned by
-    IBug.getBugNotificationRecipients().
-    """
-    implements(INotificationRecipientSet)
-
-    def __init__(self, duplicateof=None):
-        """Constructs a new BugNotificationRecipients instance.
-
-        If this bug is a duplicate, duplicateof should be used to
-        specify which bug ID it is a duplicate of.
-
-        Note that there are two duplicate situations that are
-        important:
-          - One is when this bug is a duplicate of another bug:
-            the subscribers to the main bug get notified of our
-            changes.
-          - Another is when the bug we are changing has
-            duplicates; in that case, direct subscribers of
-            duplicate bugs get notified of our changes.
-        These two situations are catered respectively by the
-        duplicateof parameter above and the addDupeSubscriber method.
-        Don't confuse them!
-        """
-        NotificationRecipientSet.__init__(self)
-        self.duplicateof = duplicateof
-
-    def _addReason(self, person, reason, header):
-        """Adds a reason (text and header) for a person.
-
-        It takes care of modifying the message when the person is notified
-        via a duplicate.
-        """
-        if self.duplicateof is not None:
-            reason = reason + " (via bug %s)" % self.duplicateof.id
-            header = header + " via Bug %s" % self.duplicateof.id
-        reason = "You received this bug notification because you %s." % reason
-        self.add(person, reason, header)
-
-    def addDupeSubscriber(self, person):
-        """Registers a subscriber of a duplicate of this bug."""
-        reason = "Subscriber of Duplicate"
-        if person.isTeam():
-            text = ("are a member of %s, which is a subscriber "
-                    "of a duplicate bug" % person.displayname)
-            reason += " @%s" % person.name
-        else:
-            text = "are a direct subscriber of a duplicate bug"
-        self._addReason(person, text, reason)
-
-    def addDirectSubscriber(self, person):
-        """Registers a direct subscriber of this bug."""
-        reason = "Subscriber"
-        if person.isTeam():
-            text = ("are a member of %s, which is a direct subscriber"
-                    % person.displayname)
-            reason += " @%s" % person.name
-        else:
-            text = "are a direct subscriber of the bug"
-        self._addReason(person, text, reason)
-
-    def addAssignee(self, person):
-        """Registers an assignee of a bugtask of this bug."""
-        reason = "Assignee"
-        if person.isTeam():
-            text = ("are a member of %s, which is a bug assignee"
-                    % person.displayname)
-            reason += " @%s" % person.name
-        else:
-            text = "are a bug assignee"
-        self._addReason(person, text, reason)
-
-    def addDistroBugSupervisor(self, person, distro):
-        """Registers a distribution bug supervisor for this bug."""
-        reason = "Bug Supervisor (%s)" % distro.displayname
-        # All displaynames in these reasons should be changed to bugtargetname
-        # (as part of bug 113262) once bugtargetname is finalized for packages
-        # (bug 113258). Changing it before then would be excessively
-        # disruptive.
-        if person.isTeam():
-            text = ("are a member of %s, which is the bug supervisor for %s" %
-                (person.displayname, distro.displayname))
-            reason += " @%s" % person.name
-        else:
-            text = "are the bug supervisor for %s" % distro.displayname
-        self._addReason(person, text, reason)
-
-    def addStructuralSubscriber(self, person, target):
-        """Registers a structural subscriber to this bug's target."""
-        reason = "Subscriber (%s)" % target.displayname
-        if person.isTeam():
-            text = ("are a member of %s, which is subscribed to %s" %
-                (person.displayname, target.displayname))
-            reason += " @%s" % person.name
-        else:
-            text = "are subscribed to %s" % target.displayname
-        self._addReason(person, text, reason)
-
-    def addUpstreamBugSupervisor(self, person, upstream):
-        """Registers an upstream bug supervisor for this bug."""
-        reason = "Bug Supervisor (%s)" % upstream.displayname
-        if person.isTeam():
-            text = ("are a member of %s, which is the bug supervisor for %s" %
-                (person.displayname, upstream.displayname))
-            reason += " @%s" % person.name
-        else:
-            text = "are the bug supervisor for %s" % upstream.displayname
-        self._addReason(person, text, reason)
-
-    def addRegistrant(self, person, upstream):
-        """Registers an upstream product registrant for this bug."""
-        reason = "Registrant (%s)" % upstream.displayname
-        if person.isTeam():
-            text = ("are a member of %s, which is the registrant for %s" %
-                (person.displayname, upstream.displayname))
-            reason += " @%s" % person.name
-        else:
-            text = "are the registrant for %s" % upstream.displayname
-        self._addReason(person, text, reason)
-
-
 def format_rfc2822_date(date):
     """Formats a date according to RFC2822's desires."""
     return formatdate(rfc822.mktime_tz(date.utctimetuple() + (0, )))
 
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2010-06-04 09:31:21 +0000
+++ lib/lp/bugs/configure.zcml	2010-06-16 08:38:25 +0000
@@ -952,7 +952,7 @@
             interface="lp.bugs.interfaces.bugnotification.IBugNotificationRecipient"/>
     </class>
     <class
-        class="canonical.launchpad.mailnotification.BugNotificationRecipients">
+        class="lp.bugs.mail.bugnotificationrecipients.BugNotificationRecipients">
         <allow
             interface="canonical.launchpad.interfaces.INotificationRecipientSet"/>
     </class>
 
=== modified file 'lib/lp/bugs/doc/bug-change.txt'
--- lib/lp/bugs/doc/bug-change.txt	2009-06-12 16:36:02 +0000
+++ lib/lp/bugs/doc/bug-change.txt	2010-06-16 08:38:25 +0000
@@ -54,7 +54,7 @@
 
 We'll create a test class that actually implements the methods we need.
 
-    >>> from canonical.launchpad.mailnotification import (
+    >>> from lp.bugs.mail.bugnotificationrecipients import (
     ...     BugNotificationRecipients)
 
     >>> example_message = factory.makeMessage(content="Hello, world")
 
=== modified file 'lib/lp/bugs/doc/bugnotificationrecipients.txt'
--- lib/lp/bugs/doc/bugnotificationrecipients.txt	2009-08-13 19:03:36 +0000
+++ lib/lp/bugs/doc/bugnotificationrecipients.txt	2010-06-16 08:38:25 +0000
@@ -63,7 +63,7 @@
 Let's up some data for our test:
 
   >>> from canonical.launchpad.interfaces import IPersonSet
-  >>> from canonical.launchpad.mailnotification import (
+  >>> from lp.bugs.mail.bugnotificationrecipients import (
   ...     BugNotificationRecipients)
   >>> debian = Distribution.selectOneBy(name="debian")
   >>> pmount = debian.getSourcePackage("pmount")
 
=== added file 'lib/lp/bugs/mail/bugnotificationrecipients.py'
--- lib/lp/bugs/mail/bugnotificationrecipients.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/mail/bugnotificationrecipients.py	2010-06-16 08:38:25 +0000
@@ -0,0 +1,158 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Code for handling bug notification recipients in bug mail."""
+
+__metaclass__ = type
+__all__ = [
+    'BugNotificationRecipients',
+    ]
+
+from zope.interface import implements
+
+from canonical.launchpad.interfaces import INotificationRecipientSet
+
+from lp.services.mail.notificationrecipientset import (
+    NotificationRecipientSet)
+
+
+class BugNotificationRecipients(NotificationRecipientSet):
+    """A set of emails and rationales notified for a bug change.
+
+    Each email address registered in a BugNotificationRecipients is
+    associated to a string and a header that explain why the address is
+    being emailed. For instance, if the email address is that of a
+    distribution bug supervisor for a bug, the string and header will make
+    that fact clear.
+
+    The string is meant to be rendered in the email footer. The header
+    is meant to be used in an X-Launchpad-Message-Rationale header.
+
+    The first rationale registered for an email address is the one
+    which will be used, regardless of other rationales being added
+    for it later. This gives us a predictable policy of preserving
+    the first reason added to the registry; the callsite should
+    ensure that the manipulation of the BugNotificationRecipients
+    instance is done in preferential order.
+
+    Instances of this class are meant to be returned by
+    IBug.getBugNotificationRecipients().
+    """
+    implements(INotificationRecipientSet)
+
+    def __init__(self, duplicateof=None):
+        """Constructs a new BugNotificationRecipients instance.
+
+        If this bug is a duplicate, duplicateof should be used to
+        specify which bug ID it is a duplicate of.
+
+        Note that there are two duplicate situations that are
+        important:
+          - One is when this bug is a duplicate of another bug:
+            the subscribers to the main bug get notified of our
+            changes.
+          - Another is when the bug we are changing has
+            duplicates; in that case, direct subscribers of
+            duplicate bugs get notified of our changes.
+        These two situations are catered respectively by the
+        duplicateof parameter above and the addDupeSubscriber method.
+        Don't confuse them!
+        """
+        NotificationRecipientSet.__init__(self)
+        self.duplicateof = duplicateof
+
+    def _addReason(self, person, reason, header):
+        """Adds a reason (text and header) for a person.
+
+        It takes care of modifying the message when the person is notified
+        via a duplicate.
+        """
+        if self.duplicateof is not None:
+            reason = reason + " (via bug %s)" % self.duplicateof.id
+            header = header + " via Bug %s" % self.duplicateof.id
+        reason = "You received this bug notification because you %s." % reason
+        self.add(person, reason, header)
+
+    def addDupeSubscriber(self, person):
+        """Registers a subscriber of a duplicate of this bug."""
+        reason = "Subscriber of Duplicate"
+        if person.isTeam():
+            text = ("are a member of %s, which is a subscriber "
+                    "of a duplicate bug" % person.displayname)
+            reason += " @%s" % person.name
+        else:
+            text = "are a direct subscriber of a duplicate bug"
+        self._addReason(person, text, reason)
+
+    def addDirectSubscriber(self, person):
+        """Registers a direct subscriber of this bug."""
+        reason = "Subscriber"
+        if person.isTeam():
+            text = ("are a member of %s, which is a direct subscriber"
+                    % person.displayname)
+            reason += " @%s" % person.name
+        else:
+            text = "are a direct subscriber of the bug"
+        self._addReason(person, text, reason)
+
+    def addAssignee(self, person):
+        """Registers an assignee of a bugtask of this bug."""
+        reason = "Assignee"
+        if person.isTeam():
+            text = ("are a member of %s, which is a bug assignee"
+                    % person.displayname)
+            reason += " @%s" % person.name
+        else:
+            text = "are a bug assignee"
+        self._addReason(person, text, reason)
+
+    def addDistroBugSupervisor(self, person, distro):
+        """Registers a distribution bug supervisor for this bug."""
+        reason = "Bug Supervisor (%s)" % distro.displayname
+        # All displaynames in these reasons should be changed to bugtargetname
+        # (as part of bug 113262) once bugtargetname is finalized for packages
+        # (bug 113258). Changing it before then would be excessively
+        # disruptive.
+        if person.isTeam():
+            text = ("are a member of %s, which is the bug supervisor for %s" %
+                (person.displayname, distro.displayname))
+            reason += " @%s" % person.name
+        else:
+            text = "are the bug supervisor for %s" % distro.displayname
+        self._addReason(person, text, reason)
+
+    def addStructuralSubscriber(self, person, target):
+        """Registers a structural subscriber to this bug's target."""
+        reason = "Subscriber (%s)" % target.displayname
+        if person.isTeam():
+            text = ("are a member of %s, which is subscribed to %s" %
+                (person.displayname, target.displayname))
+            reason += " @%s" % person.name
+        else:
+            text = "are subscribed to %s" % target.displayname
+        self._addReason(person, text, reason)
+
+    def addUpstreamBugSupervisor(self, person, upstream):
+        """Registers an upstream bug supervisor for this bug."""
+        reason = "Bug Supervisor (%s)" % upstream.displayname
+        if person.isTeam():
+            text = ("are a member of %s, which is the bug supervisor for %s" %
+                (person.displayname, upstream.displayname))
+            reason += " @%s" % person.name
+        else:
+            text = "are the bug supervisor for %s" % upstream.displayname
+        self._addReason(person, text, reason)
+
+    def addRegistrant(self, person, upstream):
+        """Registers an upstream product registrant for this bug."""
+        reason = "Registrant (%s)" % upstream.displayname
+        if person.isTeam():
+            text = ("are a member of %s, which is the registrant for %s" %
+                (person.displayname, upstream.displayname))
+            reason += " @%s" % person.name
+        else:
+            text = "are the registrant for %s" % upstream.displayname
+        self._addReason(person, text, reason)
+
+
+
 
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-06-10 18:55:22 +0000
+++ lib/lp/bugs/model/bug.py	2010-06-16 08:38:25 +0000
@@ -58,7 +58,7 @@
     IMessage, IndexedMessage)
 from lp.registry.interfaces.structuralsubscription import (
     BugNotificationLevel, IStructuralSubscriptionTarget)
-from canonical.launchpad.mailnotification import BugNotificationRecipients
+from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
 from canonical.launchpad.validators import LaunchpadValidationError
 from canonical.launchpad.webapp.interfaces import (
     IStoreSelector, DEFAULT_FLAVOR, MAIN_STORE, NotFoundError)
 
=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py	2010-01-12 21:12:12 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2010-06-16 08:38:25 +0000
@@ -21,7 +21,7 @@
 from lp.bugs.interfaces.bug import IBug, IBugSet
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
-from canonical.launchpad.mailnotification import BugNotificationRecipients
+from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
 from lp.bugs.scripts.bugnotification import (
     get_email_notifications)
 from canonical.testing import LaunchpadZopelessLayer
 
=== modified file 'lib/lp/registry/doc/structural-subscriptions.txt'
--- lib/lp/registry/doc/structural-subscriptions.txt	2010-02-20 14:16:37 +0000
+++ lib/lp/registry/doc/structural-subscriptions.txt	2010-06-16 08:38:25 +0000
@@ -86,7 +86,7 @@
     >>> from canonical.launchpad.ftests import syncUpdate
     >>> from canonical.launchpad.interfaces import (
     ...     BlueprintNotificationLevel, BugNotificationLevel)
-    >>> from canonical.launchpad.mailnotification import (
+    >>> from lp.bugs.mail.bugnotificationrecipients import (
     ...     BugNotificationRecipients)
 
 We define some utility functions for printing out bug subscriptions and