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
1=== modified file 'lib/lp/code/doc/branch-merge-proposal-notifications.txt'
2--- lib/lp/code/doc/branch-merge-proposal-notifications.txt 2010-05-27 02:04:21 +0000
3+++ lib/lp/code/doc/branch-merge-proposal-notifications.txt 2010-10-20 01:21:31 +0000
4@@ -101,7 +101,7 @@
5 ... displayname="Eric", email="eric@example.com")
6 >>> # To avoid needing to access branches, pre-populate diffs.
7 >>> bmp = source_branch.addLandingTarget(
8- ... registrant, target_branch)
9+ ... registrant, target_branch, needs_review=True)
10 >>> removeSecurityProxy(bmp).preview_diff = preview_diff
11 >>> # Fake the update preview diff as done.
12 >>> bmp.next_preview_diff_job.start()
13@@ -156,7 +156,8 @@
14 >>> bmp.deleteProposal()
15 >>> bmp = source_branch.addLandingTarget(
16 ... registrant, target_branch,
17- ... description=initial_comment, review_requests=reviewers)
18+ ... description=initial_comment, review_requests=reviewers,
19+ ... needs_review=True)
20 >>> removeSecurityProxy(bmp).preview_diff = preview_diff
21 >>> # Fake the update preview diff as done.
22 >>> bmp.next_preview_diff_job.start()
23
24=== modified file 'lib/lp/code/model/branch.py'
25--- lib/lp/code/model/branch.py 2010-10-18 03:39:28 +0000
26+++ lib/lp/code/model/branch.py 2010-10-20 01:21:31 +0000
27@@ -437,7 +437,8 @@
28 reviewer, registrant, review_type, _notify_listeners=False)
29
30 notify(NewBranchMergeProposalEvent(bmp))
31- notify(BranchMergeProposalNeedsReviewEvent(bmp))
32+ if needs_review:
33+ notify(BranchMergeProposalNeedsReviewEvent(bmp))
34
35 return bmp
36
37
38=== modified file 'lib/lp/code/model/branchmergeproposal.py'
39--- lib/lp/code/model/branchmergeproposal.py 2010-10-07 22:46:08 +0000
40+++ lib/lp/code/model/branchmergeproposal.py 2010-10-20 01:21:31 +0000
41@@ -61,6 +61,7 @@
42 WrongBranchMergeProposal,
43 )
44 from lp.code.event.branchmergeproposal import (
45+ BranchMergeProposalNeedsReviewEvent,
46 BranchMergeProposalStatusChangeEvent,
47 NewCodeReviewCommentEvent,
48 ReviewerNominatedEvent,
49@@ -412,6 +413,11 @@
50 # review state.
51 if _date_requested is None:
52 _date_requested = UTC_NOW
53+ # If we are going from work in progress to needs review, then reset
54+ # the root message id and trigger a job to send out the email.
55+ if self.queue_status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
56+ self.root_message_id = None
57+ notify(BranchMergeProposalNeedsReviewEvent(self))
58 if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:
59 self._transitionToState(BranchMergeProposalStatus.NEEDS_REVIEW)
60 self.date_review_requested = _date_requested
61
62=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
63--- lib/lp/code/model/tests/test_branchmergeproposal.py 2010-10-18 03:39:28 +0000
64+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2010-10-20 01:21:31 +0000
65@@ -3,6 +3,8 @@
66
67 # pylint: disable-msg=F0401
68
69+from __future__ import with_statement
70+
71 """Tests for BranchMergeProposals."""
72
73 __metaclass__ = type
74@@ -24,9 +26,7 @@
75
76 from canonical.database.constants import UTC_NOW
77 from canonical.launchpad.ftests import (
78- ANONYMOUS,
79 import_secret_test_key,
80- login,
81 syncUpdate,
82 )
83 from canonical.launchpad.interfaces.launchpad import IPrivacy
84@@ -77,7 +77,10 @@
85 from lp.registry.interfaces.person import IPersonSet
86 from lp.registry.interfaces.product import IProductSet
87 from lp.testing import (
88+ ANONYMOUS,
89+ login,
90 login_person,
91+ person_logged_in,
92 TestCaseWithFactory,
93 )
94 from lp.testing.factory import (
95@@ -753,8 +756,10 @@
96 def setUp(self):
97 TestCaseWithFactory.setUp(self, user='test@canonical.com')
98
99- def test_notifyOnCreate(self):
100- """Ensure that a notification is emitted on creation"""
101+ def test_notifyOnCreate_needs_review(self):
102+ # When a merge proposal is created needing review, the
103+ # BranchMergeProposalNeedsReviewEvent is raised as well as the usual
104+ # NewBranchMergeProposalEvent.
105 source_branch = self.factory.makeProductBranch()
106 target_branch = self.factory.makeProductBranch(
107 product=source_branch.product)
108@@ -762,9 +767,48 @@
109 result, events = self.assertNotifies(
110 [NewBranchMergeProposalEvent,
111 BranchMergeProposalNeedsReviewEvent],
112+ source_branch.addLandingTarget, registrant, target_branch,
113+ needs_review=True)
114+ self.assertEqual(result, events[0].object)
115+
116+ def test_notifyOnCreate_work_in_progress(self):
117+ # When a merge proposal is created as work in progress, the
118+ # BranchMergeProposalNeedsReviewEvent is not raised.
119+ source_branch = self.factory.makeProductBranch()
120+ target_branch = self.factory.makeProductBranch(
121+ product=source_branch.product)
122+ registrant = self.factory.makePerson()
123+ result, events = self.assertNotifies(
124+ [NewBranchMergeProposalEvent],
125 source_branch.addLandingTarget, registrant, target_branch)
126 self.assertEqual(result, events[0].object)
127
128+ def test_needs_review_from_work_in_progress(self):
129+ # Transitioning from work in progress to needs review raises the
130+ # BranchMergeProposalNeedsReviewEvent event.
131+ bmp = self.factory.makeBranchMergeProposal(
132+ set_state=BranchMergeProposalStatus.WORK_IN_PROGRESS)
133+ with person_logged_in(bmp.registrant):
134+ self.assertNotifies(
135+ [BranchMergeProposalNeedsReviewEvent],
136+ bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
137+
138+ def test_needs_review_no_op(self):
139+ # Calling needs review when in needs review does not notify.
140+ bmp = self.factory.makeBranchMergeProposal(
141+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
142+ with person_logged_in(bmp.registrant):
143+ self.assertNoNotification(
144+ bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
145+
146+ def test_needs_review_from_approved(self):
147+ # Calling needs review when approved does not notify either.
148+ bmp = self.factory.makeBranchMergeProposal(
149+ set_state=BranchMergeProposalStatus.CODE_APPROVED)
150+ with person_logged_in(bmp.registrant):
151+ self.assertNoNotification(
152+ bmp.setStatus, BranchMergeProposalStatus.NEEDS_REVIEW)
153+
154 def test_getNotificationRecipients(self):
155 """Ensure that recipients can be added/removed with subscribe"""
156 bmp = self.factory.makeBranchMergeProposal()
157
158=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
159--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py 2010-10-18 01:57:32 +0000
160+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py 2010-10-20 01:21:31 +0000
161@@ -28,6 +28,7 @@
162 LaunchpadZopelessLayer,
163 )
164 from lp.code.adapters.branch import BranchMergeProposalDelta
165+from lp.code.enums import BranchMergeProposalStatus
166 from lp.code.interfaces.branchmergeproposal import (
167 IBranchMergeProposalJob,
168 IBranchMergeProposalJobSource,
169@@ -326,9 +327,9 @@
170 jobs = self.job_source.iterReady()
171 self.assertEqual(0, len(jobs))
172
173- def makeBranchMergeProposal(self):
174+ def makeBranchMergeProposal(self, set_state=None):
175 # Make a merge proposal that would have a ready update diff job.
176- bmp = self.factory.makeBranchMergeProposal()
177+ bmp = self.factory.makeBranchMergeProposal(set_state=set_state)
178 self.factory.makeRevisionsForBranch(bmp.source_branch)
179 self.factory.makeRevisionsForBranch(bmp.target_branch)
180 return bmp
181@@ -337,7 +338,8 @@
182 # Once the update preview diff job has finished running, then
183 # iterReady returns the next job for the merge proposal, which is in
184 # this case the initial email job.
185- bmp = self.makeBranchMergeProposal()
186+ bmp = self.makeBranchMergeProposal(
187+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
188 [update_diff] = self.job_source.iterReady()
189 update_diff.start()
190 update_diff.complete()