launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/workitems-for-private-blueprints into lp:launchpad.

Commit message:
Filter non-Public blueprints for +upcomingwork.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1056891 in Launchpad itself: "Specification privacy: upcoming work won't load for anyone if there's any private data on it"
  https://bugs.launchpad.net/launchpad/+bug/1056891

For more details, see:
https://code.launchpad.net/~abentley/launchpad/workitems-for-private-blueprints/+merge/129736

= Summary =
Fix bug #1056891: Specification privacy: upcoming work won't load for anyone if there's any private data on it

== Proposed fix ==
Use standard filter to ensure that only specifications visible to the viewer are listed.

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of private products

== Implementation details ==
Add user parameter to getAssignedSpecificationWorkItemsDueBefore, supply view.user.

Update LaunchpadObjectFactory.makeSpecification to use milestone.product as needed.


== Tests ==
bin/test -t test_non_public_specifications -t Test_getAssignedSpecificationWorkItemsDueBefore

== Demo and Q/A ==
 1. create a project with other/proprietary license as a regular user
 2. create a milestone targetted to a date in the future (in the next 180 days for it to show up on +upcomingwork page)
 3. create a private blueprint, assign it to yourself and milestone from 2
 4. add a workitem to the blueprint
 5. check /people/+me/+upcomingwork to ensure it displays correctly for yourself
 6. log in as a different user and load that same page

It should omit the proprietary spec's workitem, but otherwise work normally.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/blueprints/browser/tests/test_person_upcomingwork.py
  lib/lp/testing/factory.py
  lib/lp/blueprints/browser/person_upcomingwork.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py
-- 
https://code.launchpad.net/~abentley/launchpad/workitems-for-private-blueprints/+merge/129736
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/workitems-for-private-blueprints into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/person_upcomingwork.py'
--- lib/lp/blueprints/browser/person_upcomingwork.py	2012-07-05 11:47:25 +0000
+++ lib/lp/blueprints/browser/person_upcomingwork.py	2012-10-15 18:58:21 +0000
@@ -277,7 +277,8 @@
     Only work items whose milestone have a due date between today and the
     given cut-off date are included in the results.
     """
-    workitems = person.getAssignedSpecificationWorkItemsDueBefore(cutoff_date)
+    workitems = person.getAssignedSpecificationWorkItemsDueBefore(cutoff_date,
+                                                                  user)
     # For every specification that has work items in the list above, create
     # one SpecWorkItemContainer holding the work items from that spec that are
     # targeted to the same milestone and assigned to this person (or its

=== modified file 'lib/lp/blueprints/browser/tests/test_person_upcomingwork.py'
--- lib/lp/blueprints/browser/tests/test_person_upcomingwork.py	2012-07-24 06:39:54 +0000
+++ lib/lp/blueprints/browser/tests/test_person_upcomingwork.py	2012-10-15 18:58:21 +0000
@@ -11,6 +11,7 @@
 
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import InformationType
 from lp.blueprints.browser.person_upcomingwork import (
     GenericWorkItem,
     getWorkItemsDueBefore,
@@ -391,6 +392,29 @@
         with anonymous_logged_in():
             self.assertIn(bugtask.bug.title, tomorrows_group)
 
+    def test_non_public_specifications(self):
+        """Work items for non-public specs are filtered correctly."""
+        person = self.factory.makePerson()
+        proprietary_spec = self.factory.makeSpecification(
+            information_type=InformationType.PROPRIETARY)
+        today_milestone = self.factory.makeMilestone(
+            dateexpected=self.today, product=proprietary_spec.product)
+        public_workitem = self.factory.makeSpecificationWorkItem(
+            assignee=person, milestone=today_milestone)
+        proprietary_workitem = self.factory.makeSpecificationWorkItem(
+            assignee=person, milestone=today_milestone,
+            specification=proprietary_spec)
+        browser = self.getViewBrowser(
+            person, view_name='+upcomingwork')
+        self.assertIn(public_workitem.specification.name, browser.contents)
+        self.assertNotIn(proprietary_workitem.specification.name,
+                         browser.contents)
+        browser = self.getViewBrowser(
+            person, view_name='+upcomingwork',
+            user=proprietary_workitem.specification.product.owner)
+        self.assertIn(proprietary_workitem.specification.name,
+                      browser.contents)
+
 
 class TestPersonUpcomingWorkView(TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-10-03 07:25:14 +0000
+++ lib/lp/registry/model/person.py	2012-10-15 18:58:21 +0000
@@ -128,6 +128,7 @@
     SpecificationSort,
     )
 from lp.blueprints.model.specification import (
+    get_specification_privacy_filter,
     HasSpecificationsMixin,
     Specification,
     )
@@ -1462,7 +1463,7 @@
         return list(Store.of(self).find(
             TeamParticipation.personID, TeamParticipation.teamID == self.id))
 
-    def getAssignedSpecificationWorkItemsDueBefore(self, date):
+    def getAssignedSpecificationWorkItemsDueBefore(self, date, user):
         """See `IPerson`."""
         from lp.registry.model.person import Person
         from lp.registry.model.product import Product
@@ -1479,7 +1480,8 @@
                           Specification.milestoneID) == Milestone.id),
             ]
         today = datetime.today().date()
-        query = AND(
+        query = And(
+            get_specification_privacy_filter(user),
             Milestone.dateexpected <= date, Milestone.dateexpected >= today,
             WorkItem.deleted == False,
             OR(WorkItem.assignee_id.is_in(self.participant_ids),

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2012-09-28 06:15:58 +0000
+++ lib/lp/registry/tests/test_person.py	2012-10-15 18:58:21 +0000
@@ -1225,7 +1225,7 @@
             milestone=self.future_milestone)
 
         workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
-            self.current_milestone.dateexpected)
+            self.current_milestone.dateexpected, self.team)
 
         self.assertEqual([workitem], list(workitems))
 
@@ -1238,7 +1238,7 @@
             title=u'workitem', specification=assigned_spec, deleted=True)
 
         workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
-            self.current_milestone.dateexpected)
+            self.current_milestone.dateexpected, self.team)
         self.assertEqual([], list(workitems))
 
     def test_workitems_assigned_to_others_working_on_blueprint(self):
@@ -1258,7 +1258,7 @@
             assignee=self.factory.makePerson())
 
         workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
-            self.current_milestone.dateexpected)
+            self.current_milestone.dateexpected, self.team)
 
         self.assertContentEqual([workitem, workitem_for_other_person],
                                 list(workitems))
@@ -1273,7 +1273,8 @@
         self.factory.makeSpecificationWorkItem(
             title=u'workitem 1', specification=spec)
 
-        workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(today)
+        workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
+            today, self.team)
 
         self.assertEqual([], list(workitems))
 
@@ -1293,7 +1294,7 @@
             milestone=self.current_milestone)
 
         workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
-            self.current_milestone.dateexpected)
+            self.current_milestone.dateexpected, self.team)
 
         self.assertEqual([workitem], list(workitems))
 
@@ -1317,10 +1318,29 @@
             assignee=self.team.teamowner)
 
         workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
-            self.current_milestone.dateexpected)
+            self.current_milestone.dateexpected, self.team)
 
         self.assertEqual([workitem], list(workitems))
 
+    def test_listings_consider_spec_visibility(self):
+        # This spec is visible only to the product owner, even though it is
+        # assigned to self.team.teamowner.  Therefore, it is listed only for
+        # product.owner, not the team.
+        product = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY)
+        milestone = self.factory.makeMilestone(
+            dateexpected=self.current_milestone.dateexpected, product=product)
+        spec = self.factory.makeSpecification(
+            milestone=milestone, information_type=InformationType.PROPRIETARY)
+        workitem = self.factory.makeSpecificationWorkItem(
+            specification=spec, assignee=self.team.teamowner)
+        workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
+            milestone.dateexpected, self.team)
+        self.assertNotIn(workitem, workitems)
+        workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
+            milestone.dateexpected, product.owner)
+        self.assertIn(workitem, workitems)
+
     def _makeProductSpec(self, milestone_dateexpected):
         assignee = self.factory.makePerson()
         with person_logged_in(self.team.teamowner):
@@ -1362,7 +1382,7 @@
         with StormStatementRecorder() as recorder:
             workitems = list(
                 self.team.getAssignedSpecificationWorkItemsDueBefore(
-                    dateexpected))
+                    dateexpected, self.team))
             for workitem in workitems:
                 workitem.assignee
                 workitem.milestone

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-10-11 04:21:07 +0000
+++ lib/lp/testing/factory.py	2012-10-15 18:58:21 +0000
@@ -2082,6 +2082,9 @@
         """
         proprietary = (information_type not in PUBLIC_INFORMATION_TYPES and
             information_type is not None)
+        if (product is None and milestone is not None and
+            milestone.productseries is not None):
+            product = milestone.productseries.product
         if distribution is None and product is None:
             if proprietary:
                 specification_sharing_policy = (
@@ -2234,7 +2237,7 @@
 
     def makeCodeImport(self, svn_branch_url=None, cvs_root=None,
                        cvs_module=None, target=None, branch_name=None,
-                       git_repo_url=None, 
+                       git_repo_url=None,
                        bzr_branch_url=None, registrant=None,
                        rcs_type=None, review_status=None):
         """Create and return a new, arbitrary code import.