Launchpad itself

Merge lp:~lifeless/launchpad/merge into lp:launchpad

  1. merge
  2. Merge into devel
Proposed by Robert Collins
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 10831
Proposed branch: lp:~lifeless/launchpad/merge
Merge into: lp:launchpad
Diff against target: 292 lines (+95/-36)
6 files modified
lib/lp/code/configure.zcml (+0/-1)
lib/lp/code/doc/branch-merge-proposals.txt (+1/-1)
lib/lp/code/interfaces/branchmergeproposal.py (+3/-3)
lib/lp/code/model/branchmergeproposal.py (+28/-25)
lib/lp/code/model/tests/test_branchmergeproposals.py (+62/-5)
lib/lp/testing/factory.py (+1/-1)
To merge this branch: bzr merge lp:~lifeless/launchpad/merge
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Review via email: mp+23523@code.launchpad.net

Commit message

Merge proposal tidy up: always dequeue when leaving the queued state; permit merge-failed; store the reviewed revid when queueing by default.

Description of the change

Fix a few bugs around merge proposals:
 - when transitioning out of 'queued', always dequeue.
 - permit transitioning to merge-failed
 - use the reviewed revid by default when queueing

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

lib/lp/code/interfaces/branchmergeproposal.py
 - change needs indenting a space
actually your indentation is all over the place in the tests too (so says the diff)

setStatus should call mergeFailed, and pass in the revision id

should also have a test where you can call setStatus for queued and pass in a revision id

review: Needs Fixing
Revision history for this message
Robert Collins (lifeless) wrote :

Ugh, there are tabs in teh diff. Fail-vim-rc-in-vm. will fix.

There are already tests for pasing in a revid to setStatus(QUEUED, revision_id=).

mergeFailed seems damaged, it uses self.merger, which isn't defined or used anywhere.

If I can change its definition, I can call it - so I'll do that change.

...

Have looked into it, mergeFailed isn't used anywhere, isn't exposed in API's and is buggy today (it doesn't dequeue completely). Its behaviour isn't tested either. To fix it and make it safe to call any point will either make setStatus significantly more complex (have to call dequeue when moving from QUEUED in every case except when moving to MERGE_FAILED), or it will make mergeFailed complex (have to accept being called when not in state QUEUED).

I looked for and could not find docs saying about whether setStatus was being deleted, or the other functions were, or something else, and couldn't find anything, so I took the approach that leaves the least code, with no reduction in capabilities - deleting mergeFailed.

Revision history for this message
Robert Collins (lifeless) wrote :

Thanks for the review.

Revision history for this message
Robert Collins (lifeless) wrote :

Tim, I've checked for the transition you were concerned about (non reviewer -> merge failed) and tightened that up appropriately, with a test.

Revision history for this message
Tim Penhey (thumper) wrote :

This is good enough for now, but I really want to refactor the guts out of this whole area.

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/configure.zcml'
2--- lib/lp/code/configure.zcml 2010-04-28 22:03:05 +0000
3+++ lib/lp/code/configure.zcml 2010-04-29 22:33:35 +0000
4@@ -246,7 +246,6 @@
5 requestReview
6 approveBranch
7 rejectBranch
8- mergeFailed
9 markAsMerged
10 resubmit
11 enqueue
12
13=== modified file 'lib/lp/code/doc/branch-merge-proposals.txt'
14--- lib/lp/code/doc/branch-merge-proposals.txt 2010-02-26 00:30:09 +0000
15+++ lib/lp/code/doc/branch-merge-proposals.txt 2010-04-29 22:33:35 +0000
16@@ -348,7 +348,7 @@
17 Anyone that has rights to edit the merge proposal can say that the
18 merge failed.
19
20- >>> proposal.mergeFailed(merger=sample_person)
21+ >>> proposal.setStatus(BranchMergeProposalStatus.MERGE_FAILED)
22 >>> print proposal.queue_status.title
23 Code failed to merge
24
25
26=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
27--- lib/lp/code/interfaces/branchmergeproposal.py 2010-03-05 03:35:10 +0000
28+++ lib/lp/code/interfaces/branchmergeproposal.py 2010-04-29 22:33:35 +0000
29@@ -338,6 +338,9 @@
30 If the proposal is not in the Approved state before this method
31 is called, approveBranch is called with the reviewer and revision_id
32 specified.
33+
34+ If None is supplied as the revision_id, the proposals
35+ reviewed_revision_id is used.
36 """
37
38 def dequeue():
39@@ -350,9 +353,6 @@
40 def moveToFrontOfQueue():
41 """Move the queue proposal to the front of the queue."""
42
43- def mergeFailed(merger):
44- """Mark the proposal as 'Code failed to merge'."""
45-
46 def markAsMerged(merged_revno=None, date_merged=None,
47 merge_reporter=None):
48 """Mark the branch merge proposal as merged.
49
50=== modified file 'lib/lp/code/model/branchmergeproposal.py'
51--- lib/lp/code/model/branchmergeproposal.py 2010-02-26 09:54:20 +0000
52+++ lib/lp/code/model/branchmergeproposal.py 2010-04-29 22:33:35 +0000
53@@ -77,18 +77,29 @@
54 [wip, needs_review, code_approved, rejected,
55 merged, merge_failed, queued, superseded
56 ] = BranchMergeProposalStatus.items
57- # Transitioning to code approved, rejected or queued from
58+ # Transitioning to code approved, rejected, failed or queued from
59 # work in progress, needs review or merge failed needs the
60 # user to be a valid reviewer, other states are fine.
61 valid_reviewer = proposal.target_branch.isPersonTrustedReviewer(user)
62- if (next_state == rejected and not valid_reviewer):
63- return False
64- # Non-reviewers can toggle between code_approved and queued, but not
65- # make anything else approved or queued.
66- elif (next_state in (code_approved, queued) and
67- from_state not in (code_approved, queued)
68- and not valid_reviewer):
69- return False
70+ reviewed_ok_states = (code_approved, queued, merge_failed)
71+ if not valid_reviewer:
72+ # Non reviewers cannot reject proposals [XXX: what about their own?]
73+ if next_state == rejected:
74+ return False
75+ # Non-reviewers can toggle within the reviewed ok states
76+ # (approved/queued/failed): they can dequeue something they spot an
77+ # environmental issue with (queued or failed to approved). Retry things
78+ # that had an environmental issue (failed or approved to queued) and note
79+ # things as failing (approved and queued to failed).
80+ # This is perhaps more generous than needed, but its not clearly wrong
81+ # - a key concern is to prevent non reviewers putting things in the
82+ # queue that haven't been oked (and thus moved to approved or one of the
83+ # workflow states that approved leads to).
84+ elif (next_state in reviewed_ok_states and
85+ from_state not in reviewed_ok_states):
86+ return False
87+ else:
88+ return True
89 else:
90 return True
91
92@@ -327,23 +338,23 @@
93 # XXX - rockstar - 9 Oct 2008 - jml suggested in a review that this
94 # would be better as a dict mapping.
95 # See bug #281060.
96+ if (self.queue_status == BranchMergeProposalStatus.QUEUED and
97+ status != BranchMergeProposalStatus.QUEUED):
98+ self.dequeue()
99 if status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
100 self.setAsWorkInProgress()
101 elif status == BranchMergeProposalStatus.NEEDS_REVIEW:
102 self.requestReview()
103 elif status == BranchMergeProposalStatus.CODE_APPROVED:
104- # Other half of the edge case. If the status is currently queued,
105- # we need to dequeue, otherwise we just approve the branch.
106- if self.queue_status == BranchMergeProposalStatus.QUEUED:
107- self.dequeue()
108- else:
109- self.approveBranch(user, revision_id)
110+ self.approveBranch(user, revision_id)
111 elif status == BranchMergeProposalStatus.REJECTED:
112 self.rejectBranch(user, revision_id)
113 elif status == BranchMergeProposalStatus.QUEUED:
114 self.enqueue(user, revision_id)
115 elif status == BranchMergeProposalStatus.MERGED:
116 self.markAsMerged(merge_reporter=user)
117+ elif status == BranchMergeProposalStatus.MERGE_FAILED:
118+ self._transitionToState(status, user=user)
119 else:
120 raise AssertionError('Unexpected queue status: ' % status)
121
122@@ -381,7 +392,7 @@
123
124 def _reviewProposal(self, reviewer, next_state, revision_id,
125 _date_reviewed=None):
126- """Set the proposal to one of the two review statuses."""
127+ """Set the proposal to next_state."""
128 # Check the reviewer can review the code for the target branch.
129 old_state = self.queue_status
130 if not self.target_branch.isPersonTrustedReviewer(reviewer):
131@@ -435,7 +446,7 @@
132 self.queue_status = BranchMergeProposalStatus.QUEUED
133 self.queue_position = position
134 self.queuer = queuer
135- self.queued_revision_id = revision_id
136+ self.queued_revision_id = revision_id or self.reviewed_revision_id
137 self.date_queued = UTC_NOW
138 self.syncUpdate()
139
140@@ -467,14 +478,6 @@
141 self.queue_position = first_entry.queue_position - 1
142 self.syncUpdate()
143
144- def mergeFailed(self, merger):
145- """See `IBranchMergeProposal`."""
146- self._transitionToState(
147- BranchMergeProposalStatus.MERGE_FAILED, merger)
148- self.merger = merger
149- # Remove from the queue.
150- self.queue_position = None
151-
152 def markAsMerged(self, merged_revno=None, date_merged=None,
153 merge_reporter=None):
154 """See `IBranchMergeProposal`."""
155
156=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
157--- lib/lp/code/model/tests/test_branchmergeproposals.py 2010-03-30 09:56:10 +0000
158+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2010-04-29 22:33:35 +0000
159@@ -117,7 +117,7 @@
160 BranchMergeProposalStatus.CODE_APPROVED: 'approveBranch',
161 BranchMergeProposalStatus.REJECTED: 'rejectBranch',
162 BranchMergeProposalStatus.MERGED: 'markAsMerged',
163- BranchMergeProposalStatus.MERGE_FAILED: 'mergeFailed',
164+ BranchMergeProposalStatus.MERGE_FAILED: 'setStatus',
165 BranchMergeProposalStatus.QUEUED: 'enqueue',
166 BranchMergeProposalStatus.SUPERSEDED: 'resubmit',
167 }
168@@ -135,17 +135,21 @@
169
170 def _attemptTransition(self, proposal, to_state):
171 """Try to transition the proposal into the state `to_state`."""
172+ kwargs = {}
173 method = getattr(proposal, self.transition_functions[to_state])
174 if to_state in (BranchMergeProposalStatus.CODE_APPROVED,
175 BranchMergeProposalStatus.REJECTED,
176 BranchMergeProposalStatus.QUEUED):
177 args = [proposal.target_branch.owner, 'some_revision_id']
178- elif to_state in (BranchMergeProposalStatus.MERGE_FAILED,
179- BranchMergeProposalStatus.SUPERSEDED):
180+ elif to_state in (BranchMergeProposalStatus.SUPERSEDED,):
181 args = [proposal.registrant]
182+ elif to_state in (BranchMergeProposalStatus.MERGE_FAILED,):
183+ # transition via setStatus.
184+ args = [to_state]
185+ kwargs = dict(user=proposal.target_branch.owner)
186 else:
187 args = []
188- method(*args)
189+ method(*args, **kwargs)
190
191 def assertGoodTransition(self, from_state, to_state):
192 """Assert that we can go from `from_state` to `to_state`."""
193@@ -267,6 +271,20 @@
194 """We can go from merge failed to any other state."""
195 self.assertAllTransitionsGood(BranchMergeProposalStatus.MERGE_FAILED)
196
197+ def test_transition_from_merge_failed_to_queued_non_reviewer(self):
198+ # Contributors can requeue to retry after environmental issues fail a
199+ # merge.
200+ proposal = self.factory.makeBranchMergeProposal()
201+ self.assertFalse(proposal.target_branch.isPersonTrustedReviewer(
202+ proposal.source_branch.owner))
203+ self.assertValidTransitions(set([
204+ BranchMergeProposalStatus.MERGE_FAILED,
205+ BranchMergeProposalStatus.CODE_APPROVED,
206+ # It is always valid to go to the same state.
207+ BranchMergeProposalStatus.QUEUED]),
208+ proposal, BranchMergeProposalStatus.QUEUED,
209+ proposal.source_branch.owner)
210+
211 def test_transitions_from_queued_dequeue(self):
212 # When a proposal is dequeued it is set to code approved, and the
213 # queue position is reset.
214@@ -298,11 +316,25 @@
215 proposal = self.factory.makeBranchMergeProposal(
216 target_branch=self.target_branch,
217 set_state=BranchMergeProposalStatus.QUEUED)
218- proposal.mergeFailed(None)
219+ proposal.setStatus(BranchMergeProposalStatus.MERGE_FAILED)
220 self.assertProposalState(
221 proposal, BranchMergeProposalStatus.MERGE_FAILED)
222 self.assertIs(None, proposal.queue_position)
223
224+ def test_transition_to_merge_failed_non_reviewer(self):
225+ # non reviewers cannot set merge-failed (target branch owners are
226+ # implicitly reviewers).
227+ proposal = self.factory.makeBranchMergeProposal()
228+ self.assertFalse(proposal.target_branch.isPersonTrustedReviewer(
229+ proposal.source_branch.owner))
230+ self.assertValidTransitions(set([
231+ # It is always valid to go to the same state.
232+ BranchMergeProposalStatus.MERGE_FAILED,
233+ BranchMergeProposalStatus.CODE_APPROVED,
234+ BranchMergeProposalStatus.QUEUED]),
235+ proposal, BranchMergeProposalStatus.MERGE_FAILED,
236+ proposal.source_branch.owner)
237+
238 def test_transitions_to_wip_resets_reviewer(self):
239 # When a proposal was approved and is moved back into work in progress
240 # the reviewer, date reviewed, and reviewed revision are all reset.
241@@ -328,6 +360,19 @@
242 self.target_branch = self.factory.makeProductBranch()
243 login_person(self.target_branch.owner)
244
245+ def test_set_status_approved_to_queued(self):
246+ # setState can change an approved merge proposal to Work In Progress,
247+ # which will set the revision id to the reviewed revision id if not
248+ # supplied.
249+ proposal = self.factory.makeBranchMergeProposal(
250+ target_branch=self.target_branch,
251+ set_state=BranchMergeProposalStatus.CODE_APPROVED)
252+ proposal.approveBranch(proposal.target_branch.owner, '250')
253+ proposal.setStatus(BranchMergeProposalStatus.QUEUED)
254+ self.assertEqual(proposal.queue_status,
255+ BranchMergeProposalStatus.QUEUED)
256+ self.assertEqual(proposal.queued_revision_id, '250')
257+
258 def test_set_status_approved_to_work_in_progress(self):
259 # setState can change an approved merge proposal to Work In Progress.
260 proposal = self.factory.makeBranchMergeProposal(
261@@ -337,6 +382,18 @@
262 self.assertEqual(proposal.queue_status,
263 BranchMergeProposalStatus.WORK_IN_PROGRESS)
264
265+ def test_set_status_queued_to_merge_failed(self):
266+ proposal = self.factory.makeBranchMergeProposal(
267+ target_branch=self.target_branch,
268+ set_state=BranchMergeProposalStatus.QUEUED)
269+ proposal.setStatus(BranchMergeProposalStatus.MERGE_FAILED)
270+ self.assertEqual(proposal.queue_status,
271+ BranchMergeProposalStatus.MERGE_FAILED)
272+ self.assertEqual(proposal.queuer, None)
273+ self.assertEqual(proposal.queued_revision_id, None)
274+ self.assertEqual(proposal.date_queued, None)
275+ self.assertEqual(proposal.queue_position, None)
276+
277 def test_set_status_wip_to_needs_review(self):
278 # setState can change the merge proposal to Needs Review.
279 proposal = self.factory.makeBranchMergeProposal(
280
281=== modified file 'lib/lp/testing/factory.py'
282--- lib/lp/testing/factory.py 2010-04-26 15:02:03 +0000
283+++ lib/lp/testing/factory.py 2010-04-29 22:33:35 +0000
284@@ -958,7 +958,7 @@
285 elif set_state == BranchMergeProposalStatus.MERGED:
286 unsafe_proposal.markAsMerged()
287 elif set_state == BranchMergeProposalStatus.MERGE_FAILED:
288- unsafe_proposal.mergeFailed(proposal.target_branch.owner)
289+ unsafe_proposal.setStatus(set_state, proposal.target_branch.owner)
290 elif set_state == BranchMergeProposalStatus.QUEUED:
291 unsafe_proposal.commit_message = self.getUniqueString(
292 'commit message')