launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/answers-mail-team into lp:launchpad.

Commit message:
Make question notification rationales more consistent, including team annotations for subscribers.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #968578 in Launchpad itself: "Unable to figure out why I got mail about a question"
  https://bugs.launchpad.net/launchpad/+bug/968578

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/answers-mail-team/+merge/269091

Make question notification rationales more consistent, including team annotations for subscribers.

This isn't a full conversion to BaseMailer, but I made use of RecipientReason from there because it handles all the rationale logic for teams.

As a bonus, this fixes an inconsistency that apparently nobody had noticed before, in which mail to answer contacts included the target name in parentheses if the answer contact was a team, but the target display name if the answer contact was a person.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/answers-mail-team into lp:launchpad.

=== modified file 'lib/lp/answers/doc/question.txt'
--- lib/lp/answers/doc/question.txt	2014-04-24 08:21:42 +0000
+++ lib/lp/answers/doc/question.txt	2015-08-25 16:27:40 +0000
@@ -212,7 +212,8 @@
     True
     >>> def print_reason(subscribers):
     ...     for person in subscribers:
-    ...         text, header = subscribers.getReason(person)
+    ...         reason, header = subscribers.getReason(person)
+    ...         text = removeSecurityProxy(reason).getReason()
     ...         print header, person.displayname, text
     >>> print_reason(subscribers)
     Asker Sample Person
@@ -239,7 +240,7 @@
     >>> verifyObject(INotificationRecipientSet, indirect_subscribers)
     True
     >>> print_reason(indirect_subscribers)
-    Answer Contact (Mozilla Firefox) No Privileges Person
+    Answer Contact (firefox) No Privileges Person
     You received this question notification because you are an answer
     contact for Mozilla Firefox.
 
@@ -273,11 +274,11 @@
     No Privileges Person
     Ubuntu Team
 
-    >>> text, header = indirect_subscribers.getReason(ubuntu_team)
-    >>> print header, text
+    >>> reason, header = indirect_subscribers.getReason(ubuntu_team)
+    >>> print header, removeSecurityProxy(reason).getReason()
     Answer Contact (ubuntu) @ubuntu-team
-    You received this question notification because you are a member of
-    Ubuntu Team, which is an answer contact for Ubuntu.
+    You received this question notification because your team Ubuntu Team is
+    an answer contact for Ubuntu.
 
 The question's assignee is also part of the indirect subscription list:
 
@@ -291,9 +292,9 @@
     No Privileges Person
     Ubuntu Team
 
-    >>> text, header = indirect_subscribers.getReason(
+    >>> reason, header = indirect_subscribers.getReason(
     ...     package_question.assignee)
-    >>> print header, text
+    >>> print header, removeSecurityProxy(reason).getReason()
     Assignee
     You received this question notification because you are the assignee for
     this question.

=== added file 'lib/lp/answers/mail/question.py'
--- lib/lp/answers/mail/question.py	1970-01-01 00:00:00 +0000
+++ lib/lp/answers/mail/question.py	2015-08-25 16:27:40 +0000
@@ -0,0 +1,46 @@
+# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+__all__ = [
+    'QuestionRecipientReason',
+    ]
+
+from lp.services.mail.basemailer import RecipientReason
+
+
+class QuestionRecipientReason(RecipientReason):
+
+    @classmethod
+    def forSubscriber(cls, subscriber, recipient):
+        header = cls.makeRationale("Subscriber", subscriber)
+        reason = (
+            "You received this question notification because "
+            "%(lc_entity_is)s a direct subscriber of the question.")
+        return cls(subscriber, recipient, header, reason)
+
+    @classmethod
+    def forAsker(cls, asker, recipient):
+        header = cls.makeRationale("Asker", asker)
+        reason = (
+            "You received this question notification because you asked the "
+            "question.")
+        return cls(asker, recipient, header, reason)
+
+    @classmethod
+    def forAssignee(cls, assignee, recipient):
+        header = cls.makeRationale("Assignee", assignee)
+        reason = (
+            "You received this question notification because "
+            "%(lc_entity_is)s the assignee for this question.")
+        return cls(assignee, recipient, header, reason)
+
+    @classmethod
+    def forAnswerContact(cls, answer_contact, recipient, target_name,
+                         target_displayname):
+        header = cls.makeRationale(
+            "Answer Contact (%s)" % target_name, answer_contact)
+        reason = (
+            "You received this question notification because "
+            "%%(lc_entity_is)s an answer contact for %s." % target_displayname)
+        return cls(answer_contact, recipient, header, reason)

=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py	2015-07-08 16:05:11 +0000
+++ lib/lp/answers/model/question.py	2015-08-25 16:27:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Question models."""
@@ -66,6 +66,7 @@
 from lp.answers.interfaces.question import IQuestion
 from lp.answers.interfaces.questioncollection import IQuestionSet
 from lp.answers.interfaces.questiontarget import IQuestionTarget
+from lp.answers.mail.question import QuestionRecipientReason
 from lp.answers.model.answercontact import AnswerContact
 from lp.answers.model.questionmessage import QuestionMessage
 from lp.answers.model.questionreopening import create_questionreopening
@@ -586,26 +587,30 @@
     @cachedproperty
     def direct_recipients(self):
         """See `IQuestion`."""
+        # Circular import.
+        from lp.registry.model.person import get_recipients
         subscribers = NotificationRecipientSet()
-        reason = ("You received this question notification because you are "
-                  "a direct subscriber of the question.")
-        subscribers.add(self.subscribers, reason, 'Subscriber')
-        if self.owner in subscribers:
-            subscribers.remove(self.owner)
-            reason = (
-                "You received this question notification because you "
-                "asked the question.")
-            subscribers.add(self.owner, reason, 'Asker')
+        for subscriber in self.subscribers:
+            if subscriber == self.owner:
+                reason_factory = QuestionRecipientReason.forAsker
+            else:
+                reason_factory = QuestionRecipientReason.forSubscriber
+            for recipient in get_recipients(subscriber):
+                reason = reason_factory(subscriber, recipient)
+                subscribers.add(recipient, reason, reason.mail_header)
         return subscribers
 
     @cachedproperty
     def indirect_recipients(self):
         """See `IQuestion`."""
+        # Circular import.
+        from lp.registry.model.person import get_recipients
         subscribers = self.target.getAnswerContactRecipients(self.language)
         if self.assignee:
-            reason = ('You received this question notification because you '
-                      'are the assignee for this question.')
-            subscribers.add(self.assignee, reason, 'Assignee')
+            for recipient in get_recipients(self.assignee):
+                reason = QuestionRecipientReason.forAssignee(
+                    self.assignee, recipient)
+                subscribers.add(recipient, reason, reason.mail_header)
         return subscribers
 
     def _newMessage(self, owner, content, action, new_status, subject=None,
@@ -1423,24 +1428,18 @@
 
     def getAnswerContactRecipients(self, language):
         """See `IQuestionTarget`."""
+        # Circular import.
+        from lp.registry.model.person import get_recipients
         if language is None:
             contacts = self.answer_contacts
         else:
             contacts = self.getAnswerContactsForLanguage(language)
         recipients = NotificationRecipientSet()
-        for person in contacts:
-            reason_start = (
-                "You received this question notification because you are ")
-            if person.is_team:
-                reason = reason_start + (
-                    'a member of %s, which is an answer contact for %s.' % (
-                        person.displayname, self.displayname))
-                header = 'Answer Contact (%s) @%s' % (self.name, person.name)
-            else:
-                reason = reason_start + (
-                    'an answer contact for %s.' % self.displayname)
-                header = 'Answer Contact (%s)' % self.displayname
-            recipients.add(person, reason, header)
+        for contact in contacts:
+            for recipient in get_recipients(contact):
+                reason = QuestionRecipientReason.forAnswerContact(
+                    contact, recipient, self.name, self.displayname)
+                recipients.add(recipient, reason, reason.mail_header)
         return recipients
 
     def removeAnswerContact(self, person, subscribed_by):

=== modified file 'lib/lp/answers/model/questionjob.py'
--- lib/lp/answers/model/questionjob.py	2015-07-09 20:06:17 +0000
+++ lib/lp/answers/model/questionjob.py	2015-08-25 16:27:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Job classes related to QuestionJob."""
@@ -193,14 +193,18 @@
             recipients = NotificationRecipientSet()
             owner = self.question.owner
             original_recipients = self.question.direct_recipients
-            if owner in original_recipients:
-                rationale, header = original_recipients.getReason(owner)
-                recipients.add(owner, rationale, header)
+            for recipient in original_recipients:
+                reason, header = original_recipients.getReason(recipient)
+                if reason.subscriber == owner:
+                    recipients.add(recipient, reason, header)
             return recipients
         elif question_recipient_set == QuestionRecipientSet.SUBSCRIBER:
             recipients = self.question.getRecipients()
-            if self.question.owner in recipients:
-                recipients.remove(self.question.owner)
+            owner = self.question.owner
+            asker_recipients = [
+                recipient for recipient in recipients
+                if recipients.getReason(recipient)[0].subscriber == owner]
+            recipients.remove(asker_recipients)
             return recipients
         elif question_recipient_set == QuestionRecipientSet.ASKER_SUBSCRIBER:
             return self.question.getRecipients()
@@ -230,9 +234,9 @@
         headers = self.headers
         recipients = self.recipients
         for email in recipients.getEmails():
-            rationale, header = recipients.getReason(email)
+            reason, header = recipients.getReason(email)
             headers['X-Launchpad-Message-Rationale'] = header
-            formatted_body = self.buildBody(rationale)
+            formatted_body = self.buildBody(reason.getReason())
             simple_sendmail(
                 self.from_address, email, self.subject, formatted_body,
                 headers)