launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13450
[Merge] lp:~abentley/launchpad/user-blueprints into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/user-blueprints into lp:launchpad with lp:~abentley/launchpad/user-blueprints-tests as a prerequisite.
Commit message:
Re-implement Person.specifications using Storm expressions.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1063275 in Launchpad itself: "user blueprints page cannot be viewed"
https://bugs.launchpad.net/launchpad/+bug/1063275
For more details, see:
https://code.launchpad.net/~abentley/launchpad/user-blueprints/+merge/129945
= 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
--
https://code.launchpad.net/~abentley/launchpad/user-blueprints/+merge/129945
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/user-blueprints into lp:launchpad.
=== 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 16:57:28 +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,38 @@
where=Or(
AccessPolicyGrantFlat.abstract_artifact_id == None,
AccessArtifact.specification_id == Specification.id))))
+
+
+def get_specification_filters(filter):
+ 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 16:57:28 +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/registry/model/person.py'
--- lib/lp/registry/model/person.py 2012-10-03 07:25:14 +0000
+++ lib/lp/registry/model/person.py 2012-10-16 16:57:28 +0000
@@ -128,6 +128,7 @@
SpecificationSort,
)
from lp.blueprints.model.specification import (
+ get_specification_filters,
HasSpecificationsMixin,
Specification,
)
@@ -818,27 +819,22 @@
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
+ 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 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]
+ 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
- completeness = False
- for option in [
- SpecificationFilter.COMPLETE,
- SpecificationFilter.INCOMPLETE]:
- if option in filter:
- completeness = True
- if completeness is False:
- filter.append(SpecificationFilter.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
@@ -846,101 +842,40 @@
# 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
+
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:
Follow ups
