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
=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
--- lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-10-04 19:50:45 +0000
+++ lib/lp/code/mail/tests/test_branchmergeproposal.py 2010-10-28 22:34:49 +0000
@@ -9,7 +9,9 @@
9from unittest import TestLoader9from unittest import TestLoader
1010
11from lazr.lifecycle.event import ObjectModifiedEvent11from lazr.lifecycle.event import ObjectModifiedEvent
12from lazr.lifecycle.snapshot import Snapshot
12import transaction13import transaction
14from zope.interface import providedBy
13from zope.security.proxy import removeSecurityProxy15from zope.security.proxy import removeSecurityProxy
1416
15from canonical.launchpad.interfaces.lpstorm import IStore17from canonical.launchpad.interfaces.lpstorm import IStore
@@ -18,8 +20,8 @@
18 DatabaseFunctionalLayer,20 DatabaseFunctionalLayer,
19 LaunchpadFunctionalLayer,21 LaunchpadFunctionalLayer,
20 )22 )
21from lp.code.adapters.branch import BranchMergeProposalDelta
22from lp.code.enums import (23from lp.code.enums import (
24 BranchMergeProposalStatus,
23 BranchSubscriptionNotificationLevel,25 BranchSubscriptionNotificationLevel,
24 CodeReviewNotificationLevel,26 CodeReviewNotificationLevel,
25 )27 )
@@ -36,7 +38,7 @@
36from lp.code.model.diff import PreviewDiff38from lp.code.model.diff import PreviewDiff
37from lp.code.subscribers.branchmergeproposal import merge_proposal_modified39from lp.code.subscribers.branchmergeproposal import merge_proposal_modified
38from lp.testing import (40from lp.testing import (
39 login_person,41 person_logged_in,
40 TestCaseWithFactory,42 TestCaseWithFactory,
41 )43 )
42from lp.testing.mail_helpers import pop_notifications44from lp.testing.mail_helpers import pop_notifications
@@ -51,7 +53,9 @@
51 super(TestMergeProposalMailing, self).setUp('admin@canonical.com')53 super(TestMergeProposalMailing, self).setUp('admin@canonical.com')
5254
53 def makeProposalWithSubscriber(self, diff_text=None,55 def makeProposalWithSubscriber(self, diff_text=None,
54 initial_comment=None, prerequisite=False):56 initial_comment=None,
57 prerequisite=False,
58 needs_review=True):
55 if diff_text is not None:59 if diff_text is not None:
56 preview_diff = PreviewDiff.create(60 preview_diff = PreviewDiff.create(
57 diff_text,61 diff_text,
@@ -68,8 +72,13 @@
68 prerequisite_branch = self.factory.makeProductBranch(product)72 prerequisite_branch = self.factory.makeProductBranch(product)
69 else:73 else:
70 prerequisite_branch = None74 prerequisite_branch = None
75 if needs_review:
76 initial_status = BranchMergeProposalStatus.NEEDS_REVIEW
77 else:
78 initial_status = BranchMergeProposalStatus.WORK_IN_PROGRESS
71 bmp = self.factory.makeBranchMergeProposal(79 bmp = self.factory.makeBranchMergeProposal(
72 registrant=registrant, product=product,80 registrant=registrant, product=product,
81 set_state=initial_status,
73 prerequisite_branch=prerequisite_branch,82 prerequisite_branch=prerequisite_branch,
74 preview_diff=preview_diff, initial_comment=initial_comment)83 preview_diff=preview_diff, initial_comment=initial_comment)
75 subscriber = self.factory.makePerson(displayname='Baz Quxx',84 subscriber = self.factory.makePerson(displayname='Baz Quxx',
@@ -308,16 +317,44 @@
308 def test_no_job_created_if_no_delta(self):317 def test_no_job_created_if_no_delta(self):
309 """Ensure None is returned if no change has been made."""318 """Ensure None is returned if no change has been made."""
310 merge_proposal, person = self.makeProposalWithSubscriber()319 merge_proposal, person = self.makeProposalWithSubscriber()
311 old_merge_proposal = BranchMergeProposalDelta.snapshot(merge_proposal)320 old_merge_proposal = Snapshot(
312 event = ObjectModifiedEvent(321 merge_proposal, providing=providedBy(merge_proposal))
313 merge_proposal, old_merge_proposal, [], merge_proposal.registrant)322 event = ObjectModifiedEvent(
314 merge_proposal_modified(merge_proposal, event)323 merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
315 self.assertIs(None, self.getProposalUpdatedEmailJob(merge_proposal))324 merge_proposal_modified(merge_proposal, event)
325 self.assertIs(None, self.getProposalUpdatedEmailJob(merge_proposal))
326
327 def test_no_job_created_if_work_in_progress(self):
328 """Ensure None is returned if no change has been made."""
329 merge_proposal, person = self.makeProposalWithSubscriber(
330 needs_review=False)
331 old_merge_proposal = Snapshot(
332 merge_proposal, providing=providedBy(merge_proposal))
333 merge_proposal.commit_message = 'new commit message'
334 merge_proposal.description = 'change description'
335 event = ObjectModifiedEvent(
336 merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
337 merge_proposal_modified(merge_proposal, event)
338 self.assertIs(None, self.getProposalUpdatedEmailJob(merge_proposal))
339
340 def test_job_created_if_work_in_progress_merged(self):
341 # If work in progress is merged, then that is email worthy.
342 merge_proposal, person = self.makeProposalWithSubscriber(
343 needs_review=False)
344 old_merge_proposal = Snapshot(
345 merge_proposal, providing=providedBy(merge_proposal))
346 merge_proposal.setStatus(BranchMergeProposalStatus.MERGED)
347 event = ObjectModifiedEvent(
348 merge_proposal, old_merge_proposal, [], merge_proposal.registrant)
349 merge_proposal_modified(merge_proposal, event)
350 job = self.getProposalUpdatedEmailJob(merge_proposal)
351 self.assertIsNot(None, job, 'Job was not created.')
316352
317 def makeProposalUpdatedEmailJob(self):353 def makeProposalUpdatedEmailJob(self):
318 """Fixture method providing a mailer for a modified merge proposal"""354 """Fixture method providing a mailer for a modified merge proposal"""
319 merge_proposal, subscriber = self.makeProposalWithSubscriber()355 merge_proposal, subscriber = self.makeProposalWithSubscriber()
320 old_merge_proposal = BranchMergeProposalDelta.snapshot(merge_proposal)356 old_merge_proposal = Snapshot(
357 merge_proposal, providing=providedBy(merge_proposal))
321 merge_proposal.requestReview()358 merge_proposal.requestReview()
322 merge_proposal.commit_message = 'new commit message'359 merge_proposal.commit_message = 'new commit message'
323 merge_proposal.description = 'change description'360 merge_proposal.description = 'change description'
@@ -339,7 +376,6 @@
339 """Ensure the right delta is filled out if there is a change."""376 """Ensure the right delta is filled out if there is a change."""
340 job, subscriber = self.makeProposalUpdatedEmailJob()377 job, subscriber = self.makeProposalUpdatedEmailJob()
341 self.assertEqual(378 self.assertEqual(
342 ' Status: Work in progress => Needs review\n\n'
343 'Commit Message changed to:\n\nnew commit message\n\n'379 'Commit Message changed to:\n\nnew commit message\n\n'
344 'Description changed to:\n\nchange description',380 'Description changed to:\n\nchange description',
345 job.delta_text)381 job.delta_text)
@@ -363,8 +399,6 @@
363 expected = dedent("""\399 expected = dedent("""\
364 The proposal to merge lp://dev/~bob/super-product/fix-foo-for-bar into lp://dev/~mary/super-product/bar has been updated.400 The proposal to merge lp://dev/~bob/super-product/fix-foo-for-bar into lp://dev/~mary/super-product/bar has been updated.
365401
366 Status: Work in progress => Needs review
367
368 Commit Message changed to:402 Commit Message changed to:
369403
370 new commit message404 new commit message
@@ -431,52 +465,63 @@
431465
432 layer = DatabaseFunctionalLayer466 layer = DatabaseFunctionalLayer
433467
434 def setUp(self):468 def getReviewEmailJobs(self, bmp):
435 TestCaseWithFactory.setUp(self)469 """Return the result set for the merge proposals review email jobs."""
436 self.owner = self.factory.makePerson()470 review_job = BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL
437 login_person(self.owner)471 return IStore(BranchMergeProposalJob).find(
438 self.bmp = self.factory.makeBranchMergeProposal(registrant=self.owner)472 BranchMergeProposalJob,
439473 BranchMergeProposalJob.branch_merge_proposal == bmp,
440 def makePersonWithHiddenEmail(self):474 BranchMergeProposalJob.job_type == review_job)
441 """Make an arbitrary person with hidden email addresses."""475
442 person = self.factory.makePerson()476 def getReviewNotificationEmail(self, bmp):
443 login_person(person)
444 person.hide_email_addresses = True
445 login_person(self.owner)
446 return person
447
448 def getReviewNotificationEmail(self):
449 """Return the review requested email job for the test's proposal."""477 """Return the review requested email job for the test's proposal."""
450 [job] = list(478 [job] = list(self.getReviewEmailJobs(bmp))
451 IStore(BranchMergeProposalJob).find(
452 BranchMergeProposalJob,
453 BranchMergeProposalJob.branch_merge_proposal == self.bmp,
454 BranchMergeProposalJob.job_type ==
455 BranchMergeProposalJobType.REVIEW_REQUEST_EMAIL))
456 return ReviewRequestedEmailJob(job)479 return ReviewRequestedEmailJob(job)
457480
481 def test_nominateReview_no_job_if_work_in_progress(self):
482 # When a reviewer is nominated for a proposal that is work in
483 # progress, no email job is created.
484 bmp = self.factory.makeBranchMergeProposal(
485 set_state=BranchMergeProposalStatus.WORK_IN_PROGRESS)
486 reviewer = self.factory.makePerson()
487 pop_notifications()
488 with person_logged_in(bmp.registrant):
489 bmp.nominateReviewer(reviewer, bmp.registrant, None)
490 # No email is sent.
491 sent_mail = pop_notifications()
492 self.assertEqual([], sent_mail)
493 # No job created.
494 job_count = self.getReviewEmailJobs(bmp).count()
495 self.assertEqual(0, job_count)
496
458 def test_nominateReview_creates_job(self):497 def test_nominateReview_creates_job(self):
459 # When a reviewer is nominated, a job is created to send out the498 # When a reviewer is nominated, a job is created to send out the
460 # review request email.499 # review request email.
500 bmp = self.factory.makeBranchMergeProposal(
501 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
461 reviewer = self.factory.makePerson()502 reviewer = self.factory.makePerson()
462 pop_notifications()503 pop_notifications()
463 self.bmp.nominateReviewer(reviewer, self.owner, None)504 with person_logged_in(bmp.registrant):
505 bmp.nominateReviewer(reviewer, bmp.registrant, None)
464 # No email is sent.506 # No email is sent.
465 sent_mail = pop_notifications()507 sent_mail = pop_notifications()
466 self.assertEqual([], sent_mail)508 self.assertEqual([], sent_mail)
467 # A job is created.509 # A job is created.
468 review_request_job = self.getReviewNotificationEmail()510 review_request_job = self.getReviewNotificationEmail(bmp)
469 self.assertEqual(self.bmp, review_request_job.branch_merge_proposal)511 self.assertEqual(bmp, review_request_job.branch_merge_proposal)
470 self.assertEqual(reviewer, review_request_job.reviewer)512 self.assertEqual(reviewer, review_request_job.reviewer)
471 self.assertEqual(self.bmp.registrant, review_request_job.requester)513 self.assertEqual(bmp.registrant, review_request_job.requester)
472514
473 def test_nominateReview_email_content(self):515 def test_nominateReview_email_content(self):
474 # The email that is sent contains the description of the proposal, and516 # The email that is sent contains the description of the proposal, and
475 # a link to the proposal.517 # a link to the proposal.
518 bmp = self.factory.makeBranchMergeProposal(
519 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
476 reviewer = self.factory.makePerson()520 reviewer = self.factory.makePerson()
477 self.bmp.description = 'This branch is awesome.'521 with person_logged_in(bmp.registrant):
478 self.bmp.nominateReviewer(reviewer, self.owner, None)522 bmp.description = 'This branch is awesome.'
479 review_request_job = self.getReviewNotificationEmail()523 bmp.nominateReviewer(reviewer, bmp.registrant, None)
524 review_request_job = self.getReviewNotificationEmail(bmp)
480 review_request_job.run()525 review_request_job.run()
481 [sent_mail] = pop_notifications()526 [sent_mail] = pop_notifications()
482 expected = dedent("""\527 expected = dedent("""\
@@ -488,25 +533,25 @@
488 %(bmp)s533 %(bmp)s
489 You are requested to review the proposed merge of %(source)s into %(target)s.534 You are requested to review the proposed merge of %(source)s into %(target)s.
490 """ % {535 """ % {
491 'source': self.bmp.source_branch.bzr_identity,536 'source': bmp.source_branch.bzr_identity,
492 'target': self.bmp.target_branch.bzr_identity,537 'target': bmp.target_branch.bzr_identity,
493 'bmp': canonical_url(self.bmp)})538 'bmp': canonical_url(bmp)})
494 self.assertEqual(expected, sent_mail.get_payload(decode=True))539 self.assertEqual(expected, sent_mail.get_payload(decode=True))
495540
496 def test_nominateReview_emails_team_address(self):541 def test_nominateReview_emails_team_address(self):
497 # If a review request is made for a team, the members of the team are542 # If a review request is made for a team, the members of the team are
498 # sent an email.543 # sent an email.
544 bmp = self.factory.makeBranchMergeProposal(
545 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
499 eric = self.factory.makePerson(546 eric = self.factory.makePerson(
500 displayname='Eric the Viking', email='eric@vikings.example.com')547 displayname='Eric the Viking', email='eric@vikings.example.com')
501 black_beard = self.factory.makePerson(548 black_beard = self.factory.makePerson(
502 displayname='Black Beard', email='black@pirates.example.com')549 displayname='Black Beard', email='black@pirates.example.com')
503 review_team = self.factory.makeTeam(owner=eric)550 review_team = self.factory.makeTeam(owner=eric, members=[black_beard])
504 login_person(eric)
505 review_team.addMember(black_beard, eric)
506 pop_notifications()551 pop_notifications()
507 login_person(self.owner)552 with person_logged_in(bmp.registrant):
508 self.bmp.nominateReviewer(review_team, self.owner, None)553 bmp.nominateReviewer(review_team, bmp.registrant, None)
509 review_request_job = self.getReviewNotificationEmail()554 review_request_job = self.getReviewNotificationEmail(bmp)
510 review_request_job.run()555 review_request_job.run()
511 sent_mail = pop_notifications()556 sent_mail = pop_notifications()
512 self.assertEqual(557 self.assertEqual(
@@ -517,11 +562,14 @@
517 def test_requestReviewWithPrivateEmail(self):562 def test_requestReviewWithPrivateEmail(self):
518 # We can request a review, even when one of the parties involved has a563 # We can request a review, even when one of the parties involved has a
519 # private email address.564 # private email address.
520 candidate = self.makePersonWithHiddenEmail()565 bmp = self.factory.makeBranchMergeProposal(
566 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
567 candidate = self.factory.makePerson(hide_email_addresses=True)
521 # Request a review and prepare the mailer.568 # Request a review and prepare the mailer.
522 self.bmp.nominateReviewer(candidate, self.owner, None)569 with person_logged_in(bmp.registrant):
570 bmp.nominateReviewer(candidate, bmp.registrant, None)
523 # Send the mail.571 # Send the mail.
524 review_request_job = self.getReviewNotificationEmail()572 review_request_job = self.getReviewNotificationEmail(bmp)
525 review_request_job.run()573 review_request_job.run()
526 mails = pop_notifications()574 mails = pop_notifications()
527 self.assertEqual(1, len(mails))575 self.assertEqual(1, len(mails))
528576
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py 2010-10-26 15:47:24 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py 2010-10-28 22:34:49 +0000
@@ -357,7 +357,8 @@
357357
358 def test_iterReady_supports_review_requested(self):358 def test_iterReady_supports_review_requested(self):
359 # iterReady will also return pending ReviewRequestedEmailJobs.359 # iterReady will also return pending ReviewRequestedEmailJobs.
360 bmp = self.makeBranchMergeProposal()360 bmp = self.makeBranchMergeProposal(
361 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
361 self.completePendingJobs()362 self.completePendingJobs()
362 reviewer = self.factory.makePerson()363 reviewer = self.factory.makePerson()
363 bmp.nominateReviewer(reviewer, bmp.registrant)364 bmp.nominateReviewer(reviewer, bmp.registrant)
@@ -380,7 +381,8 @@
380381
381 def test_iterReady_supports_updated_emails(self):382 def test_iterReady_supports_updated_emails(self):
382 # iterReady will also return pending MergeProposalUpdatedEmailJob.383 # iterReady will also return pending MergeProposalUpdatedEmailJob.
383 bmp = self.makeBranchMergeProposal()384 bmp = self.makeBranchMergeProposal(
385 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
384 self.completePendingJobs()386 self.completePendingJobs()
385 old_merge_proposal = BranchMergeProposalDelta.snapshot(bmp)387 old_merge_proposal = BranchMergeProposalDelta.snapshot(bmp)
386 bmp.commit_message = 'new commit message'388 bmp.commit_message = 'new commit message'
387389
=== modified file 'lib/lp/code/subscribers/branchmergeproposal.py'
--- lib/lp/code/subscribers/branchmergeproposal.py 2010-10-18 03:39:28 +0000
+++ lib/lp/code/subscribers/branchmergeproposal.py 2010-10-28 22:34:49 +0000
@@ -10,6 +10,7 @@
10from zope.component import getUtility10from zope.component import getUtility
1111
12from lp.code.adapters.branch import BranchMergeProposalDelta12from lp.code.adapters.branch import BranchMergeProposalDelta
13from lp.code.enums import BranchMergeProposalStatus
13from lp.code.interfaces.branchmergeproposal import (14from lp.code.interfaces.branchmergeproposal import (
14 IMergeProposalNeedsReviewEmailJobSource,15 IMergeProposalNeedsReviewEmailJobSource,
15 IMergeProposalUpdatedEmailJobSource,16 IMergeProposalUpdatedEmailJobSource,
@@ -48,6 +49,14 @@
48 from_person = None49 from_person = None
49 else:50 else:
50 from_person = IPerson(event.user)51 from_person = IPerson(event.user)
52 # If the merge proposal was work in progress, then we don't want to send
53 # out an email as the needs review email will cover that.
54 old_status = event.object_before_modification.queue_status
55 if old_status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
56 # Unless the new status is merged. If this occurs we really should
57 # send out an email.
58 if merge_proposal.queue_status != BranchMergeProposalStatus.MERGED:
59 return
51 # Create a delta of the changes. If there are no changes to report, then60 # Create a delta of the changes. If there are no changes to report, then
52 # we're done.61 # we're done.
53 delta = BranchMergeProposalDelta.construct(62 delta = BranchMergeProposalDelta.construct(
@@ -63,5 +72,8 @@
6372
64def review_requested(vote_reference, event):73def review_requested(vote_reference, event):
65 """Notify the reviewer that they have been requested to review."""74 """Notify the reviewer that they have been requested to review."""
66 getUtility(IReviewRequestedEmailJobSource).create(vote_reference)75 # Don't send email if the proposal is work in progress.
76 bmp_status = vote_reference.branch_merge_proposal.queue_status
77 if bmp_status != BranchMergeProposalStatus.WORK_IN_PROGRESS:
78 getUtility(IReviewRequestedEmailJobSource).create(vote_reference)
6779