launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13610
[Merge] lp:~abentley/launchpad/person-assigned-specs-in-progress into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/person-assigned-specs-in-progress into lp:launchpad.
Commit message:
Fix +portlet-currentfocus with proprietary specs.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1068719 in Launchpad itself: "Person overview page breaks when assigned proprietary blueprints"
https://bugs.launchpad.net/launchpad/+bug/1068719
For more details, see:
https://code.launchpad.net/~abentley/launchpad/person-assigned-specs-in-progress/+merge/130888
= 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
--
https://code.launchpad.net/~abentley/launchpad/person-assigned-specs-in-progress/+merge/130888
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/person-assigned-specs-in-progress into lp:launchpad.
=== 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-22 20:00:30 +0000
@@ -14,6 +14,7 @@
'SPECIFICATION_POLICY_ALLOWED_TYPES',
'SPECIFICATION_POLICY_DEFAULT_TYPES',
'SpecificationSet',
+ 'spec_started_clause',
]
from lazr.lifecycle.event import (
@@ -1334,3 +1335,16 @@
# A string in the filter is a text search filter.
clauses.append(fti_search(Specification, constraint))
return clauses
+
+
+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-19 16:54:35 +0000
+++ lib/lp/registry/browser/person.py 2012-10-22 20:00:30 +0000
@@ -1705,7 +1705,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-09-06 00:01:38 +0000
+++ lib/lp/registry/browser/tests/test_person.py 2012-10-22 20:00:30 +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/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2012-10-16 00:48:55 +0000
+++ lib/lp/registry/interfaces/person.py 2012-10-22 20:00:30 +0000
@@ -850,9 +850,16 @@
"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.
+
+ :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-19 16:54:35 +0000
+++ lib/lp/registry/model/person.py 2012-10-22 20:00:30 +0000
@@ -130,6 +130,7 @@
get_specification_privacy_filter,
HasSpecificationsMixin,
Specification,
+ spec_started_clause,
)
from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
from lp.bugs.interfaces.bugtarget import IBugTarget
@@ -798,17 +799,9 @@
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):
+ return self.specifications(user, in_progress=True, quantity=5,
+ sort=Desc(Specification.date_started))
@property
def unique_displayname(self):
@@ -816,7 +809,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,
@@ -830,11 +823,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.
@@ -866,12 +854,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-17 15:16:09 +0000
+++ lib/lp/registry/tests/test_person.py 2012-10-22 20:00:30 +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,
@@ -1815,3 +1816,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-19 16:54:35 +0000
+++ lib/lp/testing/factory.py 2012-10-22 20:00:30 +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
Follow ups
