Merge lp:~adeuring/launchpad/bug-1086876 into lp:launchpad

[Abel Deuring]

subscribe the assignee, drafter, approver of a non-public blueprint if they do not have yet a grant to view the blueprint.

The fix is simple: I renamed the ForeignKey attributes assignee/drafter/
approver of the class Specification to _assignee/_drafter/_approver and
defined assignee/drafter/approver as computed properties. When the
attributes are set to a new value, the new "role owner" are subscribed, if
necessary.

Running "/bin/test blueprints -vv" revealed a number of locations where
assignee/drafter/approver are used to build SQL queries; I changed this
to _assignee etc there too.

The change in lib/lp/blueprints/browser/sprint.py does not result in
additional SQL queries to load the assignee/drafter because the specs
were loaded via self.context.specifications (line 466), and the methods
HasSpecificationsMixin.specifications() already pre-load the Person
objects.

"/bin/test blueprints -vv" also revealed a completely unrelated problem
in some tests; I filed bug 1087314 and added an XXX to
lib/lp/blueprints/browser/specification.py.

tests: ./bin/test blueprints -vvt test_setting_.*_subscribes

no lint

[j.c.sackett (jcsackett)]

Thanks.

review: Approve

=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py	2012-11-30 21:26:50 +0000
+++ lib/lp/blueprints/browser/specification.py	2012-12-07 10:07:22 +0000
@@ -1440,6 +1440,9 @@
         process.stdin.close()
         output = process.stdout.read()
         err = process.stderr.read()
+        # XXX Abel Deuring 2012-12-06, bug 1087314
+        # err may just contain a warning, while the image might be rendered
+        # correctly. We should not raise an error in this case.
         if err:
             raise ProblemRenderingGraph(err, output)
         return output

=== modified file 'lib/lp/blueprints/browser/sprint.py'
--- lib/lp/blueprints/browser/sprint.py	2012-09-26 20:56:43 +0000
+++ lib/lp/blueprints/browser/sprint.py	2012-12-07 10:07:22 +0000
@@ -492,12 +492,12 @@
         for spec in model_specs:
             # Get the list of attendees that will attend the sprint.
             spec_people = people[spec.id]
-            if spec.assigneeID is not None:
-                spec_people[spec.assigneeID] = True
-                attendee_set.add(spec.assigneeID)
-            if spec.drafterID is not None:
-                spec_people[spec.drafterID] = True
-                attendee_set.add(spec.drafterID)
+            if spec.assignee is not None:
+                spec_people[spec.assignee.id] = True
+                attendee_set.add(spec.assignee.id)
+            if spec.drafter is not None:
+                spec_people[spec.drafter.id] = True
+                attendee_set.add(spec.drafter.id)
         people_by_id = dict((person.id, person) for person in
             getUtility(IPersonSet).getPrecachedPersonsFromIDs(attendee_set))
         self.specifications = [

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-12-01 15:56:06 +0000
+++ lib/lp/blueprints/model/specification.py	2012-12-07 10:07:22 +0000
@@ -182,13 +182,13 @@
         default=SpecificationDefinitionStatus.NEW)
     priority = EnumCol(schema=SpecificationPriority, notNull=True,
         default=SpecificationPriority.UNDEFINED)
-    assignee = ForeignKey(dbName='assignee', notNull=False,
-        foreignKey='Person',
-        storm_validator=validate_public_person, default=None)
-    drafter = ForeignKey(dbName='drafter', notNull=False,
-        foreignKey='Person',
-        storm_validator=validate_public_person, default=None)
-    approver = ForeignKey(dbName='approver', notNull=False,
+    _assignee = ForeignKey(dbName='assignee', notNull=False,
+        foreignKey='Person',
+        storm_validator=validate_public_person, default=None)
+    _drafter = ForeignKey(dbName='drafter', notNull=False,
+        foreignKey='Person',
+        storm_validator=validate_public_person, default=None)
+    _approver = ForeignKey(dbName='approver', notNull=False,
         foreignKey='Person',
         storm_validator=validate_public_person, default=None)
     owner = ForeignKey(
@@ -265,6 +265,42 @@
     information_type = EnumCol(
         enum=InformationType, notNull=True, default=InformationType.PUBLIC)
 
+    def set_assignee(self, person):
+        self.subscribeIfAccessGrantNeeded(person)
+        self._assignee = person
+
+    def get_assignee(self):
+        return self._assignee
+
+    assignee = property(get_assignee, set_assignee)
+
+    def set_drafter(self, person):
+        self.subscribeIfAccessGrantNeeded(person)
+        self._drafter = person
+
+    def get_drafter(self):
+        return self._drafter
+
+    drafter = property(get_drafter, set_drafter)
+
+    def set_approver(self, person):
+        self.subscribeIfAccessGrantNeeded(person)
+        self._approver = person
+
+    def get_approver(self):
+        return self._approver
+
+    approver = property(get_approver, set_approver)
+
+    def subscribeIfAccessGrantNeeded(self, person):
+        """Subscribe person if this specification is not public and if
+        the person does not already have grants to access the specification.
+        """
+        if person is None or self.userCanView(person):
+            return
+        current_user = getUtility(ILaunchBag).user
+        self.subscribe(person, subscribed_by=current_user)
+
     @cachedproperty
     def subscriptions(self):
         """Sort the subscriptions"""
@@ -1027,14 +1063,16 @@
             clauses = [SQL(clauses)]
 
         def cache_people(rows):
-            """DecoratedResultSet pre_iter_hook to eager load Person attributes."""
+            """DecoratedResultSet pre_iter_hook to eager load Person
+             attributes.
+            """
             from lp.registry.model.person import Person
             # Find the people we need:
             person_ids = set()
             for spec in rows:
-                person_ids.add(spec.assigneeID)
-                person_ids.add(spec.approverID)
-                person_ids.add(spec.drafterID)
+                person_ids.add(spec._assigneeID)
+                person_ids.add(spec._approverID)
+                person_ids.add(spec._drafterID)
             person_ids.discard(None)
             if not person_ids:
                 return
@@ -1057,7 +1095,8 @@
                     index += 1
                     decorator(person, column)
 
-        results = IStore(Specification).using(*tables).find(Specification, *clauses)
+        results = IStore(Specification).using(*tables).find(
+            Specification, *clauses)
         return DecoratedResultSet(results, pre_iter_hook=cache_people)
 
     @property
@@ -1145,7 +1184,8 @@
                 order = [Desc(Specification.datecreated), Specification.id]
 
         if prejoin_people:
-            results = self._preload_specifications_people(privacy_tables, clauses)
+            results = self._preload_specifications_people(
+                privacy_tables, clauses)
         else:
             results = store.using(*privacy_tables).find(
                 Specification, *clauses)
@@ -1190,8 +1230,8 @@
         spec = Specification(name=name, title=title, specurl=specurl,
             summary=summary, priority=priority,
             definition_status=definition_status, owner=owner,
-            approver=approver, product=product, distribution=distribution,
-            assignee=assignee, drafter=drafter, whiteboard=whiteboard)
+            _approver=approver, product=product, distribution=distribution,
+            _assignee=assignee, _drafter=drafter, whiteboard=whiteboard)
         spec.transitionToInformationType(information_type, None)
         return spec
 

=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py	2012-11-29 15:34:18 +0000
+++ lib/lp/blueprints/tests/test_specification.py	2012-12-07 10:07:22 +0000
@@ -742,3 +742,52 @@
         self.assertIn(
             blueprint1,
             list(context.specifications(user=grant.grantee)))
+
+    def run_test_setting_special_role_subscribes(self, role_name):
+        # If a user becomes the assignee, drafter or approver of a
+        # proprietary specification, they are automatically subscribed,
+        # if they do not have yet been granted access to the specification.
+        specification_sharing_policy = (
+                SpecificationSharingPolicy.PROPRIETARY_OR_PUBLIC)
+        product = self.factory.makeProduct(
+            specification_sharing_policy=specification_sharing_policy)
+        blueprint = self.makeSpec(
+            product=product, information_type=InformationType.PROPRIETARY)
+        person_with_new_role = self.factory.makePerson()
+        with person_logged_in(product.owner):
+            setattr(blueprint, role_name, person_with_new_role)
+            self.assertIsNot(
+                None, blueprint.subscription(person_with_new_role))
+
+        # Assignees/drafters/approvers are not subscribed if they already
+        # have a policy grant for the specification's target.
+        blueprint_2 = self.makeSpec(
+            product=product, information_type=InformationType.PROPRIETARY)
+        person_with_new_role_2 = self.factory.makePerson()
+        with person_logged_in(product.owner):
+            permissions = {
+                InformationType.PROPRIETARY: SharingPermission.ALL,
+                }
+            getUtility(IService, 'sharing').sharePillarInformation(
+                product, person_with_new_role_2, product.owner, permissions)
+            setattr(blueprint_2, role_name, person_with_new_role_2)
+            self.assertIs(
+                None, blueprint.subscription(person_with_new_role_2))
+
+    def test_setting_assignee_subscribes(self):
+        # If a user becomes the assignee of a proprietary specification,
+        # they are automatically subscribed, if they do not have yet
+        # been granted access to the specification.
+        self.run_test_setting_special_role_subscribes('assignee')
+
+    def test_setting_drafter_subscribes(self):
+        # If a user becomes the drafter of a proprietary specification,
+        # they are automatically subscribed, if they do not have yet
+        # been granted access to the specification.
+        self.run_test_setting_special_role_subscribes('drafter')
+
+    def test_setting_approver_subscribes(self):
+        # If a user becomes the approver of a proprietary specification,
+        # they are automatically subscribed, if they do not have yet
+        # been granted access to the specification.
+        self.run_test_setting_special_role_subscribes('approver')

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2012-10-03 08:41:47 +0000
+++ lib/lp/registry/model/distroseries.py	2012-12-07 10:07:22 +0000
@@ -888,7 +888,7 @@
 
         results = Specification.select(query, orderBy=order, limit=quantity)
         if prejoin_people:
-            results = results.prejoin(['assignee', 'approver', 'drafter'])
+            results = results.prejoin(['_assignee', '_approver', '_drafter'])
         return results
 
     def getDistroSeriesLanguage(self, language):

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2012-11-26 08:33:03 +0000
+++ lib/lp/registry/model/milestone.py	2012-12-07 10:07:22 +0000
@@ -157,7 +157,7 @@
         store = Store.of(self.target)
         origin, clauses = visible_specification_query(user)
         origin.extend([
-            LeftJoin(Person, Specification.assigneeID == Person.id),
+            LeftJoin(Person, Specification._assigneeID == Person.id),
             ])
         milestones = self._milestone_ids_expr(user)
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-11-28 05:21:33 +0000
+++ lib/lp/registry/model/person.py	2012-12-07 10:07:22 +0000
@@ -871,11 +871,11 @@
         if SpecificationFilter.CREATOR in filter:
             role_clauses.append(Specification.owner == self)
         if SpecificationFilter.ASSIGNEE in filter:
-            role_clauses.append(Specification.assignee == self)
+            role_clauses.append(Specification._assignee == self)
         if SpecificationFilter.DRAFTER in filter:
-            role_clauses.append(Specification.drafter == self)
+            role_clauses.append(Specification._drafter == self)
         if SpecificationFilter.APPROVER in filter:
-            role_clauses.append(Specification.approver == self)
+            role_clauses.append(Specification._approver == self)
         if SpecificationFilter.SUBSCRIBER in filter:
             role_clauses.append(
                 Specification.id.is_in(
@@ -1499,7 +1499,7 @@
             Milestone.dateexpected <= date, Milestone.dateexpected >= today,
             WorkItem.deleted == False,
             OR(WorkItem.assignee_id.is_in(self.participant_ids),
-               Specification.assigneeID.is_in(self.participant_ids))])
+               Specification._assigneeID.is_in(self.participant_ids))])
         result = store.using(*origin).find(WorkItem, *query)
         result.config(distinct=True)
 
@@ -1510,7 +1510,7 @@
             bulk.load_related(Distribution, specs, ['distributionID'])
             assignee_ids = set(
                 [workitem.assignee_id for workitem in workitems]
-                + [spec.assigneeID for spec in specs])
+                + [spec._assigneeID for spec in specs])
             assignee_ids.discard(None)
             bulk.load(Person, assignee_ids, store)
             milestone_ids = set(
@@ -2249,7 +2249,7 @@
             bug_task.transitionToAssignee(None)
 
         assigned_specs = Store.of(self).find(
-            Specification, assignee=self)
+            Specification, _assignee=self)
         for spec in assigned_specs:
             spec.assignee = None
 

=== modified file 'lib/lp/registry/model/productseries.py'
--- lib/lp/registry/model/productseries.py	2012-11-30 20:52:15 +0000
+++ lib/lp/registry/model/productseries.py	2012-12-07 10:07:22 +0000
@@ -443,7 +443,7 @@
 
         results = Specification.select(query, orderBy=order, limit=quantity)
         if prejoin_people:
-            results = results.prejoin(['assignee', 'approver', 'drafter'])
+            results = results.prejoin(['_assignee', '_approver', '_drafter'])
         return results
 
     def _customizeSearchParams(self, search_params):

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2012-11-20 20:52:40 +0000
+++ lib/lp/registry/model/projectgroup.py	2012-12-07 10:07:22 +0000
@@ -314,7 +314,7 @@
         results = Specification.select(query, orderBy=order, limit=quantity,
             clauseTables=clause_tables)
         if prejoin_people:
-            results = results.prejoin(['assignee', 'approver', 'drafter'])
+            results = results.prejoin(['_assignee', '_approver', '_drafter'])
         return results
 
     def _customizeSearchParams(self, search_params):