Launchpad itself

Merge lp:~thumper/launchpad/defer-wip-email into lp:launchpad

  1. defer-wip-email
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: 11769
Proposed branch: lp:~thumper/launchpad/defer-wip-email
Merge into: lp:launchpad
Prerequisite: lp:~thumper/launchpad/needs-review-event
Diff against target: 190 lines (+64/-10)
5 files modified
lib/lp/code/doc/branch-merge-proposal-notifications.txt (+3/-2)
lib/lp/code/model/branch.py (+2/-1)
lib/lp/code/model/branchmergeproposal.py (+6/-0)
lib/lp/code/model/tests/test_branchmergeproposal.py (+48/-4)
lib/lp/code/model/tests/test_branchmergeproposaljobs.py (+5/-3)
To merge this branch: bzr merge lp:~thumper/launchpad/defer-wip-email
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Review via email: mp+38791@code.launchpad.net

Commit message

Only send the initial review email when the merge proposal needs review.

Description of the change

Change the addLandingTarget method to only raise the needs review event if the proposal is actually created in needs review state.

Also raise the event when moving from work in progress to needs review.

tests:
  TestMergeProposalNotification

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/doc/branch-merge-proposal-notifications.txt'
--- lib/lp/code/doc/branch-merge-proposal-notifications.txt 2010-05-27 02:04:21 +0000
+++ lib/lp/code/doc/branch-merge-proposal-notifications.txt 2010-10-20 01:21:31 +0000
@@ -101,7 +101,7 @@
101 ... displayname="Eric", email="eric@example.com")101 ... displayname="Eric", email="eric@example.com")
102 >>> # To avoid needing to access branches, pre-populate diffs.102 >>> # To avoid needing to access branches, pre-populate diffs.
103 >>> bmp = source_branch.addLandingTarget(103 >>> bmp = source_branch.addLandingTarget(
104 ... registrant, target_branch)104 ... registrant, target_branch, needs_review=True)
105 >>> removeSecurityProxy(bmp).preview_diff = preview_diff105 >>> removeSecurityProxy(bmp).preview_diff = preview_diff
106 >>> # Fake the update preview diff as done.106 >>> # Fake the update preview diff as done.
107 >>> bmp.next_preview_diff_job.start()107 >>> bmp.next_preview_diff_job.start()
@@ -156,7 +156,8 @@
156 >>> bmp.deleteProposal()156 >>> bmp.deleteProposal()
157 >>> bmp = source_branch.addLandingTarget(157 >>> bmp = source_branch.addLandingTarget(
158 ... registrant, target_branch,158 ... registrant, target_branch,
159 ... description=initial_comment, review_requests=reviewers)159 ... description=initial_comment, review_requests=reviewers,
160 ... needs_review=True)
160 >>> removeSecurityProxy(bmp).preview_diff = preview_diff161 >>> removeSecurityProxy(bmp).preview_diff = preview_diff
161 >>> # Fake the update preview diff as done.162 >>> # Fake the update preview diff as done.
162 >>> bmp.next_preview_diff_job.start()163 >>> bmp.next_preview_diff_job.start()
163164
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2010-10-18 03:39:28 +0000
+++ lib/lp/code/model/branch.py 2010-10-20 01:21:31 +0000
@@ -437,7 +437,8 @@
437 reviewer, registrant, review_type, _notify_listeners=False)437 reviewer, registrant, review_type, _notify_listeners=False)
438438
439 notify(NewBranchMergeProposalEvent(bmp))439 notify(NewBranchMergeProposalEvent(bmp))
440 notify(BranchMergeProposalNeedsReviewEvent(bmp))440 if needs_review:
441 notify(BranchMergeProposalNeedsReviewEvent(bmp))
441442
442 return bmp443 return bmp
443444
444445
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2010-10-07 22:46:08 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2010-10-20 01:21:31 +0000
@@ -61,6 +61,7 @@
61 WrongBranchMergeProposal,61 WrongBranchMergeProposal,
62 )62 )
63from lp.code.event.branchmergeproposal import (63from lp.code.event.branchmergeproposal import (
64 BranchMergeProposalNeedsReviewEvent,
64 BranchMergeProposalStatusChangeEvent,65 BranchMergeProposalStatusChangeEvent,
65 NewCodeReviewCommentEvent,66 NewCodeReviewCommentEvent,
66 ReviewerNominatedEvent,67 ReviewerNominatedEvent,
@@ -412,6 +413,11 @@
412 # review state.413 # review state.
413 if _date_requested is None:414 if _date_requested is None:
414 _date_requested = UTC_NOW415 _date_requested = UTC_NOW
416 # If we are going from work in progress to needs review, then reset
417 # the root message id and trigger a job to send out the email.
418 if self.queue_status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
419 self.root_message_id = None
420 notify(BranchMergeProposalNeedsReviewEvent(self))
415 if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:421 if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:
416 self._transitionToState(BranchMergeProposalStatus.NEEDS_REVIEW)422 self._transitionToState(BranchMergeProposalStatus.NEEDS_REVIEW)
417 self.date_review_requested = _date_requested423 self.date_review_requested = _date_requested
418424
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2010-10-18 03:39:28 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2010-10-20 01:21:31 +0000
@@ -3,6 +3,8 @@
33
4# pylint: disable-msg=F04014# pylint: disable-msg=F0401
55
6from __future__ import with_statement
7
6"""Tests for BranchMergeProposals."""8"""Tests for BranchMergeProposals."""
79
8__metaclass__ = type10__metaclass__ = type
@@ -24,9 +26,7 @@
2426
25from canonical.database.constants import UTC_NOW27from canonical.database.constants import UTC_NOW
26from canonical.launchpad.ftests import (28from canonical.launchpad.ftests import (
27 ANONYMOUS,
28 import_secret_test_key,29 import_secret_test_key,
29 login,
30 syncUpdate,30 syncUpdate,
31 )31 )
32from canonical.launchpad.interfaces.launchpad import IPrivacy32from canonical.launchpad.interfaces.launchpad import IPrivacy
@@ -77,7 +77,10 @@
77from lp.registry.interfaces.person import IPersonSet77from lp.registry.interfaces.person import IPersonSet
78from lp.registry.interfaces.product import IProductSet78from lp.registry.interfaces.product import IProductSet
79from lp.testing import (79from lp.testing import (
80 ANONYMOUS,
81 login,
80 login_person,82 login_person,
83 person_logged_in,
81 TestCaseWithFactory,84 TestCaseWithFactory,
82 )85 )
83from lp.testing.factory import (86from lp.testing.factory import (
@@ -753,8 +756,10 @@
753 def setUp(self):756 def setUp(self):
754 TestCaseWithFactory.setUp(self, user='test@canonical.com')757 TestCaseWithFactory.setUp(self, user='test@canonical.com')
755758
756 def test_notifyOnCreate(self):759 def test_notifyOnCreate_needs_review(self):
757 """Ensure that a notification is emitted on creation"""760 # When a merge proposal is created needing review, the
761 # BranchMergeProposalNeedsReviewEvent is raised as well as the usual
762 # NewBranchMergeProposalEvent.
758 source_branch = self.factory.makeProductBranch()763 source_branch = self.factory.makeProductBranch()
759 target_branch = self.factory.makeProductBranch(764 target_branch = self.factory.makeProductBranch(
760 product=source_branch.product)765 product=source_branch.product)
@@ -762,9 +767,48 @@
762 result, events = self.assertNotifies(767 result, events = self.assertNotifies(
763 [NewBranchMergeProposalEvent,768 [NewBranchMergeProposalEvent,
764 BranchMergeProposalNeedsReviewEvent],769 BranchMergeProposalNeedsReviewEvent],
770 source_branch.addLandingTarget, registrant, target_branch,
771 needs_review=True)
772 self.assertEqual(result, events[0].object)
773
774 def test_notifyOnCreate_work_in_progress(self):
775 # When a merge proposal is created as work in progress, the
776 # BranchMergeProposalNeedsReviewEvent is not raised.
777 source_branch = self.factory.makeProductBranch()
778 target_branch = self.factory.makeProductBranch(
779 product=source_branch.product)
780 registrant = self.factory.makePerson()
781 result, events = self.assertNotifies(
782 [NewBranchMergeProposalEvent],
765 source_branch.addLandingTarget, registrant, target_branch)783 source_branch.addLandingTarget, registrant, target_branch)
766 self.assertEqual(result, events[0].object)784 self.assertEqual(result, events[0].object)
767785
786 def test_needs_review_from_work_in_progress(self):
787 # Transitioning from work in progress to needs review raises the
788 # BranchMergeProposalNeedsReviewEvent event.
789 bmp = self.factory.makeBranchMergeProposal(
790 set_state=BranchMergeProposalStatus.WORK_IN_PROGRESS)
791 with person_logged_in(bmp.registrant):
792 self.assertNotifies(
793 [BranchMergeProposalNeedsReviewEvent],
794 bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
795
796 def test_needs_review_no_op(self):
797 # Calling needs review when in needs review does not notify.
798 bmp = self.factory.makeBranchMergeProposal(
799 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
800 with person_logged_in(bmp.registrant):
801 self.assertNoNotification(
802 bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
803
804 def test_needs_review_from_approved(self):
805 # Calling needs review when approved does not notify either.
806 bmp = self.factory.makeBranchMergeProposal(
807 set_state=BranchMergeProposalStatus.CODE_APPROVED)
808 with person_logged_in(bmp.registrant):
809 self.assertNoNotification(
810 bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
811
768 def test_getNotificationRecipients(self):812 def test_getNotificationRecipients(self):
769 """Ensure that recipients can be added/removed with subscribe"""813 """Ensure that recipients can be added/removed with subscribe"""
770 bmp = self.factory.makeBranchMergeProposal()814 bmp = self.factory.makeBranchMergeProposal()
771815
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py 2010-10-18 01:57:32 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py 2010-10-20 01:21:31 +0000
@@ -28,6 +28,7 @@
28 LaunchpadZopelessLayer,28 LaunchpadZopelessLayer,
29 )29 )
30from lp.code.adapters.branch import BranchMergeProposalDelta30from lp.code.adapters.branch import BranchMergeProposalDelta
31from lp.code.enums import BranchMergeProposalStatus
31from lp.code.interfaces.branchmergeproposal import (32from lp.code.interfaces.branchmergeproposal import (
32 IBranchMergeProposalJob,33 IBranchMergeProposalJob,
33 IBranchMergeProposalJobSource,34 IBranchMergeProposalJobSource,
@@ -326,9 +327,9 @@
326 jobs = self.job_source.iterReady()327 jobs = self.job_source.iterReady()
327 self.assertEqual(0, len(jobs))328 self.assertEqual(0, len(jobs))
328329
329 def makeBranchMergeProposal(self):330 def makeBranchMergeProposal(self, set_state=None):
330 # Make a merge proposal that would have a ready update diff job.331 # Make a merge proposal that would have a ready update diff job.
331 bmp = self.factory.makeBranchMergeProposal()332 bmp = self.factory.makeBranchMergeProposal(set_state=set_state)
332 self.factory.makeRevisionsForBranch(bmp.source_branch)333 self.factory.makeRevisionsForBranch(bmp.source_branch)
333 self.factory.makeRevisionsForBranch(bmp.target_branch)334 self.factory.makeRevisionsForBranch(bmp.target_branch)
334 return bmp335 return bmp
@@ -337,7 +338,8 @@
337 # Once the update preview diff job has finished running, then338 # Once the update preview diff job has finished running, then
338 # iterReady returns the next job for the merge proposal, which is in339 # iterReady returns the next job for the merge proposal, which is in
339 # this case the initial email job.340 # this case the initial email job.
340 bmp = self.makeBranchMergeProposal()341 bmp = self.makeBranchMergeProposal(
342 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
341 [update_diff] = self.job_source.iterReady()343 [update_diff] = self.job_source.iterReady()
342 update_diff.start()344 update_diff.start()
343 update_diff.complete()345 update_diff.complete()