launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14747
[Merge] lp:~adeuring/launchpad/bug-1086876 into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-1086876 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1086876 in Launchpad itself: "The assignee, approver, drafter of a private blueprint should automatically be subscribed to the blueprint so that they have an access grant"
https://bugs.launchpad.net/launchpad/+bug/1086876
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-1086876/+merge/138514
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
--
https://code.launchpad.net/~adeuring/launchpad/bug-1086876/+merge/138514
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-1086876 into lp:launchpad.
=== 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-06 17:11: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-06 17:11: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-06 17:11: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-06 17:11: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-06 17:11: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-06 17:11: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-06 17:11: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(
=== 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-06 17:11: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-06 17:11: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):
Follow ups
