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

[Данило Шеган]

[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).

= 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

[Graham Binns (gmb)]

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)

[Gary Poster (gary)]

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.

=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml	2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/browser/configure.zcml	2011-01-31 09:32:26 +0000
@@ -1195,7 +1195,7 @@
 
     <browser:url
         for="lp.bugs.interfaces.structuralsubscription.IStructuralSubscription"
-        path_expression="string:+subscription/${subscriber/name}"
+        path_expression="string:+subscription/${id}"
         attribute_to_parent="target"/>
     <browser:navigation
         module="lp.bugs.browser.structuralsubscription"
 
=== 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-31 09:32:26 +0000
@@ -12,7 +12,6 @@
 
 from operator import attrgetter
 
-from zope.component import getUtility
 from zope.formlib import form
 from zope.schema import (
     Choice,
@@ -48,7 +47,6 @@
     IDistributionSourcePackage,
     )
 from lp.registry.interfaces.milestone import IProjectGroupMilestone
-from lp.registry.interfaces.person import IPersonSet
 from lp.services.propertycache import cachedproperty
 
 
@@ -347,8 +345,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_bugsubscriptionfilter.py'
--- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-01-31 09:32:26 +0000
@@ -61,7 +61,8 @@
         url = urlparse(canonical_url(self.subscription_filter))
         self.assertThat(url.hostname, StartsWith("bugs."))
         self.assertEqual(
-            "/bar/+subscription/foo/+filter/%d" % (
+            "/bar/+subscription/%d/+filter/%d" % (
+                self.subscription.id,
                 self.subscription_filter.id),
             url.path)
 
 
=== 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-31 09:32:26 +0000
@@ -61,10 +61,12 @@
         super(StructuralSubscriptionTraversalTestBase, self).setUp()
         login('[e-mail]')
         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,31 @@
             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.subscription_target = self.factory.makeProduct(
+            owner=self.owner, name=u"bar")
+        with person_logged_in(self.owner):
+            self.subscription = self.subscription_target.addBugSubscription(
+                self.owner, self.owner)
+        transaction.commit()
+        self.service = self.factory.makeLaunchpadService(self.owner)
+        self.ws_subscription_target = ws_object(
+            self.service, self.subscription_target)
+        self.ws_subscription = ws_object(self.service, self.subscription)
+
+    def test_getSubscriptionByID(self):
+        self.assertEqual(
+            self.ws_subscription,
+            self.ws_subscription_target.getSubscriptionByID(
+                subscription_id=self.subscription.id))
+
+
 class TestStructuralSubscriptionAPI(TestCaseWithFactory):
 
     layer = AppServerLayer
@@ -371,10 +406,10 @@
     def setUp(self):
         super(TestStructuralSubscriptionAPI, self).setUp()
         self.owner = self.factory.makePerson(name=u"foo")
-        self.structure = self.factory.makeProduct(
+        self.subscription_target = self.factory.makeProduct(
             owner=self.owner, name=u"bar")
         with person_logged_in(self.owner):
-            self.subscription = self.structure.addBugSubscription(
+            self.subscription = self.subscription_target.addBugSubscription(
                 self.owner, self.owner)
         transaction.commit()
         self.service = self.factory.makeLaunchpadService(self.owner)
 
=== 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-31 09:32:26 +0000
@@ -15,10 +15,6 @@
     'IStructuralSubscriptionTargetHelper',
     ]
 
-from lazr.enum import (
-    DBEnumeratedType,
-    DBItem,
-    )
 from lazr.restful.declarations import (
     call_with,
     export_as_webservice_entry,
@@ -58,7 +54,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 +147,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-31 09:32:26 +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-31 09:32:26 +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/stories/person/xx-person-subscriptions.txt'
--- lib/lp/registry/stories/person/xx-person-subscriptions.txt	2011-01-13 16:28:49 +0000
+++ lib/lp/registry/stories/person/xx-person-subscriptions.txt	2011-01-31 09:32:26 +0000
@@ -236,9 +236,9 @@
     ...     "http://launchpad.dev/people/+me/+structural-subscriptions")
     >>> show_create_links(admin_browser)
     mozilla-firefox in ubuntu
-    * Create a new filter --> /ubuntu/.../name16/+new-filter
+    * Create a new filter --> /ubuntu/.../+new-filter
     pmount in ubuntu
-    * Create a new filter --> /ubuntu/.../name16/+new-filter
+    * Create a new filter --> /ubuntu/.../+new-filter
 
 If the user does not have the necessary rights to create new bug
 filters the "Create" link is not shown.
 
=== 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-31 09:32:26 +0000
@@ -1,4 +1,5 @@
-= Structural Subscriptions =
+Structural Subscriptions
+========================
 
 Structural subscriptions can be obtained from any target: a project,
 project series, project group, distribution, distribution series or
@@ -37,7 +38,7 @@
     ...     '/fooix', 'addBugSubscription')
     HTTP/1.1 201 Created
     ...
-    Location: http://.../fooix/+subscription/eric
+    Location: http://.../fooix/+subscription/...
     ...
 
     >>> subscriptions = webservice.named_get(
@@ -46,11 +47,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 +63,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'
@@ -96,7 +99,8 @@
     ...     subscriber=webservice.getAbsoluteUrl('/~pythons'))
     HTTP/1.1 401 Unauthorized
     ...
-    UserCannotSubscribePerson: eric does not have permission to subscribe pythons.
+    UserCannotSubscribePerson: eric does not have permission
+    to subscribe pythons.
     <BLANKLINE>
 
 Oops, Eric isn't a team admin. Eric gets Michael to try, since he is an
@@ -110,7 +114,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 +123,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'
@@ -136,7 +141,8 @@
     ...     subscriber=webservice.getAbsoluteUrl('/~pythons'))
     HTTP/1.1 401 Unauthorized
     ...
-    UserCannotSubscribePerson: eric does not have permission to unsubscribe pythons.
+    UserCannotSubscribePerson: eric does not have permission
+    to unsubscribe pythons.
     <BLANKLINE>
 
 Michael can, though.