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
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2010-04-28 22:03:05 +0000
+++ lib/lp/code/configure.zcml 2010-04-29 22:33:35 +0000
@@ -246,7 +246,6 @@
246 requestReview246 requestReview
247 approveBranch247 approveBranch
248 rejectBranch248 rejectBranch
249 mergeFailed
250 markAsMerged249 markAsMerged
251 resubmit250 resubmit
252 enqueue251 enqueue
253252
=== modified file 'lib/lp/code/doc/branch-merge-proposals.txt'
--- lib/lp/code/doc/branch-merge-proposals.txt 2010-02-26 00:30:09 +0000
+++ lib/lp/code/doc/branch-merge-proposals.txt 2010-04-29 22:33:35 +0000
@@ -348,7 +348,7 @@
348Anyone that has rights to edit the merge proposal can say that the348Anyone that has rights to edit the merge proposal can say that the
349merge failed.349merge failed.
350350
351 >>> proposal.mergeFailed(merger=sample_person)351 >>> proposal.setStatus(BranchMergeProposalStatus.MERGE_FAILED)
352 >>> print proposal.queue_status.title352 >>> print proposal.queue_status.title
353 Code failed to merge353 Code failed to merge
354354
355355
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2010-03-05 03:35:10 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2010-04-29 22:33:35 +0000
@@ -338,6 +338,9 @@
338 If the proposal is not in the Approved state before this method338 If the proposal is not in the Approved state before this method
339 is called, approveBranch is called with the reviewer and revision_id339 is called, approveBranch is called with the reviewer and revision_id
340 specified.340 specified.
341
342 If None is supplied as the revision_id, the proposals
343 reviewed_revision_id is used.
341 """344 """
342345
343 def dequeue():346 def dequeue():
@@ -350,9 +353,6 @@
350 def moveToFrontOfQueue():353 def moveToFrontOfQueue():
351 """Move the queue proposal to the front of the queue."""354 """Move the queue proposal to the front of the queue."""
352355
353 def mergeFailed(merger):
354 """Mark the proposal as 'Code failed to merge'."""
355
356 def markAsMerged(merged_revno=None, date_merged=None,356 def markAsMerged(merged_revno=None, date_merged=None,
357 merge_reporter=None):357 merge_reporter=None):
358 """Mark the branch merge proposal as merged.358 """Mark the branch merge proposal as merged.
359359
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2010-02-26 09:54:20 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2010-04-29 22:33:35 +0000
@@ -77,18 +77,29 @@
77 [wip, needs_review, code_approved, rejected,77 [wip, needs_review, code_approved, rejected,
78 merged, merge_failed, queued, superseded78 merged, merge_failed, queued, superseded
79 ] = BranchMergeProposalStatus.items79 ] = BranchMergeProposalStatus.items
80 # Transitioning to code approved, rejected or queued from80 # Transitioning to code approved, rejected, failed or queued from
81 # work in progress, needs review or merge failed needs the81 # work in progress, needs review or merge failed needs the
82 # user to be a valid reviewer, other states are fine.82 # user to be a valid reviewer, other states are fine.
83 valid_reviewer = proposal.target_branch.isPersonTrustedReviewer(user)83 valid_reviewer = proposal.target_branch.isPersonTrustedReviewer(user)
84 if (next_state == rejected and not valid_reviewer):84 reviewed_ok_states = (code_approved, queued, merge_failed)
85 return False85 if not valid_reviewer:
86 # Non-reviewers can toggle between code_approved and queued, but not86 # Non reviewers cannot reject proposals [XXX: what about their own?]
87 # make anything else approved or queued.87 if next_state == rejected:
88 elif (next_state in (code_approved, queued) and88 return False
89 from_state not in (code_approved, queued)89 # Non-reviewers can toggle within the reviewed ok states
90 and not valid_reviewer):90 # (approved/queued/failed): they can dequeue something they spot an
91 return False91 # environmental issue with (queued or failed to approved). Retry things
92 # that had an environmental issue (failed or approved to queued) and note
93 # things as failing (approved and queued to failed).
94 # This is perhaps more generous than needed, but its not clearly wrong
95 # - a key concern is to prevent non reviewers putting things in the
96 # queue that haven't been oked (and thus moved to approved or one of the
97 # workflow states that approved leads to).
98 elif (next_state in reviewed_ok_states and
99 from_state not in reviewed_ok_states):
100 return False
101 else:
102 return True
92 else:103 else:
93 return True104 return True
94105
@@ -327,23 +338,23 @@
327 # XXX - rockstar - 9 Oct 2008 - jml suggested in a review that this338 # XXX - rockstar - 9 Oct 2008 - jml suggested in a review that this
328 # would be better as a dict mapping.339 # would be better as a dict mapping.
329 # See bug #281060.340 # See bug #281060.
341 if (self.queue_status == BranchMergeProposalStatus.QUEUED and
342 status != BranchMergeProposalStatus.QUEUED):
343 self.dequeue()
330 if status == BranchMergeProposalStatus.WORK_IN_PROGRESS:344 if status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
331 self.setAsWorkInProgress()345 self.setAsWorkInProgress()
332 elif status == BranchMergeProposalStatus.NEEDS_REVIEW:346 elif status == BranchMergeProposalStatus.NEEDS_REVIEW:
333 self.requestReview()347 self.requestReview()
334 elif status == BranchMergeProposalStatus.CODE_APPROVED:348 elif status == BranchMergeProposalStatus.CODE_APPROVED:
335 # Other half of the edge case. If the status is currently queued,349 self.approveBranch(user, revision_id)
336 # we need to dequeue, otherwise we just approve the branch.
337 if self.queue_status == BranchMergeProposalStatus.QUEUED:
338 self.dequeue()
339 else:
340 self.approveBranch(user, revision_id)
341 elif status == BranchMergeProposalStatus.REJECTED:350 elif status == BranchMergeProposalStatus.REJECTED:
342 self.rejectBranch(user, revision_id)351 self.rejectBranch(user, revision_id)
343 elif status == BranchMergeProposalStatus.QUEUED:352 elif status == BranchMergeProposalStatus.QUEUED:
344 self.enqueue(user, revision_id)353 self.enqueue(user, revision_id)
345 elif status == BranchMergeProposalStatus.MERGED:354 elif status == BranchMergeProposalStatus.MERGED:
346 self.markAsMerged(merge_reporter=user)355 self.markAsMerged(merge_reporter=user)
356 elif status == BranchMergeProposalStatus.MERGE_FAILED:
357 self._transitionToState(status, user=user)
347 else:358 else:
348 raise AssertionError('Unexpected queue status: ' % status)359 raise AssertionError('Unexpected queue status: ' % status)
349360
@@ -381,7 +392,7 @@
381392
382 def _reviewProposal(self, reviewer, next_state, revision_id,393 def _reviewProposal(self, reviewer, next_state, revision_id,
383 _date_reviewed=None):394 _date_reviewed=None):
384 """Set the proposal to one of the two review statuses."""395 """Set the proposal to next_state."""
385 # Check the reviewer can review the code for the target branch.396 # Check the reviewer can review the code for the target branch.
386 old_state = self.queue_status397 old_state = self.queue_status
387 if not self.target_branch.isPersonTrustedReviewer(reviewer):398 if not self.target_branch.isPersonTrustedReviewer(reviewer):
@@ -435,7 +446,7 @@
435 self.queue_status = BranchMergeProposalStatus.QUEUED446 self.queue_status = BranchMergeProposalStatus.QUEUED
436 self.queue_position = position447 self.queue_position = position
437 self.queuer = queuer448 self.queuer = queuer
438 self.queued_revision_id = revision_id449 self.queued_revision_id = revision_id or self.reviewed_revision_id
439 self.date_queued = UTC_NOW450 self.date_queued = UTC_NOW
440 self.syncUpdate()451 self.syncUpdate()
441452
@@ -467,14 +478,6 @@
467 self.queue_position = first_entry.queue_position - 1478 self.queue_position = first_entry.queue_position - 1
468 self.syncUpdate()479 self.syncUpdate()
469480
470 def mergeFailed(self, merger):
471 """See `IBranchMergeProposal`."""
472 self._transitionToState(
473 BranchMergeProposalStatus.MERGE_FAILED, merger)
474 self.merger = merger
475 # Remove from the queue.
476 self.queue_position = None
477
478 def markAsMerged(self, merged_revno=None, date_merged=None,481 def markAsMerged(self, merged_revno=None, date_merged=None,
479 merge_reporter=None):482 merge_reporter=None):
480 """See `IBranchMergeProposal`."""483 """See `IBranchMergeProposal`."""
481484
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py 2010-03-30 09:56:10 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2010-04-29 22:33:35 +0000
@@ -117,7 +117,7 @@
117 BranchMergeProposalStatus.CODE_APPROVED: 'approveBranch',117 BranchMergeProposalStatus.CODE_APPROVED: 'approveBranch',
118 BranchMergeProposalStatus.REJECTED: 'rejectBranch',118 BranchMergeProposalStatus.REJECTED: 'rejectBranch',
119 BranchMergeProposalStatus.MERGED: 'markAsMerged',119 BranchMergeProposalStatus.MERGED: 'markAsMerged',
120 BranchMergeProposalStatus.MERGE_FAILED: 'mergeFailed',120 BranchMergeProposalStatus.MERGE_FAILED: 'setStatus',
121 BranchMergeProposalStatus.QUEUED: 'enqueue',121 BranchMergeProposalStatus.QUEUED: 'enqueue',
122 BranchMergeProposalStatus.SUPERSEDED: 'resubmit',122 BranchMergeProposalStatus.SUPERSEDED: 'resubmit',
123 }123 }
@@ -135,17 +135,21 @@
135135
136 def _attemptTransition(self, proposal, to_state):136 def _attemptTransition(self, proposal, to_state):
137 """Try to transition the proposal into the state `to_state`."""137 """Try to transition the proposal into the state `to_state`."""
138 kwargs = {}
138 method = getattr(proposal, self.transition_functions[to_state])139 method = getattr(proposal, self.transition_functions[to_state])
139 if to_state in (BranchMergeProposalStatus.CODE_APPROVED,140 if to_state in (BranchMergeProposalStatus.CODE_APPROVED,
140 BranchMergeProposalStatus.REJECTED,141 BranchMergeProposalStatus.REJECTED,
141 BranchMergeProposalStatus.QUEUED):142 BranchMergeProposalStatus.QUEUED):
142 args = [proposal.target_branch.owner, 'some_revision_id']143 args = [proposal.target_branch.owner, 'some_revision_id']
143 elif to_state in (BranchMergeProposalStatus.MERGE_FAILED,144 elif to_state in (BranchMergeProposalStatus.SUPERSEDED,):
144 BranchMergeProposalStatus.SUPERSEDED):
145 args = [proposal.registrant]145 args = [proposal.registrant]
146 elif to_state in (BranchMergeProposalStatus.MERGE_FAILED,):
147 # transition via setStatus.
148 args = [to_state]
149 kwargs = dict(user=proposal.target_branch.owner)
146 else:150 else:
147 args = []151 args = []
148 method(*args)152 method(*args, **kwargs)
149153
150 def assertGoodTransition(self, from_state, to_state):154 def assertGoodTransition(self, from_state, to_state):
151 """Assert that we can go from `from_state` to `to_state`."""155 """Assert that we can go from `from_state` to `to_state`."""
@@ -267,6 +271,20 @@
267 """We can go from merge failed to any other state."""271 """We can go from merge failed to any other state."""
268 self.assertAllTransitionsGood(BranchMergeProposalStatus.MERGE_FAILED)272 self.assertAllTransitionsGood(BranchMergeProposalStatus.MERGE_FAILED)
269273
274 def test_transition_from_merge_failed_to_queued_non_reviewer(self):
275 # Contributors can requeue to retry after environmental issues fail a
276 # merge.
277 proposal = self.factory.makeBranchMergeProposal()
278 self.assertFalse(proposal.target_branch.isPersonTrustedReviewer(
279 proposal.source_branch.owner))
280 self.assertValidTransitions(set([
281 BranchMergeProposalStatus.MERGE_FAILED,
282 BranchMergeProposalStatus.CODE_APPROVED,
283 # It is always valid to go to the same state.
284 BranchMergeProposalStatus.QUEUED]),
285 proposal, BranchMergeProposalStatus.QUEUED,
286 proposal.source_branch.owner)
287
270 def test_transitions_from_queued_dequeue(self):288 def test_transitions_from_queued_dequeue(self):
271 # When a proposal is dequeued it is set to code approved, and the289 # When a proposal is dequeued it is set to code approved, and the
272 # queue position is reset.290 # queue position is reset.
@@ -298,11 +316,25 @@
298 proposal = self.factory.makeBranchMergeProposal(316 proposal = self.factory.makeBranchMergeProposal(
299 target_branch=self.target_branch,317 target_branch=self.target_branch,
300 set_state=BranchMergeProposalStatus.QUEUED)318 set_state=BranchMergeProposalStatus.QUEUED)
301 proposal.mergeFailed(None)319 proposal.setStatus(BranchMergeProposalStatus.MERGE_FAILED)
302 self.assertProposalState(320 self.assertProposalState(
303 proposal, BranchMergeProposalStatus.MERGE_FAILED)321 proposal, BranchMergeProposalStatus.MERGE_FAILED)
304 self.assertIs(None, proposal.queue_position)322 self.assertIs(None, proposal.queue_position)
305323
324 def test_transition_to_merge_failed_non_reviewer(self):
325 # non reviewers cannot set merge-failed (target branch owners are
326 # implicitly reviewers).
327 proposal = self.factory.makeBranchMergeProposal()
328 self.assertFalse(proposal.target_branch.isPersonTrustedReviewer(
329 proposal.source_branch.owner))
330 self.assertValidTransitions(set([
331 # It is always valid to go to the same state.
332 BranchMergeProposalStatus.MERGE_FAILED,
333 BranchMergeProposalStatus.CODE_APPROVED,
334 BranchMergeProposalStatus.QUEUED]),
335 proposal, BranchMergeProposalStatus.MERGE_FAILED,
336 proposal.source_branch.owner)
337
306 def test_transitions_to_wip_resets_reviewer(self):338 def test_transitions_to_wip_resets_reviewer(self):
307 # When a proposal was approved and is moved back into work in progress339 # When a proposal was approved and is moved back into work in progress
308 # the reviewer, date reviewed, and reviewed revision are all reset.340 # the reviewer, date reviewed, and reviewed revision are all reset.
@@ -328,6 +360,19 @@
328 self.target_branch = self.factory.makeProductBranch()360 self.target_branch = self.factory.makeProductBranch()
329 login_person(self.target_branch.owner)361 login_person(self.target_branch.owner)
330362
363 def test_set_status_approved_to_queued(self):
364 # setState can change an approved merge proposal to Work In Progress,
365 # which will set the revision id to the reviewed revision id if not
366 # supplied.
367 proposal = self.factory.makeBranchMergeProposal(
368 target_branch=self.target_branch,
369 set_state=BranchMergeProposalStatus.CODE_APPROVED)
370 proposal.approveBranch(proposal.target_branch.owner, '250')
371 proposal.setStatus(BranchMergeProposalStatus.QUEUED)
372 self.assertEqual(proposal.queue_status,
373 BranchMergeProposalStatus.QUEUED)
374 self.assertEqual(proposal.queued_revision_id, '250')
375
331 def test_set_status_approved_to_work_in_progress(self):376 def test_set_status_approved_to_work_in_progress(self):
332 # setState can change an approved merge proposal to Work In Progress.377 # setState can change an approved merge proposal to Work In Progress.
333 proposal = self.factory.makeBranchMergeProposal(378 proposal = self.factory.makeBranchMergeProposal(
@@ -337,6 +382,18 @@
337 self.assertEqual(proposal.queue_status,382 self.assertEqual(proposal.queue_status,
338 BranchMergeProposalStatus.WORK_IN_PROGRESS)383 BranchMergeProposalStatus.WORK_IN_PROGRESS)
339384
385 def test_set_status_queued_to_merge_failed(self):
386 proposal = self.factory.makeBranchMergeProposal(
387 target_branch=self.target_branch,
388 set_state=BranchMergeProposalStatus.QUEUED)
389 proposal.setStatus(BranchMergeProposalStatus.MERGE_FAILED)
390 self.assertEqual(proposal.queue_status,
391 BranchMergeProposalStatus.MERGE_FAILED)
392 self.assertEqual(proposal.queuer, None)
393 self.assertEqual(proposal.queued_revision_id, None)
394 self.assertEqual(proposal.date_queued, None)
395 self.assertEqual(proposal.queue_position, None)
396
340 def test_set_status_wip_to_needs_review(self):397 def test_set_status_wip_to_needs_review(self):
341 # setState can change the merge proposal to Needs Review.398 # setState can change the merge proposal to Needs Review.
342 proposal = self.factory.makeBranchMergeProposal(399 proposal = self.factory.makeBranchMergeProposal(
343400
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-04-26 15:02:03 +0000
+++ lib/lp/testing/factory.py 2010-04-29 22:33:35 +0000
@@ -958,7 +958,7 @@
958 elif set_state == BranchMergeProposalStatus.MERGED:958 elif set_state == BranchMergeProposalStatus.MERGED:
959 unsafe_proposal.markAsMerged()959 unsafe_proposal.markAsMerged()
960 elif set_state == BranchMergeProposalStatus.MERGE_FAILED:960 elif set_state == BranchMergeProposalStatus.MERGE_FAILED:
961 unsafe_proposal.mergeFailed(proposal.target_branch.owner)961 unsafe_proposal.setStatus(set_state, proposal.target_branch.owner)
962 elif set_state == BranchMergeProposalStatus.QUEUED:962 elif set_state == BranchMergeProposalStatus.QUEUED:
963 unsafe_proposal.commit_message = self.getUniqueString(963 unsafe_proposal.commit_message = self.getUniqueString(
964 'commit message')964 'commit message')