launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02416
[Merge] lp:~danilo/launchpad/merge-ss-with-filters-urls into lp:launchpad/db-devel
Данило Шеган has proposed merging lp:~danilo/launchpad/merge-ss-with-filters-urls into lp:launchpad/db-devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~danilo/launchpad/merge-ss-with-filters-urls/+merge/47403
= Make StructuralSubscription URLs independent of the person =
In accordance with our higher-level goal of having multiple structural subscription per-target, per-person, we need to have a URL for structural subscriptions that is not limited to one per person.
At the moment, canonical_url for a SS is /<target>/+subscription/<person> and we are changing that to be /<target>/+subscription/<id>
== Pre-implementation notes ==
I discussed with Gary the option of putting all SSs on a top-level StructuralSubscription object, and we decided to keep it simple and isolated by keeping the <target> (one of product, productseries, distroseries, milestone, sourcepackage), while not allowing one to reach SSs that don't belong there.
== Implementation details ==
I provided a getSubscriptionByID on IStructuralSubscriptionTarget class, which is the core of the implementation. Most of the other stuff is test updates.
== Tests ==
bin/test -cvvt structuralsubscription
== Demo and Q/A ==
Check that structural subscriptions appear on appropriate links on QA staging.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/browser/configure.zcml
lib/lp/registry/browser/structuralsubscription.py
lib/lp/registry/browser/tests/test_structuralsubscription.py
lib/lp/registry/interfaces/structuralsubscription.py
lib/lp/registry/stories/webservice/xx-structuralsubscription.txt
lib/lp/registry/tests/test_structuralsubscriptiontarget.py
lib/lp/registry/model/structuralsubscription.py
--
https://code.launchpad.net/~danilo/launchpad/merge-ss-with-filters-urls/+merge/47403
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/merge-ss-with-filters-urls into lp:launchpad/db-devel.
=== modified file 'lib/lp/bugs/browser/structuralsubscription.py'
--- lib/lp/bugs/browser/structuralsubscription.py 2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/browser/structuralsubscription.py 2011-01-25 15:25:39 +0000
@@ -347,8 +347,7 @@
@stepthrough('+subscription')
def traverse_structuralsubscription(self, name):
"""Traverses +subscription portions of URLs."""
- person = getUtility(IPersonSet).getByName(name)
- return self.context.getSubscription(person)
+ return self.context.getSubscriptionByID(subscription_id=int(name))
class StructuralSubscriptionMenuMixin:
=== modified file 'lib/lp/bugs/browser/tests/test_structuralsubscription.py'
--- lib/lp/bugs/browser/tests/test_structuralsubscription.py 2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/browser/tests/test_structuralsubscription.py 2011-01-25 15:25:39 +0000
@@ -61,10 +61,12 @@
super(StructuralSubscriptionTraversalTestBase, self).setUp()
login('foo.bar@xxxxxxxxxxxxx')
self.eric = self.factory.makePerson(name='eric')
- self.michael = self.factory.makePerson(name='michael')
self.setUpTarget()
- self.target.addBugSubscription(self.eric, self.eric)
+ self.subscription = self.target.addBugSubscription(
+ self.eric, self.eric)
+ # To make sure subscription.id is defined, we commit the transaction.
+ transaction.commit()
def setUpTarget(self):
self.target = self.factory.makeProduct(name='fooix')
@@ -73,7 +75,7 @@
def test_structural_subscription_traversal(self):
# Verify that an existing structural subscription can be
# reached from the target.
- request = FakeLaunchpadRequest([], ['eric'])
+ request = FakeLaunchpadRequest([], [str(self.subscription.id)])
self.assertEqual(
self.target.getSubscription(self.eric),
self.navigation(self.target, request).publishTraverse(
@@ -81,17 +83,22 @@
def test_missing_structural_subscription_traversal(self):
# Verify that a NotFound is raised when attempting to reach
- # a structural subscription for an person without one.
- request = FakeLaunchpadRequest([], ['michael'])
+ # a structural subscription which doesn't exist on the target.
+ request = FakeLaunchpadRequest([], ["0"])
self.assertRaises(
NotFound,
self.navigation(self.target, request).publishTraverse,
request, '+subscription')
- def test_missing_person_structural_subscription_traversal(self):
+ def test_other_target_structural_subscription_traversal(self):
# Verify that a NotFound is raised when attempting to reach
- # a structural subscription for a person that does not exist.
- request = FakeLaunchpadRequest([], ['doesnotexist'])
+ # a structural subscription for a different target.
+ other_target = self.factory.makeProduct()
+ other_subscription = other_target.addBugSubscription(
+ self.eric, self.eric)
+ # To get an ID, we must commit the transaction.
+ transaction.commit()
+ request = FakeLaunchpadRequest([], [str(other_subscription.id)])
self.assertRaises(
NotFound,
self.navigation(self.target, request).publishTraverse,
@@ -100,9 +107,12 @@
def test_structural_subscription_canonical_url(self):
# Verify that the canonical_url of a structural subscription
# is correct.
+ expected_url = (
+ canonical_url(self.target) + '/+subscription/' +
+ str(self.subscription.id))
self.assertEqual(
- canonical_url(self.target.getSubscription(self.eric)),
- canonical_url(self.target) + '/+subscription/eric')
+ expected_url,
+ canonical_url(self.target.getSubscription(self.eric)))
def tearDown(self):
logout()
@@ -364,6 +374,30 @@
self.view.target_label)
+class TestStructuralSubscriptionTargetAPI(TestCaseWithFactory):
+
+ layer = AppServerLayer
+
+ def setUp(self):
+ super(TestStructuralSubscriptionTargetAPI, self).setUp()
+ self.owner = self.factory.makePerson(name=u"foo")
+ self.structure = self.factory.makeProduct(
+ owner=self.owner, name=u"bar")
+ with person_logged_in(self.owner):
+ self.subscription = self.structure.addBugSubscription(
+ self.owner, self.owner)
+ transaction.commit()
+ self.service = self.factory.makeLaunchpadService(self.owner)
+ self.ws_structure = ws_object(self.service, self.structure)
+ self.ws_subscription = ws_object(self.service, self.subscription)
+
+ def test_getSubscriptionByID(self):
+ self.assertEqual(
+ self.ws_subscription,
+ self.ws_structure.getSubscriptionByID(
+ subscription_id=self.subscription.id))
+
+
class TestStructuralSubscriptionAPI(TestCaseWithFactory):
layer = AppServerLayer
=== modified file 'lib/lp/bugs/interfaces/structuralsubscription.py'
--- lib/lp/bugs/interfaces/structuralsubscription.py 2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/interfaces/structuralsubscription.py 2011-01-25 15:25:39 +0000
@@ -58,7 +58,7 @@
class IStructuralSubscriptionPublic(Interface):
"""The public parts of a subscription to a Launchpad structure."""
- id = Int(title=_('ID'), readonly=True, required=True)
+ id = exported(Int(title=_('ID'), readonly=True, required=True))
product = Int(title=_('Product'), required=False, readonly=True)
productseries = Int(
title=_('Product series'), required=False, readonly=True)
@@ -151,7 +151,14 @@
@operation_returns_entry(IStructuralSubscription)
@export_read_operation()
def getSubscription(person):
- """Return the subscription for `person`, if it exists."""
+ """Return a subscriptions for a `person`."""
+
+ @operation_parameters(
+ subscription_id=Int(title=_("Subscription ID"), required=True))
+ @operation_returns_entry(IStructuralSubscription)
+ @export_read_operation()
+ def getSubscriptionByID(subscription_id):
+ """Return a StructuralSubscription with ID `subscription_id`."""
target_type_display = Attribute("The type of the target, for display.")
=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py 2011-01-22 02:59:35 +0000
+++ lib/lp/bugs/model/structuralsubscription.py 2011-01-25 15:25:39 +0000
@@ -444,10 +444,16 @@
all_subscriptions = self.getSubscriptions(subscriber=person)
return all_subscriptions.one()
+ def getSubscriptionByID(self, subscription_id):
+ """See `IStructuralSubscriptionTarget`."""
+ subscriptions = self.getSubscriptions(
+ subscription_id=subscription_id)
+ return subscriptions.one()
+
def getSubscriptions(self,
min_bug_notification_level=
BugNotificationLevel.NOTHING,
- subscriber=None):
+ subscriber=None, subscription_id=None):
"""See `IStructuralSubscriptionTarget`."""
from lp.registry.model.person import Person
clauses = [
@@ -463,6 +469,10 @@
clauses.append(
StructuralSubscription.subscriberID==subscriber.id)
+ if subscription_id is not None:
+ clauses.append(
+ StructuralSubscription.id==subscription_id)
+
store = Store.of(self.__helper.pillar)
return store.find(
StructuralSubscription, *clauses).order_by('Person.displayname')
=== modified file 'lib/lp/bugs/tests/test_structuralsubscriptiontarget.py'
--- lib/lp/bugs/tests/test_structuralsubscriptiontarget.py 2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/tests/test_structuralsubscriptiontarget.py 2011-01-25 15:25:39 +0000
@@ -171,6 +171,33 @@
self.team, self.team_owner),
None)
+ def test_getSubscriptionByID_nothing(self):
+ login_person(self.team_owner)
+ self.assertIs(
+ None,
+ self.target.getSubscriptionByID(0))
+
+ def test_getSubscriptionByID_success(self):
+ login_person(self.team_owner)
+ # Create a subscription to return.
+ subscription = self.target.addBugSubscription(
+ self.team, self.team_owner)
+ self.assertEquals(
+ subscription,
+ self.target.getSubscriptionByID(subscription.id))
+
+ def test_getSubscriptionByID_other_target(self):
+ # No subscription is returned if one tries to get
+ # a subscription from a different target.
+ login_person(self.team_owner)
+ # Create a subscription on a different target.
+ other_target = self.factory.makeProduct()
+ subscription = other_target.addBugSubscription(
+ self.team, self.team_owner)
+ self.assertIs(
+ None,
+ self.target.getSubscriptionByID(subscription.id))
+
class FilteredStructuralSubscriptionTestBase:
"""Tests for filtered structural subscriptions."""
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml 2011-01-24 20:53:10 +0000
+++ lib/lp/registry/browser/configure.zcml 2011-01-25 15:25:39 +0000
@@ -2280,6 +2280,32 @@
template="../templates/sourcepackage-remove-packaging.pt"
/>
+<<<<<<< TREE
+=======
+ <browser:page
+ for="lp.registry.interfaces.structuralsubscription.IStructuralSubscriptionTarget"
+ name="+subscribe"
+ permission="launchpad.AnyPerson"
+ facet="bugs"
+ class="lp.registry.browser.structuralsubscription.StructuralSubscriptionView"
+ template="../templates/structural-subscriptions-manage.pt"/>
+ <browser:page
+ for="lp.registry.interfaces.structuralsubscription.IStructuralSubscriptionTarget"
+ name="+portlet-structural-subscribers"
+ facet="bugs"
+ permission="zope.Public"
+ class="lp.registry.browser.structuralsubscription.StructuralSubscribersPortletView"
+ template="../templates/structural-subscription-target-portlet-subscribers.pt"/>
+
+ <browser:url
+ for="lp.registry.interfaces.structuralsubscription.IStructuralSubscription"
+ path_expression="string:+subscription/${id}"
+ attribute_to_parent="target"/>
+ <browser:navigation
+ module="lp.registry.browser.structuralsubscription"
+ classes="StructuralSubscriptionNavigation"/>
+
+>>>>>>> MERGE-SOURCE
<browser:url
for="lp.registry.interfaces.personproduct.IPersonProduct"
path_expression="product/name"
=== modified file 'lib/lp/registry/stories/webservice/xx-structuralsubscription.txt'
--- lib/lp/registry/stories/webservice/xx-structuralsubscription.txt 2010-12-21 23:19:35 +0000
+++ lib/lp/registry/stories/webservice/xx-structuralsubscription.txt 2011-01-25 15:25:39 +0000
@@ -37,7 +37,7 @@
... '/fooix', 'addBugSubscription')
HTTP/1.1 201 Created
...
- Location: http://.../fooix/+subscription/eric
+ Location: http://.../fooix/+subscription/...
...
>>> subscriptions = webservice.named_get(
@@ -46,11 +46,12 @@
start: 0
total_size: 1
---
- bug_filters_collection_link: u'.../fooix/+subscription/eric/bug_filters'
+ bug_filters_collection_link: u'.../fooix/+subscription/.../bug_filters'
date_created: u'...'
date_last_updated: u'...'
+ id: ...
resource_type_link: u'http://.../#structural_subscription'
- self_link: u'http://.../fooix/+subscription/eric'
+ self_link: u'http://.../fooix/+subscription/...'
subscribed_by_link: u'http://.../~eric'
subscriber_link: u'http://.../~eric'
target_link: u'http://.../fooix'
@@ -61,11 +62,12 @@
>>> pprint_entry(eric_webservice.named_get(
... '/fooix', 'getSubscription',
... person=webservice.getAbsoluteUrl('/~eric')).jsonBody())
- bug_filters_collection_link: u'.../fooix/+subscription/eric/bug_filters'
+ bug_filters_collection_link: u'.../fooix/+subscription/.../bug_filters'
date_created: u'...'
date_last_updated: u'...'
+ id: ...
resource_type_link: u'http://.../#structural_subscription'
- self_link: u'http://.../fooix/+subscription/eric'
+ self_link: u'http://.../fooix/+subscription/...'
subscribed_by_link: u'http://.../~eric'
subscriber_link: u'http://.../~eric'
target_link: u'http://.../fooix'
@@ -110,7 +112,7 @@
... subscriber=webservice.getAbsoluteUrl('/~pythons'))
HTTP/1.1 201 Created
...
- Location: http://.../fooix/+subscription/pythons
+ Location: http://.../fooix/+subscription/...
...
>>> subscriptions = webservice.named_get(
@@ -119,11 +121,12 @@
start: 0
total_size: 1
---
- bug_filters_collection_link: u'.../fooix/+subscription/pythons/bug_filters'
+ bug_filters_collection_link: u'.../fooix/+subscription/.../bug_filters'
date_created: u'...'
date_last_updated: u'...'
+ id: ...
resource_type_link: u'http://.../#structural_subscription'
- self_link: u'http://.../fooix/+subscription/pythons'
+ self_link: u'http://.../fooix/+subscription/...'
subscribed_by_link: u'http://.../~michael'
subscriber_link: u'http://.../~pythons'
target_link: u'http://.../fooix'
