The proposal to merge lp:~wallyworld/launchpad/reviews-without-reviewer into
lp:launchpad/devel has been updated.
Description changed to:
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.
Implementation
The addLanding target method now assigns the default reviewer of the target
branch (accessed via the code_reviewer property) to the merge proposal.
Other business logic needed refactoring as a result of the change. In
particular, consider the processMergeProposal() method in codehandler.py - this
method creates a new merge proposal from an email message. The reviewer may or
may not be specified in the message. The original email processing logic
provided these functions:
1. explicitly set the default reviewer if the newly created merge proposal
didn't have one.
2. allow a user to specify a reviewer for an existing merge proposal.
To handle both cases, extra code was added to the merge proposal creation logic
to parse and extract from the email body any reviewer which may have been
specified. The new code makes use of existing functionality in the codehandler
methods. The extracted reviewer is passed to addLandingTarget() to create the
merge proposal, using the same logic as when a web based request is processed
by the addLandingTarget() method.
Once the merge proposal is created from the email message, any other commands
in the email body are processed. At the moment when creating a merge proposal,
I don't think there are any expected commands except 'reviewer', but the old
business logic still allowed for such a scenario and so this behaviour is being
retained. That way, new commands can be added without needing to change any
processMergeProposal() logic.
***
If it is deemed that there is no valid case to retain the current logic of
allowing commands other than "reviewer" in the merge proposal creation email,
then the result of the refactoring for this unit of work means that some extra
(unnecessary) business logic can be easily deleted.
***
Note that the existing AddReviewerEmail command, while no longer used when
creating a merge proposal, is still needed to process requests to set the
reviewer on existing merge proposals.
Tests
The easy part of this change was the actual implementation. As a result of
merge proposals now always being created with a reviewer by virtue of the fact
that the reviewer defaults to the target branch owner, a large number of tests
started to fail.
The tests executed were:
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:
TestRegisterBranchWithADefaultReviewerMergeProposalView. These 2 classes
run merge proposal tests for a
target branch without, and with, a default reviewer assigned respectively.
The BranchAddLandingTarget class has new tests:
- test_default_reviewer
- test_default_reviewer_when_owner
- test_default_reviewer_with_review_type
- test_notify_on_default_reviewer
Changed tests:
A *large* number of tests needed to be refactored since the logic in the
tests assumed a newly created merge proposal (from factory or not) would have
no reviewer unless one were specified. This included tests in:
lp.code.browser.tests.test_branchmergeproposal
lp.code.browser.tests.test_branchmergeproposallisting
lp.code.mail.tests.test_branchmergeproposal
lp.code.mail.tests.test_codehandler
lp.code.model.tests.test_branch
lp.code.model.tests.test_branchmergeproposal
xx-branchmergeproposals.txt
The lp.testing.factory makeBranchMergeProposal() method was changed to
accept an optional default reviewer and review type. To still allow testing of
functionality on merge proposals with no reviewers, a new factory method
makeBranchMergeProposalWithNoReviewers() was created.
= 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.
./lib/lp/code/mail/tests/test_codehandler.py
257: W291 trailing whitespace
627: W291 trailing whitespace
771: E202 whitespace before ')'
860: E202 whitespace before ')'
883: E202 whitespace before ')'
1161: E301 expected 1 blank line, found 2
1163: W291 trailing whitespace
241: Line exceeds 78 characters.
257: Line has trailing whitespace.
617: Line exceeds 78 characters.
623: Line exceeds 78 characters.
627: Line has trailing whitespace.
1163: Line has trailing whitespace.
--
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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
