launchpad-reviewers team mailing list archive

[Martin Packman <martin.packman@xxxxxxxxxxxxx>]

Martin Packman has proposed merging lp:~gz/launchpad/rejected_branch_merged_reviewer_1044530 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1044530 in Launchpad itself: "Marking rejected proposal as merged says reviewer approved"
  https://bugs.launchpad.net/launchpad/+bug/1044530

For more details, see:
https://code.launchpad.net/~gz/launchpad/rejected_branch_merged_reviewer_1044530/+merge/122354

A merge proposal stores details about who reviewed it and when, but not what their review was outside the current state of the proposal and votes in the comments. This means when the reviewer is set on a merged branch, launchpad assumes that whoever reviewed it must have approved the proposal. However if the reviewer in fact marked the proposal as rejected, but then the code was later merged anyway, they are listed as having approved the branch.

Arguably the transition from rejected to merged should be disallowed. However, if a branch really has been merged it seems betetr for the status to be accurate, and just omit the reviewer details (leaving any context to votes and comments). This branch adds a check when marking a branch as merged that does this.

As a side effect, changing a branch back to needs review no longer keeps the date of a previous review. The reviewer and revno were already wiped, and there doesn't seem to be any gain from the inconsistency.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/branchmergeproposal.py
  lib/lp/code/model/tests/test_branchmergeproposal.py

-- 
https://code.launchpad.net/~gz/launchpad/rejected_branch_merged_reviewer_1044530/+merge/122354
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gz/launchpad/rejected_branch_merged_reviewer_1044530 into lp:launchpad.

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2012-08-13 20:13:49 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2012-08-31 20:14:19 +0000
@@ -420,6 +420,10 @@
     def setAsWorkInProgress(self):
         """See `IBranchMergeProposal`."""
         self._transitionToState(BranchMergeProposalStatus.WORK_IN_PROGRESS)
+        self._mark_unreviewed()
+
+    def _mark_unreviewed(self):
+        """Clear metadata about a previous review."""
         self.reviewer = None
         self.date_reviewed = None
         self.reviewed_revision_id = None
@@ -443,8 +447,7 @@
             self._transitionToState(BranchMergeProposalStatus.NEEDS_REVIEW)
             self.date_review_requested = _date_requested
             # Clear out any reviewed or queued values.
-            self.reviewer = None
-            self.reviewed_revision_id = None
+            self._mark_unreviewed()
             self.queuer = None
             self.queued_revision_id = None
 
@@ -545,6 +548,7 @@
     def markAsMerged(self, merged_revno=None, date_merged=None,
                      merge_reporter=None):
         """See `IBranchMergeProposal`."""
+        old_state = self.queue_status
         self._transitionToState(
             BranchMergeProposalStatus.MERGED, merge_reporter)
         self.merged_revno = merged_revno
@@ -552,6 +556,11 @@
         # Remove from the queue.
         self.queue_position = None
 
+        # The reviewer of a merged proposal is assumed to have approved, if
+        # they rejected it remove the review metadata to avoid confusion.
+        if old_state == BranchMergeProposalStatus.REJECTED:
+            self._mark_unreviewed()
+
         if merged_revno is not None:
             branch_revision = Store.of(self).find(
                 BranchRevision,

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2012-08-14 23:27:07 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2012-08-31 20:14:19 +0000
@@ -433,6 +433,20 @@
         self.assertIs(None, proposal.date_reviewed)
         self.assertIs(None, proposal.reviewed_revision_id)
 
+    def test_transitions_from_rejected_to_merged_resets_reviewer(self):
+        # When a rejected proposal ends up being merged anyway, reset the
+        # reviewer details as they did not approve as is otherwise assumed.
+        proposal = self.factory.makeBranchMergeProposal(
+            target_branch=self.target_branch,
+            set_state=BranchMergeProposalStatus.REJECTED)
+        self.assertIsNot(None, proposal.reviewer)
+        self.assertIsNot(None, proposal.date_reviewed)
+        self.assertIsNot(None, proposal.reviewed_revision_id)
+        proposal.markAsMerged()
+        self.assertIs(None, proposal.reviewer)
+        self.assertIs(None, proposal.date_reviewed)
+        self.assertIs(None, proposal.reviewed_revision_id)
+
 
 class TestBranchMergeProposalSetStatus(TestCaseWithFactory):
     """Test the setStatus method of BranchMergeProposal."""