Merge lp:~abentley/launchpad/user-blueprints into lp:launchpad

[Aaron Bentley]

Re-implement Person.specifications using Storm expressions.

= Summary =
Implement Person.specifications using Storm expressions, to improve composability and refactoring.

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of private projects

== Implementation details ==
Extract common portions of Sprint.specifications to get_specification_filters, extend with support for SpecificationFilter.ALL and SpecificationFilter.VALID.

== Tests ==
bin/test -t TestSpecifications -m '(sprint|person)'

== Demo and Q/A ==
None

= 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/blueprints/model/sprint.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py

[Richard Harding (rharding)]

#34 The comments in here should be complete sentences ending in . per PEP8
http://www.python.org/dev/peps/pep-0008/#comments

#204 extra blank line?

review: Approve

[Aaron Bentley (abentley)]

> #34 The comments in here should be complete sentences ending in . per PEP8
> http://www.python.org/dev/peps/pep-0008/#comments

Okay. I was copying them verbatim to make it easier to see how the new code matched the old behaviour. That way, the diff matches up on the comments, since it can't match on the code.

> #204 extra blank line?

Fixed.

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-10-10 14:35:49 +0000
+++ lib/lp/blueprints/model/specification.py	2012-10-16 23:52:16 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'get_specification_filters',
     'get_specification_privacy_filter',
     'HasSpecificationsMixin',
     'recursive_blocked_query',
@@ -34,6 +35,7 @@
     Join,
     LeftJoin,
     Or,
+    Not,
     Select,
     )
 from storm.locals import (
@@ -104,6 +106,7 @@
     SQLBase,
     sqlvalues,
     )
+from lp.services.database.stormexpr import fti_search
 from lp.services.mail.helpers import get_contact_email_addresses
 from lp.services.propertycache import (
     cachedproperty,
@@ -1289,3 +1292,43 @@
                 where=Or(
                     AccessPolicyGrantFlat.abstract_artifact_id == None,
                     AccessArtifact.specification_id == Specification.id))))
+
+
+def get_specification_filters(filter):
+    """Return a list of Storm expressions for filtering Specifications.
+
+    :param filters: A collection of SpecificationFilter and/or strings.
+        Strings are used for text searches.
+    """
+    from lp.registry.model.product import Product
+    # If Product is used, it must be active.
+    clauses = [Or(Specification.product == None,
+                  Not(Specification.productID.is_in(Select(Product.id,
+                      Product.active == False))))]
+    # ALL is the trump card.
+    if SpecificationFilter.ALL in filter:
+        return clauses
+    # Look for informational specs.
+    if SpecificationFilter.INFORMATIONAL in filter:
+        clauses.append(Specification.implementation_status ==
+                       SpecificationImplementationStatus.INFORMATIONAL)
+    # Filter based on completion.  See the implementation of
+    # Specification.is_complete() for more details.
+    if SpecificationFilter.COMPLETE in filter:
+        clauses.append(Specification.storm_completeness())
+    if SpecificationFilter.INCOMPLETE in filter:
+        clauses.append(Not(Specification.storm_completeness()))
+
+    # Filter for validity. If we want valid specs only, then we should exclude
+    # all OBSOLETE or SUPERSEDED specs.
+    if SpecificationFilter.VALID in filter:
+        clauses.append(Not(Specification.definition_status.is_in([
+            SpecificationDefinitionStatus.OBSOLETE,
+            SpecificationDefinitionStatus.SUPERSEDED,
+        ])))
+    # Filter for specification text.
+    for constraint in filter:
+        if isinstance(constraint, basestring):
+            # A string in the filter is a text search filter.
+            clauses.append(fti_search(Specification, constraint))
+    return clauses

=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py	2012-10-12 01:56:34 +0000
+++ lib/lp/blueprints/model/sprint.py	2012-10-16 23:52:16 +0000
@@ -17,9 +17,7 @@
     )
 from storm.locals import (
     Desc,
-    Not,
     Or,
-    Select,
     Store,
     )
 from zope.component import getUtility
@@ -33,7 +31,6 @@
     )
 from lp.blueprints.enums import (
     SpecificationFilter,
-    SpecificationImplementationStatus,
     SpecificationSort,
     SprintSpecificationStatus,
     )
@@ -42,6 +39,7 @@
     ISprintSet,
     )
 from lp.blueprints.model.specification import (
+    get_specification_filters,
     get_specification_privacy_filter,
     HasSpecificationsMixin,
     )
@@ -59,7 +57,6 @@
     quote,
     SQLBase,
     )
-from lp.services.database.stormexpr import fti_search
 from lp.services.propertycache import cachedproperty
 
 
@@ -124,12 +121,8 @@
         """
         # 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))))]
+                 SprintSpecification.specificationID == Specification.id]
         query.append(get_specification_privacy_filter(user))
         if not filter:
             # filter could be None or [] then we decide the default
@@ -144,16 +137,6 @@
         #  - informational.
         #
 
-        # look for informational specs
-        if SpecificationFilter.INFORMATIONAL in filter:
-            query.append(Specification.implementation_status ==
-                         SpecificationImplementationStatus.INFORMATIONAL)
-        # filter based on completion. see the implementation of
-        # Specification.is_complete() for more details
-        if SpecificationFilter.COMPLETE in filter:
-            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)
@@ -168,10 +151,7 @@
         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.append(fti_search(Specification, constraint))
+        query.extend(get_specification_filters(filter))
         return query
 
     def all_specifications(self, user):

=== modified file 'lib/lp/blueprints/templates/hasspecifications-specs.pt'
--- lib/lp/blueprints/templates/hasspecifications-specs.pt	2009-11-10 17:35:17 +0000
+++ lib/lp/blueprints/templates/hasspecifications-specs.pt	2012-10-16 23:52:16 +0000
@@ -155,7 +155,7 @@
     </div>
     <div class="lesser" tal:content="structure view/batchnav/@@+navigation-links-upper" />
     <table class="listing sortable" id="speclisting"
-          tal:condition="view/specs">
+          tal:condition="not: view/specs/is_empty">
       <thead>
         <tr>
           <th tal:condition="view/show_priority">Priority</th>
@@ -229,7 +229,7 @@
     </table>
     <div class="lesser" tal:content="structure view/batchnav/@@+navigation-links-lower" />
   
-    <tal:nomatches condition="not: view/specs"
+    <tal:nomatches condition="view/specs/is_empty"
                    replace="structure context/@@+nomatches" />
 
     <p tal:condition="view/is_series">

=== modified file 'lib/lp/blueprints/templates/person-specworkload.pt'
--- lib/lp/blueprints/templates/person-specworkload.pt	2012-09-27 15:28:38 +0000
+++ lib/lp/blueprints/templates/person-specworkload.pt	2012-10-16 23:52:16 +0000
@@ -46,7 +46,7 @@
 
         <tal:specs define="specifications member/@@+specworkload/specifications">
 
-          <div style="margin-bottom: 1em;" tal:condition="specifications">
+          <div style="margin-bottom: 1em;" tal:condition="not: specifications/is_empty">
             <p>
               <a tal:replace="structure member/fmt:link">Foo Bar</a>'s
               specifications:
@@ -55,7 +55,7 @@
             <div tal:replace="structure member/@@+table-specworkload" />
           </div>
 
-          <p tal:condition="not: specifications">
+          <p tal:condition="specifications/is_empty">
             <a tal:replace="structure member/fmt:link">Foo Bar</a>
             has no outstanding specifications.<br />
           </p>

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-10-15 18:30:48 +0000
+++ lib/lp/registry/model/person.py	2012-10-16 23:52:16 +0000
@@ -122,12 +122,11 @@
     valid_name,
     )
 from lp.blueprints.enums import (
-    SpecificationDefinitionStatus,
     SpecificationFilter,
-    SpecificationImplementationStatus,
     SpecificationSort,
     )
 from lp.blueprints.model.specification import (
+    get_specification_filters,
     get_specification_privacy_filter,
     HasSpecificationsMixin,
     Specification,
@@ -819,129 +818,62 @@
     def specifications(self, user, sort=None, quantity=None, filter=None,
                        prejoin_people=True):
         """See `IHasSpecifications`."""
-
-        # Make a new list of the filter, so that we do not mutate what we
-        # were passed as a filter
-        if not filter:
-            # if no filter was passed (None or []) then we must decide the
-            # default filtering, and for a person we want related incomplete
-            # specs
-            filter = [SpecificationFilter.INCOMPLETE]
-
-        # 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
-        completeness = False
-        for option in [
-            SpecificationFilter.COMPLETE,
-            SpecificationFilter.INCOMPLETE]:
-            if option in filter:
-                completeness = True
-        if completeness is False:
-            filter.append(SpecificationFilter.INCOMPLETE)
-
-        # defaults for acceptance: in this case we have nothing to do
-        # because specs are not accepted/declined against a person
-
-        # defaults for informationalness: we don't have to do anything
-        # because the default if nothing is said is ANY
-
-        # if no roles are given then we want everything
-        linked = False
+        from lp.blueprints.model.specificationsubscription import (
+            SpecificationSubscription,
+            )
+        # Make a new copy of the filter, so that we do not mutate what we
+        # were passed as a filter.
+        if filter is None:
+            filter = set()
+        else:
+            filter = set(filter)
+
+        # 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.
+
+        # Defaults for informationalness: we don't have to do anything
+        # because the default if nothing is said is ANY.
+
         roles = set([
             SpecificationFilter.CREATOR,
             SpecificationFilter.ASSIGNEE,
             SpecificationFilter.DRAFTER,
             SpecificationFilter.APPROVER,
             SpecificationFilter.SUBSCRIBER])
-        for role in roles:
-            if role in filter:
-                linked = True
-        if not linked:
-            for role in roles:
-                filter.append(role)
-
-        # 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:
-            order = ['-Specification.datecreated', 'Specification.id']
-
-        # figure out what set of specifications we are interested in. for
-        # products, we need to be able to filter on the basis of:
-        #
-        #  - role (owner, drafter, approver, subscriber, assignee etc)
-        #  - completeness.
-        #  - informational.
-        #
-
-        # in this case the "base" is quite complicated because it is
-        # determined by the roles so lets do that first
-
-        base = '(1=0'  # we want to start with a FALSE and OR them
+        # If no roles are given, then we want everything.
+        if filter.intersection(roles) == set():
+            filter.update(roles)
+        role_clauses = []
         if SpecificationFilter.CREATOR in filter:
-            base += ' OR Specification.owner = %(my_id)d'
+            role_clauses.append(Specification.owner == self)
         if SpecificationFilter.ASSIGNEE in filter:
-            base += ' OR Specification.assignee = %(my_id)d'
+            role_clauses.append(Specification.assignee == self)
         if SpecificationFilter.DRAFTER in filter:
-            base += ' OR Specification.drafter = %(my_id)d'
+            role_clauses.append(Specification.drafter == self)
         if SpecificationFilter.APPROVER in filter:
-            base += ' OR Specification.approver = %(my_id)d'
+            role_clauses.append(Specification.approver == self)
         if SpecificationFilter.SUBSCRIBER in filter:
-            base += """ OR Specification.id in
-                (SELECT specification FROM SpecificationSubscription
-                 WHERE person = %(my_id)d)"""
-        base += ') '
-
-        # filter out specs on inactive products
-        base += """AND (Specification.product IS NULL OR
-                        Specification.product NOT IN
-                         (SELECT Product.id FROM Product
-                          WHERE Product.active IS FALSE))
-                """
-
-        base = base % {'my_id': self.id}
-
-        query = base
-        # look for informational specs
-        if SpecificationFilter.INFORMATIONAL in filter:
-            query += (' AND Specification.implementation_status = %s' %
-                quote(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
-
-        # Filter for validity. If we want valid specs only then we should
-        # exclude all OBSOLETE or SUPERSEDED specs
-        if SpecificationFilter.VALID in filter:
-            query += (
-                ' AND Specification.definition_status NOT IN ( %s, ''%s ) ' %
-                sqlvalues(SpecificationDefinitionStatus.OBSOLETE,
-                          SpecificationDefinitionStatus.SUPERSEDED))
-
-        # ALL is the trump card
-        if SpecificationFilter.ALL in filter:
-            query = base
-
-        # 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)
-
-        results = Specification.select(query, orderBy=order,
-            limit=quantity)
-        if prejoin_people:
-            results = results.prejoin(['assignee', 'approver', 'drafter'])
+            role_clauses.append(
+                Specification.id.is_in(
+                    Select(SpecificationSubscription.specificationID,
+                        [SpecificationSubscription.person == self]
+                    )))
+        clauses = [Or(*role_clauses)]
+        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))
+        if quantity is not None:
+            results = results[:quantity]
         return results
 
     # XXX: Tom Berger 2008-04-14 bug=191799: