launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01227
[Merge] lp:~wallyworld/launchpad/reviews-without-reviewer into lp:launchpad/devel
Ian Booth has proposed merging lp:~wallyworld/launchpad/reviews-without-reviewer into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
#636688 Some reviews created without a reviewer
https://bugs.launchpad.net/bugs/636688
Bug 636688 says that if no reviewer is passed in to the addLandingTarget method, then a review should be requested from the default review team of the target branch. Also while doing this change, the default reviewer added to the form should be removed. This way the user only needs to specify a reviewer if they are not the default.
Proposed fix
The addLanding target method now assigns the default reviewer of the target branch to the merge proposal, but a method parameter "assign_default_reviewer=T/F" is used to specify if this should happen or not. We only want the default reviewer to be assigned when addLandingTarget() is invoked from the "Propose Branch for Merging" page. Without the flag, other business logic failed. In particular, the processMergeProposal() method in codehandler.py needs to create a merge proposal without the new default reviewer setting, because it then subsequently parses the email body and picks up the default reviewer from there, if set.
Tests
bin/test -vvt test_branch
bin/test -vvt test_branchmergeproposal
bin/test -vvt xx-branchmergeproposal
bin/test -vvt test_codehandler
bin/test -vvt test_mergedetection
New tests:
Instead of just TestRegisterBranchMergeProposalView, a new test class has been added:
TestRegisterBranchWithNoDefaultReviewerMergeProposalView. These 2 classes run merge proposal tests for a
target branch with and without a default reviewer assigned respectively.
The BranchAddLandingTarget class has new tests:
- test_default_reviewer
- test_default_reviewer_without_enable
- test_default_reviewer_with_review_type
- test_notify_on_default_reviewer
Changed tests:
Some doc tests in xx-branchmergeproposals.txt were changed to accommodate the new behaviour of addLandingTarget
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/browser/branch.py
lib/lp/code/browser/tests/test_branchmergeproposal.py
lib/lp/code/interfaces/branch.py
lib/lp/code/interfaces/branchmergeproposal.py
lib/lp/code/mail/codehandler.py
lib/lp/code/model/branch.py
lib/lp/code/model/tests/test_branch.py
lib/lp/code/stories/branches/xx-branchmergeproposals.txt
./lib/lp/code/stories/branches/xx-branchmergeproposals.txt
208: source exceeds 78 characters.
216: want exceeds 78 characters.
664: source exceeds 78 characters.
--
https://code.launchpad.net/~wallyworld/launchpad/reviews-without-reviewer/+merge/36651
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/reviews-without-reviewer into lp:launchpad/devel.
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py 2010-08-24 10:45:57 +0000
+++ lib/lp/code/browser/branch.py 2010-09-26 10:28:49 +0000
@@ -1286,19 +1286,6 @@
page_title = label = 'Propose branch for merging'
@property
- def initial_values(self):
- """The default reviewer is the code reviewer of the target."""
- # If there is a default merge branch for the target, then default
- # the reviewer to be the review team for that branch.
- reviewer = None
- default_target = self.context.target.default_merge_target
- # Don't set the reviewer if the default branch is the same as the
- # current context.
- if default_target is not None and default_target != self.context:
- reviewer = default_target.code_reviewer
- return {'reviewer': reviewer}
-
- @property
def cancel_url(self):
return canonical_url(self.context)
@@ -1319,8 +1306,9 @@
review_requests = []
reviewer = data.get('reviewer')
+ review_type = data.get('review_type')
if reviewer is not None:
- review_requests.append((reviewer, data.get('review_type')))
+ review_requests.append((reviewer, review_type))
try:
proposal = source_branch.addLandingTarget(
@@ -1329,7 +1317,8 @@
needs_review=data['needs_review'],
description=data.get('comment'),
review_requests=review_requests,
- commit_message=data.get('commit_message'))
+ commit_message=data.get('commit_message'),
+ default_review_type=review_type, assign_default_reviewer=True)
self.next_url = canonical_url(proposal)
except InvalidBranchMergeProposal, error:
self.addError(str(error))
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2010-09-15 20:41:46 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2010-09-26 10:28:49 +0000
@@ -333,18 +333,8 @@
view.render()
-class TestRegisterBranchMergeProposalView(TestCaseWithFactory):
- """Test the merge proposal registration view."""
-
- layer = DatabaseFunctionalLayer
-
- def setUp(self):
- TestCaseWithFactory.setUp(self)
- self.target_branch = self.factory.makeProductBranch()
- self.source_branch = self.factory.makeProductBranch(
- product=self.target_branch.product)
- self.user = self.factory.makePerson()
- login_person(self.user)
+class RegisterBranchMergeProposalViewMixin:
+ """ Mixin class for BranchMergeProposalView tests."""
def _createView(self):
# Construct the view and initialize it.
@@ -361,10 +351,6 @@
self.assertEqual(self.target_branch, proposal.target_branch)
return proposal
- def assertNoPendingReviews(self, proposal):
- # There should be no votes recorded for the proposal.
- self.assertEqual([], list(proposal.votes))
-
def assertOnePendingReview(self, proposal, reviewer, review_type=None):
# There should be one pending vote for the reviewer with the specified
# review type.
@@ -378,15 +364,35 @@
else:
self.assertEqual(review_type, votes[0].review_type)
+
+class TestRegisterBranchWithNoDefaultReviewerMergeProposalView(
+ TestCaseWithFactory, RegisterBranchMergeProposalViewMixin):
+ """Test the merge proposal registration view.
+
+ The target branch has no default reviewer assigned. Hence the reviewer
+ should be set to the branch owner unless specified otherwise.
+ """
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ TestCaseWithFactory.setUp(self)
+ self.target_branch = self.factory.makeProductBranch()
+ self.source_branch = self.factory.makeProductBranch(
+ product=self.target_branch.product)
+ self.user = self.factory.makePerson()
+ login_person(self.user)
+
def test_register_simplest_case(self):
# This simplest case is where the user only specifies the target
- # branch, and not an initial comment or reviewer.
+ # branch, and not an initial comment or reviewer. The reviewer will
+ # therefore be set to the branch owner.
view = self._createView()
view.register_action.success(
{'target_branch': self.target_branch,
'needs_review': True})
proposal = self._getSourceProposal()
- self.assertNoPendingReviews(proposal)
+ self.assertOnePendingReview(proposal, self.target_branch.owner)
self.assertIs(None, proposal.description)
def test_register_work_in_progress(self):
@@ -421,7 +427,7 @@
'needs_review': True})
proposal = self._getSourceProposal()
- self.assertNoPendingReviews(proposal)
+ self.assertOnePendingReview(proposal, self.target_branch.owner)
self.assertEqual(proposal.description, "This is the description.")
def test_register_request_reviewer(self):
@@ -470,6 +476,51 @@
self.assertEqual(proposal.description, "This is the description.")
+class TestRegisterBranchMergeProposalView(
+ TestCaseWithFactory, RegisterBranchMergeProposalViewMixin):
+ """Test the merge proposal registration view.
+
+ The target branch has a default reviewer assigned.
+ """
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ TestCaseWithFactory.setUp(self)
+ # The default reviewer is albert
+ self.albert = self.factory.makePerson(name='albert')
+ self.target_branch = self.factory.makeProductBranch(
+ reviewer=self.albert)
+ self.source_branch = self.factory.makeProductBranch(
+ product=self.target_branch.product)
+ self.user = self.factory.makePerson()
+ login_person(self.user)
+
+ def test_register_simplest_case(self):
+ # A simple case is where the user only specifies the target
+ # branch, and not an initial comment or reviewer. The target branch
+ # has a reviewer so that reviewer should be used
+ view = self._createView()
+ view.register_action.success(
+ {'target_branch': self.target_branch,
+ 'needs_review': True})
+ proposal = self._getSourceProposal()
+ self.assertOnePendingReview(proposal, self.albert)
+ self.assertIs(None, proposal.description)
+
+ def test_register_request_review_type(self):
+ # We can ask for a specific review type. The target branch has a
+ # reviewer so that reviewer should be used.
+ view = self._createView()
+ view.register_action.success(
+ {'target_branch': self.target_branch,
+ 'review_type': 'god-like',
+ 'needs_review': True})
+ proposal = self._getSourceProposal()
+ self.assertOnePendingReview(proposal, self.albert, 'god-like')
+ self.assertIs(None, proposal.description)
+
+
class TestBranchMergeProposalView(TestCaseWithFactory):
layer = LaunchpadFunctionalLayer
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2010-09-09 01:18:36 +0000
+++ lib/lp/code/interfaces/branch.py 2010-09-26 10:28:49 +0000
@@ -566,7 +566,9 @@
def addLandingTarget(registrant, target_branch, prerequisite_branch=None,
date_created=None, needs_review=False,
description=None, review_requests=None,
- review_diff=None, commit_message=None):
+ review_diff=None, commit_message=None,
+ assign_default_reviewer=False,
+ default_review_type=None):
"""Create a new BranchMergeProposal with this branch as the source.
Both the target_branch and the prerequisite_branch, if it is there,
@@ -586,6 +588,11 @@
:param description: A description of the bugs fixed, features added,
or refactorings.
:param review_requests: An optional list of (`Person`, review_type).
+ :param assign_default_reviewer: If True, and no review_requests are
+ specified, set the reviwer of the merge proposal to be the default
+ reviewer of the target branch, if any.
+ :param default_review_type: If a default reviewer is used, this is the
+ review type to request.
"""
def scheduleDiffUpdates():
@@ -1030,7 +1037,7 @@
is set, the branch gets moved into the junk namespace of the branch
owner.
- :raise: `BranchTargetError` if both project and source_package are set,
+ :raise: `BranchTargetError` if both project and source_package are set
or if either the project or source_package fail to be adapted to an
IBranchTarget.
"""
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2010-09-15 20:41:46 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2010-09-26 10:28:49 +0000
@@ -676,7 +676,9 @@
class IReviewRequestedEmailJob(IRunnableJob):
"""Interface for the job to sends review request emails."""
- reviewer = Attribute('The person or team asked to do the review.')
+ reviewer = Attribute('The person or team asked to do the review. '
+ 'If left blank, then the default reviewer for the '
+ 'selected target branch will be used.')
requester = Attribute('The person who has asked for the review.')
@@ -712,6 +714,7 @@
# XXX: JonathanLange 2010-01-06: This is only used in the scanner, perhaps it
# should be moved there.
+
def notify_modified(proposal, func, *args, **kwargs):
"""Call func, then notify about the changes it made.
=== modified file 'lib/lp/code/mail/codehandler.py'
--- lib/lp/code/mail/codehandler.py 2010-08-20 20:31:18 +0000
+++ lib/lp/code/mail/codehandler.py 2010-09-26 10:28:49 +0000
@@ -76,12 +76,15 @@
class BadBranchMergeProposalAddress(Exception):
"""The user-supplied address is not an acceptable value."""
+
class InvalidBranchMergeProposalAddress(BadBranchMergeProposalAddress):
"""The user-supplied address is not an acceptable value."""
+
class NonExistantBranchMergeProposalAddress(BadBranchMergeProposalAddress):
"""The BranchMergeProposal specified by the address does not exist."""
+
class InvalidVoteString(Exception):
"""The user-supplied vote is not an acceptable value."""
@@ -540,7 +543,8 @@
# We don't currently support pulling in the revisions if
# the source branch exists and isn't stacked.
# XXX Tim Penhey 2010-07-27 bug 610292
- # We should fail here and return an oops email to the user.
+ # We should fail here and return an oops email to the
+ # user.
return db_source
self._pullRevisionsFromMergeDirectiveIntoSourceBranch(
md, target_url, bzr_source)
@@ -638,8 +642,12 @@
with globalErrorUtility.oopsMessage(
'target: %r source: %r' % (target, source)):
try:
+ # There may be a reviewer specified in the email but we don't
+ # know that yet. So tell addLandingTarget not to assign a
+ # default reviewer - will will do that below if required.
bmp = source.addLandingTarget(submitter, target,
- needs_review=True)
+ needs_review=True,
+ assign_default_reviewer=False)
context = CodeReviewEmailCommandExecutionContext(
bmp, submitter, notify_event_listeners=False)
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2010-09-21 02:46:02 +0000
+++ lib/lp/code/model/branch.py 2010-09-26 10:28:49 +0000
@@ -373,7 +373,9 @@
prerequisite_branch=None, whiteboard=None,
date_created=None, needs_review=False,
description=None, review_requests=None,
- review_diff=None, commit_message=None):
+ review_diff=None, commit_message=None,
+ assign_default_reviewer=False,
+ default_review_type=None):
"""See `IBranch`."""
if not self.target.supports_merge_proposals:
raise InvalidBranchMergeProposal(
@@ -419,6 +421,15 @@
if review_requests is None:
review_requests = []
+ # Ensure that if there is a default reviewer specified for the
+ # target branch, and the review requests list is empty, then
+ # use that default reviewer.
+ default_reviewer = None
+ if assign_default_reviewer:
+ default_reviewer = target_branch.code_reviewer
+ if default_reviewer is not None and len(review_requests) == 0:
+ review_requests.append((default_reviewer, default_review_type))
+
bmp = BranchMergeProposal(
registrant=registrant, source_branch=self,
target_branch=target_branch,
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2010-09-23 12:34:57 +0000
+++ lib/lp/code/model/tests/test_branch.py 2010-09-26 10:28:49 +0000
@@ -67,6 +67,7 @@
CannotDeleteBranch,
InvalidBranchMergeProposal,
)
+from lp.code.event.branchmergeproposal import NewBranchMergeProposalEvent
from lp.code.interfaces.branch import (
DEFAULT_BRANCH_STATUS_IN_LISTING,
IBranch,
@@ -1576,10 +1577,14 @@
self.product = self.factory.makeProduct()
self.user = self.factory.makePerson()
+ self.reviewer = self.factory.makePerson(name='johndoe')
self.source = self.factory.makeProductBranch(
name='source-branch', owner=self.user, product=self.product)
self.target = self.factory.makeProductBranch(
name='target-branch', owner=self.user, product=self.product)
+ self.target_with_default_reviewer = self.factory.makeProductBranch(
+ name='target-branch-with-reviewer', owner=self.user,
+ product=self.product, reviewer=self.reviewer)
self.prerequisite = self.factory.makeProductBranch(
name='prerequisite-branch', owner=self.user, product=self.product)
@@ -1587,6 +1592,19 @@
logout()
super(BranchAddLandingTarget, self).tearDown()
+ def assertOnePendingReview(self, proposal, reviewer, review_type=None):
+ # There should be one pending vote for the reviewer with the specified
+ # review type.
+ votes = list(proposal.votes)
+ self.assertEqual(1, len(votes))
+ self.assertEqual(reviewer, votes[0].reviewer)
+ self.assertEqual(self.user, votes[0].registrant)
+ self.assertIs(None, votes[0].comment)
+ if review_type is None:
+ self.assertIs(None, votes[0].review_type)
+ else:
+ self.assertEqual(review_type, votes[0].review_type)
+
def test_junkSource(self):
"""Junk branches cannot be used as a source for merge proposals."""
self.source.setTarget(user=self.source.owner)
@@ -1665,6 +1683,47 @@
self.source.addLandingTarget(
self.user, self.target, self.prerequisite)
+ def test_default_reviewer(self):
+ """If the target branch has a default reviewer set, this reviewer
+ should be assigned to the merge proposal, so long as the
+ assign_default_reviewer flag is True.
+ """
+ proposal = self.source.addLandingTarget(
+ self.user, self.target_with_default_reviewer,
+ assign_default_reviewer=True)
+ self.assertOnePendingReview(proposal, self.reviewer)
+
+ def test_default_reviewer_without_enable(self):
+ """If the target branch has a default reviewer set, this reviewer
+ should not be assigned to the merge proposal without the
+ assign_default_reviewer flag being set.
+ """
+ proposal = self.source.addLandingTarget(
+ self.user, self.target_with_default_reviewer)
+ self.assertEqual([], list(proposal.votes))
+
+ def test_default_reviewer_with_review_type(self):
+ """If the target branch has a default reviewer set, this reviewer
+ should be assigned to the merge proposal and the specified review type
+ used, so long as the assign_default_reviewer flag is True.
+ """
+ proposal = self.source.addLandingTarget(
+ self.user, self.target_with_default_reviewer,
+ default_review_type = 'code', assign_default_reviewer=True)
+ self.assertOnePendingReview(proposal, self.reviewer, 'code')
+
+ def test_notify_on_default_reviewer(self):
+ """Ensure that when a merge proposal is created with default reviewer
+ we only get a new branch merge proposal event and not an event for a
+ new reviewer being added.
+ """
+ result, event = self.assertNotifies(
+ NewBranchMergeProposalEvent,
+ self.source.addLandingTarget,
+ self.user, self.target_with_default_reviewer,
+ default_review_type = 'code', assign_default_reviewer=True)
+ self.assertEqual(result, event.object)
+
def test_attributeAssignment(self):
"""Smoke test to make sure the assignments are there."""
commit_message = u'Some commit message'
=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-09-15 06:02:12 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-09-26 10:28:49 +0000
@@ -10,8 +10,11 @@
>>> login('admin@xxxxxxxxxxxxx')
>>> from zope.component import getUtility
>>> from lp.code.enums import (
- ... BranchSubscriptionNotificationLevel, CodeReviewNotificationLevel)
+ ... BranchMergeProposalStatus,
+ ... BranchSubscriptionNotificationLevel,
+ ... CodeReviewNotificationLevel)
>>> from lp.registry.interfaces.person import IPersonSet
+ >>> from lp.registry.interfaces.product import IProductSet
>>> from lp.code.interfaces.branchlookup import IBranchLookup
>>> subscriber = getUtility(IPersonSet).getByEmail(
... 'foo.bar@xxxxxxxxxxxxx')
@@ -62,6 +65,8 @@
>>> nopriv_browser.getControl(
... name='field.prerequisite_branch').value = (
... '~name12/gnome-terminal/pushed')
+ >>> print_tag_with_id(nopriv_browser.contents, 'field.reviewer')
+ <BLANKLINE>
There is a cancel link shown with the buttons.
@@ -145,13 +150,16 @@
Requesting reviews
------------------
-You can request a review of a merge proposal.
-
+Any newly created merge proposal will have the reviewer set if a default
+has been defined or the branch owner will be used.
>>> sample_browser.open(klingon_proposal)
- >>> sample_browser.getLink('Request a review').click()
- >>> sample_browser.getControl('Reviewer').value = 'mark'
- >>> sample_browser.getControl('Review type').value = 'first'
- >>> sample_browser.getControl('Request Review').click()
+ >>> pending = find_tag_by_id(
+ ... sample_browser.contents, 'code-review-votes')
+ >>> print extract_text(pending)
+ Reviewer Review Type Date Requested Status
+ Sample Person ... ago Pending [Review]
+ Review via email: mp+...@xxxxxxxxxxxxxxxxxx
+ Request another review
The status of the merge proposal is updated to "Needs review".
@@ -162,14 +170,14 @@
Additional reviews can be requested.
>>> sample_browser.getLink('Request another review').click()
- >>> sample_browser.getControl('Reviewer').value = 'name12'
+ >>> sample_browser.getControl('Reviewer').value = 'mark'
>>> sample_browser.getControl('Request Review').click()
You can even re-request the same person to review, so that a new email is
sent.
>>> sample_browser.getLink('Request another review').click()
- >>> sample_browser.getControl('Reviewer').value = 'mark'
+ >>> sample_browser.getControl('Reviewer').value = 'name12'
>>> sample_browser.getControl('Review type').value = 'second'
>>> sample_browser.getControl('Request Review').click()
@@ -179,8 +187,8 @@
... sample_browser.contents, 'code-review-votes')
>>> print extract_text(pending)
Reviewer Review Type Date Requested Status
- Sample Person Pending [Review]
- Mark Shuttleworth second ... ago Pending
+ Mark Shuttleworth ... ago Pending
+ Sample Person second ... ago Pending [Review]
Review via email: mp+...@xxxxxxxxxxxxxxxxxx
Request another review
@@ -672,13 +680,22 @@
-----------------------------------------------
A branch that has a merge proposal, but no requested reviews shows this on the
-branch page.
+branch page. We create a proposal using the factory since we want to force
+one without a reviewer. This will not normally happen anymore in general usage
+since at the very least the branch owner is now automatically the reviewer.
- >>> nopriv_browser.open('http://code.launchpad.dev/~fred/fooix/feature')
- >>> nopriv_browser.getLink('Propose for merging').click()
- >>> nopriv_browser.getControl('Reviewer').value = ''
- >>> nopriv_browser.getControl('Propose Merge').click()
- >>> nopriv_browser.getLink('lp://dev/~fred/fooix/feature').click()
+ >>> login('foo.bar@xxxxxxxxxxxxx')
+ >>> source_branch = getUtility(IBranchLookup).getByUniqueName(
+ ... '~fred/fooix/feature')
+ >>> fooix = getUtility(IProductSet).getByName('fooix')
+ >>> bmp = factory.makeBranchMergeProposal(
+ ... registrant=eric, source_branch=source_branch,
+ ... target_branch=fooix.development_focus.branch,
+ ... set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+ >>> bmp_url = canonical_url(bmp)
+ >>> branch_url = canonical_url(bmp.source_branch)
+ >>> logout()
+ >>> nopriv_browser.open(branch_url)
>>> print_tag_with_id(nopriv_browser.contents, 'landing-targets')
Ready for review for merging into lp://dev/fooix
No reviews requested
Follow ups
