launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/sharing-batch-bav-error-978212 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #978212 in Launchpad itself: "Sharing batch navigation shows ugly 404 error."
  https://bugs.launchpad.net/launchpad/+bug/978212

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-batch-bav-error-978212/+merge/101685

== Implementation ==

If we have a url like lp.net/foo/bar and we have defined a stepthrough for "foo", then the traversal machinery will attempt to dispatch the next path segment "bar" to the stepthrough function. 

However, if the url is something like lp.net/foo/++model++ then because the next path segment is a zope namespace, we don't want to interpret the path as a stepthrough at all. 

This branch tweaks the traversal machinery to handle the above. I needed to add a new peek() function to StepsToGo also.

This fixes batch navigation on the +sharing page.

== Tests ==

I couldn't see any existing tests for any of the stepthrough functionality so I added a new test case for the things changed in this branch, plus a very simple test for the basic stepthrough traversal.

lp/testing/tests/test_publication.py - TestStepThrough

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/webapp/publisher.py
  lib/lp/services/webapp/servers.py
  lib/lp/testing/tests/test_publication.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/sharing-batch-bav-error-978212/+merge/101685
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-batch-bav-error-978212 into lp:launchpad.

=== modified file 'lib/lp/services/webapp/publisher.py'
--- lib/lp/services/webapp/publisher.py	2012-02-15 22:09:43 +0000
+++ lib/lp/services/webapp/publisher.py	2012-04-12 06:12:25 +0000
@@ -27,6 +27,7 @@
     ]
 
 import httplib
+import re
 
 from lazr.restful import (
     EntryResource,
@@ -91,6 +92,9 @@
 # from NotFound in web service calls.
 error_status(httplib.NOT_FOUND)(NotFound)
 
+# Used to match zope namespaces eg ++model++.
+RESERVED_NAMESPACE = re.compile('\\+\\+.*\\+\\+')
+
 
 class DecoratorAdvisor:
     """Base class for a function decorator that adds class advice.
@@ -906,19 +910,25 @@
         # If so, see if the name is in the namespace_traversals, and if so,
         # dispatch to the appropriate function.  We can optimise by changing
         # the order of these checks around a bit.
+        # If the next path step is a zope namespace eg ++model++, then we
+        # actually do not want to process the path steps as a stepthrough
+        # traversal so we just ignore it here.
         namespace_traversals = self.stepthrough_traversals
         if namespace_traversals is not None:
             if name in namespace_traversals:
                 stepstogo = request.stepstogo
                 if stepstogo:
-                    nextstep = stepstogo.consume()
-                    handler = namespace_traversals[name]
-                    try:
-                        nextobj = handler(self, nextstep)
-                    except NotFoundError:
-                        nextobj = None
-                    return self._handle_next_object(nextobj, request,
-                        nextstep)
+                    # First peek at the nextstep to see if we should ignore it.
+                    nextstep = stepstogo.peek()
+                    if not RESERVED_NAMESPACE.match(nextstep):
+                        nextstep = stepstogo.consume()
+                        handler = namespace_traversals[name]
+                        try:
+                            nextobj = handler(self, nextstep)
+                        except NotFoundError:
+                            nextobj = None
+                        return self._handle_next_object(nextobj, request,
+                            nextstep)
 
         # Next, look up views on the context object.  If a view exists,
         # use it.

=== modified file 'lib/lp/services/webapp/servers.py'
--- lib/lp/services/webapp/servers.py	2012-02-29 10:34:02 +0000
+++ lib/lp/services/webapp/servers.py	2012-04-12 06:12:25 +0000
@@ -183,6 +183,17 @@
         self.request.setTraversalStack(stack)
         return nextstep
 
+    def peek(self):
+        """Return the next path step without removing it.
+
+        Returns None if there are no path steps left.
+        """
+        stack = self.request.getTraversalStack()
+        try:
+            return stack[-1]
+        except IndexError:
+            return None
+
     def next(self):
         value = self.consume()
         if value is None:

=== modified file 'lib/lp/testing/tests/test_publication.py'
--- lib/lp/testing/tests/test_publication.py	2012-01-01 02:58:52 +0000
+++ lib/lp/testing/tests/test_publication.py	2012-04-12 06:12:25 +0000
@@ -23,15 +23,23 @@
     ILaunchBag,
     ILaunchpadRoot,
     )
-from lp.services.webapp.publisher import get_current_browser_request
+from lp.services.webapp.publisher import (
+    get_current_browser_request,
+    Navigation,
+    stepthrough,
+    )
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
     ANONYMOUS,
+    FakeLaunchpadRequest,
     login,
     login_person,
     TestCaseWithFactory,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    FunctionalLayer,
+    )
 from lp.testing.publication import test_traverse
 
 
@@ -117,3 +125,40 @@
             'http://api.launchpad.dev/devel/' + product.name)
         self.assertEqual(product, context)
         self.assertIsInstance(view, EntryResource)
+
+
+class DummyNavigation(Navigation):
+    """A simple navigation class to test traversal."""
+    def traverse(self, name):
+        return name
+
+    @stepthrough('+step')
+    def traverse_stepthrough(self, name):
+        return 'stepthrough-' + name
+
+
+class TestStepThrough(TestCaseWithFactory):
+    """Test some stepthrough traversal scenarios."""
+
+    layer = FunctionalLayer
+
+    def traverse(self, request, name):
+        """Traverse to 'segments' using a 'DummyNavigation' object.
+
+        Using the Zope traversal machinery, traverse to the path given by
+        'segments'.
+        """
+        traverser = DummyNavigation(object(), request)
+        return traverser.publishTraverse(request, name)
+
+    def test_normal_stepthrough(self):
+        # The stepthrough is processed normally.
+        request = FakeLaunchpadRequest(['~dummy'], ['fred'])
+        self.assertEqual('stepthrough-fred', self.traverse(request, '+step'))
+
+    def test_ignored_stepthrough(self):
+        # The stepthrough is ignored since the next path item is a zope
+        # namespace.
+        request = FakeLaunchpadRequest(['~dummy'], ['++model++'])
+        self.assertEqual('+step', self.traverse(request, '+step'))
+        self.assertEqual('++model++', request.stepstogo.peek())