launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-1056881-2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-1056881-2/+merge/136194

This branch allows subscribers to a proprietary blueprint to view
te blueprint page. (bug 1056881)

My first attempt to fix this bug missed two important points:

1. The main view test, test_view_for_user_with_artifact_grant() --
   now called test_view_for_subscriber_with_artifact_grant() --
   did not require traversal,
2. it just tested that people with an artifact grant can view the
   page. Hence the subscribers portlet was empty, which meant that
   another permission problem to render the subscribers portlet
   was not detected.

So test_view_for_subscriber_with_artifact_grant() no calls
getViewBrowser() instead create_initialized_view() to ensure
that "proper traversal" is done in the test. This reveealed that
Product.getSPecification() needs the permission lp.LimitedView.

Redering the subscribers portlet requires access to IProduct.drivers,
so this attribute now alsoe requires the permission lp.LimitedView.

I removed the base classes IHasDrivers and ISpecificationTarget
from IProductView and added them to IProduct.

There are no permission settings for IProduct itself, only for
several base classes. Since somes attributes defined in IHasDrivers
and ISpecificationTarget now need the permission lp.LimitedView,
while others still required lp.View, I specified the permission
attribute-by-attribue in configure.zcml.

tests:

./bin/test blueprints -vvt test_view_for_subscriber_with_artifact_grant
./bin/test registry -vvt test_product.*test_.et_permissions

no lint

-- 
https://code.launchpad.net/~adeuring/launchpad/bug-1056881-2/+merge/136194
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-1056881-2 into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/tests/test_specification.py'
--- lib/lp/blueprints/browser/tests/test_specification.py	2012-11-14 14:10:13 +0000
+++ lib/lp/blueprints/browser/tests/test_specification.py	2012-11-26 14:57:33 +0000
@@ -157,7 +157,7 @@
             self.specification.getBranchLink(branch), self.traverse(segments))
 
 
-class TestSpecificationView(TestCaseWithFactory):
+class TestSpecificationView(BrowserTestCase):
     """Test the SpecificationView."""
 
     layer = DatabaseFunctionalLayer
@@ -184,7 +184,7 @@
             extract_text(html), DocTestMatches(
                 "... Registered by Some Person ... ago ..."))
 
-    def test_view_for_user_with_artifact_grant(self):
+    def test_view_for_subscriber_with_artifact_grant(self):
         # Users with an artifact grant for a specification related to a
         # private  product can view the specification page.
         owner = self.factory.makePerson()
@@ -196,13 +196,11 @@
             spec = self.factory.makeSpecification(
                 product=product, owner=owner,
                 information_type=InformationType.PROPRIETARY)
-            getUtility(IService, 'sharing').ensureAccessGrants(
-                [user], owner, specifications=[spec])
-        with person_logged_in(user):
-            view = create_initialized_view(
-                spec, name='+index', principal=user, rootsite='blueprints')
-            # Calling render() does not raise any exceptions.
-            self.assertIn(spec.name, view.render())
+            spec.subscribe(user, subscribed_by=owner)
+            spec_name = spec.name
+        # Creating the view does not raise any exceptions.
+        view = self.getViewBrowser(spec, user=user)
+        self.assertIn(spec_name, view.contents)
 
 
 def set_blueprint_information_type(test_case, enabled):

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2012-11-22 16:26:40 +0000
+++ lib/lp/registry/configure.zcml	2012-11-26 14:57:33 +0000
@@ -1436,6 +1436,31 @@
         <require
             permission="launchpad.Edit"
             set_attributes="bug_supervisor"/>
+
+        <!-- ISpecificationTarget -->
+        <require
+            permission="launchpad.LimitedView"
+            attributes="
+                getSpecification" />
+        <require
+            permission="launchpad.View"
+            attributes="
+                _all_specifications
+                _valid_specifications
+                getAllowedSpecificationInformationTypes
+                getDefaultSpecificationInformationType
+                specifications" />
+
+        <!-- IHasDrivers -->
+        <require
+            permission="launchpad.LimitedView"
+            attributes="
+                drivers" />
+        <require
+            permission="launchpad.View"
+            attributes="
+                personHasDriverRights" />
+
     </class>
 
     <!-- ProductWithLicenses

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2012-11-24 17:49:30 +0000
+++ lib/lp/registry/interfaces/product.py	2012-11-26 14:57:33 +0000
@@ -501,12 +501,12 @@
 
 class IProductView(
     ICanGetMilestonesDirectly, IHasAppointedDriver, IHasBranches,
-    IHasDrivers, IHasExternalBugTracker,
+    IHasExternalBugTracker,
     IHasMergeProposals, IHasMilestones, IHasExpirableBugs,
     IHasMugshot, IHasSprints, IHasTranslationImports,
     ITranslationPolicy, IKarmaContext, IMakesAnnouncements,
     IOfficialBugTagTargetPublic, IHasOOPSReferences,
-    ISpecificationTarget, IHasRecipes, IHasCodeImports, IServiceUsage):
+    IHasRecipes, IHasCodeImports, IServiceUsage):
     """Public IProduct properties."""
 
     registrant = exported(
@@ -529,11 +529,6 @@
                 "than having one project team that does it all."),
             required=False, vocabulary='ValidPersonOrTeam'))
 
-    drivers = Attribute(
-        "Presents the drivers of this project as a list. A list is "
-        "required because there might be a project driver and also a "
-        "driver appointed in the overarching project group.")
-
     summary = exported(
         Summary(
             title=_('Summary'),
@@ -917,10 +912,11 @@
 
 
 class IProduct(
-    IBugTarget, IHasBugSupervisor, IProductEditRestricted,
+    IBugTarget, IHasBugSupervisor, IHasDrivers, IProductEditRestricted,
     IProductModerateRestricted, IProductDriverRestricted, IProductView,
     IProductLimitedView, IProductPublic, IQuestionTarget, IRootContext,
-    IStructuralSubscriptionTarget, IInformationType, IPillar):
+    ISpecificationTarget, IStructuralSubscriptionTarget, IInformationType,
+    IPillar):
     """A Product.
 
     The Launchpad Registry describes the open source world as ProjectGroups
@@ -931,6 +927,12 @@
 
     export_as_webservice_entry('project')
 
+    drivers = Attribute(
+        "Presents the drivers of this project as a list. A list is "
+        "required because there might be a project driver and also a "
+        "driver appointed in the overarching project group.")
+
+
 # Fix cyclic references.
 IProjectGroup['products'].value_type = Reference(IProduct)
 IProductRelease['product'].schema = IProduct

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-11-26 08:33:03 +0000
+++ lib/lp/registry/tests/test_product.py	2012-11-26 14:57:33 +0000
@@ -663,7 +663,8 @@
             'active', 'id', 'information_type', 'pillar_category', 'private',
             'userCanView',)),
         'launchpad.LimitedView': set((
-            'bugtargetdisplayname', 'displayname', 'enable_bug_expiration',
+            'bugtargetdisplayname', 'displayname', 'drivers',
+            'enable_bug_expiration', 'getSpecification',
             'icon', 'logo', 'name', 'official_answers', 'official_anything',
             'official_blueprints', 'official_codehosting', 'official_malone',
             'owner', 'parent_subscription_target', 'project', 'title', )),
@@ -684,7 +685,7 @@
             'date_next_suggest_packaging', 'datecreated', 'description',
             'development_focus', 'development_focusID',
             'direct_answer_contacts', 'distrosourcepackages',
-            'downloadurl', 'driver', 'drivers',
+            'downloadurl', 'driver',
             'enable_bugfiling_duplicate_search', 'findReferencedOOPS',
             'findSimilarFAQs', 'findSimilarQuestions', 'freshmeatproject',
             'getAllowedBugInformationTypes',
@@ -698,7 +699,7 @@
             'getFAQ', 'getFirstEntryToImport', 'getLinkedBugWatches',
             'getMergeProposals', 'getMilestone', 'getMilestonesAndReleases',
             'getQuestion', 'getQuestionLanguages', 'getPackage', 'getRelease',
-            'getSeries', 'getSpecification', 'getSubscription',
+            'getSeries', 'getSubscription',
             'getSubscriptions', 'getSupportedLanguages', 'getTimeline',
             'getTopContributors', 'getTopContributorsGroupedByCategory',
             'getTranslationGroups', 'getTranslationImportQueueEntries',