Merge lp:~abentley/launchpad/person-assigned-specs-in-progress into lp:launchpad

[Aaron Bentley]

Fix +portlet-currentfocus with proprietary specs.

= Summary =
Fix bug #1068719: Person overview page breaks when assigned proprietary blueprints

== Proposed fix ==
Enhance Person.specifications, rewrite assigned_specs_in_progress as findVisibleAssignedInProgressSpecs, extract core functionality to Person.specifications.

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of Private Projects.

== Implementation details ==
Since Person.specifications already handles privacy and is used for many searches, it makes sense to extend it to handle searches for in-progress specs.

The results should be sorted according to Specification.date_started, which is not included in SpecificationSort. Person.specification's sort parameter is extended to support Storm expressions, rather than adding a new sort to SpecificationSort, because this is more direct.

in_progress is implemented as a boolean rather than extending SpecificationFilter because this is more direct.

== Tests ==
bin/test -t in_progress -t test_assigned_blueprints.

== Demo and Q/A ==
Create a proprietary specification with status STARTED, and assign it to a Person. You should see it listed on their overview page.

Log in as an unprivileged user and look at the same person's overview page. You should not see it listed.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/testing/factory.py
  lib/lp/blueprints/model/specification.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/browser/tests/test_person.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py

[Steve Kowalik (stevenk)]

The docstring for IPerson.findVisibleAssignedInProgressSpecs() makes no mention that it will only return five specs. I'd prefer the model function also includes the usual "See `Interface'." docstring.

This also means this code will now rely on get_specification_privacy_filter rather than OOPSing. Due to the query that is run by that function, I suspect we are now trading OOPSes for timeouts. I'd *really* prefer the access mechanisms for Specifications are de-normalized like they are for bugs and branches. I'd like to see some investigation of the query that get_specification_privacy_filter runs before I approve this branch.

review: Needs Fixing (code)

[Deryck Hodge (deryck)]

There is no observable difference in the two queries.

SELECT * from Specification WHERE
    Specification.owner = 2
    OR Specification.assignee = 2
    OR Specification.drafter = 2
    OR Specification.approver = 2
    OR Specification.id IN
    (
        SELECT SpecificationSubscription.specification
            FROM SpecificationSubscription
            WHERE SpecificationSubscription.person = 2
    )
    AND (
        NOT (Specification.implementation_status IN (0, 5, 90, 95))
        OR Specification.implementation_status = 60
        AND Specification.definition_status = 10
    ) AND (
        Specification.product IS NULL
        OR NOT Specification.product IN (
            SELECT Product.id FROM Product WHERE Product.active = true)
    ) AND (
        NOT (Specification.implementation_status = 0
        OR Specification.definition_status IN (90, 95)
        OR Specification.implementation_status = 60
        AND Specification.definition_status = 10)
    );

Total runtime: 155.522 ms

SELECT * from Specification WHERE
    Specification.owner = 2
    OR Specification.assignee = 2
    OR Specification.drafter = 2
    OR Specification.approver = 2
    OR Specification.id IN
    (
        SELECT SpecificationSubscription.specification
            FROM SpecificationSubscription
            WHERE SpecificationSubscription.person = 2
    )
    AND (
        Specification.information_type IN (NULL, 1)
        OR Specification.id IN (
            SELECT Specification.id FROM Specification
                JOIN AccessPolicy ON (
                    Specification.product = AccessPolicy.product
                    OR Specification.distribution = AccessPolicy.distribution
                )
                AND Specification.information_type = AccessPolicy.type
                    JOIN AccessPolicyGrantFlat ON
                    AccessPolicy.id = AccessPolicyGrantFlat.policy
                    LEFT JOIN AccessArtifact
                        ON AccessPolicyGrantFlat.artifact = AccessArtifact.id
                    JOIN TeamParticipation
                        ON TeamParticipation.team = AccessPolicyGrantFlat.grantee
                    AND TeamParticipation.person = 2
                    WHERE AccessPolicyGrantFlat.artifact IS NULL
                    OR AccessArtifact.specification = Specification.id
        )
    )
    AND (
        NOT (Specification.implementation_status IN (0, 5, 90, 95))
        OR Specification.implementation_status = 60
        AND Specification.definition_status = 10
    ) AND (
        Specification.product IS NULL
        OR NOT Specification.product IN (
            SELECT Product.id FROM Product WHERE Product.active = true)
    ) AND (
        NOT (Specification.implementation_status = 0
        OR Specification.definition_status IN (90, 95)
        OR Specification.implementation_status = 60
        AND Specification.definition_status = 10)
    );

Total runtime: 155.263 ms

[Deryck Hodge (deryck)]

As for the larger question of denormalized tables, we looked at the work people did and see it's great work, and discussed specifically with lifeless that we might not need them in the case of blueprints because the data is so much smaller than everything else. Regardless of whether we need them or not, we'll keep a close eye on timeouts during beta and add any performance-related changes we need to stay in good shape, denormalized tables or otherwise.

[Deryck Hodge (deryck)]

I wrote "the work people did" above and meant "the work Purple did." Sorry. Was typing quickly during the sprint here.

[Steve Kowalik (stevenk)]

Firstly, the thing I'm arguing for is two denormalized *columns* on Specification which means you don't need to JOIN against APG and APGF. However, I'm not going to block this MP with this particular point.

    AND (
        Specification.information_type IN (NULL, 1)

This, however, I will block on. You can't use NULL inside an IN, it will compile as SQL, but you are likely getting results you don't expect.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12-10-25 12:29 AM, Steve Kowalik wrote:
> AND ( Specification.information_type IN (NULL, 1)
>
> This, however, I will block on. You can't use NULL inside an IN, it
> will compile as SQL, but youI are likely getting results you don't
> expect.

That SQL is not the actual SQL. We called storm.expr.compile to get
the expression, but this left us with a bunch of ? to fill in, and we
filled that part in incorrectly. The actual values that Launchpad
uses are: (1,2) i.e. PUBLIC and PUBLICSECURITY.

If there is a bug in get_specification_privacy filters, that is not a
bug in my code, so please do not block my branch on that basis.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlCI6AsACgkQ0F+nu1YWqI2DUgCfbbumkODtgiAf6NzVz5UtC43o
xbsAniDAaA78ZYXAlhpFT2vN6D9mKJwc
=fLt9
-----END PGP SIGNATURE-----

[Deryck Hodge (deryck)]

Steve, we really need to get this landed, so I'm going to mark this approved for Aaron, since I read your comments to say you don't have any other objections. I'm basing my reading of this on the fact that you wouldn't block on the queries, and the NULL thing was just our mistake in data substitution.

If there are other concerns, please reach out the either me or Aaron and we can sort them out in another branch.

review: Approve

[Steve Kowalik (stevenk)]

review: Approve (code)

=== modified file 'lib/lp/blueprints/enums.py'
--- lib/lp/blueprints/enums.py	2012-05-17 07:46:56 +0000
+++ lib/lp/blueprints/enums.py	2012-10-26 09:03:23 +0000
@@ -36,10 +36,10 @@
     feature.
 
     Note that some of the states associated with this schema correlate
-    to a "not started" definition. See Specification.started_clause for
-    further information, and make sure that it is updated (together with
-    the relevant database checks) if additional states are added that
-    are also "not started".
+    to a "not started" definition. See spec_started_clause for further
+    information, and make sure that it is updated (together with the relevant
+    database checks) if additional states are added that are also "not
+    started".
     """
     # The `UNKNOWN` state is considered "not started"
     UNKNOWN = DBItem(0, """

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-10-17 14:37:03 +0000
+++ lib/lp/blueprints/model/specification.py	2012-10-26 09:03:23 +0000
@@ -14,6 +14,7 @@
     'SPECIFICATION_POLICY_ALLOWED_TYPES',
     'SPECIFICATION_POLICY_DEFAULT_TYPES',
     'SpecificationSet',
+    'spec_started_clause',
     ]
 
 from lazr.lifecycle.event import (
@@ -576,28 +577,10 @@
         else:
             return False
 
-    # NB NB If you change this definition, please update the equivalent
-    # DB constraint Specification.specification_start_recorded_chk
-    # We choose to define "started" as the set of delivery states NOT
-    # in the values we select. Another option would be to say "anything less
-    # than a threshold" and to comment the dbschema that "anything not
-    # started should be less than the threshold". We'll see how maintainable
-    # this is.
-    started_clause = """
-        Specification.implementation_status NOT IN (%s, %s, %s, %s) OR
-        (Specification.implementation_status = %s AND
-         Specification.definition_status = %s)
-        """ % sqlvalues(SpecificationImplementationStatus.UNKNOWN.value,
-                        SpecificationImplementationStatus.NOTSTARTED.value,
-                        SpecificationImplementationStatus.DEFERRED.value,
-                        SpecificationImplementationStatus.INFORMATIONAL.value,
-                        SpecificationImplementationStatus.INFORMATIONAL.value,
-                        SpecificationDefinitionStatus.APPROVED.value)
-
     @property
     def is_started(self):
         """See ISpecification. This is a code implementation of the
-        SQL in self.started_clause
+        SQL in spec_started_clause
         """
         return (self.implementation_status not in [
                     SpecificationImplementationStatus.UNKNOWN,
@@ -1334,3 +1317,23 @@
             # A string in the filter is a text search filter.
             clauses.append(fti_search(Specification, constraint))
     return clauses
+
+
+# NB NB If you change this definition, please update the equivalent
+# DB constraint Specification.specification_start_recorded_chk
+# We choose to define "started" as the set of delivery states NOT
+# in the values we select. Another option would be to say "anything less
+# than a threshold" and to comment the dbschema that "anything not
+# started should be less than the threshold". We'll see how maintainable
+# this is.
+spec_started_clause = Or(Not(Specification.implementation_status.is_in([
+    SpecificationImplementationStatus.UNKNOWN,
+    SpecificationImplementationStatus.NOTSTARTED,
+    SpecificationImplementationStatus.DEFERRED,
+    SpecificationImplementationStatus.INFORMATIONAL,
+    ])),
+    And(Specification.implementation_status ==
+            SpecificationImplementationStatus.INFORMATIONAL,
+        Specification.definition_status ==
+            SpecificationDefinitionStatus.APPROVED
+    ))

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-10-24 23:45:42 +0000
+++ lib/lp/registry/browser/person.py	2012-10-26 09:03:23 +0000
@@ -1718,7 +1718,8 @@
     @cachedproperty
     def assigned_specs_in_progress(self):
         """Return up to 5 assigned specs that are being worked on."""
-        return list(self.context.assigned_specs_in_progress)
+        specs = self.context.findVisibleAssignedInProgressSpecs(self.user)
+        return list(specs)
 
     @property
     def has_assigned_bugs_or_specs_in_progress(self):

=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2012-10-24 23:40:33 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2012-10-26 09:03:23 +0000
@@ -21,7 +21,9 @@
 
 from lp.app.browser.lazrjs import TextAreaEditorWidget
 from lp.app.errors import NotFoundError
+from lp.app.enums import InformationType
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.blueprints.enums import SpecificationImplementationStatus
 from lp.buildmaster.enums import BuildStatus
 from lp.registry.browser.person import PersonView
 from lp.registry.browser.team import TeamInvitationView
@@ -115,7 +117,7 @@
             'http://prod.launchpad.dev/~eris', self.view.openid_identity_url)
 
 
-class TestPersonIndexView(TestCaseWithFactory):
+class TestPersonIndexView(BrowserTestCase):
 
     layer = DatabaseFunctionalLayer
 
@@ -203,6 +205,21 @@
         view = create_initialized_view(person, '+index')
         self.assertThat(view.page_description, Equals(person_description))
 
+    def test_assigned_blueprints(self):
+        person = self.factory.makePerson()
+
+        def make_started_spec(information_type):
+            enum = SpecificationImplementationStatus
+            return self.factory.makeSpecification(
+                implementation_status=enum.STARTED, assignee=person,
+                information_type=information_type)
+        public_spec = make_started_spec(InformationType.PUBLIC)
+        private_spec = make_started_spec(InformationType.PROPRIETARY)
+        with person_logged_in(None):
+            browser = self.getViewBrowser(person)
+        self.assertIn(public_spec.name, browser.contents)
+        self.assertNotIn(private_spec.name, browser.contents)
+
 
 class TestPersonViewKarma(TestCaseWithFactory):
 

=== 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:03:23 +0000
@@ -1191,7 +1191,7 @@
 But from these two, only one has started.
 
     >>> [(spec.name, spec.is_started)
-    ...  for spec in carlos.assigned_specs_in_progress]
+    ...  for spec in carlos.findVisibleAssignedInProgressSpecs(None)]
     [(u'svg-support', True)]
 
 Just for fun, lets check the SAB. He should have one spec for which he

=== 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:03:23 +0000
@@ -850,9 +850,17 @@
         "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.")
+
+    def findVisibleAssignedInProgressSpecs(user):
+        """List specifications in progress assigned to this person.
+
+        In progress means their implementation is started but not yet
+        completed.  They are sorted newest first.  No more than 5
+        specifications are returned.
+
+        :param user: The use to use for determining visibility.
+        """
+
     teamowner = exported(
         PublicPersonChoice(
             title=_('Team Owner'), required=False, readonly=False,

=== 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:03:23 +0000
@@ -133,6 +133,7 @@
     get_specification_privacy_filter,
     HasSpecificationsMixin,
     Specification,
+    spec_started_clause,
     )
 from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
 from lp.bugs.interfaces.bugtarget import IBugTarget
@@ -804,17 +805,10 @@
         return shortlist(Specification.selectBy(
             assignee=self, orderBy=['-datecreated']))
 
-    @property
-    def assigned_specs_in_progress(self):
-        replacements = sqlvalues(assignee=self)
-        replacements['started_clause'] = Specification.started_clause
-        replacements['completed_clause'] = Specification.completeness_clause
-        query = """
-            (assignee = %(assignee)s)
-            AND (%(started_clause)s)
-            AND NOT (%(completed_clause)s)
-            """ % replacements
-        return Specification.select(query, orderBy=['-date_started'], limit=5)
+    def findVisibleAssignedInProgressSpecs(self, user):
+        """See `IPerson`."""
+        return self.specifications(user, in_progress=True, quantity=5,
+                                   sort=Desc(Specification.date_started))
 
     @property
     def unique_displayname(self):
@@ -822,7 +816,7 @@
         return "%s (%s)" % (self.displayname, self.name)
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=True):
+                       prejoin_people=True, in_progress=False):
         """See `IHasSpecifications`."""
         from lp.blueprints.model.specificationsubscription import (
             SpecificationSubscription,
@@ -836,11 +830,6 @@
 
         # Now look at the filter and fill in the unsaid bits.
 
-        # Defaults for completeness: if nothing is said about completeness
-        # then we want to show INCOMPLETE.
-        if SpecificationFilter.COMPLETE not in filter:
-            filter.add(SpecificationFilter.INCOMPLETE)
-
         # Defaults for acceptance: in this case we have nothing to do
         # because specs are not accepted/declined against a person.
 
@@ -872,12 +861,24 @@
                         [SpecificationSubscription.person == self]
                     )))
         clauses = [Or(*role_clauses), get_specification_privacy_filter(user)]
+        # Defaults for completeness: if nothing is said about completeness
+        # then we want to show INCOMPLETE.
+        if SpecificationFilter.COMPLETE not in filter:
+            if (in_progress and SpecificationFilter.INCOMPLETE not in filter
+                and SpecificationFilter.ALL not in filter):
+                clauses.append(spec_started_clause)
+            filter.add(SpecificationFilter.INCOMPLETE)
+
         clauses.extend(get_specification_filters(filter))
         results = Store.of(self).find(Specification, *clauses)
         # The default sort is priority descending, so only explictly sort for
         # DATE.
         if sort == SpecificationSort.DATE:
-            results = results.order_by(Desc(Specification.datecreated))
+            sort = Desc(Specification.datecreated)
+        elif getattr(sort, 'enum', None) is SpecificationSort:
+            sort = None
+        if sort is not None:
+            results = results.order_by(sort)
         if quantity is not None:
             results = results[:quantity]
         return results

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2012-10-19 18:26:01 +0000
+++ lib/lp/registry/tests/test_person.py	2012-10-26 09:03:23 +0000
@@ -12,6 +12,7 @@
 from lazr.restful.utils import smartquote
 import pytz
 from storm.store import Store
+from storm.locals import Desc
 from testtools.matchers import (
     Equals,
     LessThan,
@@ -1907,3 +1908,59 @@
         self.assertEqual(
             [blueprint1],
             list_result(blueprint1.owner, user=grant.grantee))
+
+    def test_storm_sort(self):
+        # A Storm expression can be used to sort specs.
+        owner = self.factory.makePerson()
+        spec = self.factory.makeSpecification(owner=owner, name='a')
+        spec2 = self.factory.makeSpecification(owner=owner, name='z')
+        spec3 = self.factory.makeSpecification(owner=owner, name='b')
+        self.assertEqual([spec2, spec3, spec],
+                list(owner.specifications(owner,
+                     sort=Desc(Specification.name))))
+
+    def test_in_progress(self):
+        # In-progress filters to exclude not-started and completed.
+        enum = SpecificationImplementationStatus
+        notstarted = self.factory.makeSpecification(
+            implementation_status=enum.NOTSTARTED)
+        owner = notstarted.owner
+        started = self.factory.makeSpecification(
+            owner=owner, implementation_status=enum.STARTED)
+        self.factory.makeSpecification(
+            owner=owner, implementation_status=enum.IMPLEMENTED)
+        specs = list(owner.specifications(owner, in_progress=True))
+        self.assertEqual([started], specs)
+
+    def test_in_progress_all(self):
+        # SpecificationFilter.ALL overrides in_progress.
+        enum = SpecificationImplementationStatus
+        notstarted = self.factory.makeSpecification(
+            implementation_status=enum.NOTSTARTED)
+        owner = notstarted.owner
+        specs = list(owner.specifications(
+            owner, filter=[SpecificationFilter.ALL], in_progress=True))
+        self.assertEqual([notstarted], specs)
+
+    def test_complete_overrides_in_progress(self):
+        # SpecificationFilter.COMPLETE overrides in_progress.
+        enum = SpecificationImplementationStatus
+        started = self.factory.makeSpecification(
+            implementation_status=enum.STARTED)
+        owner = started.owner
+        implemented = self.factory.makeSpecification(
+            implementation_status=enum.IMPLEMENTED, owner=owner)
+        specs = list(owner.specifications(
+            owner, filter=[SpecificationFilter.COMPLETE], in_progress=True))
+        self.assertEqual([implemented], specs)
+
+    def test_incomplete_overrides_in_progress(self):
+        # SpecificationFilter.INCOMPLETE overrides in_progress.
+        enum = SpecificationImplementationStatus
+        notstarted = self.factory.makeSpecification(
+            implementation_status=enum.NOTSTARTED)
+        owner = notstarted.owner
+        specs = list(owner.specifications(
+            owner, filter=[SpecificationFilter.INCOMPLETE],
+            in_progress=True))
+        self.assertEqual([notstarted], specs)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-10-22 02:30:44 +0000
+++ lib/lp/testing/factory.py	2012-10-26 09:03:23 +0000
@@ -2124,11 +2124,6 @@
             distribution=distribution,
             priority=priority)
         naked_spec = removeSecurityProxy(spec)
-        if information_type is not None:
-            if proprietary:
-                naked_spec.target._ensurePolicies([information_type])
-            naked_spec.transitionToInformationType(
-                information_type, removeSecurityProxy(spec.target).owner)
         if status.name not in status_names:
             # Set the closed status after the status has a sane initial state.
             naked_spec.definition_status = status
@@ -2144,6 +2139,11 @@
         if implementation_status is not None:
             naked_spec.implementation_status = implementation_status
             naked_spec.updateLifecycleStatus(owner)
+        if information_type is not None:
+            if proprietary:
+                naked_spec.target._ensurePolicies([information_type])
+            naked_spec.transitionToInformationType(
+                information_type, removeSecurityProxy(spec.target).owner)
         return spec
 
     makeBlueprint = makeSpecification