launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/wrong-sharing-vocab-978226 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #978226 in Launchpad itself: "The sharing picker shows (untrusted) inclusive teams"
  https://bugs.launchpad.net/launchpad/+bug/978226

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/wrong-sharing-vocab-978226/+merge/101474

== Implementation ==

The root issue was that the NewPillarSharee vocab used with the sharing picker did not extend the correct base vocab. The sharing view properties were also somewhat messed up in the the vocab was specified twice - once so that the dilters could be determined and once for the picker. This was cleaned up. In addition, the pillar sharing view yui widget was changed so that it is now mandatory to specify the sharing picker header, step title and vocab. The header and steptitle properties are lifted from the vocab itself so we are guarenteed to have consistency between that the picker says and what the vocab offers.

== Tests ==

Add new test to TestNewPillarShareeVocabulary:
- test_open_teams_excluded

Update test_picker_config() in the various pillar sharing view tests to reflect the config change.

Update test_widget_can_be_instantiated() in test_pillarsharingview yui test to check the new config behaviour.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/vocabularies.py
  lib/lp/registry/browser/pillar.py
  lib/lp/registry/browser/tests/test_pillar_sharing.py
  lib/lp/registry/javascript/sharing/pillarsharingview.js
  lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js
  lib/lp/registry/tests/test_person_vocabularies.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/wrong-sharing-vocab-978226/+merge/101474
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/wrong-sharing-vocab-978226 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py	2012-04-09 15:26:37 +0000
+++ lib/lp/registry/browser/pillar.py	2012-04-11 02:24:19 +0000
@@ -26,7 +26,6 @@
     implements,
     Interface,
     )
-from zope.schema.interfaces import IVocabulary
 from zope.schema.vocabulary import getVocabularyRegistry
 from zope.security.interfaces import Unauthorized
 from zope.traversing.browser.absoluteurl import absoluteURL
@@ -49,10 +48,6 @@
     IBugTaskSet,
     )
 from lp.code.interfaces.branch import IBranch
-from lp.registry.interfaces.accesspolicy import (
-    IAccessPolicyGrantFlatSource,
-    IAccessPolicySource,
-    )
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
@@ -269,6 +264,8 @@
     page_title = "Sharing"
     label = "Sharing information"
 
+    sharing_vocabulary_name = 'NewPillarSharee'
+
     related_features = (
         'disclosure.enhanced_sharing.enabled',
         'disclosure.enhanced_sharing.writable',
@@ -291,7 +288,7 @@
     def sharing_vocabulary(self):
         registry = getVocabularyRegistry()
         return registry.get(
-            IVocabulary, 'ValidPillarOwner')
+            self.context, self.sharing_vocabulary_name)
 
     @cachedproperty
     def sharing_vocabulary_filters(self):
@@ -300,9 +297,10 @@
     @property
     def sharing_picker_config(self):
         return dict(
-            vocabulary='NewPillarSharee',
+            vocabulary=self.sharing_vocabulary_name,
             vocabulary_filters=self.sharing_vocabulary_filters,
-            header='Share with a user or team')
+            header=self.sharing_vocabulary.displayname,
+            steptitle=self.sharing_vocabulary.step_title)
 
     @property
     def json_sharing_picker_config(self):

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-04-09 20:41:42 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-04-11 02:24:19 +0000
@@ -290,9 +290,12 @@
             picker_config = simplejson.loads(view.json_sharing_picker_config)
             self.assertTrue('vocabulary_filters' in picker_config)
             self.assertEqual(
-                'Share with a user or team',
+                'Grant access to project artifacts',
                 picker_config['header'])
             self.assertEqual(
+                'Search for user or exclusive team with whom to share',
+                picker_config['steptitle'])
+            self.assertEqual(
                 'NewPillarSharee', picker_config['vocabulary'])
 
     def test_view_data_model(self):

=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js	2012-04-06 17:28:25 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js	2012-04-11 02:24:19 +0000
@@ -62,17 +62,30 @@
         }
         this.set('write_enabled', true);
 
-        var vocab = 'ValidPillarOwner';
-        var header = 'Grant access to project artifacts.';
+        var vocab;
+        var header;
+        var steptitle;
         if (Y.Lang.isValue(config)) {
             if (Y.Lang.isValue(config.header)) {
                 header = config.header;
+            } else {
+                throw new Error(
+                    "Missing header config value for sharee picker");
+            }
+            if (Y.Lang.isValue(config.steptitle)) {
+                steptitle = config.steptitle;
+            } else {
+                throw new Error(
+                    "Missing steptitle config value for sharee picker");
             }
             if (Y.Lang.isValue(config.vocabulary)) {
                 vocab = config.vocabulary;
+            } else {
+                throw new Error(
+                    "Missing vocab config value for sharee picker");
             }
         } else {
-            config = {};
+            throw new Error("Missing config for sharee picker");
         }
         var self = this;
         var new_config = Y.merge(config, {
@@ -83,6 +96,7 @@
             progressbar: true,
             progress: 50,
             headerContent: Y.Node.create("<h2></h2>").set('text', header),
+            steptitle: steptitle,
             zIndex: 1000,
             visible: false,
             information_types: LP.cache.information_types,

=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js	2012-04-06 17:28:25 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js	2012-04-11 02:24:19 +0000
@@ -54,8 +54,13 @@
         },
 
         _create_Widget: function(cfg) {
+            var config = Y.merge(cfg, {
+                header: "Grant access",
+                steptitle: "Select user",
+                vocabulary: "SharingVocab"
+            });
             var ns = Y.lp.registry.sharing.pillarsharingview;
-            return new ns.PillarSharingView(cfg);
+            return new ns.PillarSharingView(config);
         },
 
         test_library_exists: function () {
@@ -70,6 +75,12 @@
                 Y.lp.registry.sharing.pillarsharingview.PillarSharingView,
                 this.view,
                 "Sharee table failed to be instantiated");
+            // Check the picker config.
+            var sharee_picker = this.view.get('sharee_picker');
+            Y.Assert.areEqual(
+                sharee_picker.get('headerContent')
+                    .get('text'), 'Grant access');
+            Y.Assert.areEqual(sharee_picker.get('steptitle'), 'Select user');
         },
 
         // The view is correctly rendered.

=== modified file 'lib/lp/registry/tests/test_person_vocabularies.py'
--- lib/lp/registry/tests/test_person_vocabularies.py	2012-03-27 20:55:37 +0000
+++ lib/lp/registry/tests/test_person_vocabularies.py	2012-04-11 02:24:19 +0000
@@ -27,10 +27,7 @@
     TestCaseWithFactory,
     )
 from lp.testing.dbuser import dbuser
-from lp.testing.layers import (
-    DatabaseFunctionalLayer,
-    DatabaseFunctionalLayer,
-    )
+from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.matchers import HasQueryCount
 
 
@@ -377,3 +374,13 @@
         self.factory.makeAccessPolicyGrant(policy=policy, grantee=person1)
         [newsharee] = self.searchVocabulary(product, 'sharee')
         self.assertEqual(newsharee, person2)
+
+    def test_open_teams_excluded(self):
+        # Only closed teams should be available for selection.
+        product = self.factory.makeProduct()
+        self.factory.makeTeam(name='sharee1')
+        closed_team = self.factory.makeTeam(
+            name='sharee2',
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        [newsharee] = self.searchVocabulary(product, 'sharee')
+        self.assertEqual(newsharee, closed_team)

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2012-03-27 21:34:58 +0000
+++ lib/lp/registry/vocabularies.py	2012-04-11 02:24:19 +0000
@@ -1042,20 +1042,19 @@
         return obj in self._get_teams()
 
 
-class NewPillarShareeVocabulary(TeamVocabularyMixin,
-                                    ValidPersonOrTeamVocabulary):
+class NewPillarShareeVocabulary(ValidPersonOrClosedTeamVocabulary):
     """The set of people and teams with whom to share information.
 
     A person or team is eligible for sharing with if they are not already an
     existing sharee for the pillar.
     """
 
-    displayname = 'Share with a user or team'
+    displayname = 'Grant access to project artifacts'
     step_title = 'Search for user or exclusive team with whom to share'
 
     def __init__(self, context):
         assert IPillar.providedBy(context)
-        super(ValidPersonOrTeamVocabulary, self).__init__(context)
+        super(NewPillarShareeVocabulary, self).__init__(context)
         aps = getUtility(IAccessPolicySource)
         access_policies = aps.findByPillar([self.context])
         self.policy_ids = [policy.id for policy in access_policies]
@@ -1068,7 +1067,9 @@
                 WHERE policy in %s
                 )
             """ % sqlvalues(self.policy_ids))
-        return clause
+        return And(
+            clause,
+            super(NewPillarShareeVocabulary, self).extra_clause)
 
 
 class ActiveMailingListVocabulary(FilteredVocabularyBase):