Launchpad itself

Merge lp:~thumper/launchpad/auto-approve into lp:launchpad/db-devel

auto-approve
Merge into db-devel

Proposed by Tim Penhey on 2009-06-05

Status:	Rejected
Rejected by:	Stuart Bishop on 2009-06-16
Proposed branch:	lp:~thumper/launchpad/auto-approve
Merge into:	lp:launchpad/db-devel
Diff against target:	773 lines (has conflicts) Text conflict in lib/lp/code/model/branch.py
To merge this branch:	bzr merge lp:~thumper/launchpad/auto-approve
Related bugs:	Link a bug report
Related blueprints:	Auto approval policies for code reviews (High)

Reviewer	Review Type	Date Requested	Status
Jonathan Lange (community)	code	2009-06-05	Approve on 2009-06-09
Stuart Bishop (community)	db	2009-06-05	Needs Resubmitting on 2009-06-08
Review via email: mp+7087@code.launchpad.net

Revision history for this message

Tim Penhey (thumper) wrote on 2009-06-05:

Here is a summary of the changes:

database/schema/comments.sql | 1
database/schema/patch-2109-99-0.sql | 7

The database patch to add a column to products for recording the auto approval policy.

lib/lp/registry/configure.zcml | 2
lib/lp/registry/interfaces/product.py | 7
lib/lp/registry/model/product.py | 7

Adding the review_approval_policy to the Product.

lib/lp/code/interfaces/branchtarget.py | 4
lib/lp/code/model/branchtarget.py | 17 +

Adding the review_approval_policy to IBranchTarget.

lib/lp/code/interfaces/codereviewvote.py | 10
lib/lp/code/model/codereviewvote.py | 10

Add a helper attribute to get the vote from the code review comment.

lib/lp/code/model/branchmergeproposal.py | 22 +
lib/lp/code/model/tests/test_branchmergeproposals.py | 53 +++

Add in the auto approval check.

lib/lp/code/interfaces/reviewapprovalpolicies.py | 67 ++++
lib/lp/code/model/reviewapprovalpolicies.py | 124 ++++++++
lib/lp/code/model/tests/test_reviewapprovalpolicies.py | 244 +++++++++++++++++

The policies themselves and their tests.

14 files changed, 572 insertions(+), 3 deletions(-)

Tests:
TestAutomaticProposalAppoval
test_reviewapprovalpolicies

Revision history for this message

Jonathan Lange (jml) wrote on 2009-06-05:

Download full text (32.6 KiB)

Hi Tim,

Thanks for looking into this feature. People have been bugging us about it
forever, and it'll be great to give them what they want.

I notice that there's no UI for this yet. That's OK -- it's a big enough
branch and the features as implemented make sense as an atomic whole. However,
we really should get a UI asap, since we don't want this code lying dormant.

I've got quite a few comments and questions, some of them trivial, some of
them not. Overall, the code looks really good. I'm particularly pleased to see
the thorough policy tests & the clean design.

cheers,
jml

> === modified file 'database/schema/comments.sql'
> --- database/schema/comments.sql 2009-05-14 09:57:46 +0000
> +++ database/schema/comments.sql 2009-06-04 23:29:41 +0000
> @@ -593,6 +593,7 @@
> COMMENT ON COLUMN Product.reviewer_whiteboard IS 'A whiteboard for Launchpad admins, registry experts and the project owners to capture the state of current issues with the project.';
> COMMENT ON COLUMN Product.license_approved IS 'The Other/Open Source license has been approved by an administrator.';
> COMMENT ON COLUMN Product.remote_product IS 'The ID of this product on its remote bug tracker.';
> +COMMENT ON COLUMN Product.review_approval_policy IS 'An enumerated value referring to the CodeReviewApprovalPolicy to define the requirements for automatically approving or rejecting proposals based on the code reviews';
>

Please change "to define" to "that defines".

> -- ProductLicense
> COMMENT ON TABLE ProductLicense IS 'The licenses that cover the software for a product.';
>
> === added file 'database/schema/patch-2109-99-0.sql'
> --- database/schema/patch-2109-99-0.sql 1970-01-01 00:00:00 +0000
> +++ database/schema/patch-2109-99-0.sql 2009-06-04 23:29:41 +0000
> @@ -0,0 +1,7 @@
> +SET client_min_messages = ERROR;
> +
> +ALTER TABLE Product
> + ADD COLUMN review_approval_policy INT DEFAULT 1 NOT NULL;
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2109, 99, 0);
> +
>
> === modified file 'lib/lp/code/interfaces/branchtarget.py'
> --- lib/lp/code/interfaces/branchtarget.py 2009-05-19 06:12:59 +0000
> +++ lib/lp/code/interfaces/branchtarget.py 2009-06-04 23:23:15 +0000
> @@ -87,6 +87,10 @@
> supports_merge_proposals = Attribute(
> "Does this target support merge proposals at all?")
>
> + review_approval_policy = Attribute(
> + "The policy object relating to the approval policy set for the "
> + "target.")
> +
> def areBranchesMergeable(other_target):
> """Are branches from other_target mergeable into this target."""
>
>
> === modified file 'lib/lp/code/interfaces/codereviewvote.py'
> --- lib/lp/code/interfaces/codereviewvote.py 2009-04-17 10:32:16 +0000
> +++ lib/lp/code/interfaces/codereviewvote.py 2009-06-05 00:31:22 +0000
> @@ -8,14 +8,14 @@
> ]
>
> from zope.interface import Interface
> -from zope.schema import Datetime, Int, TextLine
> +from zope.schema import Choice, Datetime, Int, TextLine
>
> from canonical.launchpad import _
> from canonical.launchpad.fields import PublicPersonChoice
> from lp.code.interfaces.branchmergeproposal import (
> IBranchMergeProposal)
> from lp.code.interf...

Hi Tim,

Thanks for looking into this feature. People have been bugging us about it
forever, and it'll be great to give them what they want.

I've got quite a few comments and questions, some of them trivial, some of
them not. Overall, the code looks really good. I'm particularly pleased to see
the thorough policy tests & the clean design.

cheers,
jml

> === modified file 'database/schema/comments.sql'
> --- database/schema/comments.sql	2009-05-14 09:57:46 +0000
> +++ database/schema/comments.sql	2009-06-04 23:29:41 +0000
> @@ -593,6 +593,7 @@
>  COMMENT ON COLUMN Product.reviewer_whiteboard IS 'A whiteboard for Launchpad admins, registry experts and the project owners to capture the state of current issues with the project.';
>  COMMENT ON COLUMN Product.license_approved IS 'The Other/Open Source license has been approved by an administrator.';
>  COMMENT ON COLUMN Product.remote_product IS 'The ID of this product on its remote bug tracker.';
> +COMMENT ON COLUMN Product.review_approval_policy IS 'An enumerated value referring to the CodeReviewApprovalPolicy to define the requirements for automatically approving or rejecting proposals based on the code reviews';
>

Please change "to define" to "that defines".

>  -- ProductLicense
>  COMMENT ON TABLE ProductLicense IS 'The licenses that cover the software for a product.';
> 
> === added file 'database/schema/patch-2109-99-0.sql'
> --- database/schema/patch-2109-99-0.sql	1970-01-01 00:00:00 +0000
> +++ database/schema/patch-2109-99-0.sql	2009-06-04 23:29:41 +0000
> @@ -0,0 +1,7 @@
> +SET client_min_messages = ERROR;
> +
> +ALTER TABLE Product
> +  ADD COLUMN review_approval_policy INT DEFAULT 1 NOT NULL;
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2109, 99, 0);
> +
> 
> === modified file 'lib/lp/code/interfaces/branchtarget.py'
> --- lib/lp/code/interfaces/branchtarget.py	2009-05-19 06:12:59 +0000
> +++ lib/lp/code/interfaces/branchtarget.py	2009-06-04 23:23:15 +0000
> @@ -87,6 +87,10 @@
>      supports_merge_proposals = Attribute(
>          "Does this target support merge proposals at all?")
>  
> +    review_approval_policy = Attribute(
> +        "The policy object relating to the approval policy set for the "
> +        "target.")
> +
>      def areBranchesMergeable(other_target):
>          """Are branches from other_target mergeable into this target."""
>  
> 
> === modified file 'lib/lp/code/interfaces/codereviewvote.py'
> --- lib/lp/code/interfaces/codereviewvote.py	2009-04-17 10:32:16 +0000
> +++ lib/lp/code/interfaces/codereviewvote.py	2009-06-05 00:31:22 +0000
> @@ -8,14 +8,14 @@
>      ]
>  
>  from zope.interface import Interface
> -from zope.schema import Datetime, Int, TextLine
> +from zope.schema import Choice, Datetime, Int, TextLine
>  
>  from canonical.launchpad import _
>  from canonical.launchpad.fields import PublicPersonChoice
>  from lp.code.interfaces.branchmergeproposal import (
>      IBranchMergeProposal)
>  from lp.code.interfaces.codereviewcomment import (
> -    ICodeReviewComment)
> +    CodeReviewVote, ICodeReviewComment)
>  from lazr.restful.fields import Reference
>  from lazr.restful.declarations import (
>      export_as_webservice_entry, exported)
> @@ -66,3 +66,9 @@
>                  "The code review comment that contains the most recent vote."
>                  ),
>              required=True, schema=ICodeReviewComment))
> +
> +    vote = exported(
> +        Choice(
> +            description=_('The last review by the reviewer.'),
> +            required=False, readonly=True,
> +            vocabulary=CodeReviewVote))
>

The "by the reviewer" thing confuses me. What class is this on?

> === added file 'lib/lp/code/interfaces/reviewapprovalpolicies.py'
> --- lib/lp/code/interfaces/reviewapprovalpolicies.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/interfaces/reviewapprovalpolicies.py	2009-06-04 04:37:54 +0000
> @@ -0,0 +1,67 @@
> +# Copyright 2009 Canonical Ltd.  All rights reserved.
> +
> +"""Implementation classes for code review approval policies."""
> +

These aren't implementation classes, they're interface classes.

> +__metaclass__ = type
> +__all__ = []
> +

I think you need to put the enum & the policy interface in here.

> +
> +from zope.interface import Interface
> +
> +from lazr.enum import DBEnumeratedType, DBItem
> +
> +
> +class CodeReviewApprovalPolicy(DBEnumeratedType):
> +    """Policies for automatically approving code reviews.
> +
> +    In all the situations mentioned below, reviewer refers to a member of the
> +    review team for the target branch.  There has to be reviews by people who
> +    are in a position to say yay or nay for the merge.

The normal spelling is "yea".

I think the sentence would be clearer if it went like:
  To change the status of a proposal, there must be reviews by people who ...

Hmm. I don't know how to finish it. What are we trying to say here?

> +    """
> +
> +    NO_AUTO_CHANGE = DBItem(1, """
> +        No automatic approval
> +
> +        All approvals or rejections are done manually.
> +        """)
> +
> +    ONE_REVIEWER = DBItem(2, """
> +        One review is enough
> +
> +        The approval or rejection of one reviewer is enough to accept or
> +        reject the proposal as a whole.  If the reviewer approves the review,
> +        the proposal will be set to the approved state.  If the reviewer
> +        disapproves in the review, the proposal is rejected.  Any other review
> +        does not change the state of the proposal.
> +        """)
> +
> +    TWO_REVIEWERS = DBItem(3, """
> +        Two reviews needed
> +
> +        The proposal needs to be approved by two reviewers in order for the
> +        proposal to be approved.  If there are two approvals the proposal is
> +        approved.  If there are two rejections then the proposal is rejected.
> +        If the reviewers disagree, then the proposal status is not updated.
> +        """)
> +
> +    ALL_REQUESTED_COMPLETE = DBItem(4, """
> +        All requested reviews complete
> +
> +        All requested reviews must be complete and agree.  If all reviewers
> +        agree the status is updated.  Reviewers who abstain are ignored.
> +        """)
> +
> +
> +class ICodeReviewApprovalPolicy(Interface):
> +    """A code review approval policy."""
> +
> +    def checkReviews(reviews):
> +        """Check the status of the reviews to see if it should be approved.
> +
> +        :param reviews: An iterable of (person, review), where the
> +            person is considered a valid reviewer of the proposal, and the
> +            review is a CodeReviewVote value or None if pending.
> +        :return: BranchMergeProposalStatus.CODE_APPROVED if the proposal state
> +            should be approved, BranchMergeProposalStatus.REJECTED if the
> +            proposal should be rejected, or None if no change.
> +        """
>

At this point, I'm a little surprised there's no public interface for getting
an `ICodeReviewApprovalPolicy` from a `CodeReviewApprovalPolicy` enum. Perhaps
it's not needed.

> === modified file 'lib/lp/code/model/branchmergeproposal.py'
> --- lib/lp/code/model/branchmergeproposal.py	2009-05-17 02:23:49 +0000
> +++ lib/lp/code/model/branchmergeproposal.py	2009-06-05 01:39:33 +0000
> @@ -653,8 +653,30 @@
>          if _notify_listeners:
>              notify(NewCodeReviewCommentEvent(
>                      code_review_message, original_email))
> +        # Check for automatic approval or rejection.
> +        self._autoApproveCheck(message.owner)
>          return code_review_message
>  
> +    def _autoApproveCheck(self, reviewer):
> +        """Check the approval policy for this proposal."""
> +        # Get all the reviews pending and done by members of the review team.
> +        if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:
> +            return None

I don't think the comment matches the if statement. The idea here is that we
only want to change the state based on a review vote if the merge proposal is
in "needs review", right?

> +        reviews = [
> +            (reference.reviewer, reference.vote) for reference in self.votes
> +            if self.isPersonValidReviewer(reference.reviewer)]

Ahh, here it is.

> +        policy = self.target_branch.target.review_approval_policy

We have style guides warning against (but not forbidding) use of multiple dots
on the line.

Perhaps review_approval_policy should be a property of BranchMergeProposal?

> +        next_status = policy.checkReviews(reviews)
> +        # Auto approval approves or rejects the tip.
> +        tip_rev_id = self.target_branch.last_scanned_id
> +        if next_status == BranchMergeProposalStatus.CODE_APPROVED:
> +            self.approveBranch(reviewer, tip_rev_id)
> +        elif next_status == BranchMergeProposalStatus.REJECTED:
> +            self.rejectBranch(reviewer, tip_rev_id)
> +        else:
> +            # Nothing to do.
> +            pass
> +

Do we want to do anything special if the next_status is not CODE_APPROVED, not
REJECTED and not None? i.e. if checkReviews violated its contract?

>      def updatePreviewDiff(self, diff_content, diff_stat,
>                            source_revision_id, target_revision_id,
>                            dependent_revision_id=None, conflicts=None):
> 
> === modified file 'lib/lp/code/model/branchtarget.py'
> --- lib/lp/code/model/branchtarget.py	2009-05-14 22:56:43 +0000
> +++ lib/lp/code/model/branchtarget.py	2009-06-04 23:44:00 +0000
> @@ -17,6 +17,8 @@
>  from lp.code.interfaces.branchcollection import IAllBranches
>  from lp.code.interfaces.branchtarget import (
>      check_default_stacked_on, IBranchTarget)
> +from lp.code.model.reviewapprovalpolicies import (
> +    get_approval_policy, NoChangePolicy)
>  from lp.soyuz.interfaces.publishing import PackagePublishingPocket
>  from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
>  
> @@ -93,6 +95,11 @@
>          """See `IBranchTarget`."""
>          return True
>  
> +    @property
> +    def review_approval_policy(self):
> +        """See `IBranchTarget`."""
> +        return NoChangePolicy()
> +

FWIW, if you made get_approval_policy a public function / method that you get
get from interfaces.reviewapprovalpolicies (either by import or by utility),
then you could completely avoid importing from the model module by doing:

return get_approval_policy(CodeReviewApprovalPolicy.NO_AUTO_CHANGE)

Not only will this make the fascist a little happier, it preserves a fairly
sane barrier of abstraction.

>      def areBranchesMergeable(self, other_target):
>          """See `IBranchTarget`."""
>          # Branches are mergable into a PackageTarget if the source package
> @@ -162,6 +169,11 @@
>          """See `IBranchTarget`."""
>          return False
>  
> +    @property
> +    def review_approval_policy(self):
> +        """See `IBranchTarget`."""
> +        return NoChangePolicy()
> +
>      def areBranchesMergeable(self, other_target):
>          """See `IBranchTarget`."""
>          return False
> @@ -223,6 +235,11 @@
>          """See `IBranchTarget`."""
>          return True
>  
> +    @property
> +    def review_approval_policy(self):
> +        """See `IBranchTarget`."""
> +        return get_approval_policy(self.product.review_approval_policy)
> +
>      def areBranchesMergeable(self, other_target):
>          """See `IBranchTarget`."""
>          # Branches are mergable into a PackageTarget if the source package
>

Nice.

I'm a little disappointed to see source packages getting second-class
treatment though. Why aren't we adding policy support for them now? What plans
do we have for getting the policy stuff working for them?

> === modified file 'lib/lp/code/model/codereviewvote.py'
> --- lib/lp/code/model/codereviewvote.py	2009-04-17 10:32:16 +0000
> +++ lib/lp/code/model/codereviewvote.py	2009-06-05 00:31:22 +0000
> @@ -15,6 +15,8 @@
>  from canonical.database.datetimecol import UtcDateTimeCol
>  from canonical.database.sqlbase import SQLBase
>  from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
> +
> +
>  class CodeReviewVoteReference(SQLBase):
>      """See `ICodeReviewVote`"""
>  
> @@ -33,3 +35,11 @@
>      review_type = StringCol(default=None)
>      comment = ForeignKey(
>          dbName='vote_message', foreignKey='CodeReviewComment', default=None)
> +
> +    @property
> +    def vote(self):
> +        """Return the vote from the comment if it is there."""
> +        if self.comment is not None:
> +            return self.comment.vote
> +        else:
> +            return None
> 
> === added file 'lib/lp/code/model/reviewapprovalpolicies.py'
> --- lib/lp/code/model/reviewapprovalpolicies.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/model/reviewapprovalpolicies.py	2009-06-04 04:37:54 +0000
> @@ -0,0 +1,124 @@
> +# Copyright 2009 Canonical Ltd.  All rights reserved.
> +
> +"""Implementation classes for code review approval policies."""
> +
> +__metaclass__ = type
> +__all__ = [
> +    'get_approval_policy',
> +    'NoChangePolicy',
> +    'OneReviewerPolicy',
> +    'ReviewsCompletePolicy',
> +    'TwoReviewersPolicy',
> +    ]
> +
> +from zope.interface import implements
> +
> +from lp.code.interfaces.branchmergeproposal import BranchMergeProposalStatus
> +from lp.code.interfaces.codereviewcomment import CodeReviewVote
> +from lp.code.interfaces.reviewapprovalpolicies import (
> +    CodeReviewApprovalPolicy, ICodeReviewApprovalPolicy)
> +
> +
> +class NoChangePolicy:
> +    """A policy where no changes are made."""
> +

I think that will read better if "automatically" is added to the end of the sentence.

> +    implements(ICodeReviewApprovalPolicy)
> +
> +    def checkReviews(self, reviews):
> +        """No change, return None."""
> +        return None
> +
> +
> +class BasePolicy:
> +    """A base policy class to sum review types."""
> +
> +    def countReviews(self, reviews):
> +        """Return a dict of review counts."""
> +        result = {
> +            CodeReviewVote.APPROVE: 0,
> +            CodeReviewVote.DISAPPROVE: 0,
> +            CodeReviewVote.ABSTAIN: 0,
> +            'pending': 0,
> +            'other': 0,
> +            }
> +        for reviewer, review in reviews:
> +            if review in result:
> +                result[review] += 1
> +            elif review is None:
> +                result['pending'] += 1
> +            else:
> +                result['other'] += 1
> +        return result
> +
> +
> +class OneReviewerPolicy(BasePolicy):
> +    """A policy where one review is enough."""
> +
> +    implements(ICodeReviewApprovalPolicy)
> +
> +    def checkReviews(self, reviews):
> +        """See `ICodeReviewApprovalPolicy`."""
> +        counts = self.countReviews(reviews)
> +        approvals = counts[CodeReviewVote.APPROVE]
> +        disapprovals = counts[CodeReviewVote.DISAPPROVE]
> +        if approvals > 0 and disapprovals == 0:
> +            return BranchMergeProposalStatus.CODE_APPROVED
> +        elif disapprovals > 0 and approvals == 0:
> +            return BranchMergeProposalStatus.REJECTED
> +        else:
> +            return None
> +
> +
> +class TwoReviewersPolicy(BasePolicy):
> +    """A policy where two non-conflicting reviews are needed."""
> +
> +    implements(ICodeReviewApprovalPolicy)
> +
> +    def checkReviews(self, reviews):
> +        """See `ICodeReviewApprovalPolicy`."""
> +        counts = self.countReviews(reviews)
> +        approvals = counts[CodeReviewVote.APPROVE]
> +        disapprovals = counts[CodeReviewVote.DISAPPROVE]
> +        if approvals > 1 and disapprovals == 0:
> +            return BranchMergeProposalStatus.CODE_APPROVED
> +        elif disapprovals > 1 and approvals == 0:
> +            return BranchMergeProposalStatus.REJECTED
> +        else:
> +            return None
> +
> +
> +class ReviewsCompletePolicy(BasePolicy):
> +    """A policy where all requested reviews need to be complete.
> +
> +    A complete review is approved, disapproved or abstained.
> +    """
> +
> +    implements(ICodeReviewApprovalPolicy)
> +
> +    def checkReviews(self, reviews):
> +        """See `ICodeReviewApprovalPolicy`."""
> +        counts = self.countReviews(reviews)
> +        # If there are pending or in-progress reviews, no change.
> +        if counts['pending'] > 0 or counts['other'] > 0:
> +            return None
> +        approvals = counts[CodeReviewVote.APPROVE]
> +        disapprovals = counts[CodeReviewVote.DISAPPROVE]
> +        if approvals > 0 and disapprovals == 0:
> +            return BranchMergeProposalStatus.CODE_APPROVED
> +        elif disapprovals > 0 and approvals == 0:
> +            return BranchMergeProposalStatus.REJECTED
> +        else:
> +            return None
> +

Cool. These all work out rather well.

It would be nice if each of the classes mentioned the enum value that they are
associated with.

In fact...

> +
> +def get_approval_policy(policy):
> +    """Return a policy class based on the enumerated value."""
> +    if policy == CodeReviewApprovalPolicy.NO_AUTO_CHANGE:
> +        return NoChangePolicy()
> +    if policy == CodeReviewApprovalPolicy.ONE_REVIEWER:
> +        return OneReviewerPolicy()
> +    if policy == CodeReviewApprovalPolicy.TWO_REVIEWERS:
> +        return TwoReviewersPolicy()
> +    if policy == CodeReviewApprovalPolicy.ALL_REQUESTED_COMPLETE:
> +        return ReviewsCompletePolicy()
> +    raise AssertionError("Invalid policy value: %s" % policy)
>

... if instead of mentioning, they had the enum value as a variable, then this
method could be implemented with a loop, rather than a bunch of ifs. e.g.

all_policies = [
    NoChangePolicy,
    OneReviewerPolicy,
    TwoReviewersPolicy,
    ReviewsCompletePolicy,
  ]
for policy_factory in all_policies:
  if policy_factory.policy_value == policy:
    return policy_factory()
  raise AssertionError("Invalid policy value: %s" % policy)

You could even discover 'all_policies', rather than manually populating it.

Just a thought.

> === modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
> --- lib/lp/code/model/tests/test_branchmergeproposals.py	2009-05-12 08:38:55 +0000
> +++ lib/lp/code/model/tests/test_branchmergeproposals.py	2009-06-05 01:18:29 +0000
> @@ -37,6 +37,7 @@
>      WrongBranchMergeProposal)
>  from lp.code.interfaces.branchsubscription import (
>      BranchSubscriptionNotificationLevel, CodeReviewNotificationLevel)
> +from lp.code.interfaces.reviewapprovalpolicies import CodeReviewApprovalPolicy
>  from canonical.launchpad.interfaces.message import IMessageJob
>  from lp.registry.interfaces.person import IPersonSet
>  from lp.registry.interfaces.product import IProductSet
> @@ -1402,5 +1403,57 @@
>              removeSecurityProxy(merge_proposal.preview_diff).diff_id)
>  
>  
> +class TestAutomaticProposalAppoval(TestCaseWithFactory):
> +    """Test that the approval policy is called if set."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        TestCaseWithFactory.setUp(self)
> +        self.product = self.factory.makeProduct()
> +        login_person(self.product.owner)
> +        self.product.review_approval_policy = (
> +            CodeReviewApprovalPolicy.ONE_REVIEWER)
> +        self.bmp = self.factory.makeBranchMergeProposal(
> +            set_state=BranchMergeProposalStatus.NEEDS_REVIEW,
> +            product=self.product)
> +
> +    def assertReviewState(self, reviewer, vote, expected):
> +        """Have `reviewer` comment on the proposal and check the status."""
> +        login_person(reviewer)
> +        self.bmp.createComment(
> +            reviewer, subject=None, content="comment", vote=vote)
> +        self.assertEqual(expected, self.bmp.queue_status)
> +

I'm not a huge fan of 'assert' methods exercising the system-under-test. I'd
be happier if the test class had a vote() helper that did the createComment,
and the tests themselves made assertions about the state.

Also, it doesn't look like the 'reviewer' parameter is used much -- could we
instead just use self.bmp.target_branch.owner instead?

> +    def test_auto_approval(self):
> +        # If the product of the target branch has a policy to auto approve the
> +        # proposal with a single review, then an approve review will cause the
> +        # state to move from NEEDS_REVIEW to CODE_APPROVED.
> +        self.assertReviewState(
> +            reviewer=self.bmp.target_branch.owner,
> +            vote=CodeReviewVote.APPROVE,
> +            expected=BranchMergeProposalStatus.CODE_APPROVED)
> +
> +    def test_auto_rejection(self):
> +        # If the product of the target branch has a policy to auto reject the
> +        # proposal with a single review, then a disapprove review will cause
> +        # the state to move from NEEDS_REVIEW to REJECTED.
> +        self.assertReviewState(
> +            reviewer=self.bmp.target_branch.owner,
> +            vote=CodeReviewVote.DISAPPROVE,
> +            expected=BranchMergeProposalStatus.REJECTED)
> +

It's a shame we have to rely on the specific policy at use here, when what
we're really interested in is the fact that a policy is consulted at all.

Oh well, I think this is good enough.

> +    def test_not_needs_review(self):
> +        # Auto approval only happens if the proposal is in NEEDS_REVIEW state.
> +        # So a normally approving vote when in a different state change the
> +        # status of the proposal.
> +        login_person(self.bmp.registrant)
> +        self.bmp.setAsWorkInProgress()
> +        self.assertReviewState(
> +            reviewer=self.bmp.target_branch.owner,
> +            vote=CodeReviewVote.APPROVE,
> +            expected=BranchMergeProposalStatus.WORK_IN_PROGRESS)
> +

These are good tests, thanks.

It looks like you are missing tests to check that only people on the review
team count for policy changes.

> +
>  def test_suite():
>      return TestLoader().loadTestsFromName(__name__)
> 
> === added file 'lib/lp/code/model/tests/test_reviewapprovalpolicies.py'
> --- lib/lp/code/model/tests/test_reviewapprovalpolicies.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/model/tests/test_reviewapprovalpolicies.py	2009-06-04 04:37:54 +0000
> @@ -0,0 +1,244 @@
> +# Copyright 2009 Canonical Ltd.  All rights reserved.
> +
> +"""Tests for code review approval policies."""
> +
> +__metaclass__ = type
> +
> +import unittest
> +
> +from canonical.testing.layers import DatabaseFunctionalLayer
> +
> +from lp.code.interfaces.branchmergeproposal import BranchMergeProposalStatus
> +from lp.code.interfaces.codereviewcomment import CodeReviewVote
> +from lp.code.interfaces.reviewapprovalpolicies import (
> +    CodeReviewApprovalPolicy, ICodeReviewApprovalPolicy)
> +from lp.code.model.reviewapprovalpolicies import (
> +    get_approval_policy, NoChangePolicy, OneReviewerPolicy,
> +    ReviewsCompletePolicy, TwoReviewersPolicy)
> +from lp.testing import login_person, TestCase, TestCaseWithFactory
> +
> +
> +class TestGetApprovalPolicy(TestCase):
> +    """Test the policy factory generates the correct types."""
> +
> +    def test_no_change_policy(self):
> +        # NO_AUTO_CHANGE creates a NoChangePolicy.
> +        policy = get_approval_policy(CodeReviewApprovalPolicy.NO_AUTO_CHANGE)
> +        self.assertTrue(isinstance(policy, NoChangePolicy))
> +        self.assertTrue(ICodeReviewApprovalPolicy.providedBy(policy))
> +

Please use assertIsInstance and assertProvides rather than assertTrue here:
the error messages will be better.

> +    def test_one_reviewer_policy(self):
> +        # ONE_REVIEWER creates a OneReviewerPolicy.
> +        policy = get_approval_policy(CodeReviewApprovalPolicy.ONE_REVIEWER)
> +        self.assertTrue(isinstance(policy, OneReviewerPolicy))
> +        self.assertTrue(ICodeReviewApprovalPolicy.providedBy(policy))
> +

Ditto.

> +    def test_two_reviewers_policy(self):
> +        # TWO_REVIEWERS creates a TwoReviewersPolicy.
> +        policy = get_approval_policy(CodeReviewApprovalPolicy.TWO_REVIEWERS)
> +        self.assertTrue(isinstance(policy, TwoReviewersPolicy))
> +        self.assertTrue(ICodeReviewApprovalPolicy.providedBy(policy))
> +

Ditto.

> +    def test_all_reviews_completed_policy(self):
> +        # ALL_REQUESTED_COMPLETE creates a ReviewsCompletePolicy.
> +        policy = get_approval_policy(
> +            CodeReviewApprovalPolicy.ALL_REQUESTED_COMPLETE)
> +        self.assertTrue(isinstance(policy, ReviewsCompletePolicy))
> +        self.assertTrue(ICodeReviewApprovalPolicy.providedBy(policy))
> +

Ditto.

> +
> +class TestNoChangePolicy(TestCaseWithFactory):
> +    """Test that a no change policy doesn't change the status."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        TestCaseWithFactory.setUp(self)
> +        self.policy = NoChangePolicy()
> +
> +    def test_review_approval(self):
> +        # An review approval doesn't change the status.
> +        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +    def test_review_disapproval(self):
> +        # An review disapproval doesn't change the status.
> +        reviews = [(self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +
> +class TestOneReviewerPolicy(TestCaseWithFactory):
> +    """Test that one review is enough to change the status."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        TestCaseWithFactory.setUp(self)
> +        self.policy = OneReviewerPolicy()
> +
> +    def test_review_approval(self):
> +        # An review approval doesn't change the status.
> +        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
> +        self.assertEqual(
> +            BranchMergeProposalStatus.CODE_APPROVED,
> +            self.policy.checkReviews(reviews))
> +
> +    def test_review_disapproval(self):
> +        # An review disapproval doesn't change the status.
> +        reviews = [(self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> +        self.assertEqual(
> +            BranchMergeProposalStatus.REJECTED,
> +            self.policy.checkReviews(reviews))
> +
> +    def test_other_review(self):
> +        # An review disapproval doesn't change the status.
> +        reviews = [(self.factory.makePerson(), CodeReviewVote.ABSTAIN)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +    def test_conflicting_reviews(self):
> +        # If there is an approval and a disapproval, an update isn't
> +        # suggested.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +

This last one is worth testing, but afaict, it'll never actually happen live,
since one of the votes will take place first, thus changing the status.

Is this an issue?

> +
> +class TestTwoReviewersPolicy(TestCaseWithFactory):
> +    """Test that two reviews are needed to change the status."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        TestCaseWithFactory.setUp(self)
> +        self.policy = TwoReviewersPolicy()
> +
> +    def test_review_approval(self):
> +        # An single approval doesn't change the status.
> +        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +    def test_two_approvals(self):
> +        # Two approvals do change the status.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE)]
> +        self.assertEqual(
> +            BranchMergeProposalStatus.CODE_APPROVED,
> +            self.policy.checkReviews(reviews))
> +
> +    def test_review_disapproval(self):
> +        # An single disapproval doesn't change the status.
> +        reviews = [(self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +    def test_two_disapprovals(self):
> +        # Two disapprovals do change the status.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> +        self.assertEqual(
> +            BranchMergeProposalStatus.REJECTED,
> +            self.policy.checkReviews(reviews))
> +
> +    def test_conflicting_reviews(self):
> +        # If there is an approval and a disapproval, an update isn't
> +        # suggested.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +
> +class TestReviewsCompletePolicy(TestCaseWithFactory):
> +    """Test that all requested reviews are complete."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        TestCaseWithFactory.setUp(self)
> +        self.policy = ReviewsCompletePolicy()
> +
> +    def test_review_approval(self):
> +        # An single approval is enough to approve the proposal.
> +        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
> +        self.assertEqual(
> +            BranchMergeProposalStatus.CODE_APPROVED,
> +            self.policy.checkReviews(reviews))
> +
> +    def test_review_approval_and_pending(self):
> +        # An single approval is not good enough if there is another pending.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> +            (self.factory.makePerson(), None)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +    def test_review_approval_and_other(self):
> +        # An single approval is not good enough if there is another incomplete
> +        # review.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.NEEDS_FIXING)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +    def test_review_approval_and_abstain(self):
> +        # An single approval is good enough if the other requested reviewer
> +        # abstained.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.ABSTAIN)]
> +        self.assertEqual(
> +            BranchMergeProposalStatus.CODE_APPROVED,
> +            self.policy.checkReviews(reviews))
> +
> +    def test_review_disapproval(self):
> +        # An single disapproval is enough to reject the proposal.
> +        reviews = [(self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> +        self.assertEqual(
> +            BranchMergeProposalStatus.REJECTED,
> +            self.policy.checkReviews(reviews))
> +
> +    def test_review_disapproval_and_pending(self):
> +        # An single disapproval is not good enough if there is another
> +        # pending.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
> +            (self.factory.makePerson(), None)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +    def test_review_disapproval_and_other(self):
> +        # An single disapproval is not good enough if there is another
> +        # incomplete review.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.NEEDS_FIXING)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +
> +    def test_review_disapproval_and_abstain(self):
> +        # An single disapproval is good enough if the other requested reviewer
> +        # abstained.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.ABSTAIN)]
> +        self.assertEqual(
> +            BranchMergeProposalStatus.REJECTED,
> +            self.policy.checkReviews(reviews))
> +
> +    def test_conflicting_reviews(self):
> +        # If there is an approval and a disapproval, an update isn't
> +        # suggested.
> +        reviews = [
> +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> +        self.assertIs(None, self.policy.checkReviews(reviews))
> +

Great tests, thanks.

Cool! I bet a lot of users will like this feature, once they get to try it
out.

jml

review: Needs Fixing (code)

Revision history for this message

Tim Penhey (thumper) wrote on 2009-06-07:

Download full text (24.9 KiB)

On Fri, 05 Jun 2009 17:18:42 Jonathan Lange wrote:
> Review: Needs Fixing code
> Hi Tim,
>
> Thanks for looking into this feature. People have been bugging us about it
> forever, and it'll be great to give them what they want.
>
> I notice that there's no UI for this yet. That's OK -- it's a big enough
> branch and the features as implemented make sense as an atomic whole.
> However, we really should get a UI asap, since we don't want this code
> lying dormant.

Yes there is no web UI for this yet. However I made sure that the method to
set the review policy is exposed through the API. Beuno assures me that an
enum picker has or is about to be landed, so I suggest that we use this to set
the review policy. Since the method is exposed through the API already it
*should* be simple.

> I've got quite a few comments and questions, some of them trivial, some of
> them not. Overall, the code looks really good. I'm particularly pleased to
> see the thorough policy tests & the clean design.

Thanks.

> > === modified file 'database/schema/comments.sql'
> > --- database/schema/comments.sql 2009-05-14 09:57:46 +0000
> > +++ database/schema/comments.sql 2009-06-04 23:29:41 +0000
> > @@ -593,6 +593,7 @@
> > COMMENT ON COLUMN Product.reviewer_whiteboard IS 'A whiteboard for
> > Launchpad admins, registry experts and the project owners to capture the
> > state of current issues with the project.'; COMMENT ON COLUMN
> > Product.license_approved IS 'The Other/Open Source license has been
> > approved by an administrator.'; COMMENT ON COLUMN Product.remote_product
> > IS 'The ID of this product on its remote bug tracker.'; +COMMENT ON
> > COLUMN Product.review_approval_policy IS 'An enumerated value referring
> > to the CodeReviewApprovalPolicy to define the requirements for
> > automatically approving or rejecting proposals based on the code
> > reviews';
>
> Please change "to define" to "that defines".

Done

> > === modified file 'lib/lp/code/interfaces/codereviewvote.py'
> > --- lib/lp/code/interfaces/codereviewvote.py 2009-04-17 10:32:16 +0000
> > +++ lib/lp/code/interfaces/codereviewvote.py 2009-06-05 00:31:22 +0000
> > @@ -8,14 +8,14 @@
> > ]
> >
> > from zope.interface import Interface
> > -from zope.schema import Datetime, Int, TextLine
> > +from zope.schema import Choice, Datetime, Int, TextLine
> >
> > from canonical.launchpad import _
> > from canonical.launchpad.fields import PublicPersonChoice
> > from lp.code.interfaces.branchmergeproposal import (
> > IBranchMergeProposal)
> > from lp.code.interfaces.codereviewcomment import (
> > - ICodeReviewComment)
> > + CodeReviewVote, ICodeReviewComment)
> > from lazr.restful.fields import Reference
> > from lazr.restful.declarations import (
> > export_as_webservice_entry, exported)
> > @@ -66,3 +66,9 @@
> > "The code review comment that contains the most recent
> > vote." ),
> > required=True, schema=ICodeReviewComment))
> > +
> > + vote = exported(
> > + Choice(
> > + description=_('The last review by the reviewer.'),
> > + required=False, readonly=True,
> > + vocabulary=CodeReviewVote))
>
> Th...

Yes there is no web UI for this yet.  However I made sure that the method to 
set the review policy is exposed through the API.  Beuno assures me that an 
enum picker has or is about to be landed, so I suggest that we use this to set 
the review policy.  Since the method is exposed through the API already it 
*should* be simple.

Thanks.

> > === modified file 'database/schema/comments.sql'
> > --- database/schema/comments.sql	2009-05-14 09:57:46 +0000
> > +++ database/schema/comments.sql	2009-06-04 23:29:41 +0000
> > @@ -593,6 +593,7 @@
> >  COMMENT ON COLUMN Product.reviewer_whiteboard IS 'A whiteboard for
> > Launchpad admins, registry experts and the project owners to capture the
> > state of current issues with the project.'; COMMENT ON COLUMN
> > Product.license_approved IS 'The Other/Open Source license has been
> > approved by an administrator.'; COMMENT ON COLUMN Product.remote_product
> > IS 'The ID of this product on its remote bug tracker.'; +COMMENT ON
> > COLUMN Product.review_approval_policy IS 'An enumerated value referring
> > to the CodeReviewApprovalPolicy to define the requirements for
> > automatically approving or rejecting proposals based on the code
> > reviews';
>
> Please change "to define" to "that defines".

Done

> > === modified file 'lib/lp/code/interfaces/codereviewvote.py'
> > --- lib/lp/code/interfaces/codereviewvote.py	2009-04-17 10:32:16 +0000
> > +++ lib/lp/code/interfaces/codereviewvote.py	2009-06-05 00:31:22 +0000
> > @@ -8,14 +8,14 @@
> >      ]
> >
> >  from zope.interface import Interface
> > -from zope.schema import Datetime, Int, TextLine
> > +from zope.schema import Choice, Datetime, Int, TextLine
> >
> >  from canonical.launchpad import _
> >  from canonical.launchpad.fields import PublicPersonChoice
> >  from lp.code.interfaces.branchmergeproposal import (
> >      IBranchMergeProposal)
> >  from lp.code.interfaces.codereviewcomment import (
> > -    ICodeReviewComment)
> > +    CodeReviewVote, ICodeReviewComment)
> >  from lazr.restful.fields import Reference
> >  from lazr.restful.declarations import (
> >      export_as_webservice_entry, exported)
> > @@ -66,3 +66,9 @@
> >                  "The code review comment that contains the most recent
> > vote." ),
> >              required=True, schema=ICodeReviewComment))
> > +
> > +    vote = exported(
> > +        Choice(
> > +            description=_('The last review by the reviewer.'),
> > +            required=False, readonly=True,
> > +            vocabulary=CodeReviewVote))
>
> The "by the reviewer" thing confuses me. What class is this on?

This is on the CodeReviewVoteReference class.  This is what is returned by the 
BranchMergeProposal.votes method.  If vote is None then it is Pending.  I 
added this method to make the list comprehension in the policy simpler.

> > === added file 'lib/lp/code/interfaces/reviewapprovalpolicies.py'
> > --- lib/lp/code/interfaces/reviewapprovalpolicies.py	1970-01-01 00:00:00
> > +0000 +++ lib/lp/code/interfaces/reviewapprovalpolicies.py	2009-06-04
> > 04:37:54 +0000 @@ -0,0 +1,67 @@
> > +# Copyright 2009 Canonical Ltd.  All rights reserved.
> > +
> > +"""Implementation classes for code review approval policies."""
> > +
>
> These aren't implementation classes, they're interface classes.

Copy and paste error.

> > +__metaclass__ = type
> > +__all__ = []
> > +
>
> I think you need to put the enum & the policy interface in here.

I noticed this too just as I was submitting the review.

> > +
> > +from zope.interface import Interface
> > +
> > +from lazr.enum import DBEnumeratedType, DBItem
> > +
> > +
> > +class CodeReviewApprovalPolicy(DBEnumeratedType):
> > +    """Policies for automatically approving code reviews.
> > +
> > +    In all the situations mentioned below, reviewer refers to a member
> > of the +    review team for the target branch.  There has to be reviews
> > by people who +    are in a position to say yay or nay for the merge.
>
> The normal spelling is "yea".
>
> I think the sentence would be clearer if it went like:
>   To change the status of a proposal, there must be reviews by people who
> ...
>
> Hmm. I don't know how to finish it. What are we trying to say here?
 
    Only reviews by people in a position
    to approve the proposal are consulted to automatically change the proposal
    state.

> At this point, I'm a little surprised there's no public interface for
> getting an `ICodeReviewApprovalPolicy` from a `CodeReviewApprovalPolicy`
> enum. Perhaps it's not needed.

I did consider it, but the only place it is called from is from within the 
lp.code.model package.

> > === modified file 'lib/lp/code/model/branchmergeproposal.py'
> > --- lib/lp/code/model/branchmergeproposal.py	2009-05-17 02:23:49 +0000
> > +++ lib/lp/code/model/branchmergeproposal.py	2009-06-05 01:39:33 +0000
> > @@ -653,8 +653,30 @@
> >          if _notify_listeners:
> >              notify(NewCodeReviewCommentEvent(
> >                      code_review_message, original_email))
> > +        # Check for automatic approval or rejection.
> > +        self._autoApproveCheck(message.owner)
> >          return code_review_message
> >
> > +    def _autoApproveCheck(self, reviewer):
> > +        """Check the approval policy for this proposal."""
> > +        # Get all the reviews pending and done by members of the review
> > team. +        if self.queue_status !=
> > BranchMergeProposalStatus.NEEDS_REVIEW: +            return None
>
> I don't think the comment matches the if statement. The idea here is that
> we only want to change the state based on a review vote if the merge
> proposal is in "needs review", right?

Yes.

> > +        reviews = [
> > +            (reference.reviewer, reference.vote) for reference in
> > self.votes +            if
> > self.isPersonValidReviewer(reference.reviewer)]
>
> Ahh, here it is.
>
> > +        policy = self.target_branch.target.review_approval_policy
>
> We have style guides warning against (but not forbidding) use of multiple
> dots on the line.
>
> Perhaps review_approval_policy should be a property of BranchMergeProposal?

Do you really think this is needed?  I could add it but right now I don't see 
the point.

> > +        next_status = policy.checkReviews(reviews)
> > +        # Auto approval approves or rejects the tip.
> > +        tip_rev_id = self.target_branch.last_scanned_id
> > +        if next_status == BranchMergeProposalStatus.CODE_APPROVED:
> > +            self.approveBranch(reviewer, tip_rev_id)
> > +        elif next_status == BranchMergeProposalStatus.REJECTED:
> > +            self.rejectBranch(reviewer, tip_rev_id)
> > +        else:
> > +            # Nothing to do.
> > +            pass
> > +
>
> Do we want to do anything special if the next_status is not CODE_APPROVED,
> not REJECTED and not None? i.e. if checkReviews violated its contract?

I'll add an assert.

> > === modified file 'lib/lp/code/model/branchtarget.py'
> > --- lib/lp/code/model/branchtarget.py	2009-05-14 22:56:43 +0000
> > +++ lib/lp/code/model/branchtarget.py	2009-06-04 23:44:00 +0000
> > @@ -17,6 +17,8 @@
> >  from lp.code.interfaces.branchcollection import IAllBranches
> >  from lp.code.interfaces.branchtarget import (
> >      check_default_stacked_on, IBranchTarget)
> > +from lp.code.model.reviewapprovalpolicies import (
> > +    get_approval_policy, NoChangePolicy)
> >  from lp.soyuz.interfaces.publishing import PackagePublishingPocket
> >  from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
> >
> > @@ -93,6 +95,11 @@
> >          """See `IBranchTarget`."""
> >          return True
> >
> > +    @property
> > +    def review_approval_policy(self):
> > +        """See `IBranchTarget`."""
> > +        return NoChangePolicy()
> > +
>
> FWIW, if you made get_approval_policy a public function / method that you
> get get from interfaces.reviewapprovalpolicies (either by import or by
> utility), then you could completely avoid importing from the model module
> by doing:
>
>     return get_approval_policy(CodeReviewApprovalPolicy.NO_AUTO_CHANGE)
>
> Not only will this make the fascist a little happier, it preserves a fairly
> sane barrier of abstraction.

The fascist doesn't care about imports from the same (parent) module.
Also it would require registering a zope utility class in order to get it done 
right, and I didn't think it was worth the effort given that we can do it just 
fine without extra layers of abstraction.

> >      def areBranchesMergeable(self, other_target):
> >          """See `IBranchTarget`."""
> >          # Branches are mergable into a PackageTarget if the source
> > package @@ -162,6 +169,11 @@
> >          """See `IBranchTarget`."""
> >          return False
> >
> > +    @property
> > +    def review_approval_policy(self):
> > +        """See `IBranchTarget`."""
> > +        return NoChangePolicy()
> > +
> >      def areBranchesMergeable(self, other_target):
> >          """See `IBranchTarget`."""
> >          return False
> > @@ -223,6 +235,11 @@
> >          """See `IBranchTarget`."""
> >          return True
> >
> > +    @property
> > +    def review_approval_policy(self):
> > +        """See `IBranchTarget`."""
> > +        return get_approval_policy(self.product.review_approval_policy)
> > +
> >      def areBranchesMergeable(self, other_target):
> >          """See `IBranchTarget`."""
> >          # Branches are mergable into a PackageTarget if the source
> > package
>
> Nice.
>
> I'm a little disappointed to see source packages getting second-class
> treatment though. Why aren't we adding policy support for them now? What
> plans do we have for getting the policy stuff working for them?

We don't yet have package sets or other ways to define reasonable groupings for 
source package branches.  I'm open to ideas, but primarily my first focus was 
upstream branches, like bzr, launchpad, landscape, storm et al.

> > === added file 'lib/lp/code/model/reviewapprovalpolicies.py'
> > --- lib/lp/code/model/reviewapprovalpolicies.py	1970-01-01 00:00:00 +0000
> > +++ lib/lp/code/model/reviewapprovalpolicies.py	2009-06-04 04:37:54 +0000
> > @@ -0,0 +1,124 @@
> > +# Copyright 2009 Canonical Ltd.  All rights reserved.
> > +
> > +"""Implementation classes for code review approval policies."""
> > +
> > +__metaclass__ = type
> > +__all__ = [
> > +    'get_approval_policy',
> > +    'NoChangePolicy',
> > +    'OneReviewerPolicy',
> > +    'ReviewsCompletePolicy',
> > +    'TwoReviewersPolicy',
> > +    ]
> > +
> > +from zope.interface import implements
> > +
> > +from lp.code.interfaces.branchmergeproposal import
> > BranchMergeProposalStatus +from lp.code.interfaces.codereviewcomment
> > import CodeReviewVote +from lp.code.interfaces.reviewapprovalpolicies
> > import (
> > +    CodeReviewApprovalPolicy, ICodeReviewApprovalPolicy)
> > +
> > +
> > +class NoChangePolicy:
> > +    """A policy where no changes are made."""
> > +
>
> I think that will read better if "automatically" is added to the end of the
> sentence.
>
> > +    implements(ICodeReviewApprovalPolicy)
> > +
> > +    def checkReviews(self, reviews):
> > +        """No change, return None."""
> > +        return None
> > +
> > +
> > +class BasePolicy:
> > +    """A base policy class to sum review types."""
> > +
> > +    def countReviews(self, reviews):
> > +        """Return a dict of review counts."""
> > +        result = {
> > +            CodeReviewVote.APPROVE: 0,
> > +            CodeReviewVote.DISAPPROVE: 0,
> > +            CodeReviewVote.ABSTAIN: 0,
> > +            'pending': 0,
> > +            'other': 0,
> > +            }
> > +        for reviewer, review in reviews:
> > +            if review in result:
> > +                result[review] += 1
> > +            elif review is None:
> > +                result['pending'] += 1
> > +            else:
> > +                result['other'] += 1
> > +        return result
> > +
> > +
> > +class OneReviewerPolicy(BasePolicy):
> > +    """A policy where one review is enough."""
> > +
> > +    implements(ICodeReviewApprovalPolicy)
> > +
> > +    def checkReviews(self, reviews):
> > +        """See `ICodeReviewApprovalPolicy`."""
> > +        counts = self.countReviews(reviews)
> > +        approvals = counts[CodeReviewVote.APPROVE]
> > +        disapprovals = counts[CodeReviewVote.DISAPPROVE]
> > +        if approvals > 0 and disapprovals == 0:
> > +            return BranchMergeProposalStatus.CODE_APPROVED
> > +        elif disapprovals > 0 and approvals == 0:
> > +            return BranchMergeProposalStatus.REJECTED
> > +        else:
> > +            return None
> > +
> > +
> > +class TwoReviewersPolicy(BasePolicy):
> > +    """A policy where two non-conflicting reviews are needed."""
> > +
> > +    implements(ICodeReviewApprovalPolicy)
> > +
> > +    def checkReviews(self, reviews):
> > +        """See `ICodeReviewApprovalPolicy`."""
> > +        counts = self.countReviews(reviews)
> > +        approvals = counts[CodeReviewVote.APPROVE]
> > +        disapprovals = counts[CodeReviewVote.DISAPPROVE]
> > +        if approvals > 1 and disapprovals == 0:
> > +            return BranchMergeProposalStatus.CODE_APPROVED
> > +        elif disapprovals > 1 and approvals == 0:
> > +            return BranchMergeProposalStatus.REJECTED
> > +        else:
> > +            return None
> > +
> > +
> > +class ReviewsCompletePolicy(BasePolicy):
> > +    """A policy where all requested reviews need to be complete.
> > +
> > +    A complete review is approved, disapproved or abstained.
> > +    """
> > +
> > +    implements(ICodeReviewApprovalPolicy)
> > +
> > +    def checkReviews(self, reviews):
> > +        """See `ICodeReviewApprovalPolicy`."""
> > +        counts = self.countReviews(reviews)
> > +        # If there are pending or in-progress reviews, no change.
> > +        if counts['pending'] > 0 or counts['other'] > 0:
> > +            return None
> > +        approvals = counts[CodeReviewVote.APPROVE]
> > +        disapprovals = counts[CodeReviewVote.DISAPPROVE]
> > +        if approvals > 0 and disapprovals == 0:
> > +            return BranchMergeProposalStatus.CODE_APPROVED
> > +        elif disapprovals > 0 and approvals == 0:
> > +            return BranchMergeProposalStatus.REJECTED
> > +        else:
> > +            return None
> > +
>
> Cool. These all work out rather well.
>
> It would be nice if each of the classes mentioned the enum value that they
> are associated with.

Done.

> In fact...
>
> > +
> > +def get_approval_policy(policy):
> > +    """Return a policy class based on the enumerated value."""
> > +    if policy == CodeReviewApprovalPolicy.NO_AUTO_CHANGE:
> > +        return NoChangePolicy()
> > +    if policy == CodeReviewApprovalPolicy.ONE_REVIEWER:
> > +        return OneReviewerPolicy()
> > +    if policy == CodeReviewApprovalPolicy.TWO_REVIEWERS:
> > +        return TwoReviewersPolicy()
> > +    if policy == CodeReviewApprovalPolicy.ALL_REQUESTED_COMPLETE:
> > +        return ReviewsCompletePolicy()
> > +    raise AssertionError("Invalid policy value: %s" % policy)
>
> ... if instead of mentioning, they had the enum value as a variable, then
> this method could be implemented with a loop, rather than a bunch of ifs.
> e.g.
>
> all_policies = [
>     NoChangePolicy,
>     OneReviewerPolicy,
>     TwoReviewersPolicy,
>     ReviewsCompletePolicy,
>   ]
	> for policy_factory in all_policies:
>   if policy_factory.policy_value == policy:
>     return policy_factory()
>   raise AssertionError("Invalid policy value: %s" % policy)
>
> You could even discover 'all_policies', rather than manually populating it.

Added a simple metaclass to auto-register the policies.

> Just a thought.
>
> > === modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
> > --- lib/lp/code/model/tests/test_branchmergeproposals.py	2009-05-12
> > 08:38:55 +0000 +++
> > lib/lp/code/model/tests/test_branchmergeproposals.py	2009-06-05 01:18:29
> > +0000 @@ -37,6 +37,7 @@
> >      WrongBranchMergeProposal)
> >  from lp.code.interfaces.branchsubscription import (
> >      BranchSubscriptionNotificationLevel, CodeReviewNotificationLevel)
> > +from lp.code.interfaces.reviewapprovalpolicies import
> > CodeReviewApprovalPolicy from canonical.launchpad.interfaces.message
> > import IMessageJob
> >  from lp.registry.interfaces.person import IPersonSet
> >  from lp.registry.interfaces.product import IProductSet
> > @@ -1402,5 +1403,57 @@
> >              removeSecurityProxy(merge_proposal.preview_diff).diff_id)
> >
> >
> > +class TestAutomaticProposalAppoval(TestCaseWithFactory):
> > +    """Test that the approval policy is called if set."""
> > +
> > +    layer = DatabaseFunctionalLayer
> > +
> > +    def setUp(self):
> > +        TestCaseWithFactory.setUp(self)
> > +        self.product = self.factory.makeProduct()
> > +        login_person(self.product.owner)
> > +        self.product.review_approval_policy = (
> > +            CodeReviewApprovalPolicy.ONE_REVIEWER)
> > +        self.bmp = self.factory.makeBranchMergeProposal(
> > +            set_state=BranchMergeProposalStatus.NEEDS_REVIEW,
> > +            product=self.product)
> > +
> > +    def assertReviewState(self, reviewer, vote, expected):
> > +        """Have `reviewer` comment on the proposal and check the
> > status.""" +        login_person(reviewer)
> > +        self.bmp.createComment(
> > +            reviewer, subject=None, content="comment", vote=vote)
> > +        self.assertEqual(expected, self.bmp.queue_status)
> > +
>
> I'm not a huge fan of 'assert' methods exercising the system-under-test.
> I'd be happier if the test class had a vote() helper that did the
> createComment, and the tests themselves made assertions about the state.
>
> Also, it doesn't look like the 'reviewer' parameter is used much -- could
> we instead just use self.bmp.target_branch.owner instead?

Even though the reviewer isn't currently used, I'd like to keep it as when 
Aaron's branch about allowing any one to review again lands, we can test 
reviews by non-review team members.  Right now there is a check to make sure 
that the person can review, which is why I'm not currently testing reviews by 
non review team members.

Added an addReview method, and broke out the asserts.

> > +    def test_auto_approval(self):
> > +        # If the product of the target branch has a policy to auto
> > approve the +        # proposal with a single review, then an approve
> > review will cause the +        # state to move from NEEDS_REVIEW to
> > CODE_APPROVED.
> > +        self.assertReviewState(
> > +            reviewer=self.bmp.target_branch.owner,
> > +            vote=CodeReviewVote.APPROVE,
> > +            expected=BranchMergeProposalStatus.CODE_APPROVED)
> > +
> > +    def test_auto_rejection(self):
> > +        # If the product of the target branch has a policy to auto
> > reject the +        # proposal with a single review, then a disapprove
> > review will cause +        # the state to move from NEEDS_REVIEW to
> > REJECTED.
> > +        self.assertReviewState(
> > +            reviewer=self.bmp.target_branch.owner,
> > +            vote=CodeReviewVote.DISAPPROVE,
> > +            expected=BranchMergeProposalStatus.REJECTED)
> > +
>
> It's a shame we have to rely on the specific policy at use here, when what
> we're really interested in is the fact that a policy is consulted at all.
>
> Oh well, I think this is good enough.
>
> > +    def test_not_needs_review(self):
> > +        # Auto approval only happens if the proposal is in NEEDS_REVIEW
> > state. +        # So a normally approving vote when in a different state
> > change the +        # status of the proposal.
> > +        login_person(self.bmp.registrant)
> > +        self.bmp.setAsWorkInProgress()
> > +        self.assertReviewState(
> > +            reviewer=self.bmp.target_branch.owner,
> > +            vote=CodeReviewVote.APPROVE,
> > +            expected=BranchMergeProposalStatus.WORK_IN_PROGRESS)
> > +
>
> These are good tests, thanks.
>
> It looks like you are missing tests to check that only people on the review
> team count for policy changes.

See above comment.

> > === added file 'lib/lp/code/model/tests/test_reviewapprovalpolicies.py'
> > --- lib/lp/code/model/tests/test_reviewapprovalpolicies.py	1970-01-01
> > 00:00:00 +0000 +++
> > lib/lp/code/model/tests/test_reviewapprovalpolicies.py	2009-06-04
> > 04:37:54 +0000 @@ -0,0 +1,244 @@
> > +# Copyright 2009 Canonical Ltd.  All rights reserved.
> > +
> > +"""Tests for code review approval policies."""
> > +
> > +__metaclass__ = type
> > +
> > +import unittest
> > +
> > +from canonical.testing.layers import DatabaseFunctionalLayer
> > +
> > +from lp.code.interfaces.branchmergeproposal import
> > BranchMergeProposalStatus +from lp.code.interfaces.codereviewcomment
> > import CodeReviewVote +from lp.code.interfaces.reviewapprovalpolicies
> > import (
> > +    CodeReviewApprovalPolicy, ICodeReviewApprovalPolicy)
> > +from lp.code.model.reviewapprovalpolicies import (
> > +    get_approval_policy, NoChangePolicy, OneReviewerPolicy,
> > +    ReviewsCompletePolicy, TwoReviewersPolicy)
> > +from lp.testing import login_person, TestCase, TestCaseWithFactory
> > +
> > +
> > +class TestGetApprovalPolicy(TestCase):
> > +    """Test the policy factory generates the correct types."""
> > +
> > +    def test_no_change_policy(self):
> > +        # NO_AUTO_CHANGE creates a NoChangePolicy.
> > +        policy =
> > get_approval_policy(CodeReviewApprovalPolicy.NO_AUTO_CHANGE) +       
> > self.assertTrue(isinstance(policy, NoChangePolicy))
> > +        self.assertTrue(ICodeReviewApprovalPolicy.providedBy(policy))
> > +
>
> Please use assertIsInstance and assertProvides rather than assertTrue here:
> the error messages will be better.

Hmm.. didn't know about those.  Thanks.

> > +class TestNoChangePolicy(TestCaseWithFactory):
> > +    """Test that a no change policy doesn't change the status."""
> > +
> > +    layer = DatabaseFunctionalLayer
> > +
> > +    def setUp(self):
> > +        TestCaseWithFactory.setUp(self)
> > +        self.policy = NoChangePolicy()
> > +
> > +    def test_review_approval(self):
> > +        # An review approval doesn't change the status.
> > +        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
> > +        self.assertIs(None, self.policy.checkReviews(reviews))
> > +
> > +    def test_review_disapproval(self):
> > +        # An review disapproval doesn't change the status.
> > +        reviews = [(self.factory.makePerson(),
> > CodeReviewVote.DISAPPROVE)] +        self.assertIs(None,
> > self.policy.checkReviews(reviews))
> > +
> > +
> > +class TestOneReviewerPolicy(TestCaseWithFactory):
> > +    """Test that one review is enough to change the status."""
> > +
> > +    layer = DatabaseFunctionalLayer
> > +
> > +    def setUp(self):
> > +        TestCaseWithFactory.setUp(self)
> > +        self.policy = OneReviewerPolicy()
> > +
> > +    def test_review_approval(self):
> > +        # An review approval doesn't change the status.
> > +        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
> > +        self.assertEqual(
> > +            BranchMergeProposalStatus.CODE_APPROVED,
> > +            self.policy.checkReviews(reviews))
> > +
> > +    def test_review_disapproval(self):
> > +        # An review disapproval doesn't change the status.
> > +        reviews = [(self.factory.makePerson(),
> > CodeReviewVote.DISAPPROVE)] +        self.assertEqual(
> > +            BranchMergeProposalStatus.REJECTED,
> > +            self.policy.checkReviews(reviews))
> > +
> > +    def test_other_review(self):
> > +        # An review disapproval doesn't change the status.
> > +        reviews = [(self.factory.makePerson(), CodeReviewVote.ABSTAIN)]
> > +        self.assertIs(None, self.policy.checkReviews(reviews))
> > +
> > +    def test_conflicting_reviews(self):
> > +        # If there is an approval and a disapproval, an update isn't
> > +        # suggested.
> > +        reviews = [
> > +            (self.factory.makePerson(), CodeReviewVote.APPROVE),
> > +            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
> > +        self.assertIs(None, self.policy.checkReviews(reviews))
> > +
>
> This last one is worth testing, but afaict, it'll never actually happen
> live, since one of the votes will take place first, thus changing the
> status.
>
> Is this an issue?

It could be in the situation where it is auto approved, but someone 
disapproves and moves it back to needs review.  If this happens we don't want 
to be automatically changing the state every time someone comments.

I only thought up this case when writing the tests :)  It wasn't in the 
original plan.

> Great tests, thanks.

Thanks.

> Cool! I bet a lot of users will like this feature, once they get to try it
> out.

aa.diff

lp:~thumper/launchpad/auto-approve updated on 2009-06-07

8127. By Tim Penhey on 2009-06-07: Added simple metaclass for policy discovery.
8128. By Tim Penhey on 2009-06-07: Grammar improvement.
8129. By Tim Penhey on 2009-06-07: Small comment fixes and extra assert.
8130. By Tim Penhey on 2009-06-07: Test improvements.

Revision history for this message

Stuart Bishop (stub) wrote on 2009-06-08:

Download full text (7.5 KiB)

As discussed, the flag needs to move from the Product table to the Branch table to support future use cases.

Unmerged revisions

8131. By Tim Penhey on 2009-06-09: Move the enum from Product -> Branch and fix everything.
8130. By Tim Penhey on 2009-06-07: Test improvements.
8129. By Tim Penhey on 2009-06-07: Small comment fixes and extra assert.
8128. By Tim Penhey on 2009-06-07: Grammar improvement.
8127. By Tim Penhey on 2009-06-07: Added simple metaclass for policy discovery.
8126. By Tim Penhey on 2009-06-05: Missed the id.
8125. By Tim Penhey on 2009-06-05: Add implementation tests.
8124. By Tim Penhey on 2009-06-05: Implement the auto approve.
8123. By Tim Penhey on 2009-06-05: Add a helper vote attribute to the CodeReviewVoteReference.
8122. By Tim Penhey on 2009-06-04: Hook up the attribute with the target.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Celso Providelo

Colin Watson

Launchpad code reviewers from Canonical

Mantavya Gajjar (Open ERP)

Stuart Bishop

Tim Penhey

William Grant

to status/vote changes:

Tomi Karjalainen

Launchpad itself

Merge lp:~thumper/launchpad/auto-approve into lp:launchpad/db-devel

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers

 === modified file 'database/schema/comments.sql'
 --- database/schema/comments.sql	2009-06-04 23:29:41 +0000
 +++ database/schema/comments.sql	2009-06-07 21:55:53 +0000
@@ -593,7 +593,7 @@
  COMMENT ON COLUMN Product.reviewer_whiteboard IS 'A whiteboard for Launchpad admins, registry experts and the project owners to capture the state of current issues with the project.';
  COMMENT ON COLUMN Product.license_approved IS 'The Other/Open Source license has been approved by an administrator.';
  COMMENT ON COLUMN Product.remote_product IS 'The ID of this product on its remote bug tracker.';
--COMMENT ON COLUMN Product.review_approval_policy IS 'An enumerated value referring to the CodeReviewApprovalPolicy to define the requirements for automatically approving or rejecting proposals based on the code reviews';
++COMMENT ON COLUMN Product.review_approval_policy IS 'An enumerated value referring to the CodeReviewApprovalPolicy that defines the requirements for automatically approving or rejecting proposals based on the code reviews';
  -- ProductLicense
  COMMENT ON TABLE ProductLicense IS 'The licenses that cover the software for a product.';
 === modified file 'lib/lp/code/interfaces/reviewapprovalpolicies.py'
 --- lib/lp/code/interfaces/reviewapprovalpolicies.py	2009-06-04 04:37:54 +0000
 +++ lib/lp/code/interfaces/reviewapprovalpolicies.py	2009-06-07 22:05:57 +0000
@@ -1,9 +1,12 @@
  # Copyright 2009 Canonical Ltd.  All rights reserved.
--"""Implementation classes for code review approval policies."""
++"""Interfaces and enums for code review approval policies."""
  __metaclass__ = type
--__all__ = []
++__all__ = [
++    'CodeReviewApprovalPolicy',
++    'ICodeReviewApprovalPolicy',
++    ]
  from zope.interface import Interface
@@ -15,8 +18,9 @@
      """Policies for automatically approving code reviews.
      In all the situations mentioned below, reviewer refers to a member of the
--    review team for the target branch.  There has to be reviews by people who
--    are in a position to say yay or nay for the merge.
++    review team for the target branch.  Only reviews by people in a position
++    to approve the proposal are consulted to automatically change the proposal
++    state.
      """
      NO_AUTO_CHANGE = DBItem(1, """
 === modified file 'lib/lp/code/model/branchmergeproposal.py'
 --- lib/lp/code/model/branchmergeproposal.py	2009-06-05 01:39:33 +0000
 +++ lib/lp/code/model/branchmergeproposal.py	2009-06-07 22:08:53 +0000
@@ -659,9 +659,10 @@
      def _autoApproveCheck(self, reviewer):
          """Check the approval policy for this proposal."""
++        # Only look to change the status if the proposal needs review.
++        if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:
++            return None
          # Get all the reviews pending and done by members of the review team.
--        if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:
--            return None
          reviews = [
              (reference.reviewer, reference.vote) for reference in self.votes
              if self.isPersonValidReviewer(reference.reviewer)]
@@ -673,9 +674,11 @@
              self.approveBranch(reviewer, tip_rev_id)
          elif next_status == BranchMergeProposalStatus.REJECTED:
              self.rejectBranch(reviewer, tip_rev_id)
--        else:
++        elif next_status in None:
              # Nothing to do.
              pass
++        else:
++            raise AssertionError('Unexpected result from approval policy.')
      def updatePreviewDiff(self, diff_content, diff_stat,
                            source_revision_id, target_revision_id,
 === modified file 'lib/lp/code/model/reviewapprovalpolicies.py'
 --- lib/lp/code/model/reviewapprovalpolicies.py	2009-06-04 04:37:54 +0000
 +++ lib/lp/code/model/reviewapprovalpolicies.py	2009-06-07 22:24:18 +0000
@@ -19,11 +19,31 @@
      CodeReviewApprovalPolicy, ICodeReviewApprovalPolicy)
++APPROVAL_POLICIES = {}
++
++
++class PolicyMetaClass(type):
++    """A meta-class to automatically register the policies."""
++
++    def __new__(cls, classname, bases, classdict):
++        """Create the policy and add it to the global dict."""
++        instance = type.__new__(cls, classname, bases, classdict)
++        # If the class has a policy attribute, register with the global dict.
++        if 'policy' in classdict:
++            global APPROVAL_POLICIES
++            APPROVAL_POLICIES[instance.policy] = instance
++        return instance
++
++
  class NoChangePolicy:
--    """A policy where no changes are made."""
++    """A policy where no changes are made automatically."""
++
++    __metaclass__ = PolicyMetaClass
      implements(ICodeReviewApprovalPolicy)
++    policy = CodeReviewApprovalPolicy.NO_AUTO_CHANGE
++
      def checkReviews(self, reviews):
          """No change, return None."""
          return None
@@ -32,6 +52,8 @@
  class BasePolicy:
      """A base policy class to sum review types."""
++    __metaclass__ = PolicyMetaClass
++
      def countReviews(self, reviews):
          """Return a dict of review counts."""
          result = {
@@ -56,6 +78,8 @@
      implements(ICodeReviewApprovalPolicy)
++    policy = CodeReviewApprovalPolicy.ONE_REVIEWER
++
      def checkReviews(self, reviews):
          """See `ICodeReviewApprovalPolicy`."""
          counts = self.countReviews(reviews)
@@ -74,6 +98,8 @@
      implements(ICodeReviewApprovalPolicy)
++    policy = CodeReviewApprovalPolicy.TWO_REVIEWERS
++
      def checkReviews(self, reviews):
          """See `ICodeReviewApprovalPolicy`."""
          counts = self.countReviews(reviews)
@@ -95,6 +121,8 @@
      implements(ICodeReviewApprovalPolicy)
++    policy = CodeReviewApprovalPolicy.ALL_REQUESTED_COMPLETE
++
      def checkReviews(self, reviews):
          """See `ICodeReviewApprovalPolicy`."""
          counts = self.countReviews(reviews)
@@ -113,12 +141,8 @@
  def get_approval_policy(policy):
      """Return a policy class based on the enumerated value."""
--    if policy == CodeReviewApprovalPolicy.NO_AUTO_CHANGE:
--        return NoChangePolicy()
--    if policy == CodeReviewApprovalPolicy.ONE_REVIEWER:
--        return OneReviewerPolicy()
--    if policy == CodeReviewApprovalPolicy.TWO_REVIEWERS:
--        return TwoReviewersPolicy()
--    if policy == CodeReviewApprovalPolicy.ALL_REQUESTED_COMPLETE:
--        return ReviewsCompletePolicy()
--    raise AssertionError("Invalid policy value: %s" % policy)
++    try:
++        policy_class = APPROVAL_POLICIES[policy]
++        return policy_class()
++    except KeyError:
++        raise AssertionError("Invalid policy value: %s" % policy)
 === modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
 --- lib/lp/code/model/tests/test_branchmergeproposals.py	2009-06-05 01:18:29 +0000
 +++ lib/lp/code/model/tests/test_branchmergeproposals.py	2009-06-07 22:33:19 +0000
@@ -1418,30 +1418,30 @@
              set_state=BranchMergeProposalStatus.NEEDS_REVIEW,
              product=self.product)
--    def assertReviewState(self, reviewer, vote, expected):
--        """Have `reviewer` comment on the proposal and check the status."""
++    def addReview(self, reviewer, vote):
++        """Add a review by the reviewer with the specified vote."""
          login_person(reviewer)
          self.bmp.createComment(
              reviewer, subject=None, content="comment", vote=vote)
--        self.assertEqual(expected, self.bmp.queue_status)
++
++    def assertReviewState(self, reviewer, vote, expected):
++        """Have `reviewer` comment on the proposal and check the status."""
      def test_auto_approval(self):
          # If the product of the target branch has a policy to auto approve the
          # proposal with a single review, then an approve review will cause the
          # state to move from NEEDS_REVIEW to CODE_APPROVED.
--        self.assertReviewState(
--            reviewer=self.bmp.target_branch.owner,
--            vote=CodeReviewVote.APPROVE,
--            expected=BranchMergeProposalStatus.CODE_APPROVED)
++        self.addReview(self.bmp.target_branch.owner, CodeReviewVote.APPROVE)
++        self.assertEqual(
++            BranchMergeProposalStatus.CODE_APPROVED, self.bmp.queue_status)
      def test_auto_rejection(self):
          # If the product of the target branch has a policy to auto reject the
          # proposal with a single review, then a disapprove review will cause
          # the state to move from NEEDS_REVIEW to REJECTED.
--        self.assertReviewState(
--            reviewer=self.bmp.target_branch.owner,
--            vote=CodeReviewVote.DISAPPROVE,
--            expected=BranchMergeProposalStatus.REJECTED)
++        self.addReview(self.bmp.target_branch.owner, CodeReviewVote.DISAPPROVE)
++        self.assertEqual(
++            BranchMergeProposalStatus.REJECTED, self.bmp.queue_status)
      def test_not_needs_review(self):
          # Auto approval only happens if the proposal is in NEEDS_REVIEW state.
@@ -1449,10 +1449,9 @@
          # status of the proposal.
          login_person(self.bmp.registrant)
          self.bmp.setAsWorkInProgress()
--        self.assertReviewState(
--            reviewer=self.bmp.target_branch.owner,
--            vote=CodeReviewVote.APPROVE,
--            expected=BranchMergeProposalStatus.WORK_IN_PROGRESS)
++        self.addReview(self.bmp.target_branch.owner, CodeReviewVote.APPROVE)
++        self.assertEqual(
++            BranchMergeProposalStatus.WORK_IN_PROGRESS, self.bmp.queue_status)
  def test_suite():
 === modified file 'lib/lp/code/model/tests/test_reviewapprovalpolicies.py'
 --- lib/lp/code/model/tests/test_reviewapprovalpolicies.py	2009-06-04 04:37:54 +0000
 +++ lib/lp/code/model/tests/test_reviewapprovalpolicies.py	2009-06-07 22:37:57 +0000
@@ -15,7 +15,7 @@
  from lp.code.model.reviewapprovalpolicies import (
      get_approval_policy, NoChangePolicy, OneReviewerPolicy,
      ReviewsCompletePolicy, TwoReviewersPolicy)
--from lp.testing import login_person, TestCase, TestCaseWithFactory
++from lp.testing import TestCase, TestCaseWithFactory
  class TestGetApprovalPolicy(TestCase):
@@ -24,27 +24,27 @@
      def test_no_change_policy(self):
          # NO_AUTO_CHANGE creates a NoChangePolicy.
          policy = get_approval_policy(CodeReviewApprovalPolicy.NO_AUTO_CHANGE)
--        self.assertTrue(isinstance(policy, NoChangePolicy))
--        self.assertTrue(ICodeReviewApprovalPolicy.providedBy(policy))
++        self.assertIsInstance(policy, NoChangePolicy)
++        self.assertProvides(policy, ICodeReviewApprovalPolicy)
      def test_one_reviewer_policy(self):
          # ONE_REVIEWER creates a OneReviewerPolicy.
          policy = get_approval_policy(CodeReviewApprovalPolicy.ONE_REVIEWER)
--        self.assertTrue(isinstance(policy, OneReviewerPolicy))
--        self.assertTrue(ICodeReviewApprovalPolicy.providedBy(policy))
++        self.assertIsInstance(policy, OneReviewerPolicy)
++        self.assertProvides(policy, ICodeReviewApprovalPolicy)
      def test_two_reviewers_policy(self):
          # TWO_REVIEWERS creates a TwoReviewersPolicy.
          policy = get_approval_policy(CodeReviewApprovalPolicy.TWO_REVIEWERS)
--        self.assertTrue(isinstance(policy, TwoReviewersPolicy))
--        self.assertTrue(ICodeReviewApprovalPolicy.providedBy(policy))
++        self.assertIsInstance(policy, TwoReviewersPolicy)
++        self.assertProvides(policy, ICodeReviewApprovalPolicy)
      def test_all_reviews_completed_policy(self):
          # ALL_REQUESTED_COMPLETE creates a ReviewsCompletePolicy.
          policy = get_approval_policy(
              CodeReviewApprovalPolicy.ALL_REQUESTED_COMPLETE)
--        self.assertTrue(isinstance(policy, ReviewsCompletePolicy))
--        self.assertTrue(ICodeReviewApprovalPolicy.providedBy(policy))
++        self.assertIsInstance(policy, ReviewsCompletePolicy)
++        self.assertProvides(policy, ICodeReviewApprovalPolicy)
  class TestNoChangePolicy(TestCaseWithFactory):

1	=== modified file 'database/schema/comments.sql'
2	--- database/schema/comments.sql 2009-06-07 22:27:26 +0000
3	+++ database/schema/comments.sql 2009-06-09 01:25:39 +0000
4	@@ -45,6 +45,7 @@
5	COMMENT ON COLUMN Branch.stacked_on IS 'The Launchpad branch that this branch is stacked on (if any).';
6	COMMENT ON COLUMN Branch.distroseries IS 'The distribution series that the branch belongs to.';
7	COMMENT ON COLUMN Branch.sourcepackagename IS 'The source package this is a branch of.';
8	+COMMENT ON COLUMN Branch.review_approval_policy IS 'An enumerated value referring to the CodeReviewApprovalPolicy that defines the requirements for automatically approving or rejecting proposals based on the code reviews';
9
10	-- BranchJob
11
12	@@ -593,7 +594,6 @@
13	COMMENT ON COLUMN Product.reviewer_whiteboard IS 'A whiteboard for Launchpad admins, registry experts and the project owners to capture the state of current issues with the project.';
14	COMMENT ON COLUMN Product.license_approved IS 'The Other/Open Source license has been approved by an administrator.';
15	COMMENT ON COLUMN Product.remote_product IS 'The ID of this product on its remote bug tracker.';
16	-COMMENT ON COLUMN Product.review_approval_policy IS 'An enumerated value referring to the CodeReviewApprovalPolicy that defines the requirements for automatically approving or rejecting proposals based on the code reviews';
17
18	-- ProductLicense
19	COMMENT ON TABLE ProductLicense IS 'The licenses that cover the software for a product.';
20
21	=== modified file 'database/schema/patch-2109-99-0.sql'
22	--- database/schema/patch-2109-99-0.sql 2009-06-04 23:29:41 +0000
23	+++ database/schema/patch-2109-99-0.sql 2009-06-09 01:25:39 +0000
24	@@ -1,6 +1,6 @@
25	SET client_min_messages = ERROR;
26
27	-ALTER TABLE Product
28	+ALTER TABLE Branch
29	ADD COLUMN review_approval_policy INT DEFAULT 1 NOT NULL;
30
31	INSERT INTO LaunchpadDatabaseRevision VALUES (2109, 99, 0);
32
33	=== modified file 'lib/lp/code/configure.zcml'
34	--- lib/lp/code/configure.zcml 2009-05-20 21:39:25 +0000
35	+++ lib/lp/code/configure.zcml 2009-06-09 01:25:39 +0000
36	@@ -401,6 +401,7 @@
37	commitsForDays
38	needs_upgrading
39	isBranchMergeable
40	+ review_approval_policy
41	"/>
42	<require
43	permission="launchpad.Edit"
44	@@ -412,7 +413,7 @@
45	last_scanned last_scanned_id revision_count branch_type
46	reviewer branch_format repository_format
47	control_format stacked_on merge_queue
48	- merge_control_status"/>
49	+ merge_control_status review_approval_policy"/>
50	<require
51	permission="launchpad.AnyPerson"
52	set_attributes="whiteboard"/>
53
54	=== modified file 'lib/lp/code/interfaces/branch.py'
55	--- lib/lp/code/interfaces/branch.py 2009-05-20 21:39:25 +0000
56	+++ lib/lp/code/interfaces/branch.py 2009-06-09 01:25:39 +0000
57	@@ -87,6 +87,7 @@
58	from canonical.launchpad.validators import LaunchpadValidationError
59	from lp.code.interfaces.branchlookup import IBranchLookup
60	from lp.code.interfaces.branchtarget import IHasBranchTarget
61	+from lp.code.interfaces.reviewapprovalpolicies import CodeReviewApprovalPolicy
62	from canonical.launchpad.interfaces.launchpad import (
63	IHasOwner, ILaunchpadCelebrities)
64	from lp.registry.interfaces.person import IPerson
65	@@ -895,6 +896,12 @@
66	readonly=True,
67	value_type=Reference(IPerson)))
68
69	+ review_approval_policy = exported(
70	+ Choice(
71	+ title=_("The policy defining the automatical approval of code "
72	+ " reviews"),
73	+ vocabulary=CodeReviewApprovalPolicy))
74	+
75	date_created = exported(
76	Datetime(
77	title=_('Date Created'),
78
79	=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
80	--- lib/lp/code/interfaces/branchmergeproposal.py 2009-05-13 20:03:39 +0000
81	+++ lib/lp/code/interfaces/branchmergeproposal.py 2009-06-09 01:25:39 +0000
82	@@ -303,6 +303,10 @@
83	description=_('Any emails sent to this address will result'
84	'in comments being added.')))
85
86	+ review_approval_policy = Attribute(
87	+ "The policy object relating to the approval policy set for the "
88	+ "target branch.")
89	+
90	@operation_parameters(
91	id=Int(
92	title=_("A CodeReviewComment ID.")))
93
94	=== modified file 'lib/lp/code/interfaces/branchtarget.py'
95	--- lib/lp/code/interfaces/branchtarget.py 2009-06-04 23:23:15 +0000
96	+++ lib/lp/code/interfaces/branchtarget.py 2009-06-09 01:25:39 +0000
97	@@ -87,10 +87,6 @@
98	supports_merge_proposals = Attribute(
99	"Does this target support merge proposals at all?")
100
101	- review_approval_policy = Attribute(
102	- "The policy object relating to the approval policy set for the "
103	- "target.")
104	-
105	def areBranchesMergeable(other_target):
106	"""Are branches from other_target mergeable into this target."""
107
108
109	=== modified file 'lib/lp/code/model/branch.py'
110	--- lib/lp/code/model/branch.py 2009-05-20 21:39:25 +0000
111	+++ lib/lp/code/model/branch.py 2009-06-09 01:25:39 +0000
112	@@ -41,6 +41,7 @@
113
114	from lp.code.interfaces.branch import (BranchFormat, RepositoryFormat,
115	BRANCH_FORMAT_UPGRADE_PATH, REPOSITORY_FORMAT_UPGRADE_PATH)
116	+from lp.code.interfaces.reviewapprovalpolicies import CodeReviewApprovalPolicy
117	from lp.code.mail.branch import send_branch_modified_notifications
118	from lp.code.model.branchmergeproposal import (
119	BranchMergeProposal)
120	@@ -196,6 +197,11 @@
121	joinColumn='branch',
122	orderBy='id')
123
124	+ review_approval_policy = EnumCol(
125	+ dbName='review_approval_policy', notNull=True,
126	+ schema=CodeReviewApprovalPolicy,
127	+ default=CodeReviewApprovalPolicy.NO_AUTO_CHANGE)
128	+
129	date_created = UtcDateTimeCol(notNull=True, default=DEFAULT)
130	date_last_modified = UtcDateTimeCol(notNull=True, default=DEFAULT)
131
132
133	=== modified file 'lib/lp/code/model/branchmergeproposal.py'
134	--- lib/lp/code/model/branchmergeproposal.py 2009-06-07 22:27:57 +0000
135	+++ lib/lp/code/model/branchmergeproposal.py 2009-06-09 01:25:39 +0000
136	@@ -42,6 +42,7 @@
137	from lp.code.model.codereviewcomment import CodeReviewComment
138	from lp.code.model.codereviewvote import (
139	CodeReviewVoteReference)
140	+from lp.code.model.reviewapprovalpolicies import get_approval_policy
141	from canonical.launchpad.database.diff import Diff, PreviewDiff, StaticDiff
142	from lp.services.job.model.job import Job
143	from canonical.launchpad.database.message import (
144	@@ -657,6 +658,11 @@
145	self._autoApproveCheck(message.owner)
146	return code_review_message
147
148	+ @property
149	+ def review_approval_policy(self):
150	+ """See `IBranchMergeProposal`."""
151	+ return get_approval_policy(self.target_branch.review_approval_policy)
152	+
153	def _autoApproveCheck(self, reviewer):
154	"""Check the approval policy for this proposal."""
155	# Only look to change the status if the proposal needs review.
156	@@ -666,7 +672,7 @@
157	reviews = [
158	(reference.reviewer, reference.vote) for reference in self.votes
159	if self.isPersonValidReviewer(reference.reviewer)]
160	- policy = self.target_branch.target.review_approval_policy
161	+ policy = self.review_approval_policy
162	next_status = policy.checkReviews(reviews)
163	# Auto approval approves or rejects the tip.
164	tip_rev_id = self.target_branch.last_scanned_id
165
166	=== modified file 'lib/lp/code/model/branchtarget.py'
167	--- lib/lp/code/model/branchtarget.py 2009-06-04 23:44:00 +0000
168	+++ lib/lp/code/model/branchtarget.py 2009-06-09 01:25:39 +0000
169	@@ -17,8 +17,6 @@
170	from lp.code.interfaces.branchcollection import IAllBranches
171	from lp.code.interfaces.branchtarget import (
172	check_default_stacked_on, IBranchTarget)
173	-from lp.code.model.reviewapprovalpolicies import (
174	- get_approval_policy, NoChangePolicy)
175	from lp.soyuz.interfaces.publishing import PackagePublishingPocket
176	from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
177
178	@@ -95,11 +93,6 @@
179	"""See `IBranchTarget`."""
180	return True
181
182	- @property
183	- def review_approval_policy(self):
184	- """See `IBranchTarget`."""
185	- return NoChangePolicy()
186	-
187	def areBranchesMergeable(self, other_target):
188	"""See `IBranchTarget`."""
189	# Branches are mergable into a PackageTarget if the source package
190	@@ -169,11 +162,6 @@
191	"""See `IBranchTarget`."""
192	return False
193
194	- @property
195	- def review_approval_policy(self):
196	- """See `IBranchTarget`."""
197	- return NoChangePolicy()
198	-
199	def areBranchesMergeable(self, other_target):
200	"""See `IBranchTarget`."""
201	return False
202	@@ -235,11 +223,6 @@
203	"""See `IBranchTarget`."""
204	return True
205
206	- @property
207	- def review_approval_policy(self):
208	- """See `IBranchTarget`."""
209	- return get_approval_policy(self.product.review_approval_policy)
210	-
211	def areBranchesMergeable(self, other_target):
212	"""See `IBranchTarget`."""
213	# Branches are mergable into a PackageTarget if the source package
214
215	=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
216	--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-06-07 22:38:26 +0000
217	+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-06-09 01:25:39 +0000
218	@@ -1410,13 +1410,11 @@
219
220	def setUp(self):
221	TestCaseWithFactory.setUp(self)
222	- self.product = self.factory.makeProduct()
223	- login_person(self.product.owner)
224	- self.product.review_approval_policy = (
225	- CodeReviewApprovalPolicy.ONE_REVIEWER)
226	self.bmp = self.factory.makeBranchMergeProposal(
227	- set_state=BranchMergeProposalStatus.NEEDS_REVIEW,
228	- product=self.product)
229	+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
230	+ branch = self.bmp.target_branch
231	+ login_person(branch.owner)
232	+ branch.review_approval_policy = CodeReviewApprovalPolicy.ONE_REVIEWER
233
234	def addReview(self, reviewer, vote):
235	"""Add a review by the reviewer with the specified vote."""
236	@@ -1424,9 +1422,6 @@
237	self.bmp.createComment(
238	reviewer, subject=None, content="comment", vote=vote)
239
240	- def assertReviewState(self, reviewer, vote, expected):
241	- """Have `reviewer` comment on the proposal and check the status."""
242	-
243	def test_auto_approval(self):
244	# If the product of the target branch has a policy to auto approve the
245	# proposal with a single review, then an approve review will cause the
246
247	=== modified file 'lib/lp/registry/configure.zcml'
248	--- lib/lp/registry/configure.zcml 2009-06-04 23:41:44 +0000
249	+++ lib/lp/registry/configure.zcml 2009-06-09 01:25:39 +0000
250	@@ -1069,7 +1069,7 @@
251	official_codehosting official_malone owner
252	programminglang project
253	redeemSubscriptionVoucher releaseroot
254	- remote_product review_approval_policy screenshotsurl
255	+ remote_product screenshotsurl
256	security_contact sourceforgeproject
257	summary title translationgroup
258	translationpermission wikiurl"/>
259
260	=== modified file 'lib/lp/registry/interfaces/product.py'
261	--- lib/lp/registry/interfaces/product.py 2009-06-04 23:41:44 +0000
262	+++ lib/lp/registry/interfaces/product.py 2009-06-09 01:25:39 +0000
263	@@ -42,7 +42,6 @@
264	IBranchMergeProposal, BranchMergeProposalStatus)
265	from lp.code.interfaces.branchvisibilitypolicy import (
266	IHasBranchVisibilityPolicy)
267	-from lp.code.interfaces.reviewapprovalpolicies import CodeReviewApprovalPolicy
268	from canonical.launchpad.interfaces.bugtarget import (
269	IBugTarget, IOfficialBugTagTargetPublic, IOfficialBugTagTargetRestricted)
270	from lp.registry.interfaces.karma import IKarmaContext
271	@@ -528,12 +527,6 @@
272	readonly=True,
273	value_type=Reference(schema=IBranch)))
274
275	- review_approval_policy = exported(
276	- Choice(
277	- title=_("The policy defining the automatical approval of code "
278	- " reviews"),
279	- vocabulary=CodeReviewApprovalPolicy))
280	-
281	bounties = Attribute(_("The bounties that are related to this product."))
282
283	translatable_packages = Attribute(
284
285	=== modified file 'lib/lp/registry/model/product.py'
286	--- lib/lp/registry/model/product.py 2009-06-04 23:41:44 +0000
287	+++ lib/lp/registry/model/product.py 2009-06-09 01:25:39 +0000
288	@@ -32,7 +32,6 @@
289	from canonical.database.datetimecol import UtcDateTimeCol
290	from canonical.database.enumcol import EnumCol
291	from canonical.database.sqlbase import quote, SQLBase, sqlvalues
292	-from lp.code.interfaces.reviewapprovalpolicies import CodeReviewApprovalPolicy
293	from lp.code.model.branch import BranchSet
294	from lp.code.model.branchvisibilitypolicy import (
295	BranchVisibilityPolicyMixin)
296	@@ -519,11 +518,6 @@
297	branches = SQLMultipleJoin('Branch', joinColumn='product',
298	orderBy='id')
299
300	- review_approval_policy = EnumCol(
301	- dbName='review_approval_policy', notNull=True,
302	- schema=CodeReviewApprovalPolicy,
303	- default=CodeReviewApprovalPolicy.NO_AUTO_CHANGE)
304	-
305	serieses = SQLMultipleJoin('ProductSeries', joinColumn='product',
306	orderBy='name')
307
308
309	=== modified file 'scripts/rosetta/message-sharing-merge.py' (properties changed: -x to +x)

 === modified file 'database/schema/comments.sql'
 --- database/schema/comments.sql	2009-06-08 07:26:58 +0000
 +++ database/schema/comments.sql	2009-06-09 10:45:18 +0000
@@ -45,6 +45,7 @@
  COMMENT ON COLUMN Branch.stacked_on IS 'The Launchpad branch that this branch is stacked on (if any).';
  COMMENT ON COLUMN Branch.distroseries IS 'The distribution series that the branch belongs to.';
  COMMENT ON COLUMN Branch.sourcepackagename IS 'The source package this is a branch of.';
++COMMENT ON COLUMN Branch.review_approval_policy IS 'An enumerated value referring to the CodeReviewApprovalPolicy that defines the requirements for automatically approving or rejecting proposals based on the code reviews';
  -- BranchJob
 === added file 'database/schema/patch-2109-99-0.sql'
 --- database/schema/patch-2109-99-0.sql	1970-01-01 00:00:00 +0000
 +++ database/schema/patch-2109-99-0.sql	2009-06-09 10:45:18 +0000
@@ -0,0 +1,7 @@
++SET client_min_messages = ERROR;
++
++ALTER TABLE Branch
++  ADD COLUMN review_approval_policy INT DEFAULT 1 NOT NULL;
++
++INSERT INTO LaunchpadDatabaseRevision VALUES (2109, 99, 0);
++
 === modified file 'lib/lp/code/configure.zcml'
 --- lib/lp/code/configure.zcml	2009-06-07 01:53:39 +0000
 +++ lib/lp/code/configure.zcml	2009-06-09 10:45:18 +0000
@@ -393,6 +393,7 @@
                      commitsForDays
                      needs_upgrading
                      isBranchMergeable
++                    review_approval_policy
                      "/>
      <require
          permission="launchpad.Edit"
@@ -404,7 +405,7 @@
                          last_scanned last_scanned_id revision_count branch_type
                          reviewer branch_format repository_format
                          control_format stacked_on merge_queue
--                        merge_control_status"/>
++                        merge_control_status review_approval_policy"/>
      <require
          permission="launchpad.AnyPerson"
          set_attributes="whiteboard"/>
 === modified file 'lib/lp/code/interfaces/branch.py'
 --- lib/lp/code/interfaces/branch.py	2009-06-07 01:53:39 +0000
 +++ lib/lp/code/interfaces/branch.py	2009-06-09 10:45:18 +0000
@@ -92,6 +92,7 @@
  from canonical.launchpad.validators import LaunchpadValidationError
  from lp.code.interfaces.branchlookup import IBranchLookup
  from lp.code.interfaces.branchtarget import IHasBranchTarget
++from lp.code.interfaces.reviewapprovalpolicies import CodeReviewApprovalPolicy
  from canonical.launchpad.interfaces.launchpad import (
      IHasOwner, ILaunchpadCelebrities)
  from lp.registry.interfaces.person import IPerson
@@ -918,6 +919,12 @@
              readonly=True,
              value_type=Reference(IPerson)))
++    review_approval_policy = exported(
++        Choice(
++            title=_("The policy defining the automatical approval of code "
++                    " reviews"),
++            vocabulary=CodeReviewApprovalPolicy))
++
      date_created = exported(
          Datetime(
              title=_('Date Created'),
 === modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
 --- lib/lp/code/interfaces/branchmergeproposal.py	2009-06-02 08:20:49 +0000
 +++ lib/lp/code/interfaces/branchmergeproposal.py	2009-06-09 10:45:18 +0000
@@ -303,6 +303,10 @@
              description=_('Any emails sent to this address will result'
                            'in comments being added.')))
++    review_approval_policy = Attribute(
++        "The policy object relating to the approval policy set for the "
++        "target branch.")
++
      @operation_parameters(
          id=Int(
              title=_("A CodeReviewComment ID.")))
 === modified file 'lib/lp/code/interfaces/codereviewvote.py'
 --- lib/lp/code/interfaces/codereviewvote.py	2009-04-17 10:32:16 +0000
 +++ lib/lp/code/interfaces/codereviewvote.py	2009-06-09 10:45:18 +0000
@@ -8,14 +8,14 @@
+     ]
  from zope.interface import Interface
--from zope.schema import Datetime, Int, TextLine
++from zope.schema import Choice, Datetime, Int, TextLine
  from canonical.launchpad import _
  from canonical.launchpad.fields import PublicPersonChoice
  from lp.code.interfaces.branchmergeproposal import (
      IBranchMergeProposal)
  from lp.code.interfaces.codereviewcomment import (
--    ICodeReviewComment)
++    CodeReviewVote, ICodeReviewComment)
  from lazr.restful.fields import Reference
  from lazr.restful.declarations import (
      export_as_webservice_entry, exported)
@@ -66,3 +66,9 @@
                  "The code review comment that contains the most recent vote."
                  ),
              required=True, schema=ICodeReviewComment))
++
++    vote = exported(
++        Choice(
++            description=_('The last review by the reviewer.'),
++            required=False, readonly=True,
++            vocabulary=CodeReviewVote))
 === added file 'lib/lp/code/interfaces/reviewapprovalpolicies.py'
 --- lib/lp/code/interfaces/reviewapprovalpolicies.py	1970-01-01 00:00:00 +0000
 +++ lib/lp/code/interfaces/reviewapprovalpolicies.py	2009-06-09 10:45:18 +0000
@@ -0,0 +1,71 @@
++# Copyright 2009 Canonical Ltd.  All rights reserved.
++
++"""Interfaces and enums for code review approval policies."""
++
++__metaclass__ = type
++__all__ = [
++    'CodeReviewApprovalPolicy',
++    'ICodeReviewApprovalPolicy',
++    ]
++
++
++from zope.interface import Interface
++
++from lazr.enum import DBEnumeratedType, DBItem
++
++
++class CodeReviewApprovalPolicy(DBEnumeratedType):
++    """Policies for automatically approving code reviews.
++
++    In all the situations mentioned below, reviewer refers to a member of the
++    review team for the target branch.  Only reviews by people in a position
++    to approve the proposal are consulted to automatically change the proposal
++    state.
++    """
++
++    NO_AUTO_CHANGE = DBItem(1, """
++        No automatic approval
++
++        All approvals or rejections are done manually.
++        """)
++
++    ONE_REVIEWER = DBItem(2, """
++        One review is enough
++
++        The approval or rejection of one reviewer is enough to accept or
++        reject the proposal as a whole.  If the reviewer approves the review,
++        the proposal will be set to the approved state.  If the reviewer
++        disapproves in the review, the proposal is rejected.  Any other review
++        does not change the state of the proposal.
++        """)
++
++    TWO_REVIEWERS = DBItem(3, """
++        Two reviews needed
++
++        The proposal needs to be approved by two reviewers in order for the
++        proposal to be approved.  If there are two approvals the proposal is
++        approved.  If there are two rejections then the proposal is rejected.
++        If the reviewers disagree, then the proposal status is not updated.
++        """)
++
++    ALL_REQUESTED_COMPLETE = DBItem(4, """
++        All requested reviews complete
++
++        All requested reviews must be complete and agree.  If all reviewers
++        agree the status is updated.  Reviewers who abstain are ignored.
++        """)
++
++
++class ICodeReviewApprovalPolicy(Interface):
++    """A code review approval policy."""
++
++    def checkReviews(reviews):
++        """Check the status of the reviews to see if it should be approved.
++
++        :param reviews: An iterable of (person, review), where the
++            person is considered a valid reviewer of the proposal, and the
++            review is a CodeReviewVote value or None if pending.
++        :return: BranchMergeProposalStatus.CODE_APPROVED if the proposal state
++            should be approved, BranchMergeProposalStatus.REJECTED if the
++            proposal should be rejected, or None if no change.
++        """
 === modified file 'lib/lp/code/model/branch.py'
 --- lib/lp/code/model/branch.py	2009-06-04 18:30:55 +0000
 +++ lib/lp/code/model/branch.py	2009-06-09 10:45:18 +0000
@@ -39,6 +39,12 @@
  from canonical.launchpad.webapp.interfaces import (
      IStoreSelector, MAIN_STORE, SLAVE_FLAVOR)
++<<<<<<< TREE
++=======
++from lp.code.interfaces.branch import (BranchFormat, RepositoryFormat,
++    BRANCH_FORMAT_UPGRADE_PATH, REPOSITORY_FORMAT_UPGRADE_PATH)
++from lp.code.interfaces.reviewapprovalpolicies import CodeReviewApprovalPolicy
++>>>>>>> MERGE-SOURCE
  from lp.code.mail.branch import send_branch_modified_notifications
  from lp.code.model.branchmergeproposal import (
       BranchMergeProposal, BranchMergeProposalGetter)
@@ -195,6 +201,11 @@
          joinColumn='branch',
          orderBy='id')
++    review_approval_policy = EnumCol(
++        dbName='review_approval_policy', notNull=True,
++        schema=CodeReviewApprovalPolicy,
++        default=CodeReviewApprovalPolicy.NO_AUTO_CHANGE)
++
      date_created = UtcDateTimeCol(notNull=True, default=DEFAULT)
      date_last_modified = UtcDateTimeCol(notNull=True, default=DEFAULT)
 === modified file 'lib/lp/code/model/branchmergeproposal.py'
 --- lib/lp/code/model/branchmergeproposal.py	2009-06-05 15:04:29 +0000
 +++ lib/lp/code/model/branchmergeproposal.py	2009-06-09 10:45:18 +0000
@@ -42,6 +42,7 @@
  from lp.code.model.codereviewcomment import CodeReviewComment
  from lp.code.model.codereviewvote import (
      CodeReviewVoteReference)
++from lp.code.model.reviewapprovalpolicies import get_approval_policy
  from canonical.launchpad.database.diff import Diff, PreviewDiff, StaticDiff
  from lp.services.job.model.job import Job
  from canonical.launchpad.database.message import (
@@ -636,8 +637,38 @@
          if _notify_listeners:
              notify(NewCodeReviewCommentEvent(
                      code_review_message, original_email))
++        # Check for automatic approval or rejection.
++        self._autoApproveCheck(message.owner)
          return code_review_message
++    @property
++    def review_approval_policy(self):
++        """See `IBranchMergeProposal`."""
++        return get_approval_policy(self.target_branch.review_approval_policy)
++
++    def _autoApproveCheck(self, reviewer):
++        """Check the approval policy for this proposal."""
++        # Only look to change the status if the proposal needs review.
++        if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:
++            return None
++        # Get all the reviews pending and done by members of the review team.
++        reviews = [
++            (reference.reviewer, reference.vote) for reference in self.votes
++            if self.isPersonValidReviewer(reference.reviewer)]
++        policy = self.review_approval_policy
++        next_status = policy.checkReviews(reviews)
++        # Auto approval approves or rejects the tip.
++        tip_rev_id = self.target_branch.last_scanned_id
++        if next_status == BranchMergeProposalStatus.CODE_APPROVED:
++            self.approveBranch(reviewer, tip_rev_id)
++        elif next_status == BranchMergeProposalStatus.REJECTED:
++            self.rejectBranch(reviewer, tip_rev_id)
++        elif next_status in None:
++            # Nothing to do.
++            pass
++        else:
++            raise AssertionError('Unexpected result from approval policy.')
++
      def updatePreviewDiff(self, diff_content, diff_stat,
                            source_revision_id, target_revision_id,
                            dependent_revision_id=None, conflicts=None):
 === modified file 'lib/lp/code/model/codereviewvote.py'
 --- lib/lp/code/model/codereviewvote.py	2009-04-17 10:32:16 +0000
 +++ lib/lp/code/model/codereviewvote.py	2009-06-09 10:45:18 +0000
@@ -15,6 +15,8 @@
  from canonical.database.datetimecol import UtcDateTimeCol
  from canonical.database.sqlbase import SQLBase
  from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
++
++
  class CodeReviewVoteReference(SQLBase):
      """See `ICodeReviewVote`"""
@@ -33,3 +35,11 @@
      review_type = StringCol(default=None)
      comment = ForeignKey(
          dbName='vote_message', foreignKey='CodeReviewComment', default=None)
++
++    @property
++    def vote(self):
++        """Return the vote from the comment if it is there."""
++        if self.comment is not None:
++            return self.comment.vote
++        else:
++            return None
 === added file 'lib/lp/code/model/reviewapprovalpolicies.py'
 --- lib/lp/code/model/reviewapprovalpolicies.py	1970-01-01 00:00:00 +0000
 +++ lib/lp/code/model/reviewapprovalpolicies.py	2009-06-09 10:45:18 +0000
@@ -0,0 +1,148 @@
++# Copyright 2009 Canonical Ltd.  All rights reserved.
++
++"""Implementation classes for code review approval policies."""
++
++__metaclass__ = type
++__all__ = [
++    'get_approval_policy',
++    'NoChangePolicy',
++    'OneReviewerPolicy',
++    'ReviewsCompletePolicy',
++    'TwoReviewersPolicy',
++    ]
++
++from zope.interface import implements
++
++from lp.code.interfaces.branchmergeproposal import BranchMergeProposalStatus
++from lp.code.interfaces.codereviewcomment import CodeReviewVote
++from lp.code.interfaces.reviewapprovalpolicies import (
++    CodeReviewApprovalPolicy, ICodeReviewApprovalPolicy)
++
++
++APPROVAL_POLICIES = {}
++
++
++class PolicyMetaClass(type):
++    """A meta-class to automatically register the policies."""
++
++    def __new__(cls, classname, bases, classdict):
++        """Create the policy and add it to the global dict."""
++        instance = type.__new__(cls, classname, bases, classdict)
++        # If the class has a policy attribute, register with the global dict.
++        if 'policy' in classdict:
++            global APPROVAL_POLICIES
++            APPROVAL_POLICIES[instance.policy] = instance
++        return instance
++
++
++class NoChangePolicy:
++    """A policy where no changes are made automatically."""
++
++    __metaclass__ = PolicyMetaClass
++
++    implements(ICodeReviewApprovalPolicy)
++
++    policy = CodeReviewApprovalPolicy.NO_AUTO_CHANGE
++
++    def checkReviews(self, reviews):
++        """No change, return None."""
++        return None
++
++
++class BasePolicy:
++    """A base policy class to sum review types."""
++
++    __metaclass__ = PolicyMetaClass
++
++    def countReviews(self, reviews):
++        """Return a dict of review counts."""
++        result = {
++            CodeReviewVote.APPROVE: 0,
++            CodeReviewVote.DISAPPROVE: 0,
++            CodeReviewVote.ABSTAIN: 0,
++            'pending': 0,
++            'other': 0,
++            }
++        for reviewer, review in reviews:
++            if review in result:
++                result[review] += 1
++            elif review is None:
++                result['pending'] += 1
++            else:
++                result['other'] += 1
++        return result
++
++
++class OneReviewerPolicy(BasePolicy):
++    """A policy where one review is enough."""
++
++    implements(ICodeReviewApprovalPolicy)
++
++    policy = CodeReviewApprovalPolicy.ONE_REVIEWER
++
++    def checkReviews(self, reviews):
++        """See `ICodeReviewApprovalPolicy`."""
++        counts = self.countReviews(reviews)
++        approvals = counts[CodeReviewVote.APPROVE]
++        disapprovals = counts[CodeReviewVote.DISAPPROVE]
++        if approvals > 0 and disapprovals == 0:
++            return BranchMergeProposalStatus.CODE_APPROVED
++        elif disapprovals > 0 and approvals == 0:
++            return BranchMergeProposalStatus.REJECTED
++        else:
++            return None
++
++
++class TwoReviewersPolicy(BasePolicy):
++    """A policy where two non-conflicting reviews are needed."""
++
++    implements(ICodeReviewApprovalPolicy)
++
++    policy = CodeReviewApprovalPolicy.TWO_REVIEWERS
++
++    def checkReviews(self, reviews):
++        """See `ICodeReviewApprovalPolicy`."""
++        counts = self.countReviews(reviews)
++        approvals = counts[CodeReviewVote.APPROVE]
++        disapprovals = counts[CodeReviewVote.DISAPPROVE]
++        if approvals > 1 and disapprovals == 0:
++            return BranchMergeProposalStatus.CODE_APPROVED
++        elif disapprovals > 1 and approvals == 0:
++            return BranchMergeProposalStatus.REJECTED
++        else:
++            return None
++
++
++class ReviewsCompletePolicy(BasePolicy):
++    """A policy where all requested reviews need to be complete.
++
++    A complete review is approved, disapproved or abstained.
++    """
++
++    implements(ICodeReviewApprovalPolicy)
++
++    policy = CodeReviewApprovalPolicy.ALL_REQUESTED_COMPLETE
++
++    def checkReviews(self, reviews):
++        """See `ICodeReviewApprovalPolicy`."""
++        counts = self.countReviews(reviews)
++        # If there are pending or in-progress reviews, no change.
++        if counts['pending'] > 0 or counts['other'] > 0:
++            return None
++        approvals = counts[CodeReviewVote.APPROVE]
++        disapprovals = counts[CodeReviewVote.DISAPPROVE]
++        if approvals > 0 and disapprovals == 0:
++            return BranchMergeProposalStatus.CODE_APPROVED
++        elif disapprovals > 0 and approvals == 0:
++            return BranchMergeProposalStatus.REJECTED
++        else:
++            return None
++
++
++def get_approval_policy(policy):
++    """Return a policy class based on the enumerated value."""
++    try:
++        policy_class = APPROVAL_POLICIES[policy]
++        return policy_class()
++    except KeyError:
++        raise AssertionError("Invalid policy value: %s" % policy)
 === modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
 --- lib/lp/code/model/tests/test_branchmergeproposals.py	2009-06-04 20:50:32 +0000
 +++ lib/lp/code/model/tests/test_branchmergeproposals.py	2009-06-09 10:45:18 +0000
@@ -39,6 +39,7 @@
      IMergeProposalCreatedJob, WrongBranchMergeProposal)
  from lp.code.interfaces.branchsubscription import (
      BranchSubscriptionNotificationLevel, CodeReviewNotificationLevel)
++from lp.code.interfaces.reviewapprovalpolicies import CodeReviewApprovalPolicy
  from canonical.launchpad.interfaces.message import IMessageJob
  from lp.registry.interfaces.person import IPersonSet
  from lp.registry.interfaces.product import IProductSet
@@ -1422,5 +1423,51 @@
              removeSecurityProxy(merge_proposal.preview_diff).diff_id)
++class TestAutomaticProposalAppoval(TestCaseWithFactory):
++    """Test that the approval policy is called if set."""
++
++    layer = DatabaseFunctionalLayer
++
++    def setUp(self):
++        TestCaseWithFactory.setUp(self)
++        self.bmp = self.factory.makeBranchMergeProposal(
++            set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
++        branch = self.bmp.target_branch
++        login_person(branch.owner)
++        branch.review_approval_policy = CodeReviewApprovalPolicy.ONE_REVIEWER
++
++    def addReview(self, reviewer, vote):
++        """Add a review by the reviewer with the specified vote."""
++        login_person(reviewer)
++        self.bmp.createComment(
++            reviewer, subject=None, content="comment", vote=vote)
++
++    def test_auto_approval(self):
++        # If the product of the target branch has a policy to auto approve the
++        # proposal with a single review, then an approve review will cause the
++        # state to move from NEEDS_REVIEW to CODE_APPROVED.
++        self.addReview(self.bmp.target_branch.owner, CodeReviewVote.APPROVE)
++        self.assertEqual(
++            BranchMergeProposalStatus.CODE_APPROVED, self.bmp.queue_status)
++
++    def test_auto_rejection(self):
++        # If the product of the target branch has a policy to auto reject the
++        # proposal with a single review, then a disapprove review will cause
++        # the state to move from NEEDS_REVIEW to REJECTED.
++        self.addReview(self.bmp.target_branch.owner, CodeReviewVote.DISAPPROVE)
++        self.assertEqual(
++            BranchMergeProposalStatus.REJECTED, self.bmp.queue_status)
++
++    def test_not_needs_review(self):
++        # Auto approval only happens if the proposal is in NEEDS_REVIEW state.
++        # So a normally approving vote when in a different state change the
++        # status of the proposal.
++        login_person(self.bmp.registrant)
++        self.bmp.setAsWorkInProgress()
++        self.addReview(self.bmp.target_branch.owner, CodeReviewVote.APPROVE)
++        self.assertEqual(
++            BranchMergeProposalStatus.WORK_IN_PROGRESS, self.bmp.queue_status)
++
++
  def test_suite():
      return TestLoader().loadTestsFromName(__name__)
 === added file 'lib/lp/code/model/tests/test_reviewapprovalpolicies.py'
 --- lib/lp/code/model/tests/test_reviewapprovalpolicies.py	1970-01-01 00:00:00 +0000
 +++ lib/lp/code/model/tests/test_reviewapprovalpolicies.py	2009-06-09 10:45:18 +0000
@@ -0,0 +1,244 @@
++# Copyright 2009 Canonical Ltd.  All rights reserved.
++
++"""Tests for code review approval policies."""
++
++__metaclass__ = type
++
++import unittest
++
++from canonical.testing.layers import DatabaseFunctionalLayer
++
++from lp.code.interfaces.branchmergeproposal import BranchMergeProposalStatus
++from lp.code.interfaces.codereviewcomment import CodeReviewVote
++from lp.code.interfaces.reviewapprovalpolicies import (
++    CodeReviewApprovalPolicy, ICodeReviewApprovalPolicy)
++from lp.code.model.reviewapprovalpolicies import (
++    get_approval_policy, NoChangePolicy, OneReviewerPolicy,
++    ReviewsCompletePolicy, TwoReviewersPolicy)
++from lp.testing import TestCase, TestCaseWithFactory
++
++
++class TestGetApprovalPolicy(TestCase):
++    """Test the policy factory generates the correct types."""
++
++    def test_no_change_policy(self):
++        # NO_AUTO_CHANGE creates a NoChangePolicy.
++        policy = get_approval_policy(CodeReviewApprovalPolicy.NO_AUTO_CHANGE)
++        self.assertIsInstance(policy, NoChangePolicy)
++        self.assertProvides(policy, ICodeReviewApprovalPolicy)
++
++    def test_one_reviewer_policy(self):
++        # ONE_REVIEWER creates a OneReviewerPolicy.
++        policy = get_approval_policy(CodeReviewApprovalPolicy.ONE_REVIEWER)
++        self.assertIsInstance(policy, OneReviewerPolicy)
++        self.assertProvides(policy, ICodeReviewApprovalPolicy)
++
++    def test_two_reviewers_policy(self):
++        # TWO_REVIEWERS creates a TwoReviewersPolicy.
++        policy = get_approval_policy(CodeReviewApprovalPolicy.TWO_REVIEWERS)
++        self.assertIsInstance(policy, TwoReviewersPolicy)
++        self.assertProvides(policy, ICodeReviewApprovalPolicy)
++
++    def test_all_reviews_completed_policy(self):
++        # ALL_REQUESTED_COMPLETE creates a ReviewsCompletePolicy.
++        policy = get_approval_policy(
++            CodeReviewApprovalPolicy.ALL_REQUESTED_COMPLETE)
++        self.assertIsInstance(policy, ReviewsCompletePolicy)
++        self.assertProvides(policy, ICodeReviewApprovalPolicy)
++
++
++class TestNoChangePolicy(TestCaseWithFactory):
++    """Test that a no change policy doesn't change the status."""
++
++    layer = DatabaseFunctionalLayer
++
++    def setUp(self):
++        TestCaseWithFactory.setUp(self)
++        self.policy = NoChangePolicy()
++
++    def test_review_approval(self):
++        # An review approval doesn't change the status.
++        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++    def test_review_disapproval(self):
++        # An review disapproval doesn't change the status.
++        reviews = [(self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++
++class TestOneReviewerPolicy(TestCaseWithFactory):
++    """Test that one review is enough to change the status."""
++
++    layer = DatabaseFunctionalLayer
++
++    def setUp(self):
++        TestCaseWithFactory.setUp(self)
++        self.policy = OneReviewerPolicy()
++
++    def test_review_approval(self):
++        # An review approval doesn't change the status.
++        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
++        self.assertEqual(
++            BranchMergeProposalStatus.CODE_APPROVED,
++            self.policy.checkReviews(reviews))
++
++    def test_review_disapproval(self):
++        # An review disapproval doesn't change the status.
++        reviews = [(self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
++        self.assertEqual(
++            BranchMergeProposalStatus.REJECTED,
++            self.policy.checkReviews(reviews))
++
++    def test_other_review(self):
++        # An review disapproval doesn't change the status.
++        reviews = [(self.factory.makePerson(), CodeReviewVote.ABSTAIN)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++    def test_conflicting_reviews(self):
++        # If there is an approval and a disapproval, an update isn't
++        # suggested.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.APPROVE),
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++
++class TestTwoReviewersPolicy(TestCaseWithFactory):
++    """Test that two reviews are needed to change the status."""
++
++    layer = DatabaseFunctionalLayer
++
++    def setUp(self):
++        TestCaseWithFactory.setUp(self)
++        self.policy = TwoReviewersPolicy()
++
++    def test_review_approval(self):
++        # An single approval doesn't change the status.
++        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++    def test_two_approvals(self):
++        # Two approvals do change the status.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.APPROVE),
++            (self.factory.makePerson(), CodeReviewVote.APPROVE)]
++        self.assertEqual(
++            BranchMergeProposalStatus.CODE_APPROVED,
++            self.policy.checkReviews(reviews))
++
++    def test_review_disapproval(self):
++        # An single disapproval doesn't change the status.
++        reviews = [(self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++    def test_two_disapprovals(self):
++        # Two disapprovals do change the status.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
++        self.assertEqual(
++            BranchMergeProposalStatus.REJECTED,
++            self.policy.checkReviews(reviews))
++
++    def test_conflicting_reviews(self):
++        # If there is an approval and a disapproval, an update isn't
++        # suggested.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.APPROVE),
++            (self.factory.makePerson(), CodeReviewVote.APPROVE),
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.APPROVE),
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++
++class TestReviewsCompletePolicy(TestCaseWithFactory):
++    """Test that all requested reviews are complete."""
++
++    layer = DatabaseFunctionalLayer
++
++    def setUp(self):
++        TestCaseWithFactory.setUp(self)
++        self.policy = ReviewsCompletePolicy()
++
++    def test_review_approval(self):
++        # An single approval is enough to approve the proposal.
++        reviews = [(self.factory.makePerson(), CodeReviewVote.APPROVE)]
++        self.assertEqual(
++            BranchMergeProposalStatus.CODE_APPROVED,
++            self.policy.checkReviews(reviews))
++
++    def test_review_approval_and_pending(self):
++        # An single approval is not good enough if there is another pending.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.APPROVE),
++            (self.factory.makePerson(), None)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++    def test_review_approval_and_other(self):
++        # An single approval is not good enough if there is another incomplete
++        # review.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.APPROVE),
++            (self.factory.makePerson(), CodeReviewVote.NEEDS_FIXING)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++    def test_review_approval_and_abstain(self):
++        # An single approval is good enough if the other requested reviewer
++        # abstained.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.APPROVE),
++            (self.factory.makePerson(), CodeReviewVote.ABSTAIN)]
++        self.assertEqual(
++            BranchMergeProposalStatus.CODE_APPROVED,
++            self.policy.checkReviews(reviews))
++
++    def test_review_disapproval(self):
++        # An single disapproval is enough to reject the proposal.
++        reviews = [(self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
++        self.assertEqual(
++            BranchMergeProposalStatus.REJECTED,
++            self.policy.checkReviews(reviews))
++
++    def test_review_disapproval_and_pending(self):
++        # An single disapproval is not good enough if there is another
++        # pending.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
++            (self.factory.makePerson(), None)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++    def test_review_disapproval_and_other(self):
++        # An single disapproval is not good enough if there is another
++        # incomplete review.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
++            (self.factory.makePerson(), CodeReviewVote.NEEDS_FIXING)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++    def test_review_disapproval_and_abstain(self):
++        # An single disapproval is good enough if the other requested reviewer
++        # abstained.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE),
++            (self.factory.makePerson(), CodeReviewVote.ABSTAIN)]
++        self.assertEqual(
++            BranchMergeProposalStatus.REJECTED,
++            self.policy.checkReviews(reviews))
++
++    def test_conflicting_reviews(self):
++        # If there is an approval and a disapproval, an update isn't
++        # suggested.
++        reviews = [
++            (self.factory.makePerson(), CodeReviewVote.APPROVE),
++            (self.factory.makePerson(), CodeReviewVote.DISAPPROVE)]
++        self.assertIs(None, self.policy.checkReviews(reviews))
++
++
++def test_suite():
++    return unittest.TestLoader().loadTestsFromName(__name__)
 === modified file 'lib/lp/registry/model/product.py'
 --- lib/lp/registry/model/product.py	2009-06-02 08:20:49 +0000
 +++ lib/lp/registry/model/product.py	2009-06-09 10:45:18 +0000
@@ -517,6 +517,7 @@
      branches = SQLMultipleJoin('Branch', joinColumn='product',
          orderBy='id')
++
      serieses = SQLMultipleJoin('ProductSeries', joinColumn='product',
          orderBy='name')