launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/change-branch-owner into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/change-branch-owner/+merge/89376

Allow users to change the branch owner to a private team using the UI.

    Launchpad bug: https://bugs.launchpad.net/bugs/918816
    Pre-implementation: My inner rage

The branch edit page does not permit me to set the branch owner to
a private team. The list only contains myself and my public teams, yet the
project that branch belongs to has explicitly set a policy for the
my private team.

--------------------------------------------------------------------

RULES

    * The IBranch owner uses an vocabulary that excludes private teams.
      This is probably a mistake because the vocab's name implies it
      includes private teams.


QA

    * Visit a branch you own and choose Change details.
    * Verify your private teams are include in the owner list.
    * Select a private team an submit.
    * Verify the branch is owned by the private team.


LINT

    lib/lp/code/browser/tests/test_branch.py
    lib/lp/code/interfaces/branch.py
    lib/lp/registry/vocabularies.py
    lib/lp/registry/vocabularies.zcml
    lib/lp/registry/tests/test_user_vocabularies.py


TEST

    ./bin/test -vvc lp.registry.tests.test_user_vocabularies
    ./bin/test -vvc -t TestBranchEditView lp.code.browser.tests.test_branch
    ./bin/test -vvc -t xx-branch-edit lp.code.tests.test_doc


IMPLEMENTATION

Refactored the base vocabulary use by the current vocab to use a attr to
guard the inclusion of private teams. Subclassed the current vocabulary
to redefine the attr to allow private teams. Updated all the user
vocabulary tests to run on the database functional layer. I removed an
odd docstring that was probably a paste error.
    lib/lp/registry/vocabularies.py
    lib/lp/registry/vocabularies.zcml
    lib/lp/registry/tests/test_user_vocabularies.py

Updated the interface to use the new vocabulary.
    lib/lp/code/browser/tests/test_branch.py
    lib/lp/code/interfaces/branch.py
-- 
https://code.launchpad.net/~sinzui/launchpad/change-branch-owner/+merge/89376
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/change-branch-owner into lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-01-15 13:32:27 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-01-20 03:44:25 +0000
@@ -929,6 +929,25 @@
         with person_logged_in(person):
             self.assertEquals(person, branch.owner)
 
+    def test_private_owner_is_ok(self):
+        # A branch's owner can be changed to a private team permitted by the
+        # visibility policy.
+        person = self.factory.makePerson()
+        branch = self.factory.makeProductBranch(owner=person)
+        team = self.factory.makeTeam(
+            owner=person, displayname="Private team",
+            visibility=PersonVisibility.PRIVATE)
+        branch.product.setBranchVisibilityTeamPolicy(
+            None, BranchVisibilityRule.FORBIDDEN)
+        branch.product.setBranchVisibilityTeamPolicy(
+            team, BranchVisibilityRule.PRIVATE)
+        browser = self.getUserBrowser(
+            canonical_url(branch) + '/+edit', user=person)
+        browser.getControl("Owner").displayValue = ["Private team"]
+        browser.getControl("Change Branch").click()
+        with person_logged_in(person):
+            self.assertEquals(team, branch.owner)
+
 
 class TestBranchUpgradeView(TestCaseWithFactory):
 

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/interfaces/branch.py	2012-01-20 03:44:25 +0000
@@ -294,7 +294,7 @@
         PersonChoice(
             title=_('Owner'),
             required=True, readonly=True,
-            vocabulary='UserTeamsParticipationPlusSelf',
+            vocabulary='AllUserTeamsParticipationPlusSelf',
             description=_("Either yourself or a team you are a member of. "
                           "This controls who can modify the branch.")))
 

=== modified file 'lib/lp/registry/tests/test_user_vocabularies.py'
--- lib/lp/registry/tests/test_user_vocabularies.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/tests/test_user_vocabularies.py	2012-01-20 03:44:25 +0000
@@ -21,13 +21,13 @@
     login_person,
     TestCaseWithFactory,
     )
-from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.layers import DatabaseFunctionalLayer
 
 
 class TestUserTeamsParticipationPlusSelfVocabulary(TestCaseWithFactory):
     """Test that the UserTeamsParticipationPlusSelf behaves as expected."""
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def _vocabTermValues(self):
         """Return the token values for the vocab."""
@@ -81,13 +81,34 @@
         self.assertEqual([user, alpha, bravo], self._vocabTermValues())
 
 
+class TestAllUserTeamsParticipationPlusSelfVocabulary(TestCaseWithFactory):
+    """Test that the AllUserTeamsParticipationPlusSelf behaves as expected."""
+
+    layer = DatabaseFunctionalLayer
+
+    def _vocabTermValues(self):
+        """Return the token values for the vocab."""
+        vocabulary_registry = getVocabularyRegistry()
+        vocab = vocabulary_registry.get(
+            None, 'AllUserTeamsParticipationPlusSelf')
+        return [term.value for term in vocab]
+
+    def test_user_no_private_teams(self):
+        # Private teams are shown in the vocabulary.
+        team_owner = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            owner=team_owner, visibility=PersonVisibility.PRIVATE)
+        login_person(team_owner)
+        self.assertEqual([team_owner, team], self._vocabTermValues())
+
+
 class TestAllUserTeamsParticipationVocabulary(TestCaseWithFactory):
     """AllUserTeamsParticipation contains all teams joined by a user.
 
     This includes private teams.
     """
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def _vocabTermValues(self):
         """Return the token values for the vocab."""

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2012-01-06 11:08:30 +0000
+++ lib/lp/registry/vocabularies.py	2012-01-20 03:44:25 +0000
@@ -27,6 +27,7 @@
     'ActiveMailingListVocabulary',
     'AdminMergeablePersonVocabulary',
     'AllUserTeamsParticipationVocabulary',
+    'AllUserTeamsParticipationPlusSelfVocabulary',
     'CommercialProjectsVocabulary',
     'DistributionOrProductOrProjectGroupVocabulary',
     'DistributionOrProductVocabulary',
@@ -360,10 +361,12 @@
 
 
 class UserTeamsParticipationVocabulary(SQLObjectVocabularyBase):
-    """Describes the teams in which the current user participates."""
+    """Describes the public teams in which the current user participates."""
     _table = Person
     _orderBy = 'displayname'
 
+    INCLUDE_PRIVATE_TEAM = False
+
     def toTerm(self, obj):
         """See `IVocabulary`."""
         return SimpleTerm(obj, obj.name, obj.unique_displayname)
@@ -376,7 +379,8 @@
         if launchbag.user:
             user = launchbag.user
             for team in user.teams_participated_in:
-                if team.visibility == PersonVisibility.PUBLIC:
+                if (team.visibility == PersonVisibility.PUBLIC
+                    or self.INCLUDE_PRIVATE_TEAM):
                     yield self.toTerm(team)
 
     def getTermByToken(self, token):
@@ -1143,7 +1147,7 @@
     UserTeamsParticipationVocabulary):
     """A vocabulary containing the public teams that the logged
     in user participates in, along with the logged in user themselves.
-    """    """All `IProduct` objects vocabulary."""
+    """
 
     def __iter__(self):
         logged_in_user = getUtility(ILaunchBag).user
@@ -1161,6 +1165,18 @@
         return super_class.getTermByToken(token)
 
 
+class AllUserTeamsParticipationPlusSelfVocabulary(
+    UserTeamsParticipationPlusSelfVocabulary):
+    """All public and private teams participates in and himself.
+
+    This redefines UserTeamsParticipationVocabulary to include private teams
+    and it includes the logged in user from
+    UserTeamsParticipationPlusSelfVocabulary.
+    """
+
+    INCLUDE_PRIVATE_TEAM = True
+
+
 class UserTeamsParticipationPlusSelfSimpleDisplayVocabulary(
     UserTeamsParticipationPlusSelfVocabulary):
     """Like UserTeamsParticipationPlusSelfVocabulary but the term title is

=== modified file 'lib/lp/registry/vocabularies.zcml'
--- lib/lp/registry/vocabularies.zcml	2011-12-24 17:49:30 +0000
+++ lib/lp/registry/vocabularies.zcml	2012-01-20 03:44:25 +0000
@@ -304,6 +304,18 @@
 
 
   <securedutility
+    name="AllUserTeamsParticipationPlusSelf"
+    component="lp.registry.vocabularies.AllUserTeamsParticipationPlusSelfVocabulary"
+    provides="zope.schema.interfaces.IVocabularyFactory"
+    >
+    <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
+  </securedutility>
+
+  <class class="lp.registry.vocabularies.AllUserTeamsParticipationPlusSelfVocabulary">
+    <allow interface="lp.services.webapp.vocabulary.IHugeVocabulary"/>
+  </class>
+
+  <securedutility
     name="ActiveMailingList"
     component="lp.registry.vocabularies.ActiveMailingListVocabulary"
     provides="zope.schema.interfaces.IVocabularyFactory"