launchpad-reviewers team mailing list archive

[Diogo Matsubara <matsubara@xxxxxxxxxxxxx>]

Diogo Matsubara has proposed merging lp:~matsubara/launchpad/bug-606184-pageid-for-collections into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #606184 API Pageid for collections is 'scopedcollection:collectionresource'  which does not mention the origin page id
  https://bugs.launchpad.net/bugs/606184


== Summary ==

This branch fixes bug 606184 by including the origin page resource in the page
id for ICollectionResource views.

== Proposed fix ==

When the page id is generated for API requests, the code inspects if the view
is a ICollectionResource, then inspects the type_url attribute and add part of
it as the collection identifier to the page id.


== Pre-implementation notes ==

I had a pre-imp with Gary. Initially Gary suggested that this could be
accomplished with adapters and I tried that route but got stuck. Then we
agreed to change the implementation to be simpler so I could move this forward.


== Implementation details ==

The origin page resource comes from ICollectionResource.type_url, so I used
that as the identifier for the given collection and added that as part of the
page id. That way when engineers are analysing such OOPS summaries they'll be
able to easily see if the OOPS is in their domain or not, as requested in
the bug report.


== Tests ==

bin/test -tt test_pageid

== Demo and Q/A ==

Once the code lands on staging, it's enough to check a staging oops summary
and see if the "scopedcollection:collectionresource" oopses contain the
additional information about the origin page resource.

== Lint ==

No lint left over in the two files modified by this branch.

-- 
https://code.launchpad.net/~matsubara/launchpad/bug-606184-pageid-for-collections/+merge/33801
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~matsubara/launchpad/bug-606184-pageid-for-collections into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/servers.py	2010-08-26 16:32:55 +0000
@@ -63,6 +63,18 @@
 
 from canonical.cachedproperty import cachedproperty
 from canonical.config import config
+<<<<<<< TREE
+=======
+
+from canonical.lazr.interfaces.feed import IFeed
+from lazr.restful.interfaces import (
+    ICollectionResource, IWebServiceConfiguration, IWebServiceVersion)
+from lazr.restful.publisher import (
+    WebServicePublicationMixin, WebServiceRequestTraversal)
+
+from lp.app.errors import UnexpectedFormData
+from lp.testopenid.interfaces.server import ITestOpenIDApplication
+>>>>>>> MERGE-SOURCE
 from canonical.launchpad.interfaces.launchpad import (
     IFeedsApplication,
     IPrivateApplication,
@@ -1154,6 +1166,10 @@
         """Add the web service named operation (if any) to the page ID."""
         pageid = super(WebServicePublication, self).constructPageID(
             view, context)
+        if ICollectionResource.providedBy(view):
+            collection_identifier = view.type_url.split('/')[-1]
+            if collection_identifier:
+                pageid += ':' + collection_identifier
         op = (view.request.get('ws.op')
             or view.request.query_string_params.get('ws.op'))
         if op:

=== modified file 'lib/canonical/launchpad/webapp/tests/test_pageid.py'
--- lib/canonical/launchpad/webapp/tests/test_pageid.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/tests/test_pageid.py	2010-08-26 16:32:55 +0000
@@ -5,7 +5,8 @@
 __metaclass__ = type
 
 
-import unittest
+from zope.interface import implements
+from lazr.restful.interfaces import ICollectionResource
 
 from canonical.launchpad.webapp.servers import WebServicePublication
 from lp.testing import TestCase
@@ -34,6 +35,16 @@
         self.request = FakeRequest()
 
 
+class FakeCollectionResourceView(FakeView):
+    """A view object that provides ICollectionResource."""
+    implements(ICollectionResource)
+
+    def __init__(self):
+        super(FakeCollectionResourceView, self).__init__()
+        self.type_url = (
+            u'https://launchpad.dev/api/devel/#origin-page-resource')
+
+
 class TestWebServicePageIDs(TestCase):
     """Ensure that the web service enhances the page ID correctly."""
 
@@ -68,3 +79,23 @@
         self.view.request.query_string_params['ws.op'] = 'operation-name-2'
         self.assertEqual(
             self.makePageID(), 'FakeContext:FakeView:operation-name-2')
+
+
+class TestCollectionResourcePageIDs(TestCase):
+    """Ensure page ids for collections display the origin page resource."""
+
+    def setUp(self):
+        super(TestCollectionResourcePageIDs, self).setUp()
+        self.publication = WebServicePublication(db=None)
+        self.view = FakeCollectionResourceView()
+        self.context = FakeContext()
+
+    def makePageID(self):
+        return self.publication.constructPageID(self.view, self.context)
+
+    def test_origin_pageid_for_collection(self):
+        # When the view provides a ICollectionResource, make sure the origin
+        # page resource is included in the page ID.
+        self.assertEqual(
+            self.makePageID(),
+            'FakeContext:FakeCollectionResourceView:#origin-page-resource')