Launchpad itself

Merge lp:~thumper/launchpad/less-wip-emails into lp:launchpad

  1. less-wip-emails
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 11826
Proposed branch: lp:~thumper/launchpad/less-wip-emails
Merge into: lp:launchpad
Diff against target: 330 lines (+117/-55)
3 files modified
lib/lp/code/mail/tests/test_branchmergeproposal.py (+100/-52)
lib/lp/code/model/tests/test_branchmergeproposaljobs.py (+4/-2)
lib/lp/code/subscribers/branchmergeproposal.py (+13/-1)
To merge this branch: bzr merge lp:~thumper/launchpad/less-wip-emails
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) code Approve
Review via email: mp+39489@code.launchpad.net

Commit message

Don't send email for work in progress merge proposals when requesting reviews or modifying the proposal.

Description of the change

During the QA of the change where the initial code review email doesn't go out for work in progress merge proposals, it became clear that we should be suppressing other emails, for example the review requests while work in progress, and the state change emails.

This means we won't get an email that says WIP -> Needs Review, followed by the initial review email, we'll just get the initial review email.

Also all of the reviewers get the initial review request, so extra review request emails don't go out when the proposal is work in progress.

tests:
  code.mail.tests.test_branchmergeproposal

There was also some test refactoring to have the tests use updated factory methods instead of rolling their own.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Looks good. Only the slightest of superficial remarks. To recap from IRC, "there is no email job created" isn't very nice to read; "with person_logged_in(...)" would probably make a few tests a bit cleaner; the final condition in the IStore.find() isn't sufficiently indented.

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/mail/tests/test_branchmergeproposal.py'
2--- lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-10-04 19:50:45 +0000
3+++ lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-10-28 22:34:49 +0000
4@@ -9,7 +9,9 @@
5 from unittest import TestLoader
6
7 from lazr.lifecycle.event import ObjectModifiedEvent
8+from lazr.lifecycle.snapshot import Snapshot
9 import transaction
10+from zope.interface import providedBy
11 from zope.security.proxy import removeSecurityProxy
12
13 from canonical.launchpad.interfaces.lpstorm import IStore
14@@ -18,8 +20,8 @@
15 DatabaseFunctionalLayer,
16 LaunchpadFunctionalLayer,
17 )
18-from lp.code.adapters.branch import BranchMergeProposalDelta
19 from lp.code.enums import (
20+ BranchMergeProposalStatus,
21 BranchSubscriptionNotificationLevel,
22 CodeReviewNotificationLevel,
23 )
24@@ -36,7 +38,7 @@
25 from lp.code.model.diff import PreviewDiff
26 from lp.code.subscribers.branchmergeproposal import merge_proposal_modified
27 from lp.testing import (
28- login_person,
29+ person_logged_in,
30 TestCaseWithFactory,
31 )
32 from lp.testing.mail_helpers import pop_notifications
33@@ -51,7 +53,9 @@
34 super(TestMergeProposalMailing, self).setUp('admin@canonical.com')
35
36 def makeProposalWithSubscriber(self, diff_text=None,
37- initial_comment=None, prerequisite=False):
38+ initial_comment=None,
39+ prerequisite=False,
40+ needs_review=True):
41 if diff_text is not None:
42 preview_diff = PreviewDiff.create(
43 diff_text,
44@@ -68,8 +72,13 @@
45 prerequisite_branch = self.factory.makeProductBranch(product)
46 else:
47 prerequisite_branch = None
48+ if needs_review:
49+ initial_status = BranchMergeProposalStatus.NEEDS_REVIEW
50+ else:
51+ initial_status = BranchMergeProposalStatus.WORK_IN_PROGRESS
52 bmp = self.factory.makeBranchMergeProposal(
53 registrant=registrant, product=product,
54+ set_state=initial_status,
55 prerequisite_branch=prerequisite_branch,
56 preview_diff=preview_diff, initial_comment=initial_comment)
57 subscriber = self.factory.makePerson(displayname='Baz Quxx',
58@@ -308,16 +317,44 @@
59 def test_no_job_created_if_no_delta(self):
60 """Ensure None is returned if no change has been made."""
61 merge_proposal, person = self.makeProposalWithSubscriber()
62- old_merge_proposal = BranchMergeProposalDelta.snapshot(merge_proposal)
63- event = ObjectModifiedEvent(
64- merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
65- merge_proposal_modified(merge_proposal, event)
66- self.assertIs(None, self.getProposalUpdatedEmailJob(merge_proposal))
67+ old_merge_proposal = Snapshot(
68+ merge_proposal, providing=providedBy(merge_proposal))
69+ event = ObjectModifiedEvent(
70+ merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
71+ merge_proposal_modified(merge_proposal, event)
72+ self.assertIs(None, self.getProposalUpdatedEmailJob(merge_proposal))
73+
74+ def test_no_job_created_if_work_in_progress(self):
75+ """Ensure None is returned if no change has been made."""
76+ merge_proposal, person = self.makeProposalWithSubscriber(
77+ needs_review=False)
78+ old_merge_proposal = Snapshot(
79+ merge_proposal, providing=providedBy(merge_proposal))
80+ merge_proposal.commit_message = 'new commit message'
81+ merge_proposal.description = 'change description'
82+ event = ObjectModifiedEvent(
83+ merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
84+ merge_proposal_modified(merge_proposal, event)
85+ self.assertIs(None, self.getProposalUpdatedEmailJob(merge_proposal))
86+
87+ def test_job_created_if_work_in_progress_merged(self):
88+ # If work in progress is merged, then that is email worthy.
89+ merge_proposal, person = self.makeProposalWithSubscriber(
90+ needs_review=False)
91+ old_merge_proposal = Snapshot(
92+ merge_proposal, providing=providedBy(merge_proposal))
93+ merge_proposal.setStatus(BranchMergeProposalStatus.MERGED)
94+ event = ObjectModifiedEvent(
95+ merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
96+ merge_proposal_modified(merge_proposal, event)
97+ job = self.getProposalUpdatedEmailJob(merge_proposal)
98+ self.assertIsNot(None, job, 'Job was not created.')
99
100 def makeProposalUpdatedEmailJob(self):
101 """Fixture method providing a mailer for a modified merge proposal"""
102 merge_proposal, subscriber = self.makeProposalWithSubscriber()
103- old_merge_proposal = BranchMergeProposalDelta.snapshot(merge_proposal)
104+ old_merge_proposal = Snapshot(
105+ merge_proposal, providing=providedBy(merge_proposal))
106 merge_proposal.requestReview()
107 merge_proposal.commit_message = 'new commit message'
108 merge_proposal.description = 'change description'
109@@ -339,7 +376,6 @@
110 """Ensure the right delta is filled out if there is a change."""
111 job, subscriber = self.makeProposalUpdatedEmailJob()
112 self.assertEqual(
113- ' Status: Work in progress => Needs review\n\n'
114 'Commit Message changed to:\n\nnew commit message\n\n'
115 'Description changed to:\n\nchange description',
116 job.delta_text)
117@@ -363,8 +399,6 @@
118 expected = dedent("""\
119 The proposal to merge lp://dev/~bob/super-product/fix-foo-for-bar into lp://dev/~mary/super-product/bar has been updated.
120
121- Status: Work in progress => Needs review
122-
123 Commit Message changed to:
124
125 new commit message
126@@ -431,52 +465,63 @@
127
128 layer = DatabaseFunctionalLayer
129
130- def setUp(self):
131- TestCaseWithFactory.setUp(self)
132- self.owner = self.factory.makePerson()
133- login_person(self.owner)
134- self.bmp = self.factory.makeBranchMergeProposal(registrant=self.owner)
135-
136- def makePersonWithHiddenEmail(self):
137- """Make an arbitrary person with hidden email addresses."""
138- person = self.factory.makePerson()
139- login_person(person)
140- person.hide_email_addresses = True
141- login_person(self.owner)
142- return person
143-
144- def getReviewNotificationEmail(self):
145+ def getReviewEmailJobs(self, bmp):
146+ """Return the result set for the merge proposals review email jobs."""
147+ review_job = BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL
148+ return IStore(BranchMergeProposalJob).find(
149+ BranchMergeProposalJob,
150+ BranchMergeProposalJob.branch_merge_proposal == bmp,
151+ BranchMergeProposalJob.job_type == review_job)
152+
153+ def getReviewNotificationEmail(self, bmp):
154 """Return the review requested email job for the test's proposal."""
155- [job] = list(
156- IStore(BranchMergeProposalJob).find(
157- BranchMergeProposalJob,
158- BranchMergeProposalJob.branch_merge_proposal == self.bmp,
159- BranchMergeProposalJob.job_type ==
160- BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL))
161+ [job] = list(self.getReviewEmailJobs(bmp))
162 return ReviewRequestedEmailJob(job)
163
164+ def test_nominateReview_no_job_if_work_in_progress(self):
165+ # When a reviewer is nominated for a proposal that is work in
166+ # progress, no email job is created.
167+ bmp = self.factory.makeBranchMergeProposal(
168+ set_state=BranchMergeProposalStatus.WORK_IN_PROGRESS)
169+ reviewer = self.factory.makePerson()
170+ pop_notifications()
171+ with person_logged_in(bmp.registrant):
172+ bmp.nominateReviewer(reviewer, bmp.registrant, None)
173+ # No email is sent.
174+ sent_mail = pop_notifications()
175+ self.assertEqual([], sent_mail)
176+ # No job created.
177+ job_count = self.getReviewEmailJobs(bmp).count()
178+ self.assertEqual(0, job_count)
179+
180 def test_nominateReview_creates_job(self):
181 # When a reviewer is nominated, a job is created to send out the
182 # review request email.
183+ bmp = self.factory.makeBranchMergeProposal(
184+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
185 reviewer = self.factory.makePerson()
186 pop_notifications()
187- self.bmp.nominateReviewer(reviewer, self.owner, None)
188+ with person_logged_in(bmp.registrant):
189+ bmp.nominateReviewer(reviewer, bmp.registrant, None)
190 # No email is sent.
191 sent_mail = pop_notifications()
192 self.assertEqual([], sent_mail)
193 # A job is created.
194- review_request_job = self.getReviewNotificationEmail()
195- self.assertEqual(self.bmp, review_request_job.branch_merge_proposal)
196+ review_request_job = self.getReviewNotificationEmail(bmp)
197+ self.assertEqual(bmp, review_request_job.branch_merge_proposal)
198 self.assertEqual(reviewer, review_request_job.reviewer)
199- self.assertEqual(self.bmp.registrant, review_request_job.requester)
200+ self.assertEqual(bmp.registrant, review_request_job.requester)
201
202 def test_nominateReview_email_content(self):
203 # The email that is sent contains the description of the proposal, and
204 # a link to the proposal.
205+ bmp = self.factory.makeBranchMergeProposal(
206+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
207 reviewer = self.factory.makePerson()
208- self.bmp.description = 'This branch is awesome.'
209- self.bmp.nominateReviewer(reviewer, self.owner, None)
210- review_request_job = self.getReviewNotificationEmail()
211+ with person_logged_in(bmp.registrant):
212+ bmp.description = 'This branch is awesome.'
213+ bmp.nominateReviewer(reviewer, bmp.registrant, None)
214+ review_request_job = self.getReviewNotificationEmail(bmp)
215 review_request_job.run()
216 [sent_mail] = pop_notifications()
217 expected = dedent("""\
218@@ -488,25 +533,25 @@
219 %(bmp)s
220 You are requested to review the proposed merge of %(source)s into %(target)s.
221 """ % {
222- 'source': self.bmp.source_branch.bzr_identity,
223- 'target': self.bmp.target_branch.bzr_identity,
224- 'bmp': canonical_url(self.bmp)})
225+ 'source': bmp.source_branch.bzr_identity,
226+ 'target': bmp.target_branch.bzr_identity,
227+ 'bmp': canonical_url(bmp)})
228 self.assertEqual(expected, sent_mail.get_payload(decode=True))
229
230 def test_nominateReview_emails_team_address(self):
231 # If a review request is made for a team, the members of the team are
232 # sent an email.
233+ bmp = self.factory.makeBranchMergeProposal(
234+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
235 eric = self.factory.makePerson(
236 displayname='Eric the Viking', email='eric@vikings.example.com')
237 black_beard = self.factory.makePerson(
238 displayname='Black Beard', email='black@pirates.example.com')
239- review_team = self.factory.makeTeam(owner=eric)
240- login_person(eric)
241- review_team.addMember(black_beard, eric)
242+ review_team = self.factory.makeTeam(owner=eric, members=[black_beard])
243 pop_notifications()
244- login_person(self.owner)
245- self.bmp.nominateReviewer(review_team, self.owner, None)
246- review_request_job = self.getReviewNotificationEmail()
247+ with person_logged_in(bmp.registrant):
248+ bmp.nominateReviewer(review_team, bmp.registrant, None)
249+ review_request_job = self.getReviewNotificationEmail(bmp)
250 review_request_job.run()
251 sent_mail = pop_notifications()
252 self.assertEqual(
253@@ -517,11 +562,14 @@
254 def test_requestReviewWithPrivateEmail(self):
255 # We can request a review, even when one of the parties involved has a
256 # private email address.
257- candidate = self.makePersonWithHiddenEmail()
258+ bmp = self.factory.makeBranchMergeProposal(
259+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
260+ candidate = self.factory.makePerson(hide_email_addresses=True)
261 # Request a review and prepare the mailer.
262- self.bmp.nominateReviewer(candidate, self.owner, None)
263+ with person_logged_in(bmp.registrant):
264+ bmp.nominateReviewer(candidate, bmp.registrant, None)
265 # Send the mail.
266- review_request_job = self.getReviewNotificationEmail()
267+ review_request_job = self.getReviewNotificationEmail(bmp)
268 review_request_job.run()
269 mails = pop_notifications()
270 self.assertEqual(1, len(mails))
271
272=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
273--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py 2010-10-26 15:47:24 +0000
274+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py 2010-10-28 22:34:49 +0000
275@@ -357,7 +357,8 @@
276
277 def test_iterReady_supports_review_requested(self):
278 # iterReady will also return pending ReviewRequestedEmailJobs.
279- bmp = self.makeBranchMergeProposal()
280+ bmp = self.makeBranchMergeProposal(
281+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
282 self.completePendingJobs()
283 reviewer = self.factory.makePerson()
284 bmp.nominateReviewer(reviewer, bmp.registrant)
285@@ -380,7 +381,8 @@
286
287 def test_iterReady_supports_updated_emails(self):
288 # iterReady will also return pending MergeProposalUpdatedEmailJob.
289- bmp = self.makeBranchMergeProposal()
290+ bmp = self.makeBranchMergeProposal(
291+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
292 self.completePendingJobs()
293 old_merge_proposal = BranchMergeProposalDelta.snapshot(bmp)
294 bmp.commit_message = 'new commit message'
295
296=== modified file 'lib/lp/code/subscribers/branchmergeproposal.py'
297--- lib/lp/code/subscribers/branchmergeproposal.py 2010-10-18 03:39:28 +0000
298+++ lib/lp/code/subscribers/branchmergeproposal.py 2010-10-28 22:34:49 +0000
299@@ -10,6 +10,7 @@
300 from zope.component import getUtility
301
302 from lp.code.adapters.branch import BranchMergeProposalDelta
303+from lp.code.enums import BranchMergeProposalStatus
304 from lp.code.interfaces.branchmergeproposal import (
305 IMergeProposalNeedsReviewEmailJobSource,
306 IMergeProposalUpdatedEmailJobSource,
307@@ -48,6 +49,14 @@
308 from_person = None
309 else:
310 from_person = IPerson(event.user)
311+ # If the merge proposal was work in progress, then we don't want to send
312+ # out an email as the needs review email will cover that.
313+ old_status = event.object_before_modification.queue_status
314+ if old_status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
315+ # Unless the new status is merged. If this occurs we really should
316+ # send out an email.
317+ if merge_proposal.queue_status != BranchMergeProposalStatus.MERGED:
318+ return
319 # Create a delta of the changes. If there are no changes to report, then
320 # we're done.
321 delta = BranchMergeProposalDelta.construct(
322@@ -63,5 +72,8 @@
323
324 def review_requested(vote_reference, event):
325 """Notify the reviewer that they have been requested to review."""
326- getUtility(IReviewRequestedEmailJobSource).create(vote_reference)
327+ # Don't send email if the proposal is work in progress.
328+ bmp_status = vote_reference.branch_merge_proposal.queue_status
329+ if bmp_status != BranchMergeProposalStatus.WORK_IN_PROGRESS:
330+ getUtility(IReviewRequestedEmailJobSource).create(vote_reference)
331