Launchpad itself

Merge lp://staging/~sinzui/launchpad/question-email-1 into lp://staging/launchpad

question-email-1
Merge into devel

Proposed by Curtis Hovey on 2011-04-28

Status:

Merged

Approved by:

Curtis Hovey on 2011-04-28

Approved revision:

no longer in the source branch.

Merged at revision:

12949

Proposed branch:

lp://staging/~sinzui/launchpad/question-email-1

Merge into:

lp://staging/launchpad

Diff against target:

513 lines (+308/-28)

5 files modified

lib/lp/answers/enums.py (+30/-0)
lib/lp/answers/interfaces/questionjob.py (+16/-7)
lib/lp/answers/model/question.py (+3/-1)
lib/lp/answers/model/questionjob.py (+69/-4)
lib/lp/answers/tests/test_questionjob.py (+190/-16)

To merge this branch:

bzr merge lp://staging/~sinzui/launchpad/question-email-1

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
j.c.sackett (community)		2011-04-28	Approve on 2011-04-28
Review via email: mp+59414@code.staging.launchpad.net

Description of the change

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.

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

Revision history for this message

j.c.sackett (jcsackett) wrote on 2011-04-28:

Looks good to land.

I have a minor quibble in a comment (that I guess I missed when reviewing your previous branch.

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.

That param should be ":param subject: The user of the email." Looks like just a minor typo.

This is *obviously* not a blocker to land. :-P

review: Approve