launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Richard Harding has proposed merging lp:~rharding/launchpad/remove_assigned_specs_1068817 into lp:launchpad with lp:~rharding/launchpad/fix_flash_1066898 as a prerequisite.

Commit message:
Remove person.assigned_specs due to limited use and to prevent possible leakage.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1068817 in Launchpad itself: "Person.assigned_specs is an attractive nuisance"
  https://bugs.launchpad.net/launchpad/+bug/1068817

For more details, see:
https://code.launchpad.net/~rharding/launchpad/remove_assigned_specs_1068817/+merge/131558

= Summary =

person.assigned_specs is a limited used property that can be a potential way
to shoot yourself in the foot since it doesn't deal with checking permissions
on items.

Its single use case is in tests and deactivating an account when you do in
fact want to make sure you catch all specifications that need to be
unassigned.

== Pre Implementation ==

Had a chat with Aaron around the reasoning for cleaning this up.

== Implementation Notes ==

We just replace the simple query directly into the deactivate account work.
It's doing a lot of work and this doesn't add much to it.

The larger update is to the tests since many of the doctests used this as a
shortcut to get to the list of assigned specs.


== Tests ==

registry/doc/person.txt
registry/doc/person-account.txt

-- 
https://code.launchpad.net/~rharding/launchpad/remove_assigned_specs_1068817/+merge/131558
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/remove_assigned_specs_1068817 into lp:launchpad.

=== modified file 'lib/lp/registry/doc/person-account.txt'
--- lib/lp/registry/doc/person-account.txt	2012-10-24 07:49:54 +0000
+++ lib/lp/registry/doc/person-account.txt	2012-10-26 09:02:00 +0000
@@ -12,6 +12,7 @@
 process. Matsubara's account was created during a code import.
 
     >>> from zope.component import getUtility
+    >>> from lp.blueprints.enums import SpecificationFilter
     >>> from lp.services.identity.interfaces.emailaddress import (
     ...     IEmailAddressSet,
     ...     )
@@ -100,7 +101,8 @@
     >>> foobar.searchTasks(params).count() > 0
     True
 
-    >>> len(foobar.assigned_specs) > 0
+    >>> foobar.specifications(
+    ...     foobar, filter=[SpecificationFilter.ASSIGNEE]).count() > 0
     True
 
     >>> foobar_pillars = []
@@ -173,7 +175,8 @@
 
 ...no assigned specs...
 
-    >>> len(foobar.assigned_specs)
+    >>> foobar.specifications(
+    ...     foobar, filter=[SpecificationFilter.ASSIGNEE]).count()
     0
 
 ...no owned teams...

=== modified file 'lib/lp/registry/doc/person.txt'
--- lib/lp/registry/doc/person.txt	2012-10-23 02:30:37 +0000
+++ lib/lp/registry/doc/person.txt	2012-10-26 09:02:00 +0000
@@ -1183,7 +1183,9 @@
 
 These 2 specifications are assigned to Carlos:
 
-    >>> for spec in carlos.assigned_specs:
+    >>> assigned_specs = carlos.specifications(
+    ...     carlos, filter=[SpecificationFilter.ASSIGNEE])
+    >>> for spec in assigned_specs:
     ...     print spec.name
     svg-support
     extension-manager-upgrades

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-10-24 23:45:42 +0000
+++ lib/lp/registry/interfaces/person.py	2012-10-26 09:02:00 +0000
@@ -848,8 +848,6 @@
         "Any specifications related to this person, either because the are "
         "a subscriber, or an assignee, or a drafter, or the creator. "
         "Sorted newest-first.")
-    assigned_specs = Attribute(
-        "Specifications assigned to this person, sorted newest first.")
     assigned_specs_in_progress = Attribute(
         "Specifications assigned to this person whose implementation is "
         "started but not yet completed, sorted newest first.")

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-10-24 23:45:42 +0000
+++ lib/lp/registry/model/person.py	2012-10-26 09:02:00 +0000
@@ -798,12 +798,6 @@
                 person=self, time_zone=time_zone, latitude=latitude,
                 longitude=longitude, last_modified_by=user)
 
-    # specification-related joins
-    @property
-    def assigned_specs(self):
-        return shortlist(Specification.selectBy(
-            assignee=self, orderBy=['-datecreated']))
-
     @property
     def assigned_specs_in_progress(self):
         replacements = sqlvalues(assignee=self)
@@ -2174,8 +2168,8 @@
         product_set = getUtility(IProductSet)
         non_public_products = product_set.get_users_private_products(self)
         if non_public_products.count() != 0:
-            errors.append(('This account cannot be deactivated because it owns '
-                        'the following non-public products: ') +
+            errors.append(('This account cannot be deactivated because it owns'
+                        ' the following non-public products: ') +
                         ','.join([p.name for p in non_public_products]))
 
         if self.account_status != AccountStatus.ACTIVE:
@@ -2225,8 +2219,13 @@
                "Bugtask %s assignee isn't the one expected: %s != %s" % (
                     bug_task.id, bug_task.assignee.name, self.name))
             bug_task.transitionToAssignee(None)
-        for spec in self.assigned_specs:
+
+        assigned_specs = self.specifications(
+            self,
+            filter=[SpecificationFilter.ASSIGNEE])
+        for spec in assigned_specs:
             spec.assignee = None
+
         registry_experts = getUtility(ILaunchpadCelebrities).registry_experts
         for team in Person.selectBy(teamowner=self):
             team.teamowner = registry_experts