launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03476
[Merge] lp:~sinzui/launchpad/question-email-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/question-email-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/question-email-1/+merge/59414
Update QuestionJob to send emails.
Launchpad bug: https://bugs.launchpad.net/bugs/772363
Pre-implementation: jcsackett, lifeless
Sending email is the primary reason question pages time out. The emails can be
queued, and the Lp job process could send them. The job wants the common
message and common headers. The job will get the question recipients, build
the message by adding the recipient's reason footer and header, then send it.
This branch copies and reimplements the send email rules from
QuestionNotification.
--------------------------------------------------------------------
RULES
Extend the basic job to do the work of sending emails to recipients
* Implement the run() method based on QuestionNotification.send()
* Added the helper methods as used by QuestionNotification if
the job does not provide them.
* Add rules to handle the four kinds of recipient sets required by
QuestionNotification.
QA
None. This is an incremental branch for unreachable code.
LINT
lib/lp/answers/enums.py
lib/lp/answers/interfaces/questionjob.py
lib/lp/answers/model/question.py
lib/lp/answers/model/questionjob.py
lib/lp/answers/tests/test_questionjob.py
TEST
./bin/test -vv -test_questionjob
IMPLEMENTATION
Testing revealed that removing the owner mutated the subscriber set, then
re-reading the code, it was clear that the question.direct_recipients set
was mutated before the call. We want to cache/freeze the direct and indirect
recipient sets, while allowing call sites to manipulate them.
lib/lp/answers/model/question.py
Added an enum to describe the four kinds of recipient sets that
QuestionNotification uses.
lib/lp/answers/enums.py
Revised the create() because QuestionNotification need to state which
recipient set gets the email.
Added/updated run, from_address, recipients, and buildBody, all based on the
existing code from QuestionNotification. Some of the tests were copied too,
but the QuestionNotification code and tests cannot be removed until its
run() method is changed to queue().
lib/lp/answers/interfaces/questionjob.py
lib/lp/answers/model/questionjob.py
lib/lp/answers/tests/test_questionjob.py
--
https://code.launchpad.net/~sinzui/launchpad/question-email-1/+merge/59414
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/question-email-1 into lp:launchpad.
=== modified file 'lib/lp/answers/enums.py'
--- lib/lp/answers/enums.py 2011-04-26 19:20:47 +0000
+++ lib/lp/answers/enums.py 2011-04-28 19:26:29 +0000
@@ -14,6 +14,7 @@
'QuestionJobType',
'QuestionParticipation',
'QuestionPriority',
+ 'QuestionRecipientSet',
'QUESTION_STATUS_DEFAULT_SEARCH',
'QuestionSort',
'QuestionStatus',
@@ -104,6 +105,35 @@
""")
+class QuestionRecipientSet(EnumeratedType):
+ """The kinds of recipients who will receive notification."""
+
+ ASKER = Item("""
+ Asker
+
+ The person who asked the question.
+ """)
+
+ SUBSCRIBER = Item("""
+ Subscriber
+
+ The question's direct and indirect subscribers, exception for
+ the asker.
+ """)
+
+ ASKER_SUBSCRIBER = Item("""
+ Asker and Subscriber
+
+ The question's direct and indirect subscribers, including the asker.
+ """)
+
+ CONTACT = Item("""
+ Contact
+
+ All the answer contacts for the question's target.
+ """)
+
+
class QuestionParticipation(EnumeratedType):
"""The different ways a person can be involved in a question.
=== modified file 'lib/lp/answers/interfaces/questionjob.py'
--- lib/lp/answers/interfaces/questionjob.py 2011-04-27 17:11:02 +0000
+++ lib/lp/answers/interfaces/questionjob.py 2011-04-28 19:26:29 +0000
@@ -56,22 +56,31 @@
subject = Attribute('The subject of the email.')
body = Attribute(
- 'The body of the email that is common to all recpients.')
+ 'The body of the email that is common to all recipients.')
headers = Attribute(
- 'The headers of the email that are common to all recpients.')
+ 'The headers of the email that are common to all recipients.')
+
+ from_address = Attribute(
+ 'The formatted email address for the user and question.')
+
+ recipients = Attribute('The recipient of the email.')
+
+ def buildBody(rationale):
+ """Return the formatted email body with the rationale included."""
class IQuestionEmailJobSource(IJobSource):
"""An interface for acquiring IQuestionJob."""
- def create(question, user, subject, body, headers):
+ def create(question, user, recipient_set, subject, body, headers):
"""Create a new IQuestionJob.
:param question: An `IQuestion`.
:param user: An `IPerson`.
+ :param recipient_set: A `QuestionRecipientSet`.
:param subject: A'The subject of the email.
- :param body: The text of the email that is common to all recpients.
- :parma headers: A dict of headers for the email that are common to
- all recpients.
+ :param body: The text of the email that is common to all recipients.
+ :param headers: A dict of headers for the email that are common to
+ all recipients.
"""
=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py 2011-04-27 13:59:57 +0000
+++ lib/lp/answers/model/question.py 2011-04-28 19:26:29 +0000
@@ -553,7 +553,9 @@
def getRecipients(self):
"""See `IQuestion`."""
- subscribers = self.direct_recipients
+ # return a mutable instance of the cached recipients.
+ subscribers = NotificationRecipientSet()
+ subscribers.update(self.direct_recipients)
subscribers.update(self.indirect_recipients)
return subscribers
=== modified file 'lib/lp/answers/model/questionjob.py'
--- lib/lp/answers/model/questionjob.py 2011-04-27 15:40:19 +0000
+++ lib/lp/answers/model/questionjob.py 2011-04-28 19:26:29 +0000
@@ -25,12 +25,20 @@
from lazr.delegates import delegates
+from canonical.config import config
from canonical.database.enumcol import EnumCol
from canonical.launchpad.interfaces.lpstorm import (
IMasterStore,
)
+from canonical.launchpad.mail import (
+ format_address,
+ simple_sendmail,
+ )
from canonical.launchpad.scripts import log
-from lp.answers.enums import QuestionJobType
+from lp.answers.enums import (
+ QuestionJobType,
+ QuestionRecipientSet,
+ )
from lp.answers.interfaces.questionjob import (
IQuestionJob,
IQuestionEmailJob,
@@ -41,6 +49,8 @@
from lp.services.database.stormbase import StormBase
from lp.services.job.model.job import Job
from lp.services.job.runner import BaseRunnableJob
+from lp.services.mail.mailwrapper import MailWrapper
+from lp.services.mail.notificationrecipientset import NotificationRecipientSet
from lp.services.propertycache import cachedproperty
@@ -101,10 +111,11 @@
class_job_type = QuestionJobType.EMAIL
@classmethod
- def create(cls, question, user, subject, body, headers):
+ def create(cls, question, user, recipient_set, subject, body, headers):
"""See `IQuestionJob`."""
metadata = {
'user': user.id,
+ 'recipient_set': recipient_set.name,
'subject': subject,
'body': body,
'headers': headers,
@@ -161,12 +172,66 @@
"""See `IRunnableJob`."""
return self.user
+ @property
+ def from_address(self):
+ """See `IQuestionEmailJob`."""
+ address = 'question%s@%s' % (
+ self.question.id, config.answertracker.email_domain)
+ return format_address(self.user.displayname, address)
+
+ @property
+ def recipients(self):
+ """See `IQuestionEmailJob`."""
+ term = QuestionRecipientSet.getTermByToken(
+ self.metadata['recipient_set'])
+ question_recipient_set = term.value
+ if question_recipient_set == QuestionRecipientSet.ASKER:
+ 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)
+ return recipients
+ elif question_recipient_set == QuestionRecipientSet.SUBSCRIBER:
+ recipients = self.question.getRecipients()
+ if self.question.owner in recipients:
+ recipients.remove(self.question.owner)
+ return recipients
+ elif question_recipient_set == QuestionRecipientSet.ASKER_SUBSCRIBER:
+ return self.question.getRecipients()
+ elif question_recipient_set == QuestionRecipientSet.CONTACT:
+ return self.question.target.getAnswerContactRecipients(None)
+ else:
+ raise ValueError(
+ 'Unsupported QuestionRecipientSet value: %s' %
+ question_recipient_set)
+
+ def buildBody(self, rationale):
+ """See `IQuestionEmailJob`."""
+ wrapper = MailWrapper()
+ body_parts = [self.body, wrapper.format(rationale)]
+ if '\n-- ' not in self.body:
+ body_parts.insert(1, '-- ')
+ return '\n'.join(body_parts)
+
def run(self):
- """Send emails."""
+ """See `IRunnableJob`.
+
+ Send emails to all the question recipients.
+ """
log.debug(
"%s will send email for question %s.",
self.log_name, self.question.id)
- # Extract and adapt QuestionNotification.send().
+ headers = self.headers
+ recipients = self.recipients
+ for email in recipients.getEmails():
+ rationale, header = recipients.getReason(email)
+ headers['X-Launchpad-Message-Rationale'] = header
+ formatted_body = self.buildBody(rationale)
+ simple_sendmail(
+ self.from_address, email, self.subject, formatted_body,
+ headers)
log.debug(
"%s has sent email for question %s.",
self.log_name, self.question.id)
=== modified file 'lib/lp/answers/tests/test_questionjob.py'
--- lib/lp/answers/tests/test_questionjob.py 2011-04-27 17:13:41 +0000
+++ lib/lp/answers/tests/test_questionjob.py 2011-04-28 19:26:29 +0000
@@ -5,13 +5,28 @@
__metaclass__ = type
+import transaction
+
+from zope.component import getUtility
+
+from canonical.launchpad.mail import format_address
+from canonical.launchpad.scripts import log
from canonical.testing import DatabaseFunctionalLayer
-from lp.answers.enums import QuestionJobType
+from lp.answers.enums import (
+ QuestionJobType,
+ QuestionRecipientSet,
+ )
from lp.answers.model.questionjob import (
QuestionJob,
QuestionEmailJob,
)
-from lp.testing import TestCaseWithFactory
+from lp.services.log.logger import BufferLogger
+from lp.services.mail import stub
+from lp.services.worlddata.interfaces.language import ILanguageSet
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
class QuestionJobTestCase(TestCaseWithFactory):
@@ -52,17 +67,31 @@
headers = {'X-Launchpad-Question': 'question metadata'}
return user, subject, body, headers
+ def addAnswerContact(self, question):
+ contact = self.factory.makePerson()
+ with person_logged_in(contact):
+ lang_set = getUtility(ILanguageSet)
+ contact.addLanguage(lang_set['en'])
+ question.target.addAnswerContact(contact)
+ return contact
+
def test_create(self):
# The create class method converts the extra job arguments
# to metadata.
question = self.factory.makeQuestion()
user, subject, body, headers = self.makeUserSubjectBodyHeaders()
- job = QuestionEmailJob.create(question, user, subject, body, headers)
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
self.assertEqual(QuestionJobType.EMAIL, job.job_type)
self.assertEqual(question, job.question)
self.assertContentEqual(
- ['body', 'headers', 'subject', 'user'], job.metadata.keys())
+ ['body', 'headers', 'recipient_set', 'subject', 'user'],
+ job.metadata.keys())
self.assertEqual(user.id, job.metadata['user'])
+ self.assertEqual(
+ QuestionRecipientSet.SUBSCRIBER.name,
+ job.metadata['recipient_set'])
self.assertEqual(subject, job.metadata['subject'])
self.assertEqual(body, job.metadata['body'])
self.assertEqual(
@@ -74,11 +103,14 @@
question = self.factory.makeQuestion()
user, subject, ignore, headers = self.makeUserSubjectBodyHeaders()
job_1 = QuestionEmailJob.create(
- question, user, subject, 'one', headers)
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, 'one', headers)
job_2 = QuestionEmailJob.create(
- question, user, subject, 'two', headers)
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, 'two', headers)
job_3 = QuestionEmailJob.create(
- question, user, subject, 'three', headers)
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, 'three', headers)
job_1.start()
job_1.complete()
job_ids = [job.id for job in QuestionEmailJob.iterReady()]
@@ -88,42 +120,66 @@
# The user property matches the user passed to create().
question = self.factory.makeQuestion()
user, subject, body, headers = self.makeUserSubjectBodyHeaders()
- job = QuestionEmailJob.create(question, user, subject, body, headers)
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
self.assertEqual(user, job.user)
def test_subject(self):
# The subject property matches the subject passed to create().
question = self.factory.makeQuestion()
user, subject, body, headers = self.makeUserSubjectBodyHeaders()
- job = QuestionEmailJob.create(question, user, subject, body, headers)
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
self.assertEqual(body, job.body)
def test_body(self):
# The body property matches the body passed to create().
question = self.factory.makeQuestion()
user, subject, body, headers = self.makeUserSubjectBodyHeaders()
- job = QuestionEmailJob.create(question, user, subject, body, headers)
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
self.assertEqual(body, job.body)
def test_headers(self):
# The headers property matches the headers passed to create().
question = self.factory.makeQuestion()
user, subject, body, headers = self.makeUserSubjectBodyHeaders()
- job = QuestionEmailJob.create(question, user, subject, body, headers)
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
self.assertEqual(headers, job.headers)
+ def test_from_address(self):
+ # The from_address is the question with the user displayname.
+ question = self.factory.makeQuestion()
+ user, subject, body, headers = self.makeUserSubjectBodyHeaders()
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
+ address = format_address(
+ user.displayname,
+ "question%s@xxxxxxxxxxxxxxxxxxxxx" % question.id)
+ self.assertEqual(address, job.from_address)
+
def test_log_name(self):
# The log_name property matches the class name.
question = self.factory.makeQuestion()
user, subject, body, headers = self.makeUserSubjectBodyHeaders()
- job = QuestionEmailJob.create(question, user, subject, body, headers)
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
self.assertEqual(job.__class__.__name__, job.log_name)
def test_getOopsVars(self):
# The getOopsVars() method adds the question and user to the vars.
question = self.factory.makeQuestion()
user, subject, body, headers = self.makeUserSubjectBodyHeaders()
- job = QuestionEmailJob.create(question, user, subject, body, headers)
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
oops_vars = job.getOopsVars()
self.assertTrue(('question', question.id) in oops_vars)
self.assertTrue(('user', user.name) in oops_vars)
@@ -132,9 +188,127 @@
# The getErrorRecipients method matches the user.
question = self.factory.makeQuestion()
user, subject, body, headers = self.makeUserSubjectBodyHeaders()
- job = QuestionEmailJob.create(question, user, subject, body, headers)
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
self.assertEqual(user, job.getErrorRecipients())
+ def test_recipients_asker(self):
+ # The recipients property contains the question owner.
+ question = self.factory.makeQuestion()
+ self.addAnswerContact(question)
+ user, subject, body, headers = self.makeUserSubjectBodyHeaders()
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.ASKER,
+ subject, body, headers)
+ recipients = [
+ person for email, person in job.recipients.getRecipientPersons()]
+ self.assertEqual(1, len(recipients))
+ self.assertEqual(question.owner, recipients[0])
+
+ def test_recipients_subscriber(self):
+ # The recipients property matches the question recipients,
+ # excluding the question owner.
+ question = self.factory.makeQuestion()
+ self.addAnswerContact(question)
+ user, subject, body, headers = self.makeUserSubjectBodyHeaders()
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
+ recipients = [
+ person for email, person in job.recipients.getRecipientPersons()]
+ self.assertFalse(question.owner in recipients)
+ question_recipients = [
+ person
+ for em, person in question.getRecipients().getRecipientPersons()
+ if person != question.owner]
+ self.assertContentEqual(
+ question_recipients, recipients)
+
+ def test_recipients_asker_subscriber(self):
+ # The recipients property matches the question recipients.
+ question = self.factory.makeQuestion()
+ self.addAnswerContact(question)
+ user, subject, body, headers = self.makeUserSubjectBodyHeaders()
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.ASKER_SUBSCRIBER,
+ subject, body, headers)
+ self.assertContentEqual(
+ question.getRecipients().getRecipientPersons(),
+ job.recipients.getRecipientPersons())
+
+ def test_recipients_contact(self):
+ # The recipients property matches the question target answer contacts.
+ question = self.factory.makeQuestion()
+ self.addAnswerContact(question)
+ user, subject, body, headers = self.makeUserSubjectBodyHeaders()
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.CONTACT,
+ subject, body, headers)
+ recipients = [
+ person for email, person in job.recipients.getRecipientPersons()]
+ self.assertContentEqual(
+ question.target.getAnswerContactRecipients(None),
+ recipients)
+
+ def test_buildBody_with_separator(self):
+ # A body with a separator is preserved.
+ question = self.factory.makeQuestion()
+ user, subject, body, headers = self.makeUserSubjectBodyHeaders()
+ body = 'body\n-- '
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
+ formatted_body = job.buildBody('rationale')
+ self.assertEqual(
+ 'body\n-- \nrationale', formatted_body)
+
+ def test_buildBody_without_separator(self):
+ # A separator will added to body if one is not present.
+ question = self.factory.makeQuestion()
+ user, subject, body, headers = self.makeUserSubjectBodyHeaders()
+ body = 'body -- mdash'
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
+ formatted_body = job.buildBody('rationale')
+ self.assertEqual(
+ 'body -- mdash\n-- \nrationale', formatted_body)
+
+ def test_buildBody_wrapping(self):
+ # The rationale is wrapped and added to the body.
+ question = self.factory.makeQuestion()
+ user, subject, body, headers = self.makeUserSubjectBodyHeaders()
+ body = 'body\n-- '
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.SUBSCRIBER,
+ subject, body, headers)
+ rationale_1 = (
+ 'You received this email because you are indirectly subscribed '
+ 'to this')
+ rationale_2 = 'question via the ~illuminati team.'
+ rationale = '%s %s' % (rationale_1, rationale_2)
+ formatted_body = job.buildBody(rationale)
+ expected_rationale = '%s\n%s' % (rationale_1, rationale_2)
+ self.assertEqual(
+ body + '\n' + expected_rationale, formatted_body)
+
def test_run(self):
- # The email is sent to all the recipents.
- pass
+ # The email is sent to all the recipients.
+ question = self.factory.makeQuestion()
+ self.addAnswerContact(question)
+ user, subject, body, headers = self.makeUserSubjectBodyHeaders()
+ job = QuestionEmailJob.create(
+ question, user, QuestionRecipientSet.ASKER_SUBSCRIBER,
+ subject, body, headers)
+ logger = BufferLogger()
+ with log.use(logger):
+ job.run()
+ self.assertEqual(
+ ["DEBUG QuestionEmailJob will send email for question %s." %
+ question.id,
+ "DEBUG QuestionEmailJob has sent email for question %s." %
+ question.id],
+ logger.getLogBuffer().splitlines())
+ transaction.commit()
+ self.assertEqual(2, len(stub.test_emails))
