launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

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.