Merge lp:~gmb/launchpad/refactor-bugnotificationbuilder-bug-594211-part-2 into lp:launchpad

[Graham Binns]

A BugNotificationRecipients class has been added as part of the work to refactor bug notifications towards using BaseMailer.

BugNotification Refactoring: The saga continues
===============================================

Ah, what fun. This branch is the eventual culmination of several weeks'
development work. It's actually about two days' worth of work - there's
a whole lot of crap that I've had to revert to get tests to pass; I
realise now that my strategy was too ambitious in the first place, so
I've pared this branch down to the simplest it could be whilst still
adding value (shame I couldn't have hear Curtis' Epic talk two weeks
earlier).

Anyway.

This branch adds a BugNotificationRecipientReason class; this is the
next step in refactoring bug notifications to use BaseMailer. The new
class isn't hooked up to anything yet; I'll do that in a subsequent
branch since it seems to break every test *evar* (there are a lot of
tests that rely on notification reasons behaving like strings for some
reason).

I've added unittests to cover the new class.

No lint.

[Abel Deuring (adeuring)]

nice work!

review: Approve (code)

[Robert Collins (lifeless)]

That looks ok, but I wonder if it would be better using a factory
approach rather than class method helpers : it smells to me like you
are setting yourself up for an if:elif:elif:else: scenario, which is
painful and generally leads to bugs.

-Rob

[Graham Binns (gmb)]

On 22 July 2010 20:46, Robert Collins <email address hidden> wrote:
> That looks ok, but I wonder if it would be better using a factory
> approach rather than class method helpers : it smells to me like you
> are setting yourself up for an if:elif:elif:else: scenario, which is
> painful and generally leads to bugs.

Possibly; I was following the pattern that's used elsewhere by
RecipientReason descendants, but there's no guarantee that that's the
Right Way to do things.

When I return from leave I'll be working on some pretty heavy
refactoring; I suspect I'll find out then whether this is the wrong
approach and will refactor accordingly.

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2010-07-22 12:17:41 +0000
+++ lib/lp/bugs/configure.zcml	2010-07-22 19:14:44 +0000
@@ -981,4 +981,12 @@
     <class class="lp.bugs.model.bug.FileBugData">
         <allow interface="lp.bugs.interfaces.bug.IFileBugData" />
     </class>
+
+  <class class="lp.bugs.mail.bugnotificationrecipients.BugNotificationRecipientReason">
+    <allow attributes="
+        bug
+        getReason
+        mail_header
+        recipient"/>
+  </class>
 </configure>

=== modified file 'lib/lp/bugs/mail/bugnotificationrecipients.py'
--- lib/lp/bugs/mail/bugnotificationrecipients.py	2010-07-09 19:41:05 +0000
+++ lib/lp/bugs/mail/bugnotificationrecipients.py	2010-07-22 19:14:44 +0000
@@ -12,10 +12,111 @@
 
 from canonical.launchpad.interfaces import INotificationRecipientSet
 
+from lp.services.mail.basemailer import RecipientReason
 from lp.services.mail.notificationrecipientset import (
     NotificationRecipientSet)
 
 
+class BugNotificationRecipientReason(RecipientReason):
+    """A `RecipientReason` subsclass specifically for `BugNotification`s."""
+
+    def _getTemplateValues(self):
+        template_values = (
+            super(BugNotificationRecipientReason, self)._getTemplateValues())
+        if self.recipient != self.subscriber or self.subscriber.is_team:
+            template_values['entity_is'] = (
+                'You are a member of %s, which is' %
+                self.subscriber.displayname)
+            template_values['lc_entity_is'] = (
+                'you are a member of %s, which is' %
+                self.subscriber.displayname)
+        return template_values
+
+    @classmethod
+    def makeRationale(cls, rationale, person, duplicate_bug=None):
+        """See `RecipientReason.makeRationale`."""
+        rationale = RecipientReason.makeRationale(rationale, person)
+        if duplicate_bug is not None:
+            rationale = "%s via Bug %s" % (rationale, duplicate_bug.id)
+
+        return rationale
+
+    @classmethod
+    def _getReasonTemplate(cls, reason_string, duplicate_bug=None):
+        """Return a reason template to pass to __init__()."""
+        if duplicate_bug is not None:
+            reason_suffix = " (via bug %s)." % duplicate_bug.id
+        else:
+            reason_suffix = "."
+
+        reason_parts = {
+            'prefix':
+                "You received this bug notification because %(lc_entity_is)s",
+            'reason': reason_string,
+            'suffix': reason_suffix,
+            }
+        return "%(prefix)s %(reason)s%(suffix)s" % reason_parts
+
+    @classmethod
+    def forDupeSubscriber(cls, person, duplicate_bug):
+        """Return a `BugNotificationRecipientReason` for a dupe subscriber.
+        """
+        header = cls.makeRationale(
+            'Subscriber of Duplicate', person, duplicate_bug)
+
+        reason = cls._getReasonTemplate(
+            "a direct subscriber of a duplicate bug (via bug %s)" %
+                duplicate_bug.id)
+        return cls(person, person, header, reason)
+
+    @classmethod
+    def forDirectSubscriber(cls, person, duplicate_bug=None):
+        """Return a `BugNotificationRecipientReason` for a direct subscriber.
+        """
+        header = cls.makeRationale("Subscriber", person, duplicate_bug)
+        reason = cls._getReasonTemplate(
+            "a direct subscriber of the bug", duplicate_bug)
+        return cls(person, person, header, reason)
+
+    @classmethod
+    def forAssignee(cls, person, duplicate_bug=None):
+        """Return a `BugNotificationRecipientReason` for a bug assignee."""
+        header = cls.makeRationale("Assignee", person, duplicate_bug)
+        reason = cls._getReasonTemplate("a bug assignee", duplicate_bug)
+        return cls(person, person, header, reason)
+
+    @classmethod
+    def forBugSupervisor(cls, person, target, duplicate_bug=None):
+        """Return a `BugNotificationRecipientReason` for a bug supervisor."""
+        # 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.
+        header = cls.makeRationale(
+            "Bug Supervisor (%s)" % target.displayname, person, duplicate_bug)
+        reason = cls._getReasonTemplate(
+            "the bug supervisor for %s" % target.displayname, duplicate_bug)
+        return cls(person, person, header, reason)
+
+    @classmethod
+    def forStructuralSubscriber(cls, person, target, duplicate_bug=None):
+        """Return a recipient reason for a structural subscriber."""
+        header = cls.makeRationale(
+            "Subscriber (%s)" % target.displayname, person, duplicate_bug)
+        reason = cls._getReasonTemplate(
+            "subscribed to %s" % target.displayname, duplicate_bug)
+        return cls(person, person, header, reason)
+
+    @classmethod
+    def forRegistrant(cls, person, target, duplicate_bug=None):
+        """Return a recipient reason for a registrant."""
+        header = cls.makeRationale(
+            "Registrant (%s)" % target.displayname, person, duplicate_bug)
+        reason = cls._getReasonTemplate(
+            "the registrant for %s" % target.displayname, duplicate_bug)
+        return cls(person, person, header, reason)
+
+
 class BugNotificationRecipients(NotificationRecipientSet):
     """A set of emails and rationales notified for a bug change.
 
@@ -155,6 +256,3 @@
         else:
             text = "are the registrant for %s" % upstream.displayname
         self._addReason(person, text, reason)
-
-
-

=== added file 'lib/lp/bugs/mail/tests/test_bugnotificationrecipients.py'
--- lib/lp/bugs/mail/tests/test_bugnotificationrecipients.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/mail/tests/test_bugnotificationrecipients.py	2010-07-22 19:14:44 +0000
@@ -0,0 +1,189 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the bugnotificationrecipients module."""
+
+__metaclass__ = type
+
+import unittest
+
+from canonical.testing import DatabaseFunctionalLayer
+
+from lp.bugs.mail.bugnotificationrecipients import (
+    BugNotificationRecipientReason)
+from lp.testing import TestCaseWithFactory
+
+
+class BugNotificationRecipientReasonTestCase(TestCaseWithFactory):
+    """A TestCase for the `BugNotificationRecipientReason` class."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(BugNotificationRecipientReasonTestCase, self).setUp()
+        self.person = self.factory.makePerson()
+        self.team = self.factory.makeTeam(owner=self.person)
+
+    def _assertReasonAndHeaderAreCorrect(self, recipient_reason,
+                                         expected_reason, expected_header):
+        self.assertEqual(expected_header, recipient_reason.mail_header)
+        self.assertEqual(expected_reason, recipient_reason.getReason())
+
+    def test_forDupeSubscriber(self):
+        duplicate_bug = self.factory.makeBug()
+        reason = BugNotificationRecipientReason.forDupeSubscriber(
+            self.person, duplicate_bug)
+
+        expected_header = (
+            'Subscriber of Duplicate via Bug %s' % duplicate_bug.id)
+        expected_reason = (
+            'You received this bug notification because you are a direct '
+            'subscriber of a duplicate bug (via bug %s).' % duplicate_bug.id)
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forDupeSubscriber_for_team(self):
+        duplicate_bug = self.factory.makeBug()
+        reason = BugNotificationRecipientReason.forDupeSubscriber(
+            self.team, duplicate_bug)
+
+        expected_header = 'Subscriber of Duplicate @%s via Bug %s' % (
+            self.team.name, duplicate_bug.id)
+        expected_reason = (
+            'You received this bug notification because you are a member of '
+            '%s, which is a direct subscriber of a duplicate bug (via bug '
+            '%s).' %
+                (self.team.displayname, duplicate_bug.id))
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forDirectSubscriber(self):
+        reason = BugNotificationRecipientReason.forDirectSubscriber(
+            self.person)
+
+        expected_header = "Subscriber"
+        expected_reason = (
+            "You received this bug notification because you are a direct "
+            "subscriber of the bug.")
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forDirectSubscriber_for_team(self):
+        reason = BugNotificationRecipientReason.forDirectSubscriber(
+            self.team)
+
+        expected_header = "Subscriber @%s" % self.team.name
+        expected_reason = (
+            "You received this bug notification because you are a member "
+            "of %s, which is a direct subscriber of the bug." %
+                self.team.displayname)
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forAssignee(self):
+        reason = BugNotificationRecipientReason.forAssignee(self.person)
+
+        expected_header = "Assignee"
+        expected_reason = (
+            "You received this bug notification because you are a bug "
+            "assignee.")
+
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forAssignee_for_team(self):
+        reason = BugNotificationRecipientReason.forAssignee(self.team)
+
+        expected_header = "Assignee @%s" % self.team.name
+        expected_reason = (
+            "You received this bug notification because you are a member "
+            "of %s, which is a bug assignee." % self.team.displayname)
+
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forBugSupervisor(self):
+        distro = self.factory.makeDistribution()
+        reason = BugNotificationRecipientReason.forBugSupervisor(
+            self.person, distro)
+
+        expected_header = "Bug Supervisor (%s)" % distro.displayname
+        expected_reason = (
+            "You received this bug notification because you are the bug "
+            "supervisor for %s." % distro.displayname)
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forBugSupervisor_for_team(self):
+        distro = self.factory.makeDistribution()
+        reason = BugNotificationRecipientReason.forBugSupervisor(
+            self.team, distro)
+
+        expected_header = "Bug Supervisor (%s) @%s" % (
+            distro.displayname, self.team.name)
+        expected_reason = (
+            "You received this bug notification because you are a member "
+            "of %s, which is the bug supervisor for %s." %
+                (self.team.displayname, distro.displayname))
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forStructuralSubscriber(self):
+        target = self.factory.makeProduct()
+        reason = BugNotificationRecipientReason.forStructuralSubscriber(
+            self.person, target)
+
+        expected_header = "Subscriber (%s)" % target.displayname
+        expected_reason = (
+            "You received this bug notification because you are subscribed "
+            "to %s." % target.displayname)
+
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forStructuralSubscriber_for_team(self):
+        target = self.factory.makeProduct()
+        reason = BugNotificationRecipientReason.forStructuralSubscriber(
+            self.team, target)
+
+        expected_header = "Subscriber (%s) @%s" % (
+            target.displayname, self.team.name)
+        expected_reason = (
+            "You received this bug notification because you are a member "
+            "of %s, which is subscribed to %s." %
+                (self.team.displayname, target.displayname))
+
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forRegistrant(self):
+        target = self.factory.makeProduct()
+        reason = BugNotificationRecipientReason.forRegistrant(
+            self.person, target)
+
+        expected_header = "Registrant (%s)" % target.displayname
+        expected_reason = (
+            "You received this bug notification because you are the "
+            "registrant for %s." % target.displayname)
+
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+    def test_forRegistrant_for_team(self):
+        target = self.factory.makeProduct()
+        reason = BugNotificationRecipientReason.forRegistrant(
+            self.team, target)
+
+        expected_header = "Registrant (%s) @%s" % (
+            target.displayname, self.team.name)
+        expected_reason = (
+            "You received this bug notification because you are a member "
+            "of %s, which is the registrant for %s." % (
+                self.team.displayname, target.displayname))
+
+        self._assertReasonAndHeaderAreCorrect(
+            reason, expected_reason, expected_header)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)