launchpad-reviewers team mailing list archive

[Guilherme Salgado <salgado@xxxxxxxxxxxxx>]

Guilherme Salgado has proposed merging lp:~salgado/launchpad/layer-specific-navigation into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This branch makes it possible to register navigation classes for a
specific layer.

This is to allow us to use a different navigation class for, say,
IDistribution on the vostok vhost.

One issue I've encountered is that the navigation directive will
unconditionally register the nav classes for IXMLRPCRequest (the layer
of the xmlrpc vhost) as well, and that will cause a
ConfigurationConflict when you have more than one navigation class used
for a given context. One solution to this is to add yet another
parameter to the navigation directive (used_for_xmlrpc=True) and use
that to decide whether or not to register the nav class for
IXMLRPCRequest.

I'm not particularly happy with that solution as it doesn't make it
possible for vostok to overwrite the navigation that is used on the
xmlrpc vhost, but that may not be a problem in practice as we need
custom navigation classes only because we want to remove traversals from
the existing ones.
-- 
https://code.launchpad.net/~salgado/launchpad/layer-specific-navigation/+merge/32339
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~salgado/launchpad/layer-specific-navigation into lp:launchpad.

=== modified file 'lib/canonical/launchpad/doc/navigation.txt'
--- lib/canonical/launchpad/doc/navigation.txt	2010-08-02 02:13:52 +0000
+++ lib/canonical/launchpad/doc/navigation.txt	2010-08-11 15:19:44 +0000
@@ -234,7 +234,7 @@
 
 == ZCML for browser:navigation ==
 
-The zcml processor `browser:navigation` processes navigation classes.
+The zcml processor `browser:navigation` registers navigation classes.
 
     >>> class ThingSetView:
     ...
@@ -266,6 +266,14 @@
     ... </configure>
     ... """)
 
+Once registered, we can look the navigation up using getMultiAdapter().
+
+    >>> from zope.component import getMultiAdapter
+    >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+    >>> from zope.publisher.interfaces.browser import IBrowserPublisher
+    >>> navigation = getMultiAdapter(
+    ...     (thingset, LaunchpadTestRequest()), IBrowserPublisher, name='')
+
 This time, we get the view object for the page that was registered.
 
     >>> navigation.publishTraverse(request, 'thingview')

=== modified file 'lib/canonical/launchpad/webapp/metazcml.py'
--- lib/canonical/launchpad/webapp/metazcml.py	2010-08-05 21:53:44 +0000
+++ lib/canonical/launchpad/webapp/metazcml.py	2010-08-11 15:19:44 +0000
@@ -20,7 +20,7 @@
 from zope.publisher.interfaces.browser import (
     IBrowserPublisher, IBrowserRequest, IDefaultBrowserLayer)
 from zope.publisher.interfaces.xmlrpc import IXMLRPCRequest
-from zope.schema import TextLine
+from zope.schema import Bool, TextLine
 from zope.security.checker import Checker, CheckerPublic
 from zope.security.interfaces import IPermission
 from zope.security.permission import Permission
@@ -193,6 +193,16 @@
 class INavigationDirective(IGlueDirective):
     """Hook up traversal etc."""
 
+    layer = GlobalInterface(
+        title=u"The layer where this navigation is going to be available.",
+        required=False)
+
+    used_for_xmlrpc = Bool(
+        title=u"Is this the navigation used for XMLRPC requests as well?",
+        description=(u"For any given context, only one navigation class can "
+                     "be used for XMLRPC requests."),
+        required=False)
+
 
 class IFeedsDirective(IGlueDirective):
     """Hook up feeds."""
@@ -267,7 +277,8 @@
                           layer=layer, class_=feedclass)
 
 
-def navigation(_context, module, classes):
+def navigation(_context, module, classes, layer=IDefaultBrowserLayer,
+               used_for_xmlrpc=True):
     """Handler for the `INavigationDirective`."""
     if not inspect.ismodule(module):
         raise TypeError("module attribute must be a module: %s, %s" %
@@ -281,19 +292,17 @@
         for_ = [navclass.usedfor]
 
         # Register the navigation as the traversal component.
-        layer = IDefaultBrowserLayer
         provides = IBrowserPublisher
         name = ''
         view(_context, factory, layer, name, for_,
-                permission=PublicPermission, provides=provides,
-                allowed_interface=[IBrowserPublisher])
-        #view(_context, factory, layer, name, for_,
-        #     permission=PublicPermission, provides=provides)
+             permission=PublicPermission, provides=provides,
+             allowed_interface=[IBrowserPublisher])
 
         # Also register the navigation as a traversal component for XMLRPC.
-        xmlrpc_layer = IXMLRPCRequest
-        view(_context, factory, xmlrpc_layer, name, for_,
-             permission=PublicPermission, provides=provides)
+        if used_for_xmlrpc:
+            xmlrpc_layer = IXMLRPCRequest
+            view(_context, factory, xmlrpc_layer, name, for_,
+                 permission=PublicPermission, provides=provides)
 
 
 class InterfaceInstanceDispatcher:

=== added file 'lib/canonical/launchpad/webapp/tests/test_navigation.py'
--- lib/canonical/launchpad/webapp/tests/test_navigation.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_navigation.py	2010-08-11 15:19:44 +0000
@@ -0,0 +1,109 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from zope.component import ComponentLookupError, getMultiAdapter
+from zope.configuration import xmlconfig
+from zope.interface import implements, Interface
+from zope.publisher.interfaces.browser import (
+    IBrowserPublisher, IDefaultBrowserLayer)
+from zope.testing.cleanup import cleanUp
+
+from canonical.launchpad.webapp import Navigation
+
+from lp.testing import TestCase
+
+
+class TestNavigationDirective(TestCase):
+
+    def test_default_layer(self):
+        # By default all navigation classes are registered for
+        # IDefaultBrowserLayer.
+        directive = """ 
+            <browser:navigation
+                module="%(this)s" classes="ThingNavigation"/>
+            """ % dict(this=this)
+        xmlconfig.string(zcml_configure % directive)
+        navigation = getMultiAdapter(
+            (Thing(), DefaultBrowserLayer()), IBrowserPublisher, name='')
+        self.assertIsInstance(navigation, ThingNavigation)
+
+    def test_specific_layer(self):
+        # If we specify a layer when registering a navigation class, it will
+        # only be available on that layer.
+        directive = """
+            <browser:navigation
+                module="%(this)s" classes="OtherThingNavigation"
+                layer="%(this)s.IOtherLayer" />
+            """ % dict(this=this)
+        xmlconfig.string(zcml_configure % directive)
+        self.assertRaises(
+            ComponentLookupError,
+            getMultiAdapter,
+            (Thing(), DefaultBrowserLayer()), IBrowserPublisher, name='')
+
+        navigation = getMultiAdapter(
+            (Thing(), OtherLayer()), IBrowserPublisher, name='')
+        self.assertIsInstance(navigation, OtherThingNavigation)
+
+    def test_multiple_navigations_for_single_context(self):
+        # It is possible to have multiple navigation classes for a given
+        # context class as long as they specify different layers and only one
+        # of them is used for XMLRPC requests.
+        directive = """ 
+            <browser:navigation
+                module="%(this)s" classes="ThingNavigation"/>
+            <browser:navigation
+                module="%(this)s" classes="OtherThingNavigation"
+                layer="%(this)s.IOtherLayer"
+                used_for_xmlrpc="false" />
+            """ % dict(this=this)
+        xmlconfig.string(zcml_configure % directive)
+
+        navigation = getMultiAdapter(
+            (Thing(), DefaultBrowserLayer()), IBrowserPublisher, name='')
+        other_navigation = getMultiAdapter(
+            (Thing(), OtherLayer()), IBrowserPublisher, name='')
+        self.assertNotEqual(navigation, other_navigation)
+
+    def tearDown(self):
+        TestCase.tearDown(self)
+        cleanUp()
+
+
+class DefaultBrowserLayer:
+    implements(IDefaultBrowserLayer)
+
+
+class IThing(Interface):
+    pass
+
+
+class Thing(object):
+    implements(IThing)
+
+
+class ThingNavigation(Navigation):
+    usedfor = IThing
+
+
+class OtherThingNavigation(Navigation):
+    usedfor = IThing
+
+
+class IOtherLayer(Interface):
+    pass
+
+
+class OtherLayer:
+    implements(IOtherLayer)
+
+
+this = "canonical.launchpad.webapp.tests.test_navigation"
+zcml_configure = """
+    <configure xmlns:browser="http://namespaces.zope.org/browser";>
+      <include package="canonical.launchpad.webapp" file="meta.zcml" />
+      %s
+    </configure>
+    """