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