launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03427
[Merge] lp:~sinzui/launchpad/question-recipients-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/question-recipients-0 into lp:launchpad with lp:~sinzui/launchpad/bugtask-create-question-1 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #185427 in Launchpad itself: "Question emails must explain you are the asker"
https://bugs.launchpad.net/launchpad/+bug/185427
Bug #769160 in Launchpad itself: "Question.get*Recipients could be cached"
https://bugs.launchpad.net/launchpad/+bug/769160
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/question-recipients-0/+merge/58858
Question email performance and reason improvements.
Launchpad bug:
https://bugs.launchpad.net/bugs/769160
https://bugs.launchpad.net/bugs/185427
Pre-implementation: jcsackett
bug #769160 "Question.get*Recipients could be cached"
There are several bugs tracking timeouts caused by sending emails. While
we want to send emails out of proc, there is an immediate benefit to
reducing the cost of repetitive queries building the recipients. All the
call sites of the recipient methods are working with static data; is
cannot change once the set is built. All the callsites use this data after
the subscriber set could have changed. Changing the methods to cached
properties will benefit BugTask :+create-question and presumably will
benefit a job system that sends multiple messages for a single question.
bug #185427 "Question emails must explain you are the asker"
The question owner is ambiguously described as a subscriber. This looked
odd in the tests I was reading while adding calls to clear the cache.
I suck, I wrote code that describes the question asker as just a
subscriber. I even found a bug that tried to explain the ambiguity.
--------------------------------------------------------------------
RULES
bug #769160 "Question.get*Recipients could be cached"
* Change getDirectRecipients and getIndirectRecipients to
cached properties: get_direct_recipients and get_indirect_recipients
bug #185427 "Question emails must explain you are the asker"
* Update getDirectRecipients to replace the default reason for the
owner with a specific one.
QA
bug #769160 "Question.get*Recipients could be cached"
* None, though converting a bug to a question will suffice as a
sanity check to verify that emails are still sent.
bug #185427 "Question emails must explain you are the asker"
* Ask a question.
* Verify the email footer states you got the email because
"you asked the question"
* Verify the X-Launchpad-Message-Rationale states you are the Asker.
LINT
lib/lp/answers/configure.zcml
lib/lp/answers/notification.py
lib/lp/answers/doc/notifications.txt
lib/lp/answers/doc/question.txt
lib/lp/answers/interfaces/question.py
lib/lp/answers/model/question.py
TEST
./bin/test -vv -t doc/question.txt -t answers.*doc/notifications.txt
IMPLEMENTATION
Changed getDirectRecipients and getIndirectRecipients to cached properties:
get_direct_recipients and get_indirect_recipients. Updated notification.py
to reuse the cached recipients. Updated the question test to clear the
cache as needed.
lib/lp/answers/configure.zcml
lib/lp/answers/interfaces/question.py
lib/lp/answers/model/question.py
lib/lp/answers/notification.py
lib/lp/answers/doc/question.txt
Updated get_direct_recipients to state that the question.owner is the
asker. Lots of mechanical test changes followed. You can see some of the
narrative in the diff that clearly purport the question owner was merely
described as a subscriber :(
lib/lp/answers/model/question.py
lib/lp/answers/doc/question.txt
lib/lp/answers/doc/notifications.txt
--
https://code.launchpad.net/~sinzui/launchpad/question-recipients-0/+merge/58858
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/question-recipients-0 into lp:launchpad.
=== modified file 'lib/lp/answers/configure.zcml'
--- lib/lp/answers/configure.zcml 2011-03-23 18:59:10 +0000
+++ lib/lp/answers/configure.zcml 2011-04-22 22:01:36 +0000
@@ -89,7 +89,7 @@
can_request_info can_give_info can_give_answer
can_confirm_answer can_reopen
subscriptions isSubscribed getRecipients
- getDirectRecipients getIndirectRecipients
+ get_direct_recipients get_indirect_recipients
getDirectSubscribers getIndirectSubscribers"
/>
<require
=== modified file 'lib/lp/answers/doc/notifications.txt'
--- lib/lp/answers/doc/notifications.txt 2011-04-22 22:01:35 +0000
+++ lib/lp/answers/doc/notifications.txt 2011-04-22 22:01:36 +0000
@@ -64,8 +64,7 @@
I insert the install CD in the CD-ROM drive, but it won't boot.
<BLANKLINE>
--...
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
The notification also includes a 'X-Launchpad-Question' header that
contains information about the question.
@@ -78,7 +77,7 @@
contains in short format the reason for the user to be contacted.
>>> print add_notification['X-Launchpad-Message-Rationale']
- Subscriber
+ Asker
Register the Ubuntu Team as Ubuntu's answer contact, so that they get
notified about the changes as well:
@@ -361,8 +360,7 @@
this email or enter your reply at the following page:
http://.../ubuntu/+question/...
<BLANKLINE>
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
Of course, if the owner unsubscribe from the question, he won't receives
a notification.
@@ -454,8 +452,7 @@
entering more information about your problem:
http://.../ubuntu/+question/...
<BLANKLINE>
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
Notifications for reopen()
@@ -521,8 +518,7 @@
Please provide some help to a newbie.
<BLANKLINE>
--...
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
Notifications for giveAnswer()
@@ -588,8 +584,7 @@
following page to enter your feedback:
http://.../ubuntu/+question/...
<BLANKLINE>
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
Notifications for confirm()
@@ -633,8 +628,7 @@
I've installed BootX and the installer CD is now booting. Thanks!
<BLANKLINE>
--...
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
Notifications for addComment()
@@ -676,8 +670,7 @@
be very slow.
<BLANKLINE>
--...
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
Notifications for reject()
@@ -726,8 +719,7 @@
the following page:
http://.../ubuntu/+question/...
<BLANKLINE>
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
Notifications for setStatus()
@@ -771,8 +763,7 @@
The rejection was a mistake.
<BLANKLINE>
--...
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
Notifications for linkFAQ()
@@ -1021,8 +1012,7 @@
spoken by none of the registered Ubuntu answer contacts.
<BLANKLINE>
--...
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
No notification will be sent to the answer contacts when this question
is modified. Only the owner will receive a modification notification
@@ -1050,8 +1040,7 @@
spoken by none of the registered Ubuntu answer contacts.
<BLANKLINE>
--...
- You received this question notification because you are a direct
- subscriber of the question.
+ You received this question notification because you asked the question.
Localized Questions and Teams
=== modified file 'lib/lp/answers/doc/question.txt'
--- lib/lp/answers/doc/question.txt 2011-03-23 16:28:51 +0000
+++ lib/lp/answers/doc/question.txt 2011-04-22 22:01:36 +0000
@@ -194,7 +194,7 @@
The getDirectSubscribers() method returns a sorted list of subscribers.
This method iterates like the NotificationRecipientSet returned by the
-getDirectRecipients() method.
+get_direct_recipients method.
>>> for person in firefox_question.getDirectSubscribers():
... print person.displayname
@@ -210,10 +210,10 @@
The people on the subscription list are said to be directly subscribed to the
question. They explicitly chose to get notifications about that particular
-question. This list of people is available through the getDirectRecipients()
+question. This list of people is available through the get_direct_recipients
method.
- >>> subscribers = firefox_question.getDirectRecipients()
+ >>> subscribers = firefox_question.get_direct_recipients
That method returns an INotificationRecipientSet, containing the direct
subscribers along with the rationale for contacting them.
@@ -227,8 +227,8 @@
... text, header = subscribers.getReason(person)
... print header, person.displayname, text
>>> print_reason(subscribers)
- Subscriber Sample Person You received this question notification
- because you are a direct subscriber of the question.
+ Asker Sample Person
+ You received this question notification because you asked the question.
There is also a list of 'indirect' subscribers to the question. These are
people that didn't explicitly subscribe to the question, but that will receive
@@ -236,7 +236,7 @@
part of the indirect subscribers list.
# There are no answer contacts on the firefox product.
- >>> list(firefox_question.getIndirectRecipients())
+ >>> list(firefox_question.get_indirect_recipients)
[]
>>> from lp.services.worlddata.interfaces.language import ILanguageSet
@@ -245,7 +245,9 @@
>>> firefox.addAnswerContact(no_priv)
True
- >>> indirect_subscribers = firefox_question.getIndirectRecipients()
+ >>> from lp.services.propertycache import get_property_cache
+ >>> del get_property_cache(firefox_question).get_indirect_recipients
+ >>> indirect_subscribers = firefox_question.get_indirect_recipients
>>> verifyObject(INotificationRecipientSet, indirect_subscribers)
True
>>> print_reason(indirect_subscribers)
@@ -276,7 +278,8 @@
>>> print_subscribers(package_question)
Sample Person
- >>> indirect_subscribers = package_question.getIndirectRecipients()
+ >>> del get_property_cache(firefox_question).get_indirect_recipients
+ >>> indirect_subscribers = package_question.get_indirect_recipients
>>> for person in indirect_subscribers:
... print person.displayname
No Privileges Person
@@ -292,7 +295,8 @@
>>> login('admin@xxxxxxxxxxxxx')
>>> package_question.assignee = getUtility(IPersonSet).getByName('name16')
- >>> indirect_subscribers = package_question.getIndirectRecipients()
+ >>> del get_property_cache(package_question).get_indirect_recipients
+ >>> indirect_subscribers = package_question.get_indirect_recipients
>>> for person in indirect_subscribers:
... print person.displayname
Foo Bar
@@ -306,7 +310,7 @@
You received this question notification because you are the assignee for
this question.
-The getIndirectSubscribers() method iterates like the getIndirectRecipients()
+The getIndirectSubscribers() method iterates like the get_indirect_recipients
method, but it returns a sorted list instead of a NotificationRecipientSet.
It too contains the question assignee.
=== modified file 'lib/lp/answers/interfaces/question.py'
--- lib/lp/answers/interfaces/question.py 2011-04-14 19:37:44 +0000
+++ lib/lp/answers/interfaces/question.py 2011-04-22 22:01:36 +0000
@@ -458,22 +458,13 @@
notify along the rationale for doing so.
"""
- def getDirectRecipients():
- """Return the set of persons who are subscribed to this question.
-
- :return: An `INotificationRecipientSet` containing the persons to
- notify along the rationale for doing so.
- """
-
- def getIndirectRecipients():
- """Return the set of persons implicitly subscribed to this question.
-
- That includes the answer contacts for the question's target as well
- as the question's assignee.
-
- :return: An `INotificationRecipientSet` containing the persons to
- notify along the rationale for doing so.
- """
+ get_direct_recipients = Attribute(
+ "Return An `INotificationRecipientSet` containing the persons to "
+ "notify along the rationale for doing so.")
+
+ get_indirect_recipients = Attribute(
+ "Return the INotificationRecipientSet of answer contacts for the "
+ "question's target as well as the question's assignee.")
@operation_parameters(
comment_number=Int(
=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py 2011-04-22 22:01:35 +0000
+++ lib/lp/answers/model/question.py 2011-04-22 22:01:36 +0000
@@ -67,7 +67,6 @@
from canonical.launchpad.helpers import is_english_variant
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.launchpad.interfaces.message import IMessage
-from canonical.launchpad.mailnotification import NotificationRecipientSet
from lp.answers.interfaces.faq import IFAQ
from lp.answers.interfaces.question import (
InvalidQuestionStateError,
@@ -111,6 +110,8 @@
)
from lp.registry.interfaces.sourcepackage import ISourcePackage
from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
+from lp.services.mail.notificationrecipientset import NotificationRecipientSet
+from lp.services.propertycache import cachedproperty
from lp.services.worlddata.interfaces.language import ILanguage
from lp.services.worlddata.model.language import Language
@@ -535,7 +536,7 @@
def getDirectSubscribers(self):
"""See `IQuestion`.
- This method is sorted so that it iterates like getDirectRecipients().
+ This method is sorted so that it iterates like get_direct_recipients.
"""
return sorted(
self.subscribers, key=operator.attrgetter('displayname'))
@@ -544,7 +545,7 @@
"""See `IQuestion`.
This method adds the assignee and is sorted so that it iterates like
- getIndirectRecipients().
+ get_indirect_recipients.
"""
subscribers = set(
self.target.getAnswerContactsForLanguage(self.language))
@@ -554,19 +555,27 @@
def getRecipients(self):
"""See `IQuestion`."""
- subscribers = self.getDirectRecipients()
- subscribers.update(self.getIndirectRecipients())
+ subscribers = self.get_direct_recipients
+ subscribers.update(self.get_indirect_recipients)
return subscribers
- def getDirectRecipients(self):
+ @cachedproperty
+ def get_direct_recipients(self):
"""See `IQuestion`."""
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')
return subscribers
- def getIndirectRecipients(self):
+ @cachedproperty
+ def get_indirect_recipients(self):
"""See `IQuestion`."""
subscribers = self.target.getAnswerContactRecipients(self.language)
if self.assignee:
=== modified file 'lib/lp/answers/notification.py'
--- lib/lp/answers/notification.py 2011-04-22 22:01:35 +0000
+++ lib/lp/answers/notification.py 2011-04-22 22:01:36 +0000
@@ -439,8 +439,8 @@
"""Return the owner of the question if he's still subscribed."""
recipients = NotificationRecipientSet()
owner = self.question.owner
- if self.question.isSubscribed(owner):
- original_recipients = self.question.getDirectRecipients()
+ original_recipients = self.question.get_direct_recipients
+ if owner in self.question.get_direct_recipients:
rationale, header = original_recipients.getReason(owner)
recipients.add(owner, rationale, header)
return recipients
