Launchpad itself

Merge lp:~thumper/launchpad/new-reviewer-email-job into lp:launchpad

  1. new-reviewer-email-job
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merge reported by: Tim Penhey
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/new-reviewer-email-job
Merge into: lp:launchpad
Prerequisite: lp:~thumper/launchpad/code-review-comment-email-job
Diff against target: 415 lines (+201/-34)
8 files modified
lib/lp/code/configure.zcml (+10/-0)
lib/lp/code/interfaces/branchmergeproposal.py (+19/-0)
lib/lp/code/mail/branch.py (+6/-7)
lib/lp/code/mail/branchmergeproposal.py (+3/-16)
lib/lp/code/mail/tests/test_branch.py (+7/-3)
lib/lp/code/mail/tests/test_branchmergeproposal.py (+79/-5)
lib/lp/code/model/branchmergeproposal.py (+2/-1)
lib/lp/code/model/branchmergeproposaljob.py (+75/-2)
To merge this branch: bzr merge lp:~thumper/launchpad/new-reviewer-email-job
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+21610@code.launchpad.net

Description of the change

This branch adds the job to send the notification email for review requests.

lib/lp/code/configure.zcml
 - simple registering of the job and the job source

lib/lp/code/interfaces/branchmergeproposal.py
 - define the interface for the job itself and the utility to create them

lib/lp/code/mail/branch.py
 - since the details are now stored in the job, we pass through a few
   more details into the BMPMailer.forReviewer static function.

lib/lp/code/mail/branchmergeproposal.py
 - vastly simplify the event listner function.
   NOTE: this method is renamed in a later pipe.

lib/lp/code/mail/tests/test_branch.py and
lib/lp/code/model/branchmergeproposal.py
 - since the static constructor function changed the parameters, the tests
   also had to be updated.

lib/lp/code/mail/tests/test_branchmergeproposal.py
 - some of the existing tests had to be augmented with job.run in order
   to send the email that they were checking before
 - other tests are checking the job creation.

lib/lp/code/model/branchmergeproposaljob.py
 - the implementation of the new job
 - just defers to the existing BMPMailer functionality

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :
Download full text (8.1 KiB)

Hi Tim,

There are some real issues that 'make lint' exposes. Good luck
finding them amongst all of the noise. :)

Otherwise this is a fine branch with the exception of a few items marked below.

Thanks.

--Brad

> === modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
> --- lib/lp/code/interfaces/branchmergeproposal.py 2010-03-29 00:59:29 +0000
> +++ lib/lp/code/interfaces/branchmergeproposal.py 2010-03-29 00:59:31 +0000
> @@ -613,6 +615,23 @@
> """Create a job to email subscribers about the comment."""
>
>
> +class IReviewRequestedEmailJob(IRunnableJob):
> + """Interface for the job to sends review request emails."""
> +
> + reviewer = Attribute('The person or team asked to do the review.')
> + requester = Attribute('The person who as asked for the review.')

s/as/has/

> === modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
> --- lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-03-05 03:35:10 +0000
> +++ lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-03-29 00:59:31 +0000
> @@ -431,6 +437,76 @@
> login_person(self.owner)
> return person
>
> + def getReviewNotificationEmail(self):
> + """Return the review requested email job for the test's proposal."""
> + [job] = list(
> + IStore(BranchMergeProposalJob).find(
> + BranchMergeProposalJob,
> + BranchMergeProposalJob.branch_merge_proposal == self.bmp,
> + BranchMergeProposalJob.job_type ==
> + BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL))
> + return ReviewRequestedEmailJob(job)
> +
> + def test_nominateReview_creates_job(self):
> + # When a reviewer is nominated, a job is created to send out the
> + # review request email.
> + reviewer = self.factory.makePerson()
> + pop_notifications()
> + vote_reference = self.bmp.nominateReviewer(reviewer, self.owner, None)
> + # No email is sent.
> + sent_mail = pop_notifications()
> + self.assertEqual([], sent_mail)
> + # A job is created.
> + review_request_job = self.getReviewNotificationEmail()
> + self.assertEqual(self.bmp, review_request_job.branch_merge_proposal)
> + self.assertEqual(reviewer, review_request_job.reviewer)
> + self.assertEqual(self.bmp.registrant, review_request_job.requester)
> +
> + def test_nominateReview_email_content(self):
> + # The email that is sent contains the description of the proposal, and
> + # a link to the proposal.
> + reviewer = self.factory.makePerson()
> + self.bmp.description = 'This branch is awesome.'
> + vote_reference = self.bmp.nominateReviewer(reviewer, self.owner, None)
> + review_request_job = self.getReviewNotificationEmail()
> + review_request_job.run()
> + [sent_mail] = pop_notifications()
> + self.assertEqual(
> + dedent("""\
> + You have been requested to review the proposed merge of %(source)s into %(target)s.
> +
> + This branch is awesome.
> +
> + --
> + %(bmp)s
> + You ...

Read more...

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/configure.zcml'
2--- lib/lp/code/configure.zcml 2010-03-29 00:59:29 +0000
3+++ lib/lp/code/configure.zcml 2010-03-29 00:59:31 +0000
4@@ -933,6 +933,16 @@
5 </class>
6
7 <securedutility
8+ component="lp.code.model.branchmergeproposaljob.ReviewRequestedEmailJob"
9+ provides="lp.code.interfaces.branchmergeproposal.IReviewRequestedEmailJobSource">
10+ <allow interface="lp.code.interfaces.branchmergeproposal.IReviewRequestedEmailJobSource"/>
11+ </securedutility>
12+ <class class="lp.code.model.branchmergeproposaljob.ReviewRequestedEmailJob">
13+ <allow interface="lp.services.job.interfaces.job.IRunnableJob" />
14+ <allow interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalJob" />
15+ </class>
16+
17+ <securedutility
18 component="lp.code.model.branchjob.BranchUpgradeJob"
19 provides="lp.code.interfaces.branchjob.IBranchUpgradeJobSource">
20 <allow interface="lp.code.interfaces.branchjob.IBranchUpgradeJobSource"/>
21
22=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
23--- lib/lp/code/interfaces/branchmergeproposal.py 2010-03-29 00:59:29 +0000
24+++ lib/lp/code/interfaces/branchmergeproposal.py 2010-03-29 00:59:31 +0000
25@@ -18,6 +18,8 @@
26 'ICreateMergeProposalJobSource',
27 'IMergeProposalCreatedJob',
28 'IMergeProposalCreatedJobSource',
29+ 'IReviewRequestedEmailJob',
30+ 'IReviewRequestedEmailJobSource',
31 'IUpdatePreviewDiffJobSource',
32 'notify_modified',
33 ]
34@@ -613,6 +615,23 @@
35 """Create a job to email subscribers about the comment."""
36
37
38+class IReviewRequestedEmailJob(IRunnableJob):
39+ """Interface for the job to sends review request emails."""
40+
41+ reviewer = Attribute('The person or team asked to do the review.')
42+ requester = Attribute('The person who as asked for the review.')
43+
44+
45+class IReviewRequestedEmailJobSource(IJobSource):
46+ """Create or retrieve jobs that email review requests."""
47+
48+ def create(review_request):
49+ """Create a job to email a review request.
50+
51+ :param review_request: A vote reference for the requested review.
52+ """
53+
54+
55 # XXX: JonathanLange 2010-01-06: This is only used in the scanner, perhaps it
56 # should be moved there.
57 def notify_modified(proposal, func, *args, **kwargs):
58
59=== modified file 'lib/lp/code/mail/branch.py'
60--- lib/lp/code/mail/branch.py 2010-03-09 16:58:30 +0000
61+++ lib/lp/code/mail/branch.py 2010-03-29 00:59:31 +0000
62@@ -69,23 +69,22 @@
63 review_level=subscription.review_level)
64
65 @classmethod
66- def forReviewer(cls, vote_reference, recipient,
67+ def forReviewer(cls, branch_merge_proposal, pending_review, reviewer,
68 branch_identity_cache=None):
69 """Construct RecipientReason for a reviewer.
70
71 The reviewer will be the sole recipient.
72 """
73- merge_proposal = vote_reference.branch_merge_proposal
74- branch = merge_proposal.source_branch
75- if vote_reference.comment is None:
76+ branch = branch_merge_proposal.source_branch
77+ if pending_review:
78 reason_template = (
79 '%(entity_is)s requested to review %(merge_proposal)s.')
80 else:
81 reason_template = (
82 '%(entity_is)s reviewing %(merge_proposal)s.')
83- return cls(vote_reference.reviewer, recipient, branch,
84- 'Reviewer', reason_template, merge_proposal,
85- branch_identity_cache=branch_identity_cache)
86+ return cls(reviewer, reviewer, branch, 'Reviewer',
87+ reason_template, branch_merge_proposal,
88+ branch_identity_cache=branch_identity_cache)
89
90 @classmethod
91 def forRegistrant(cls, merge_proposal, branch_identity_cache=None):
92
93=== modified file 'lib/lp/code/mail/branchmergeproposal.py'
94--- lib/lp/code/mail/branchmergeproposal.py 2010-02-17 08:57:04 +0000
95+++ lib/lp/code/mail/branchmergeproposal.py 2010-03-29 00:59:31 +0000
96@@ -15,8 +15,8 @@
97 from lp.code.adapters.branch import BranchMergeProposalDelta
98 from lp.code.enums import CodeReviewNotificationLevel
99 from lp.code.interfaces.branchmergeproposal import (
100- IMergeProposalCreatedJobSource)
101-from lp.code.mail.branch import BranchMailer, RecipientReason
102+ IMergeProposalCreatedJobSource, IReviewRequestedEmailJobSource)
103+from lp.code.mail.branch import BranchMailer
104 from lp.registry.interfaces.person import IPerson
105 from lp.services.mail.basemailer import BaseMailer
106
107@@ -46,20 +46,7 @@
108
109 def send_review_requested_notifications(vote_reference, event):
110 """Notify the reviewer that they have been requested to review."""
111- # XXX: rockstar - 9 Oct 2008 - If the reviewer is a team, don't send
112- # email. This is to stop the abuse of a user spamming all members of
113- # a team by requesting them to review a (possibly unrelated) branch.
114- # Ideally we'd come up with a better solution, but I can't think of
115- # one yet. In all other places we are emailing subscribers directly
116- # rather than people that haven't subscribed.
117- # See bug #281056. (affects IBranchMergeProposal)
118- if not vote_reference.reviewer.is_team:
119- reason = RecipientReason.forReviewer(
120- vote_reference, vote_reference.reviewer)
121- mailer = BMPMailer.forReviewRequest(
122- reason, vote_reference.branch_merge_proposal,
123- vote_reference.registrant)
124- mailer.sendAll()
125+ getUtility(IReviewRequestedEmailJobSource).create(vote_reference)
126
127
128 class BMPMailer(BranchMailer):
129
130=== modified file 'lib/lp/code/mail/tests/test_branch.py'
131--- lib/lp/code/mail/tests/test_branch.py 2009-11-12 09:00:35 +0000
132+++ lib/lp/code/mail/tests/test_branch.py 2010-03-29 00:59:31 +0000
133@@ -59,8 +59,11 @@
134
135 def test_forReviewer(self):
136 """Test values when created from a branch subscription."""
137- ignored, vote_reference, subscriber = self.makeReviewerAndSubscriber()
138- reason = RecipientReason.forReviewer(vote_reference, subscriber)
139+ merge_proposal, vote_reference, subscriber = (
140+ self.makeReviewerAndSubscriber())
141+ pending_review = vote_reference.comment is None
142+ reason = RecipientReason.forReviewer(
143+ merge_proposal, pending_review, subscriber)
144 self.assertEqual(subscriber, reason.subscriber)
145 self.assertEqual(subscriber, reason.recipient)
146 self.assertEqual(
147@@ -68,7 +71,8 @@
148
149 def test_getReasonReviewer(self):
150 bmp, vote_reference, subscriber = self.makeReviewerAndSubscriber()
151- reason = RecipientReason.forReviewer(vote_reference, subscriber)
152+ pending_review = vote_reference.comment is None
153+ reason = RecipientReason.forReviewer(bmp, pending_review, subscriber)
154 self.assertEqual(
155 'You are requested to review the proposed merge of %s into %s.'
156 % (bmp.source_branch.bzr_identity,
157
158=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
159--- lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-03-05 03:35:10 +0000
160+++ lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-03-29 00:59:31 +0000
161@@ -5,10 +5,12 @@
162
163 from difflib import unified_diff
164 from unittest import TestLoader
165+from textwrap import dedent
166 import transaction
167
168 from zope.security.proxy import removeSecurityProxy
169
170+from canonical.launchpad.interfaces import IStore
171 from canonical.testing import (
172 DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
173
174@@ -21,6 +23,9 @@
175 from lp.code.mail.branchmergeproposal import (
176 BMPMailer, send_merge_proposal_modified_notifications)
177 from lp.code.model.branch import update_trigger_modified_fields
178+from lp.code.model.branchmergeproposaljob import (
179+ BranchMergeProposalJob, BranchMergeProposalJobType,
180+ ReviewRequestedEmailJob)
181 from lp.code.model.codereviewvote import CodeReviewVoteReference
182 from canonical.launchpad.webapp import canonical_url
183 from lp.testing import login_person, TestCaseWithFactory
184@@ -381,7 +386,8 @@
185 request = CodeReviewVoteReference(
186 branch_merge_proposal=merge_proposal, reviewer=candidate,
187 registrant=requester)
188- return RecipientReason.forReviewer(request, candidate), requester
189+ reason = RecipientReason.forReviewer(merge_proposal, True, candidate)
190+ return reason, requester
191
192 def test_forReviewRequest(self):
193 """Test creating a mailer for a review request."""
194@@ -431,6 +437,76 @@
195 login_person(self.owner)
196 return person
197
198+ def getReviewNotificationEmail(self):
199+ """Return the review requested email job for the test's proposal."""
200+ [job] = list(
201+ IStore(BranchMergeProposalJob).find(
202+ BranchMergeProposalJob,
203+ BranchMergeProposalJob.branch_merge_proposal == self.bmp,
204+ BranchMergeProposalJob.job_type ==
205+ BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL))
206+ return ReviewRequestedEmailJob(job)
207+
208+ def test_nominateReview_creates_job(self):
209+ # When a reviewer is nominated, a job is created to send out the
210+ # review request email.
211+ reviewer = self.factory.makePerson()
212+ pop_notifications()
213+ vote_reference = self.bmp.nominateReviewer(reviewer, self.owner, None)
214+ # No email is sent.
215+ sent_mail = pop_notifications()
216+ self.assertEqual([], sent_mail)
217+ # A job is created.
218+ review_request_job = self.getReviewNotificationEmail()
219+ self.assertEqual(self.bmp, review_request_job.branch_merge_proposal)
220+ self.assertEqual(reviewer, review_request_job.reviewer)
221+ self.assertEqual(self.bmp.registrant, review_request_job.requester)
222+
223+ def test_nominateReview_email_content(self):
224+ # The email that is sent contains the description of the proposal, and
225+ # a link to the proposal.
226+ reviewer = self.factory.makePerson()
227+ self.bmp.description = 'This branch is awesome.'
228+ vote_reference = self.bmp.nominateReviewer(reviewer, self.owner, None)
229+ review_request_job = self.getReviewNotificationEmail()
230+ review_request_job.run()
231+ [sent_mail] = pop_notifications()
232+ self.assertEqual(
233+ dedent("""\
234+ You have been requested to review the proposed merge of %(source)s into %(target)s.
235+
236+ This branch is awesome.
237+
238+ --
239+ %(bmp)s
240+ You are requested to review the proposed merge of %(source)s into %(target)s.
241+ """ % {
242+ 'source': self.bmp.source_branch.bzr_identity,
243+ 'target': self.bmp.target_branch.bzr_identity,
244+ 'bmp': canonical_url(self.bmp)}),
245+ sent_mail.get_payload(decode=True))
246+
247+ def test_nominateReview_emails_team_address(self):
248+ # If a review request is made for a team, the members of the team are
249+ # sent an email.
250+ eric = self.factory.makePerson(
251+ displayname='Eric the Viking', email='eric@vikings.example.com')
252+ black_beard = self.factory.makePerson(
253+ displayname='Black Beard', email='black@pirates.example.com')
254+ review_team = self.factory.makeTeam(owner=eric)
255+ login_person(eric)
256+ review_team.addMember(black_beard, eric)
257+ pop_notifications()
258+ login_person(self.owner)
259+ vote_reference = self.bmp.nominateReviewer(review_team, self.owner, None)
260+ review_request_job = self.getReviewNotificationEmail()
261+ review_request_job.run()
262+ sent_mail = pop_notifications()
263+ self.assertEqual(
264+ ['Black Beard <black@pirates.example.com>',
265+ 'Eric the Viking <eric@vikings.example.com>'],
266+ sorted(mail['to'] for mail in sent_mail))
267+
268 def test_requestReviewWithPrivateEmail(self):
269 # We can request a review, even when one of the parties involved has a
270 # private email address.
271@@ -438,11 +514,9 @@
272 # Request a review and prepare the mailer.
273 vote_reference = self.bmp.nominateReviewer(
274 candidate, self.owner, None)
275- reason = RecipientReason.forReviewer(vote_reference, candidate)
276- mailer = BMPMailer.forReviewRequest(reason, self.bmp, self.owner)
277 # Send the mail.
278- pop_notifications()
279- mailer.sendAll()
280+ review_request_job = self.getReviewNotificationEmail()
281+ review_request_job.run()
282 mails = pop_notifications()
283 self.assertEqual(1, len(mails))
284 candidate = removeSecurityProxy(candidate)
285
286=== modified file 'lib/lp/code/model/branchmergeproposal.py'
287--- lib/lp/code/model/branchmergeproposal.py 2010-02-26 09:54:20 +0000
288+++ lib/lp/code/model/branchmergeproposal.py 2010-03-29 00:59:31 +0000
289@@ -281,8 +281,9 @@
290 for review in self.votes:
291 reviewer = review.reviewer
292 if not reviewer.is_team:
293+ pending = review.comment is None
294 recipients[reviewer] = RecipientReason.forReviewer(
295- review, reviewer,
296+ self, pending, reviewer,
297 branch_identity_cache=branch_identity_cache)
298 # If the registrant of the proposal is getting emails, update the
299 # rationale to say that they registered it. Don't however send them
300
301=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
302--- lib/lp/code/model/branchmergeproposaljob.py 2010-03-29 00:59:29 +0000
303+++ lib/lp/code/model/branchmergeproposaljob.py 2010-03-29 00:59:31 +0000
304@@ -1,4 +1,4 @@
305-# Copyright 2009 Canonical Ltd. This software is licensed under the
306+# Copyright 2009, 2010 Canonical Ltd. This software is licensed under the
307 # GNU Affero General Public License version 3 (see the file LICENSE).
308
309
310@@ -20,6 +20,7 @@
311 'CodeReviewCommentEmailJob',
312 'CreateMergeProposalJob',
313 'MergeProposalCreatedJob',
314+ 'ReviewRequestedEmailJob',
315 'UpdatePreviewDiffJob',
316 ]
317
318@@ -51,13 +52,16 @@
319 IBranchMergeProposalJob, ICodeReviewCommentEmailJob,
320 ICodeReviewCommentEmailJobSource, ICreateMergeProposalJob,
321 ICreateMergeProposalJobSource, IMergeProposalCreatedJob,
322- IMergeProposalCreatedJobSource, IUpdatePreviewDiffJobSource,
323+ IMergeProposalCreatedJobSource, IReviewRequestedEmailJob,
324+ IReviewRequestedEmailJobSource, IUpdatePreviewDiffJobSource,
325 )
326+from lp.code.mail.branch import RecipientReason
327 from lp.code.mail.branchmergeproposal import BMPMailer
328 from lp.code.mail.codereviewcomment import CodeReviewCommentMailer
329 from lp.code.model.branchmergeproposal import BranchMergeProposal
330 from lp.code.model.diff import PreviewDiff
331 from lp.codehosting.vfs import get_multi_server, get_scanner_server
332+from lp.registry.interfaces.person import IPersonSet
333 from lp.services.job.model.job import Job
334 from lp.services.job.interfaces.job import IRunnableJob
335 from lp.services.job.runner import BaseRunnableJob
336@@ -86,6 +90,13 @@
337 reviewers.
338 """)
339
340+ REVIEW_REQUEST_EMAIL = DBItem(3, """
341+ Send the review request email to the requested reviewer.
342+
343+ This job sends an email to the requested reviewer, or members of the
344+ requested reviewer team asking them to review the proposal.
345+ """)
346+
347
348 class BranchMergeProposalJob(Storm):
349 """Base class for jobs related to branch merge proposals."""
350@@ -418,3 +429,65 @@
351 """Return a list of email-ids to notify about user errors."""
352 commenter = self.code_review_comment.message.owner
353 return [commenter.preferredemail]
354+
355+
356+class ReviewRequestedEmailJob(BranchMergeProposalJobDerived):
357+ """Send email to the reviewer telling them to review the proposal.
358+
359+ Provides class methods to create and retrieve such jobs.
360+ """
361+
362+ implements(IReviewRequestedEmailJob)
363+
364+ classProvides(IReviewRequestedEmailJobSource)
365+
366+ class_job_type = BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL
367+
368+ def run(self):
369+ """See `IRunnableJob`."""
370+ reason = RecipientReason.forReviewer(
371+ self.branch_merge_proposal, True, self.reviewer)
372+ mailer = BMPMailer.forReviewRequest(
373+ reason, self.branch_merge_proposal, self.requester)
374+ mailer.sendAll()
375+
376+ @classmethod
377+ def create(cls, review_request):
378+ """See `IReviewRequestedEmailJobSource`."""
379+ metadata = cls.getMetadata(review_request)
380+ bmp = review_request.branch_merge_proposal
381+ job = BranchMergeProposalJob(bmp, cls.class_job_type, metadata)
382+ return cls(job)
383+
384+ @staticmethod
385+ def getMetadata(review_request):
386+ return {
387+ 'reviewer': review_request.reviewer.name,
388+ 'requester': review_request.registrant.name,
389+ }
390+
391+ @property
392+ def reviewer(self):
393+ """The person or team who has been asked to review."""
394+ return getUtility(IPersonSet).getByName(self.metadata['reviewer'])
395+
396+ @property
397+ def requester(self):
398+ """The person who requested the review to be done."""
399+ return getUtility(IPersonSet).getByName(self.metadata['requester'])
400+
401+ def getOopsVars(self):
402+ """See `IRunnableJob`."""
403+ vars = BranchMergeProposalJobDerived.getOopsVars(self)
404+ vars.extend([
405+ ('reviewer', self.metadata['reviewer']),
406+ ('requester', self.metadata['requester']),
407+ ])
408+ return vars
409+
410+ def getErrorRecipients(self):
411+ """Return a list of email-ids to notify about user errors."""
412+ recipients = []
413+ if self.requester is not None:
414+ recipients.append(self.requester.preferredemail)
415+ return recipients