launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/reviewer-inclusive-team-1009832 into lp:launchpad.

Requested reviews:
  Curtis Hovey (sinzui)
Related bugs:
  Bug #1009832 in Launchpad itself: "UI implies reviewers can be inclusive teams"
  https://bugs.launchpad.net/launchpad/+bug/1009832

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/reviewer-inclusive-team-1009832/+merge/120323

== Implementation ==

I hope I have done this correctly. I have implemented the suggestions from the bug comments:

"1. We want to ensure that series branches (the basis of stacking) are owned by users and exclusive teams! Lp ensures the maintainer and drivers who work with the code are exclusive, and the same rule should apply to the branch."

I modified the AllUserTeamsParticipationPlusSelf vocabulary which is used for branch owners. If the context is a branch, and branch.associatedProductSeries() > 1, then only return exclusive teams.

The above vocabulary is used to set the owner of a branch, so all branches from now on which are owned by teams must have the team be an exclusive team.

"2. I am not certain of the value of asking an members of open teams to do a review. The review is done by a user and the user must represent the project, or be an authority on an issue. In the project team has to be exclusive in this case. The team of authority will loose its authority if anyone can join and represent the team without regard to skill or policy. Community teams that do review provide opinion, but do not have the authority to approve the merge."

I modified the vocabulary definitions used to for the reviewer of a branch:
1. branch default reviewer
2. branch merge proposal create
3. branch reassign reviewer

In all 3 cases above, only individuals or exclusive teams can be selected.

We will need to fix up any current branches with inclusive teams as owners or reviewers. There are currently 1089 branches with inclusive teams for owners or default reviewers. See https://pastebin.canonical.com/72527/

== Tests ==

Add tests for new vocab behaviour.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/codereviewvote.py
  lib/lp/code/interfaces/branch.py
  lib/lp/code/interfaces/codereviewvote.py
  lib/lp/registry/vocabularies.py
  lib/lp/registry/vocabularies.zcml
  lib/lp/registry/tests/test_user_vocabularies.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/reviewer-inclusive-team-1009832/+merge/120323
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/code/browser/codereviewvote.py'
--- lib/lp/code/browser/codereviewvote.py	2012-06-29 08:40:05 +0000
+++ lib/lp/code/browser/codereviewvote.py	2012-08-20 04:54:26 +0000
@@ -26,7 +26,7 @@
 
     reviewer = PublicPersonChoice(title=_('Reviewer'), required=True,
             description=_('A person who you want to review this.'),
-            vocabulary='ValidPersonOrTeam')
+            vocabulary='ValidBranchReviewer')
 
 
 class CodeReviewVoteReassign(LaunchpadFormView):

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2012-08-16 20:13:14 +0000
+++ lib/lp/code/interfaces/branch.py	2012-08-20 04:54:26 +0000
@@ -300,8 +300,9 @@
             title=_('Owner'),
             required=True, readonly=True,
             vocabulary='AllUserTeamsParticipationPlusSelf',
-            description=_("Either yourself or a team you are a member of. "
-                          "This controls who can modify the branch.")))
+            description=_("Either yourself or an exclusive team you are a "
+                          "member of. This controls who can modify the "
+                          "branch.")))
 
     # Distroseries and sourcepackagename are exported together as
     # the sourcepackage.
@@ -999,10 +1000,10 @@
         PublicPersonChoice(
             title=_('Review Team'),
             required=False,
-            vocabulary='ValidPersonOrTeam',
-            description=_("The reviewer of a branch is the person or team "
-                          "that is responsible for reviewing proposals and "
-                          "merging into this branch.")))
+            vocabulary='ValidBranchReviewer',
+            description=_("The reviewer of a branch is the person or "
+                          "exclusive team that is responsible for reviewing "
+                          "proposals and merging into this branch.")))
 
     url = exported(
         BranchURIField(

=== modified file 'lib/lp/code/interfaces/codereviewvote.py'
--- lib/lp/code/interfaces/codereviewvote.py	2011-12-24 16:54:44 +0000
+++ lib/lp/code/interfaces/codereviewvote.py	2012-08-20 04:54:26 +0000
@@ -61,7 +61,7 @@
         PersonChoice(
             title=_('Reviewer'), required=True,
             description=_('A person who you want to review this.'),
-            vocabulary='ValidPersonOrTeam'))
+            vocabulary='ValidBranchReviewer'))
 
     review_type = exported(
         TextLine(

=== modified file 'lib/lp/registry/tests/test_user_vocabularies.py'
--- lib/lp/registry/tests/test_user_vocabularies.py	2012-01-20 03:28:17 +0000
+++ lib/lp/registry/tests/test_user_vocabularies.py	2012-08-20 04:54:26 +0000
@@ -8,6 +8,7 @@
 from zope.component import getUtility
 from zope.schema.vocabulary import getVocabularyRegistry
 
+from lp.registry.enums import TeamMembershipPolicy
 from lp.registry.interfaces.person import PersonVisibility
 from lp.registry.model.person import Person
 from lp.services.webapp.interfaces import (
@@ -86,11 +87,11 @@
 
     layer = DatabaseFunctionalLayer
 
-    def _vocabTermValues(self):
+    def _vocabTermValues(self, context=None):
         """Return the token values for the vocab."""
         vocabulary_registry = getVocabularyRegistry()
         vocab = vocabulary_registry.get(
-            None, 'AllUserTeamsParticipationPlusSelf')
+            context, 'AllUserTeamsParticipationPlusSelf')
         return [term.value for term in vocab]
 
     def test_user_no_private_teams(self):
@@ -101,6 +102,32 @@
         login_person(team_owner)
         self.assertEqual([team_owner, team], self._vocabTermValues())
 
+    def test_only_exclusive_teams_for_series_branches(self):
+        # For series branches, only exclusive teams are permitted in the vocab.
+        branch = self.factory.makeBranch()
+        self.factory.makeProductSeries(branch=branch)
+        team_owner = self.factory.makePerson()
+        self.factory.makeTeam(
+            owner=team_owner, membership_policy=TeamMembershipPolicy.OPEN)
+        exclusive_team = self.factory.makeTeam(
+            owner=team_owner, membership_policy=TeamMembershipPolicy.MODERATED)
+        login_person(team_owner)
+        self.assertEqual(
+            [team_owner, exclusive_team], self._vocabTermValues(branch))
+
+    def test_all_teams_for_non_series_branches(self):
+        # For non series branches, all teams are permitted in the vocab.
+        branch = self.factory.makeBranch()
+        team_owner = self.factory.makePerson()
+        inclusive_team = self.factory.makeTeam(
+            owner=team_owner, membership_policy=TeamMembershipPolicy.OPEN)
+        exclusive_team = self.factory.makeTeam(
+            owner=team_owner, membership_policy=TeamMembershipPolicy.MODERATED)
+        login_person(team_owner)
+        self.assertContentEqual(
+            [team_owner, exclusive_team, inclusive_team],
+            self._vocabTermValues(branch))
+
 
 class TestAllUserTeamsParticipationVocabulary(TestCaseWithFactory):
     """AllUserTeamsParticipation contains all teams joined by a user.

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2012-08-13 21:33:47 +0000
+++ lib/lp/registry/vocabularies.py	2012-08-20 04:54:26 +0000
@@ -103,6 +103,7 @@
 
 from lp.blueprints.interfaces.specification import ISpecification
 from lp.bugs.interfaces.bugtask import IBugTask
+from lp.code.interfaces.branch import IBranch
 from lp.registry.enums import (
     EXCLUSIVE_TEAM_POLICY,
     PersonVisibility,
@@ -379,6 +380,8 @@
 
     INCLUDE_PRIVATE_TEAM = False
 
+    EXCLUSIVE_TEAMS_ONLY = False
+
     def toTerm(self, obj):
         """See `IVocabulary`."""
         return SimpleTerm(obj, obj.name, obj.unique_displayname)
@@ -391,8 +394,10 @@
         if launchbag.user:
             user = launchbag.user
             for team in user.teams_participated_in:
-                if (team.visibility == PersonVisibility.PUBLIC
-                    or self.INCLUDE_PRIVATE_TEAM):
+                if ((team.visibility == PersonVisibility.PUBLIC
+                    or self.INCLUDE_PRIVATE_TEAM) and
+                    (team.membership_policy in EXCLUSIVE_TEAM_POLICY
+                    or not self.EXCLUSIVE_TEAMS_ONLY)):
                     yield self.toTerm(team)
 
     def getTermByToken(self, token):
@@ -1225,6 +1230,13 @@
 
     INCLUDE_PRIVATE_TEAM = True
 
+    def __init__(self, context=None):
+        super_class = super(AllUserTeamsParticipationPlusSelfVocabulary, self)
+        super_class.__init__(context)
+        if IBranch.providedBy(context):
+            self.EXCLUSIVE_TEAMS_ONLY = (
+                len(list(context.associatedProductSeries())) > 0)
+
 
 class UserTeamsParticipationPlusSelfSimpleDisplayVocabulary(
     UserTeamsParticipationPlusSelfVocabulary):

=== modified file 'lib/lp/registry/vocabularies.zcml'
--- lib/lp/registry/vocabularies.zcml	2012-08-13 20:16:04 +0000
+++ lib/lp/registry/vocabularies.zcml	2012-08-20 04:54:26 +0000
@@ -375,6 +375,14 @@
     <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
   </securedutility>
 
+  <securedutility
+    name="ValidBranchReviewer"
+    component="lp.registry.vocabularies.ValidPersonOrExclusiveTeamVocabulary"
+    provides="zope.schema.interfaces.IVocabularyFactory"
+    >
+    <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
+  </securedutility>
+
   <class class="lp.registry.vocabularies.ValidPersonOrExclusiveTeamVocabulary">
     <allow interface="lp.services.webapp.vocabulary.IHugeVocabulary"/>
   </class>