Merge lp:~danilo/launchpad/merge-ss-with-filters-urls into lp:launchpad/db-devel

Proposed by Данило Шеган
Status: Work in progress
Proposed branch: lp:~danilo/launchpad/merge-ss-with-filters-urls
Merge into: lp:launchpad/db-devel
Diff against target: 393 lines (+117/-38)
9 files modified
lib/lp/bugs/browser/configure.zcml (+1/-1)
lib/lp/bugs/browser/structuralsubscription.py (+1/-4)
lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py (+2/-1)
lib/lp/bugs/browser/tests/test_structuralsubscription.py (+47/-12)
lib/lp/bugs/interfaces/structuralsubscription.py (+9/-6)
lib/lp/bugs/model/structuralsubscription.py (+11/-1)
lib/lp/bugs/tests/test_structuralsubscriptiontarget.py (+27/-0)
lib/lp/registry/stories/person/xx-person-subscriptions.txt (+2/-2)
lib/lp/registry/stories/webservice/xx-structuralsubscription.txt (+17/-11)
To merge this branch: bzr merge lp:~danilo/launchpad/merge-ss-with-filters-urls
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+47403@code.launchpad.net

Commit message

[r=gmb][ui=none][no-qa] Switch structural subscription URLs to not be based on person names by using IDs instead (planning to have more than one per person).

Description of the change

= 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

To post a comment you must log in.
10158. By Данило Шеган

Lint fixes.

10159. By Данило Шеган

Merge db-devel.

Revision history for this message
Graham Binns (gmb) wrote :

Just one minor change needed, which we discussed on IRC:

[13:57] gmb:
danilo: self.structure and self.ws_structure in TestStructuralSubscriptionTargetAPI are a bit ambiguous - enough so that a reader would need to find their definition to find out what they are, anyway. I think that self.subscription_target and .ws_subscription_target would be better names. What do you think?
[13:57] danilos:
gmb, agreed!

review: Approve (code)
10160. By Данило Шеган

Use `subscription_target` instead of amazingly clear `structure` for variable names for subscription targets in tests.

Revision history for this message
Gary Poster (gary) wrote :

As you had mentioned at the sprint, after looking deeper in the code, I think we will be productive faster by sticking to one subscription and multiple separate filter records. We will move bug_notification_level from structural subscription to filter. I am working on a branch to do that now.

We may return to this model (and the goal of collapsing structural subscriptions and filters) in a later increment, or maybe not. For now, let's hold off on this branch.

10161. By Данило Шеган

Fix failing tests.

Unmerged revisions

10161. By Данило Шеган

Fix failing tests.

10160. By Данило Шеган

Use `subscription_target` instead of amazingly clear `structure` for variable names for subscription targets in tests.

10159. By Данило Шеган

Merge db-devel.

10158. By Данило Шеган

Lint fixes.

10157. By Данило Шеган

Update test for the URLs.

10156. By Данило Шеган

Add a webservice API for getSubscriptionByID.

10155. By Данило Шеган

Expose StructuralSubscription.id.

10154. By Данило Шеган

Merged merge-ss-with-filters-sofar into merge-ss-with-filters-urls.

10153. By Данило Шеган

Fix web service test to use ID in URLs.

10152. By Данило Шеган

Make StructuralSubscription URLs use IDs to prepare for more than one per-person, per-target.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/browser/configure.zcml'
2--- lib/lp/bugs/browser/configure.zcml 2011-01-21 08:12:29 +0000
3+++ lib/lp/bugs/browser/configure.zcml 2011-01-31 09:32:26 +0000
4@@ -1195,7 +1195,7 @@
5
6 <browser:url
7 for="lp.bugs.interfaces.structuralsubscription.IStructuralSubscription"
8- path_expression="string:+subscription/${subscriber/name}"
9+ path_expression="string:+subscription/${id}"
10 attribute_to_parent="target"/>
11 <browser:navigation
12 module="lp.bugs.browser.structuralsubscription"
13
14=== modified file 'lib/lp/bugs/browser/structuralsubscription.py'
15--- lib/lp/bugs/browser/structuralsubscription.py 2011-01-21 08:12:29 +0000
16+++ lib/lp/bugs/browser/structuralsubscription.py 2011-01-31 09:32:26 +0000
17@@ -12,7 +12,6 @@
18
19 from operator import attrgetter
20
21-from zope.component import getUtility
22 from zope.formlib import form
23 from zope.schema import (
24 Choice,
25@@ -48,7 +47,6 @@
26 IDistributionSourcePackage,
27 )
28 from lp.registry.interfaces.milestone import IProjectGroupMilestone
29-from lp.registry.interfaces.person import IPersonSet
30 from lp.services.propertycache import cachedproperty
31
32
33@@ -347,8 +345,7 @@
34 @stepthrough('+subscription')
35 def traverse_structuralsubscription(self, name):
36 """Traverses +subscription portions of URLs."""
37- person = getUtility(IPersonSet).getByName(name)
38- return self.context.getSubscription(person)
39+ return self.context.getSubscriptionByID(subscription_id=int(name))
40
41
42 class StructuralSubscriptionMenuMixin:
43
44=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py'
45--- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-01-21 08:12:29 +0000
46+++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-01-31 09:32:26 +0000
47@@ -61,7 +61,8 @@
48 url = urlparse(canonical_url(self.subscription_filter))
49 self.assertThat(url.hostname, StartsWith("bugs."))
50 self.assertEqual(
51- "/bar/+subscription/foo/+filter/%d" % (
52+ "/bar/+subscription/%d/+filter/%d" % (
53+ self.subscription.id,
54 self.subscription_filter.id),
55 url.path)
56
57
58=== modified file 'lib/lp/bugs/browser/tests/test_structuralsubscription.py'
59--- lib/lp/bugs/browser/tests/test_structuralsubscription.py 2011-01-21 08:12:29 +0000
60+++ lib/lp/bugs/browser/tests/test_structuralsubscription.py 2011-01-31 09:32:26 +0000
61@@ -61,10 +61,12 @@
62 super(StructuralSubscriptionTraversalTestBase, self).setUp()
63 login('foo.bar@canonical.com')
64 self.eric = self.factory.makePerson(name='eric')
65- self.michael = self.factory.makePerson(name='michael')
66
67 self.setUpTarget()
68- self.target.addBugSubscription(self.eric, self.eric)
69+ self.subscription = self.target.addBugSubscription(
70+ self.eric, self.eric)
71+ # To make sure subscription.id is defined, we commit the transaction.
72+ transaction.commit()
73
74 def setUpTarget(self):
75 self.target = self.factory.makeProduct(name='fooix')
76@@ -73,7 +75,7 @@
77 def test_structural_subscription_traversal(self):
78 # Verify that an existing structural subscription can be
79 # reached from the target.
80- request = FakeLaunchpadRequest([], ['eric'])
81+ request = FakeLaunchpadRequest([], [str(self.subscription.id)])
82 self.assertEqual(
83 self.target.getSubscription(self.eric),
84 self.navigation(self.target, request).publishTraverse(
85@@ -81,17 +83,22 @@
86
87 def test_missing_structural_subscription_traversal(self):
88 # Verify that a NotFound is raised when attempting to reach
89- # a structural subscription for an person without one.
90- request = FakeLaunchpadRequest([], ['michael'])
91+ # a structural subscription which doesn't exist on the target.
92+ request = FakeLaunchpadRequest([], ["0"])
93 self.assertRaises(
94 NotFound,
95 self.navigation(self.target, request).publishTraverse,
96 request, '+subscription')
97
98- def test_missing_person_structural_subscription_traversal(self):
99+ def test_other_target_structural_subscription_traversal(self):
100 # Verify that a NotFound is raised when attempting to reach
101- # a structural subscription for a person that does not exist.
102- request = FakeLaunchpadRequest([], ['doesnotexist'])
103+ # a structural subscription for a different target.
104+ other_target = self.factory.makeProduct()
105+ other_subscription = other_target.addBugSubscription(
106+ self.eric, self.eric)
107+ # To get an ID, we must commit the transaction.
108+ transaction.commit()
109+ request = FakeLaunchpadRequest([], [str(other_subscription.id)])
110 self.assertRaises(
111 NotFound,
112 self.navigation(self.target, request).publishTraverse,
113@@ -100,9 +107,12 @@
114 def test_structural_subscription_canonical_url(self):
115 # Verify that the canonical_url of a structural subscription
116 # is correct.
117+ expected_url = (
118+ canonical_url(self.target) + '/+subscription/' +
119+ str(self.subscription.id))
120 self.assertEqual(
121- canonical_url(self.target.getSubscription(self.eric)),
122- canonical_url(self.target) + '/+subscription/eric')
123+ expected_url,
124+ canonical_url(self.target.getSubscription(self.eric)))
125
126 def tearDown(self):
127 logout()
128@@ -364,6 +374,31 @@
129 self.view.target_label)
130
131
132+class TestStructuralSubscriptionTargetAPI(TestCaseWithFactory):
133+
134+ layer = AppServerLayer
135+
136+ def setUp(self):
137+ super(TestStructuralSubscriptionTargetAPI, self).setUp()
138+ self.owner = self.factory.makePerson(name=u"foo")
139+ self.subscription_target = self.factory.makeProduct(
140+ owner=self.owner, name=u"bar")
141+ with person_logged_in(self.owner):
142+ self.subscription = self.subscription_target.addBugSubscription(
143+ self.owner, self.owner)
144+ transaction.commit()
145+ self.service = self.factory.makeLaunchpadService(self.owner)
146+ self.ws_subscription_target = ws_object(
147+ self.service, self.subscription_target)
148+ self.ws_subscription = ws_object(self.service, self.subscription)
149+
150+ def test_getSubscriptionByID(self):
151+ self.assertEqual(
152+ self.ws_subscription,
153+ self.ws_subscription_target.getSubscriptionByID(
154+ subscription_id=self.subscription.id))
155+
156+
157 class TestStructuralSubscriptionAPI(TestCaseWithFactory):
158
159 layer = AppServerLayer
160@@ -371,10 +406,10 @@
161 def setUp(self):
162 super(TestStructuralSubscriptionAPI, self).setUp()
163 self.owner = self.factory.makePerson(name=u"foo")
164- self.structure = self.factory.makeProduct(
165+ self.subscription_target = self.factory.makeProduct(
166 owner=self.owner, name=u"bar")
167 with person_logged_in(self.owner):
168- self.subscription = self.structure.addBugSubscription(
169+ self.subscription = self.subscription_target.addBugSubscription(
170 self.owner, self.owner)
171 transaction.commit()
172 self.service = self.factory.makeLaunchpadService(self.owner)
173
174=== modified file 'lib/lp/bugs/interfaces/structuralsubscription.py'
175--- lib/lp/bugs/interfaces/structuralsubscription.py 2011-01-21 08:12:29 +0000
176+++ lib/lp/bugs/interfaces/structuralsubscription.py 2011-01-31 09:32:26 +0000
177@@ -15,10 +15,6 @@
178 'IStructuralSubscriptionTargetHelper',
179 ]
180
181-from lazr.enum import (
182- DBEnumeratedType,
183- DBItem,
184- )
185 from lazr.restful.declarations import (
186 call_with,
187 export_as_webservice_entry,
188@@ -58,7 +54,7 @@
189 class IStructuralSubscriptionPublic(Interface):
190 """The public parts of a subscription to a Launchpad structure."""
191
192- id = Int(title=_('ID'), readonly=True, required=True)
193+ id = exported(Int(title=_('ID'), readonly=True, required=True))
194 product = Int(title=_('Product'), required=False, readonly=True)
195 productseries = Int(
196 title=_('Product series'), required=False, readonly=True)
197@@ -151,7 +147,14 @@
198 @operation_returns_entry(IStructuralSubscription)
199 @export_read_operation()
200 def getSubscription(person):
201- """Return the subscription for `person`, if it exists."""
202+ """Return a subscriptions for a `person`."""
203+
204+ @operation_parameters(
205+ subscription_id=Int(title=_("Subscription ID"), required=True))
206+ @operation_returns_entry(IStructuralSubscription)
207+ @export_read_operation()
208+ def getSubscriptionByID(subscription_id):
209+ """Return a StructuralSubscription with ID `subscription_id`."""
210
211 target_type_display = Attribute("The type of the target, for display.")
212
213
214=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
215--- lib/lp/bugs/model/structuralsubscription.py 2011-01-22 02:59:35 +0000
216+++ lib/lp/bugs/model/structuralsubscription.py 2011-01-31 09:32:26 +0000
217@@ -444,10 +444,16 @@
218 all_subscriptions = self.getSubscriptions(subscriber=person)
219 return all_subscriptions.one()
220
221+ def getSubscriptionByID(self, subscription_id):
222+ """See `IStructuralSubscriptionTarget`."""
223+ subscriptions = self.getSubscriptions(
224+ subscription_id=subscription_id)
225+ return subscriptions.one()
226+
227 def getSubscriptions(self,
228 min_bug_notification_level=
229 BugNotificationLevel.NOTHING,
230- subscriber=None):
231+ subscriber=None, subscription_id=None):
232 """See `IStructuralSubscriptionTarget`."""
233 from lp.registry.model.person import Person
234 clauses = [
235@@ -463,6 +469,10 @@
236 clauses.append(
237 StructuralSubscription.subscriberID==subscriber.id)
238
239+ if subscription_id is not None:
240+ clauses.append(
241+ StructuralSubscription.id==subscription_id)
242+
243 store = Store.of(self.__helper.pillar)
244 return store.find(
245 StructuralSubscription, *clauses).order_by('Person.displayname')
246
247=== modified file 'lib/lp/bugs/tests/test_structuralsubscriptiontarget.py'
248--- lib/lp/bugs/tests/test_structuralsubscriptiontarget.py 2011-01-21 08:12:29 +0000
249+++ lib/lp/bugs/tests/test_structuralsubscriptiontarget.py 2011-01-31 09:32:26 +0000
250@@ -171,6 +171,33 @@
251 self.team, self.team_owner),
252 None)
253
254+ def test_getSubscriptionByID_nothing(self):
255+ login_person(self.team_owner)
256+ self.assertIs(
257+ None,
258+ self.target.getSubscriptionByID(0))
259+
260+ def test_getSubscriptionByID_success(self):
261+ login_person(self.team_owner)
262+ # Create a subscription to return.
263+ subscription = self.target.addBugSubscription(
264+ self.team, self.team_owner)
265+ self.assertEquals(
266+ subscription,
267+ self.target.getSubscriptionByID(subscription.id))
268+
269+ def test_getSubscriptionByID_other_target(self):
270+ # No subscription is returned if one tries to get
271+ # a subscription from a different target.
272+ login_person(self.team_owner)
273+ # Create a subscription on a different target.
274+ other_target = self.factory.makeProduct()
275+ subscription = other_target.addBugSubscription(
276+ self.team, self.team_owner)
277+ self.assertIs(
278+ None,
279+ self.target.getSubscriptionByID(subscription.id))
280+
281
282 class FilteredStructuralSubscriptionTestBase:
283 """Tests for filtered structural subscriptions."""
284
285=== modified file 'lib/lp/registry/stories/person/xx-person-subscriptions.txt'
286--- lib/lp/registry/stories/person/xx-person-subscriptions.txt 2011-01-13 16:28:49 +0000
287+++ lib/lp/registry/stories/person/xx-person-subscriptions.txt 2011-01-31 09:32:26 +0000
288@@ -236,9 +236,9 @@
289 ... "http://launchpad.dev/people/+me/+structural-subscriptions")
290 >>> show_create_links(admin_browser)
291 mozilla-firefox in ubuntu
292- * Create a new filter --> /ubuntu/.../name16/+new-filter
293+ * Create a new filter --> /ubuntu/.../+new-filter
294 pmount in ubuntu
295- * Create a new filter --> /ubuntu/.../name16/+new-filter
296+ * Create a new filter --> /ubuntu/.../+new-filter
297
298 If the user does not have the necessary rights to create new bug
299 filters the "Create" link is not shown.
300
301=== modified file 'lib/lp/registry/stories/webservice/xx-structuralsubscription.txt'
302--- lib/lp/registry/stories/webservice/xx-structuralsubscription.txt 2010-12-21 23:19:35 +0000
303+++ lib/lp/registry/stories/webservice/xx-structuralsubscription.txt 2011-01-31 09:32:26 +0000
304@@ -1,4 +1,5 @@
305-= Structural Subscriptions =
306+Structural Subscriptions
307+========================
308
309 Structural subscriptions can be obtained from any target: a project,
310 project series, project group, distribution, distribution series or
311@@ -37,7 +38,7 @@
312 ... '/fooix', 'addBugSubscription')
313 HTTP/1.1 201 Created
314 ...
315- Location: http://.../fooix/+subscription/eric
316+ Location: http://.../fooix/+subscription/...
317 ...
318
319 >>> subscriptions = webservice.named_get(
320@@ -46,11 +47,12 @@
321 start: 0
322 total_size: 1
323 ---
324- bug_filters_collection_link: u'.../fooix/+subscription/eric/bug_filters'
325+ bug_filters_collection_link: u'.../fooix/+subscription/.../bug_filters'
326 date_created: u'...'
327 date_last_updated: u'...'
328+ id: ...
329 resource_type_link: u'http://.../#structural_subscription'
330- self_link: u'http://.../fooix/+subscription/eric'
331+ self_link: u'http://.../fooix/+subscription/...'
332 subscribed_by_link: u'http://.../~eric'
333 subscriber_link: u'http://.../~eric'
334 target_link: u'http://.../fooix'
335@@ -61,11 +63,12 @@
336 >>> pprint_entry(eric_webservice.named_get(
337 ... '/fooix', 'getSubscription',
338 ... person=webservice.getAbsoluteUrl('/~eric')).jsonBody())
339- bug_filters_collection_link: u'.../fooix/+subscription/eric/bug_filters'
340+ bug_filters_collection_link: u'.../fooix/+subscription/.../bug_filters'
341 date_created: u'...'
342 date_last_updated: u'...'
343+ id: ...
344 resource_type_link: u'http://.../#structural_subscription'
345- self_link: u'http://.../fooix/+subscription/eric'
346+ self_link: u'http://.../fooix/+subscription/...'
347 subscribed_by_link: u'http://.../~eric'
348 subscriber_link: u'http://.../~eric'
349 target_link: u'http://.../fooix'
350@@ -96,7 +99,8 @@
351 ... subscriber=webservice.getAbsoluteUrl('/~pythons'))
352 HTTP/1.1 401 Unauthorized
353 ...
354- UserCannotSubscribePerson: eric does not have permission to subscribe pythons.
355+ UserCannotSubscribePerson: eric does not have permission
356+ to subscribe pythons.
357 <BLANKLINE>
358
359 Oops, Eric isn't a team admin. Eric gets Michael to try, since he is an
360@@ -110,7 +114,7 @@
361 ... subscriber=webservice.getAbsoluteUrl('/~pythons'))
362 HTTP/1.1 201 Created
363 ...
364- Location: http://.../fooix/+subscription/pythons
365+ Location: http://.../fooix/+subscription/...
366 ...
367
368 >>> subscriptions = webservice.named_get(
369@@ -119,11 +123,12 @@
370 start: 0
371 total_size: 1
372 ---
373- bug_filters_collection_link: u'.../fooix/+subscription/pythons/bug_filters'
374+ bug_filters_collection_link: u'.../fooix/+subscription/.../bug_filters'
375 date_created: u'...'
376 date_last_updated: u'...'
377+ id: ...
378 resource_type_link: u'http://.../#structural_subscription'
379- self_link: u'http://.../fooix/+subscription/pythons'
380+ self_link: u'http://.../fooix/+subscription/...'
381 subscribed_by_link: u'http://.../~michael'
382 subscriber_link: u'http://.../~pythons'
383 target_link: u'http://.../fooix'
384@@ -136,7 +141,8 @@
385 ... subscriber=webservice.getAbsoluteUrl('/~pythons'))
386 HTTP/1.1 401 Unauthorized
387 ...
388- UserCannotSubscribePerson: eric does not have permission to unsubscribe pythons.
389+ UserCannotSubscribePerson: eric does not have permission
390+ to unsubscribe pythons.
391 <BLANKLINE>
392
393 Michael can, though.

Subscribers

People subscribed via source and target branches

to status/vote changes: