Merge lp:~abentley/launchpad/storm-sprint-queries into lp:launchpad

[Aaron Bentley]

Convert Sprint.specifications to use storm expressions.

= Summary =
Update Sprint.specifications to use storm expressions

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of private projects

== Implementation details ==
Convert Sprint.specifications to use storm expressions, so that we can add privacy checks. Also, remove support for prejoin_people. This is justifiable because with the new code, the Sprint index page actually emits fewer queries. Since sorting by priority is the default, it is not explicitly implemented.

Reduce the signature of specificationLinks to those values that are actually used.

Tweak specificationtarget-assignments.pt, because it relied on the old ResultSet evaluating False when it was empty.

Implement stormexpr.fti_search because it was needed to emit a Storm expression.

== Tests ==
bin/test test_sprint

== Demo and Q/A ==
The sprint pages should work, and should list relevant blueprints.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/database/stormexpr.py
  lib/lp/blueprints/templates/specificationtarget-assignments.pt
  lib/lp/blueprints/model/tests/test_sprint.py
  lib/lp/blueprints/model/specification.py
  lib/lp/blueprints/model/sprint.py
  lib/lp/testing/_webservice.py
  lib/lp/blueprints/browser/tests/test_sprint.py

[j.c.sackett (jcsackett)]

This looks alright, but I'm not sure I see the value in the completeness expression being a class method on the Specification class--it seems like something that could just be defined as a clause separately in that code rather than a class method, and then used as needed. Is there some other reason for this implementation?

Also: I concur with the lack of a need for an explicit implementation for Priority, but that should probably be added in a comment in the relevant block of code.

review: Needs Information

[Aaron Bentley (abentley)]

> This looks alright, but I'm not sure I see the value in the completeness
> expression being a class method on the Specification class--it seems like
> something that could just be defined as a clause separately in that code

Which code? Sprint.specification_filter? It should not be defined there, because it is more generally useful. We will need it when we convert other implementations of IHasSpecifications.specifications to Storm.

> rather than a class method, and then used as needed. Is there some other
> reason for this implementation?

I thought it was valuable to have it directly underneath the non-Storm version, Specification.completeness.

> Also: I concur with the lack of a need for an explicit implementation for
> Priority, but that should probably be added in a comment in the relevant block
> of code.

There is a comment:
212 + # fall back to default, which is priority, descending.

Does it need improvement?

[j.c.sackett (jcsackett)]

Ah, I missed that there was a precedent for the storm_completeness class method. Given that, I concur with your reasoning.

The comment is fine--I must have just missed it when reading through.

review: Approve

=== modified file 'lib/lp/blueprints/browser/tests/test_sprint.py'
--- lib/lp/blueprints/browser/tests/test_sprint.py	2012-09-27 19:29:23 +0000
+++ lib/lp/blueprints/browser/tests/test_sprint.py	2012-09-27 19:29:23 +0000
@@ -39,4 +39,4 @@
             link.acceptBy(sprint.owner)
         with QueryCollector() as recorder:
             self.getViewBrowser(sprint)
-        self.assertThat(recorder, HasQueryCount(Equals(33)))
+        self.assertThat(recorder, HasQueryCount(Equals(30)))

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-09-24 13:43:00 +0000
+++ lib/lp/blueprints/model/specification.py	2012-09-27 19:29:23 +0000
@@ -532,6 +532,24 @@
                         SpecificationImplementationStatus.INFORMATIONAL.value,
                         SpecificationDefinitionStatus.APPROVED.value))
 
+    @classmethod
+    def storm_completeness(cls):
+        """Storm version of the above."""
+        return Or(
+            cls.implementation_status ==
+                SpecificationImplementationStatus.IMPLEMENTED,
+            cls.definition_status.is_in([
+                SpecificationDefinitionStatus.OBSOLETE,
+                SpecificationDefinitionStatus.SUPERSEDED,
+                ]),
+            And(
+                cls.implementation_status ==
+                    SpecificationImplementationStatus.INFORMATIONAL,
+                cls.definition_status ==
+                    SpecificationDefinitionStatus.APPROVED
+                ),
+            )
+
     @property
     def is_complete(self):
         """See `ISpecification`."""

=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py	2011-12-30 06:14:56 +0000
+++ lib/lp/blueprints/model/sprint.py	2012-09-27 19:29:23 +0000
@@ -15,7 +15,13 @@
     ForeignKey,
     StringCol,
     )
-from storm.locals import Store
+from storm.locals import (
+    Desc,
+    Not,
+    Or,
+    Select,
+    Store,
+    )
 from zope.component import getUtility
 from zope.interface import implements
 
@@ -50,6 +56,7 @@
     quote,
     SQLBase,
     )
+from lp.services.database.stormexpr import fti_search
 
 
 class Sprint(SQLBase, HasDriversMixin, HasSpecificationsMixin):
@@ -111,9 +118,14 @@
         specifications() method because we want to reuse this query in the
         specificationLinks() method.
         """
-
-        # Make a new list of the filter, so that we do not mutate what we
-        # were passed as a filter
+        # import here to avoid circular deps
+        from lp.blueprints.model.specification import Specification
+        from lp.registry.model.product import Product
+        query = [SprintSpecification.sprintID == self.id,
+                 SprintSpecification.specificationID == Specification.id,
+                 Or(Specification.product == None,
+                    Not(Specification.productID.is_in(Select(Product.id,
+                        Product.active == False))))]
         if not filter:
             # filter could be None or [] then we decide the default
             # which for a sprint is to show everything approved
@@ -126,55 +138,35 @@
         #  - acceptance for sprint agenda.
         #  - informational.
         #
-        base = """SprintSpecification.sprint = %s AND
-                  SprintSpecification.specification = Specification.id AND
-                  (Specification.product IS NULL OR
-                   Specification.product NOT IN
-                    (SELECT Product.id FROM Product
-                     WHERE Product.active IS FALSE))
-                  """ % quote(self)
-        query = base
 
         # look for informational specs
         if SpecificationFilter.INFORMATIONAL in filter:
-            query += (' AND Specification.implementation_status = %s' %
-              quote(SpecificationImplementationStatus.INFORMATIONAL))
-
-        # import here to avoid circular deps
-        from lp.blueprints.model.specification import Specification
-
+            query.append(Specification.implementation_status ==
+                         SpecificationImplementationStatus.INFORMATIONAL)
         # filter based on completion. see the implementation of
         # Specification.is_complete() for more details
-        completeness = Specification.completeness_clause
-
         if SpecificationFilter.COMPLETE in filter:
-            query += ' AND ( %s ) ' % completeness
-        elif SpecificationFilter.INCOMPLETE in filter:
-            query += ' AND NOT ( %s ) ' % completeness
-
+            query.append(Specification.storm_completeness())
+        if SpecificationFilter.INCOMPLETE in filter:
+            query.append(Not(Specification.storm_completeness()))
+        sprint_status = []
         # look for specs that have a particular SprintSpecification
         # status (proposed, accepted or declined)
         if SpecificationFilter.ACCEPTED in filter:
-            query += ' AND SprintSpecification.status = %s' % (
-                quote(SprintSpecificationStatus.ACCEPTED))
-        elif SpecificationFilter.PROPOSED in filter:
-            query += ' AND SprintSpecification.status = %s' % (
-                quote(SprintSpecificationStatus.PROPOSED))
-        elif SpecificationFilter.DECLINED in filter:
-            query += ' AND SprintSpecification.status = %s' % (
-                quote(SprintSpecificationStatus.DECLINED))
-
-        # ALL is the trump card
-        if SpecificationFilter.ALL in filter:
-            query = base
-
+            sprint_status.append(SprintSpecificationStatus.ACCEPTED)
+        if SpecificationFilter.PROPOSED in filter:
+            sprint_status.append(SprintSpecificationStatus.PROPOSED)
+        if SpecificationFilter.DECLINED in filter:
+            sprint_status.append(SprintSpecificationStatus.DECLINED)
+        statuses = [SprintSpecification.status == status for status in
+                    sprint_status]
+        if len(statuses) > 0:
+            query.append(Or(*statuses))
         # Filter for specification text
         for constraint in filter:
             if isinstance(constraint, basestring):
                 # a string in the filter is a text search filter
-                query += ' AND Specification.fti @@ ftq(%s) ' % quote(
-                    constraint)
-
+                query.append(fti_search(Specification, constraint))
         return query
 
     @property
@@ -187,59 +179,39 @@
         return self.specifications(filter=[SpecificationFilter.ALL])
 
     def specifications(self, sort=None, quantity=None, filter=None,
-                       prejoin_people=True):
+                       prejoin_people=False):
         """See IHasSpecifications."""
-
+        # prejoin_people  is provided only for interface compatibility and
+        # prejoin_people=True is not implemented.
+        assert not prejoin_people
+        if filter is None:
+            filter = set([SpecificationFilter.ACCEPTED])
         query = self.spec_filter_clause(filter=filter)
-        if filter == None:
-            filter = []
-
         # import here to avoid circular deps
         from lp.blueprints.model.specification import Specification
-
-        # sort by priority descending, by default
-        if sort is None or sort == SpecificationSort.PRIORITY:
-            order = ['-priority', 'Specification.definition_status',
-                     'Specification.name']
-        elif sort == SpecificationSort.DATE:
+        results = Store.of(self).find(Specification, *query)
+        if sort == SpecificationSort.DATE:
+            order = (Desc(SprintSpecification.date_created), Specification.id)
             # we need to establish if the listing will show specs that have
             # been decided only, or will include proposed specs.
-            show_proposed = set([
-                SpecificationFilter.ALL,
-                SpecificationFilter.PROPOSED,
-                ])
-            if len(show_proposed.intersection(set(filter))) > 0:
-                # we are showing proposed specs so use the date proposed
-                # because not all specs will have a date decided.
-                order = ['-SprintSpecification.date_created',
-                         'Specification.id']
-            else:
+            if (SpecificationFilter.ALL not in filter and
+                SpecificationFilter.PROPOSED not in filter):
                 # this will show only decided specs so use the date the spec
                 # was accepted or declined for the sprint
-                order = ['-SprintSpecification.date_decided',
-                         '-SprintSpecification.date_created',
-                         'Specification.id']
-
-        results = Specification.select(query, orderBy=order, limit=quantity,
-            clauseTables=['SprintSpecification'])
-        if prejoin_people:
-            results = results.prejoin(['assignee', 'approver', 'drafter'])
+                order = (Desc(SprintSpecification.date_decided),) + order
+            results = results.order_by(*order)
+        else:
+            assert sort is None or sort == SpecificationSort.PRIORITY
+            # fall back to default, which is priority, descending.
+        if quantity is not None:
+            results = results[:quantity]
         return results
 
-    def specificationLinks(self, sort=None, quantity=None, filter=None):
+    def specificationLinks(self, filter=None):
         """See `ISprint`."""
-
         query = self.spec_filter_clause(filter=filter)
-
-        # sort by priority descending, by default
-        if sort is None or sort == SpecificationSort.PRIORITY:
-            order = ['-priority', 'status', 'name']
-        elif sort == SpecificationSort.DATE:
-            order = ['-datecreated', 'id']
-
-        results = SprintSpecification.select(query,
-            clauseTables=['Specification'], orderBy=order, limit=quantity)
-        return results.prejoin(['specification'])
+        result = Store.of(self).find(SprintSpecification, *query)
+        return result
 
     def getSpecificationLink(self, speclink_id):
         """See `ISprint`.

=== modified file 'lib/lp/blueprints/templates/specificationtarget-assignments.pt'
--- lib/lp/blueprints/templates/specificationtarget-assignments.pt	2010-09-15 00:07:54 +0000
+++ lib/lp/blueprints/templates/specificationtarget-assignments.pt	2012-09-27 19:29:23 +0000
@@ -16,14 +16,14 @@
 
 <div metal:fill-slot="main">
 
-  <p class="portlet" tal:condition="not: view/specs" id="no-blueprints">
+  <p class="portlet" tal:condition="view/specs/is_empty" id="no-blueprints">
     There are no open blueprints.
   </p>
 
-  <div tal:condition="view/specs" class="portlet">
+  <div tal:condition="not: view/specs/is_empty" class="portlet">
     <p>
       This listing shows the assignment of work for
-      blueprints currently associated with 
+      blueprints currently associated with
       <span tal:replace="context/displayname">Mozilla</span>.
       The drafter is responsible for getting the specification correctly
       written up and approved.
@@ -84,7 +84,7 @@
             </a>
           </td>
           <td>
-            <a tal:condition="spec/approver" 
+            <a tal:condition="spec/approver"
                tal:replace="structure spec/approver/fmt:link">
               Approver Foo
             </a>

=== modified file 'lib/lp/services/database/stormexpr.py'
--- lib/lp/services/database/stormexpr.py	2012-07-24 07:24:58 +0000
+++ lib/lp/services/database/stormexpr.py	2012-09-27 19:29:23 +0000
@@ -11,6 +11,7 @@
     'ColumnSelect',
     'Concatenate',
     'CountDistinct',
+    'fti_search',
     'Greatest',
     'get_where_for_reference',
     'NullCount',
@@ -29,8 +30,12 @@
     In,
     NamedFunc,
     Or,
-    )
-from storm.info import get_obj_info
+    SQL,
+    )
+from storm.info import (
+    get_cls_info,
+    get_obj_info,
+    )
 
 
 class ColumnSelect(Expr):
@@ -191,3 +196,11 @@
             [_remote_variables(relation, value) for value in others])
     else:
         return Or(*[relation.get_where_for_local(value) for value in others])
+
+
+def fti_search(table, text):
+    """An expression ensuring that table rows match the specified text."""
+    table = get_cls_info(table).table
+    tables = (table,)
+    text = unicode(text)
+    return SQL('%s.fti @@ ftq(?)' % table.name, params=(text,), tables=tables)