Review: Approve
Diff comments: > === modified file 'lib/lp/registry/model/product.py' > --- lib/lp/registry/model/product.py 2015-06-24 21:14:20 +0000 > +++ lib/lp/registry/model/product.py 2015-06-26 06:42:59 +0000 > @@ -38,7 +38,9 @@ > from storm.locals import ( > And, > Desc, > + Int, > Join, > + List, > Not, > Or, > Select, > @@ -439,6 +441,11 @@ > name='remote_product', allow_none=True, default=None) > vcs = EnumCol(enum=VCSType, notNull=False) > > + # Cache of AccessPolicy.ids that convey launchpad.LimitedView. > + # Unlike artifacts' cached access_policies, an AccessArtifactGrant > + # to an artifact in the policy is sufficient for access. > + access_policies = List(type=Int()) > + > @property > def date_next_suggest_packaging(self): > """See `IProduct` > @@ -582,6 +589,7 @@ > self.setSpecificationSharingPolicy( > specification_policy_default[value]) > self._ensurePolicies([value]) > + self._cacheAccessPolicies() In the creation case, you call _cacheAccessPolicies after calling _ensurePolicies. In the revocation case, you call _cacheAccessPolicies from _pruneUnusedPolicies. It would be more consistent and harder to get wrong in future if you moved this call as well as that in _prepare_to_set_sharing_policy to the end of _ensurePolicies. > > information_type = property(_get_information_type, _set_information_type) > > @@ -753,6 +761,7 @@ > raise ProprietaryProduct( > "The project is %s." % self.information_type.title) > self._ensurePolicies(allowed_types[var]) > + self._cacheAccessPolicies() > > def setBranchSharingPolicy(self, branch_sharing_policy): > """See `IProductEditRestricted`.""" > @@ -815,6 +824,23 @@ > grants.append((p, self.owner, self.owner)) > getUtility(IAccessPolicyGrantSource).grant(grants) > > + def _cacheAccessPolicies(self): > + # Update the cache of AccessPolicy.ids for which an > + # AccessPolicyGrant or AccessArtifactGrant is sufficient to > + # convey launchpad.LimitedView on this Product. > + # > + # We only need a cache for proprietary types, and it only > + # includes proprietary policies in case a policy like Private > + # Security was somehow left around when a project was > + # transitioned to Proprietary. > + if self.information_type in PROPRIETARY_INFORMATION_TYPES: > + self.access_policies = [ > + policy.id for policy in > + getUtility(IAccessPolicySource).findByPillar([self]) > + if policy.type in PROPRIETARY_INFORMATION_TYPES] Perhaps this could use AccessPolicySource.find instead of filtering by type afterwards. > + else: > + self.access_policies = None > + > def _pruneUnusedPolicies(self): > allowed_bug_types = set( > BUG_POLICY_ALLOWED_TYPES.get( > @@ -840,6 +866,7 @@ > and apa_source.findByPolicy([ap]).is_empty()] > getUtility(IAccessPolicyGrantSource).revokeByPolicy(unused_aps) > ap_source.delete([(ap.pillar, ap.type) for ap in unused_aps]) > + self._cacheAccessPolicies() > > @cachedproperty > def commercial_subscription(self): > > === modified file 'lib/lp/registry/tests/test_product.py' > --- lib/lp/registry/tests/test_product.py 2015-06-25 07:39:40 +0000 > +++ lib/lp/registry/tests/test_product.py 2015-06-26 06:42:59 +0000 > @@ -527,6 +527,40 @@ > SpecificationSharingPolicy.PROPRIETARY, > product.specification_sharing_policy) > > + def test_cacheAccessPolicies(self): > + # Product.access_policies is a list caching AccessPolicy.ids for > + # which an AccessPolicyGrant or AccessArtifactGrant gives a > + # principal LimitedView on the Product. > + aps = getUtility(IAccessPolicySource) > + > + # Public projects don't need a cache. > + product = self.factory.makeProduct() > + naked_product = removeSecurityProxy(product) > + self.assertContentEqual( > + [InformationType.USERDATA, InformationType.PRIVATESECURITY], > + [p.type for p in aps.findByPillar([product])]) > + self.assertIs(None, naked_product.access_policies) > + > + # A private project normally just allows the Proprietary policy, > + # even if there is still another policy like Private Security. > + naked_product.information_type = InformationType.PROPRIETARY > + [prop_policy] = aps.find([(product, InformationType.PROPRIETARY)]) > + self.assertEqual([prop_policy.id], naked_product.access_policies) > + > + # If we switch it back to public, the cache is no longer > + # required. > + naked_product.information_type = InformationType.PUBLIC > + self.assertIs(None, naked_product.access_policies) > + > + # Projects can also be Embargoed because of reasons. Since they > + # can have both Proprietary and Embargoed artifacts, and someone > + # who can see either needs LimitedView on the pillar they're on, > + # both policies are permissible. > + naked_product.information_type = InformationType.EMBARGOED > + [emb_policy] = aps.find([(product, InformationType.EMBARGOED)]) > + self.assertContentEqual( > + [prop_policy.id, emb_policy.id], naked_product.access_policies) > + > def test_checkInformationType_bug_supervisor(self): > # Bug supervisors of proprietary products must not have inclusive > # membership policies. > -- https://code.launchpad.net/~wgrant/launchpad/product-aps-set/+merge/263062 Your team Launchpad code reviewers is subscribed to branch lp:launchpad. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
