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."""
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
