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.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