launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12574
[Merge] lp:~abentley/launchpad/storm-sprint-queries into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/storm-sprint-queries into lp:launchpad with lp:~abentley/launchpad/new-tests as a prerequisite.
Commit message:
Convert Sprint.specifications to use storm expressions.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~abentley/launchpad/storm-sprint-queries/+merge/126770
= 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
--
https://code.launchpad.net/~abentley/launchpad/storm-sprint-queries/+merge/126770
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/storm-sprint-queries into lp:launchpad.
=== 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)
Follow ups
