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

[Robert Collins]

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

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

[Tim Penhey (thumper)]

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

[Robert Collins (lifeless)]

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.

[Robert Collins (lifeless)]

Thanks for the review.

[Robert Collins (lifeless)]

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

[Tim Penhey (thumper)]

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

review: Approve

=== 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 @@
                     requestReview
                     approveBranch
                     rejectBranch
-                    mergeFailed
                     markAsMerged
                     resubmit
                     enqueue

=== 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 @@
 Anyone that has rights to edit the merge proposal can say that the
 merge failed.
 
-    >>> proposal.mergeFailed(merger=sample_person)
+    >>> proposal.setStatus(BranchMergeProposalStatus.MERGE_FAILED)
     >>> print proposal.queue_status.title
     Code failed to merge
 

=== 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 @@
         If the proposal is not in the Approved state before this method
         is called, approveBranch is called with the reviewer and revision_id
         specified.
+
+        If None is supplied as the revision_id, the proposals
+        reviewed_revision_id is used.
         """
 
     def dequeue():
@@ -350,9 +353,6 @@
     def moveToFrontOfQueue():
         """Move the queue proposal to the front of the queue."""
 
-    def mergeFailed(merger):
-        """Mark the proposal as 'Code failed to merge'."""
-
     def markAsMerged(merged_revno=None, date_merged=None,
                      merge_reporter=None):
         """Mark the branch merge proposal as merged.

=== 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 @@
     [wip, needs_review, code_approved, rejected,
      merged, merge_failed, queued, superseded
      ] = BranchMergeProposalStatus.items
-    # Transitioning to code approved, rejected or queued from
+    # Transitioning to code approved, rejected, failed or queued from
     # work in progress, needs review or merge failed needs the
     # user to be a valid reviewer, other states are fine.
     valid_reviewer = proposal.target_branch.isPersonTrustedReviewer(user)
-    if (next_state == rejected and not valid_reviewer):
-        return False
-    # Non-reviewers can toggle between code_approved and queued, but not
-    # make anything else approved or queued.
-    elif (next_state in (code_approved, queued) and
-          from_state not in (code_approved, queued)
-          and not valid_reviewer):
-        return False
+    reviewed_ok_states = (code_approved, queued, merge_failed)
+    if not valid_reviewer:
+        # Non reviewers cannot reject proposals [XXX: what about their own?]
+        if next_state == rejected:
+            return False
+        # Non-reviewers can toggle within the reviewed ok states
+        # (approved/queued/failed): they can dequeue something they spot an
+        # environmental issue with (queued or failed to approved). Retry things
+        # that had an environmental issue (failed or approved to queued) and note
+        # things as failing (approved and queued to failed).
+        # This is perhaps more generous than needed, but its not clearly wrong
+        # - a key concern is to prevent non reviewers putting things in the 
+        # queue that haven't been oked (and thus moved to approved or one of the
+        # workflow states that approved leads to).
+        elif (next_state in reviewed_ok_states and
+              from_state not in reviewed_ok_states):
+            return False
+        else:
+            return True
     else:
         return True
 
@@ -327,23 +338,23 @@
         # XXX - rockstar - 9 Oct 2008 - jml suggested in a review that this
         # would be better as a dict mapping.
         # See bug #281060.
+        if (self.queue_status == BranchMergeProposalStatus.QUEUED and
+            status != BranchMergeProposalStatus.QUEUED):
+            self.dequeue()
         if status == BranchMergeProposalStatus.WORK_IN_PROGRESS:
             self.setAsWorkInProgress()
         elif status == BranchMergeProposalStatus.NEEDS_REVIEW:
             self.requestReview()
         elif status == BranchMergeProposalStatus.CODE_APPROVED:
-            # Other half of the edge case.  If the status is currently queued,
-            # we need to dequeue, otherwise we just approve the branch.
-            if self.queue_status == BranchMergeProposalStatus.QUEUED:
-                self.dequeue()
-            else:
-                self.approveBranch(user, revision_id)
+            self.approveBranch(user, revision_id)
         elif status == BranchMergeProposalStatus.REJECTED:
             self.rejectBranch(user, revision_id)
         elif status == BranchMergeProposalStatus.QUEUED:
             self.enqueue(user, revision_id)
         elif status == BranchMergeProposalStatus.MERGED:
             self.markAsMerged(merge_reporter=user)
+        elif status == BranchMergeProposalStatus.MERGE_FAILED:
+            self._transitionToState(status, user=user)
         else:
             raise AssertionError('Unexpected queue status: ' % status)
 
@@ -381,7 +392,7 @@
 
     def _reviewProposal(self, reviewer, next_state, revision_id,
                         _date_reviewed=None):
-        """Set the proposal to one of the two review statuses."""
+        """Set the proposal to next_state."""
         # Check the reviewer can review the code for the target branch.
         old_state = self.queue_status
         if not self.target_branch.isPersonTrustedReviewer(reviewer):
@@ -435,7 +446,7 @@
         self.queue_status = BranchMergeProposalStatus.QUEUED
         self.queue_position = position
         self.queuer = queuer
-        self.queued_revision_id = revision_id
+        self.queued_revision_id = revision_id or self.reviewed_revision_id
         self.date_queued = UTC_NOW
         self.syncUpdate()
 
@@ -467,14 +478,6 @@
         self.queue_position = first_entry.queue_position - 1
         self.syncUpdate()
 
-    def mergeFailed(self, merger):
-        """See `IBranchMergeProposal`."""
-        self._transitionToState(
-            BranchMergeProposalStatus.MERGE_FAILED, merger)
-        self.merger = merger
-        # Remove from the queue.
-        self.queue_position = None
-
     def markAsMerged(self, merged_revno=None, date_merged=None,
                      merge_reporter=None):
         """See `IBranchMergeProposal`."""

=== 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 @@
         BranchMergeProposalStatus.CODE_APPROVED: 'approveBranch',
         BranchMergeProposalStatus.REJECTED: 'rejectBranch',
         BranchMergeProposalStatus.MERGED: 'markAsMerged',
-        BranchMergeProposalStatus.MERGE_FAILED: 'mergeFailed',
+        BranchMergeProposalStatus.MERGE_FAILED: 'setStatus',
         BranchMergeProposalStatus.QUEUED: 'enqueue',
         BranchMergeProposalStatus.SUPERSEDED: 'resubmit',
         }
@@ -135,17 +135,21 @@
 
     def _attemptTransition(self, proposal, to_state):
         """Try to transition the proposal into the state `to_state`."""
+        kwargs = {}
         method = getattr(proposal, self.transition_functions[to_state])
         if to_state in (BranchMergeProposalStatus.CODE_APPROVED,
                         BranchMergeProposalStatus.REJECTED,
                         BranchMergeProposalStatus.QUEUED):
             args = [proposal.target_branch.owner, 'some_revision_id']
-        elif to_state in (BranchMergeProposalStatus.MERGE_FAILED,
-                          BranchMergeProposalStatus.SUPERSEDED):
+        elif to_state in (BranchMergeProposalStatus.SUPERSEDED,):
             args = [proposal.registrant]
+        elif to_state in (BranchMergeProposalStatus.MERGE_FAILED,):
+            # transition via setStatus.
+            args = [to_state]
+            kwargs = dict(user=proposal.target_branch.owner)
         else:
             args = []
-        method(*args)
+        method(*args, **kwargs)
 
     def assertGoodTransition(self, from_state, to_state):
         """Assert that we can go from `from_state` to `to_state`."""
@@ -267,6 +271,20 @@
         """We can go from merge failed to any other state."""
         self.assertAllTransitionsGood(BranchMergeProposalStatus.MERGE_FAILED)
 
+    def test_transition_from_merge_failed_to_queued_non_reviewer(self):
+        # Contributors can requeue to retry after environmental issues fail a
+        # merge.
+        proposal = self.factory.makeBranchMergeProposal()
+        self.assertFalse(proposal.target_branch.isPersonTrustedReviewer(
+            proposal.source_branch.owner))
+        self.assertValidTransitions(set([
+                BranchMergeProposalStatus.MERGE_FAILED,
+                BranchMergeProposalStatus.CODE_APPROVED,
+                # It is always valid to go to the same state.
+                BranchMergeProposalStatus.QUEUED]),
+            proposal, BranchMergeProposalStatus.QUEUED,
+            proposal.source_branch.owner)
+
     def test_transitions_from_queued_dequeue(self):
         # When a proposal is dequeued it is set to code approved, and the
         # queue position is reset.
@@ -298,11 +316,25 @@
         proposal = self.factory.makeBranchMergeProposal(
             target_branch=self.target_branch,
             set_state=BranchMergeProposalStatus.QUEUED)
-        proposal.mergeFailed(None)
+        proposal.setStatus(BranchMergeProposalStatus.MERGE_FAILED)
         self.assertProposalState(
             proposal, BranchMergeProposalStatus.MERGE_FAILED)
         self.assertIs(None, proposal.queue_position)
 
+    def test_transition_to_merge_failed_non_reviewer(self):
+        # non reviewers cannot set merge-failed (target branch owners are
+        # implicitly reviewers).
+        proposal = self.factory.makeBranchMergeProposal()
+        self.assertFalse(proposal.target_branch.isPersonTrustedReviewer(
+            proposal.source_branch.owner))
+        self.assertValidTransitions(set([
+                # It is always valid to go to the same state.
+                BranchMergeProposalStatus.MERGE_FAILED,
+                BranchMergeProposalStatus.CODE_APPROVED,
+                BranchMergeProposalStatus.QUEUED]),
+            proposal, BranchMergeProposalStatus.MERGE_FAILED,
+            proposal.source_branch.owner)
+
     def test_transitions_to_wip_resets_reviewer(self):
         # When a proposal was approved and is moved back into work in progress
         # the reviewer, date reviewed, and reviewed revision are all reset.
@@ -328,6 +360,19 @@
         self.target_branch = self.factory.makeProductBranch()
         login_person(self.target_branch.owner)
 
+    def test_set_status_approved_to_queued(self):
+        # setState can change an approved merge proposal to Work In Progress,
+        # which will set the revision id to the reviewed revision id if not
+        # supplied.
+        proposal = self.factory.makeBranchMergeProposal(
+            target_branch=self.target_branch,
+            set_state=BranchMergeProposalStatus.CODE_APPROVED)
+        proposal.approveBranch(proposal.target_branch.owner, '250')
+        proposal.setStatus(BranchMergeProposalStatus.QUEUED)
+        self.assertEqual(proposal.queue_status,
+            BranchMergeProposalStatus.QUEUED)
+        self.assertEqual(proposal.queued_revision_id, '250')
+
     def test_set_status_approved_to_work_in_progress(self):
         # setState can change an approved merge proposal to Work In Progress.
         proposal = self.factory.makeBranchMergeProposal(
@@ -337,6 +382,18 @@
         self.assertEqual(proposal.queue_status,
             BranchMergeProposalStatus.WORK_IN_PROGRESS)
 
+    def test_set_status_queued_to_merge_failed(self):
+        proposal = self.factory.makeBranchMergeProposal(
+            target_branch=self.target_branch,
+            set_state=BranchMergeProposalStatus.QUEUED)
+        proposal.setStatus(BranchMergeProposalStatus.MERGE_FAILED)
+        self.assertEqual(proposal.queue_status,
+            BranchMergeProposalStatus.MERGE_FAILED)
+        self.assertEqual(proposal.queuer, None)
+        self.assertEqual(proposal.queued_revision_id, None)
+        self.assertEqual(proposal.date_queued, None)
+        self.assertEqual(proposal.queue_position, None)
+
     def test_set_status_wip_to_needs_review(self):
         # setState can change the merge proposal to Needs Review.
         proposal = self.factory.makeBranchMergeProposal(

=== 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 @@
         elif set_state == BranchMergeProposalStatus.MERGED:
             unsafe_proposal.markAsMerged()
         elif set_state == BranchMergeProposalStatus.MERGE_FAILED:
-            unsafe_proposal.mergeFailed(proposal.target_branch.owner)
+            unsafe_proposal.setStatus(set_state, proposal.target_branch.owner)
         elif set_state == BranchMergeProposalStatus.QUEUED:
             unsafe_proposal.commit_message = self.getUniqueString(
                 'commit message')