launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/pageids into lp:launchpad.

Commit message:
Make sensible pageids for help folders and ++model++.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1005730 in Launchpad itself: "bad pageid breaks page performance report"
  https://bugs.launchpad.net/launchpad/+bug/1005730

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/pageids/+merge/126081

The page performance report cannot handle Page-Ids with spaces. Launchpad's
standard views always provide sensible __name__ and __class__.__name__ attrs
to construct them. Two cases where views are dynamically generated for
templates and resources do not provide the expected attributes, or the
code does not know how to find the data.

--------------------------------------------------------------------
RULES

    Pre-implementation: no one
    * HelpFolders are missing the __name__ and that is trivial to register
      with the dynamic view class.
    * The ++model++ example is a case were the context is also a view, and
      it is dynamically generated from a page template. Either the
      registration can modified, or constructPageID() must learn how to
      handle the recursive issue.
      * Handling at least one-level of recursion will prevent future breaks
        for future dynamic view classes from happening.


QA

    * Visit https://blueprints.qastaging.launchpad.net/+help-blueprints/workitems-help.html/++profile++show
    * Verify the Page-Id is just "RootObject:+help-blueprints", not dir
      path
    * Visit https://bugs.qastaging.launchpad.net/ubuntu/+bugs/++model++
    * Expect a timeout, view the oops.
    * Verify the Page-id is Distribution:+bugs:JsonModelNamespaceView,
      no path to the template.


LINT

    lib/lp/services/inlinehelp/README.txt
    lib/lp/services/inlinehelp/zcml.py
    lib/lp/services/webapp/publication.py
    lib/lp/services/webapp/doc/webapp-publication.txt
    lib/lp/services/webapp/tests/test_pageid.py


LoC:
    This add about 40 lines of code, but I have a 3000+ line credit.


TEST

    ./bin/test -vvc -t README lp.services.inlinehelp
    ./bin/test -vvc -t PageID lp.services.webapp.tests.test_pageid
    ./bin/test -vvc -t webapp-publication.txt lp.services.webapp.tests.test_doc


IMPLEMENTATION

Use the name from the zcml as the help folder's __name__ attr.
    lib/lp/services/inlinehelp/README.txt
    lib/lp/services/inlinehelp/zcml.py

Recuse through constructPageID() when the context is a dynamic view
created from a page template. I converted from doctests to unittests.
    lib/lp/services/webapp/publication.py
    lib/lp/services/webapp/doc/webapp-publication.txt
    lib/lp/services/webapp/tests/test_pageid.py
-- 
https://code.launchpad.net/~sinzui/launchpad/pageids/+merge/126081
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/pageids into lp:launchpad.

=== modified file 'lib/lp/services/inlinehelp/README.txt'
--- lib/lp/services/inlinehelp/README.txt	2011-12-30 09:00:06 +0000
+++ lib/lp/services/inlinehelp/README.txt	2012-09-24 19:52:23 +0000
@@ -45,13 +45,19 @@
 
     >>> from zope.component import queryMultiAdapter
     >>> from lp.services.webapp.publisher import rootObject
-    >>> help = queryMultiAdapter((rootObject, request), name="+help")
-
-    >>> help.folder == help_folder
-    True
-
-    >>> isinstance(help, HelpFolder)
-    True
+    >>> help_view = queryMultiAdapter((rootObject, request), name="+help")
+
+    >>> help_view.folder == help_folder
+    True
+
+    >>> isinstance(help_view, HelpFolder)
+    True
+
+    >>> print help_view.__name__
+    +help
+
+    >>> print help_view.__class__.__name__
+    +help for /tmp/help...
 
 
 Cleanup

=== modified file 'lib/lp/services/inlinehelp/zcml.py'
--- lib/lp/services/inlinehelp/zcml.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/inlinehelp/zcml.py	2012-09-24 19:52:23 +0000
@@ -37,7 +37,8 @@
     """Create a help folder subclass and register it with the ZCA."""
 
     help_folder = type(
-        str('%s for %s' % (name, folder)), (HelpFolder, ), {'folder': folder})
+        str('%s for %s' % (name, folder)), (HelpFolder, ),
+        {'folder': folder, '__name__': name})
 
     defineChecker(
         help_folder,

=== modified file 'lib/lp/services/webapp/doc/webapp-publication.txt'
--- lib/lp/services/webapp/doc/webapp-publication.txt	2012-08-14 23:37:21 +0000
+++ lib/lp/services/webapp/doc/webapp-publication.txt	2012-09-24 19:52:23 +0000
@@ -623,39 +623,6 @@
     >>> print request._orig_env['launchpad.pageid']
     TestContext:TestView
 
-When a view is registered using ZCML, the Zope machinery will store
-the name under which the view was registered in the view's '__name__'
-attribute. For these views, that's the name that will be used in
-the pageid.
-
-    >>> view.__name__ = '+test'
-    >>> publication.callObject(request, view)
-    u'Result'
-    >>> print request._orig_env['launchpad.pageid']
-    TestContext:+test
-
-Views registered through ZCML using the attribute property, are really a
-bounded method. These views have the same pageid as their class.
-
-    >>> del request._orig_env['launchpad.pageid']
-    >>> publication.callObject(request, view.__call__)
-    u'Result'
-    >>> print request._orig_env['launchpad.pageid']
-    TestContext:+test
-
-If the published object doesn't have a context attribute (unlike most
-views), the pageid will be empty.
-
-    >>> request, publication = get_request_and_publication()
-    >>> request.setPrincipal(auth_utility.unauthenticatedPrincipal())
-
-    >>> def callable_publication():
-    ...     return u'Result'
-    >>> publication.callObject(request, callable_publication)
-    u'Result'
-    >>> request._orig_env['launchpad.pageid']
-    ''
-
 
 Tick counts
 -----------

=== modified file 'lib/lp/services/webapp/publication.py'
--- lib/lp/services/webapp/publication.py	2012-08-09 03:36:26 +0000
+++ lib/lp/services/webapp/publication.py	2012-09-24 19:52:23 +0000
@@ -14,6 +14,7 @@
 import traceback
 import urllib
 
+from lazr.restful.utils import safe_hasattr
 from lazr.uri import (
     InvalidURIError,
     URI,
@@ -380,7 +381,7 @@
             uri = uri.replace(query=query_string)
         return str(uri)
 
-    def constructPageID(self, view, context):
+    def constructPageID(self, view, context, view_names=()):
         """Given a view, figure out what its page ID should be.
 
         This provides a hook point for subclasses to override.
@@ -392,11 +393,18 @@
             # is accessible in the instance __name__ attribute. We use
             # that if it's available, otherwise fall back to the class
             # name.
-            if getattr(view, '__name__', None) is not None:
+            if safe_hasattr(view, '__name__'):
                 view_name = view.__name__
             else:
                 view_name = view.__class__.__name__
-            pageid = '%s:%s' % (context.__class__.__name__, view_name)
+            names = [view_name] + list(view_names)
+            context_name = context.__class__.__name__
+            if ' ' in context_name and safe_hasattr(context, 'context'):
+                # This is a view of a generated view class,
+                # such as ++model++ view of Product:+bugs. Recuse!
+                return self.constructPageID(context, context.context, names)
+            view_names = ':'.join(names)
+            pageid = '%s:%s' % (context_name, view_names)
         # The view name used in the pageid usually comes from ZCML and so
         # it will be a unicode string although it shouldn't.  To avoid
         # problems we encode it into ASCII.

=== modified file 'lib/lp/services/webapp/tests/test_pageid.py'
--- lib/lp/services/webapp/tests/test_pageid.py	2011-12-24 17:49:30 +0000
+++ lib/lp/services/webapp/tests/test_pageid.py	2012-09-24 19:52:23 +0000
@@ -8,6 +8,7 @@
 from lazr.restful.interfaces import ICollectionResource
 from zope.interface import implements
 
+from lp.services.webapp.publication import LaunchpadBrowserPublication
 from lp.services.webapp.servers import WebServicePublication
 from lp.testing import TestCase
 
@@ -34,6 +35,9 @@
         self.context = FakeContext()
         self.request = FakeRequest()
 
+    def __call__(self):
+        return 'result'
+
 
 class FakeCollectionResourceView(FakeView):
     """A view object that provides ICollectionResource."""
@@ -45,6 +49,56 @@
             u'https://launchpad.dev/api/devel/#milestone-page-resource')
 
 
+class LaunchpadBrowserPublicationPageIDTestCase(TestCase):
+    """Ensure that the web service enhances the page ID correctly."""
+
+    def setUp(self):
+        super(LaunchpadBrowserPublicationPageIDTestCase, self).setUp()
+        self.publication = LaunchpadBrowserPublication(db=None)
+        self.view = FakeView()
+        self.context = FakeContext()
+
+    def test_pageid_without_context(self):
+        # The pageid is an empty string if there is no context.
+        self.assertEqual('', self.publication.constructPageID(self.view, None))
+
+    def test_pageid_view_without_name(self):
+        # The view. __class__.__name__ is used if the view does not have a
+        # __name__ attribute.
+        self.assertEqual(
+            'FakeContext:FakeView',
+            self.publication.constructPageID(self.view, self.context))
+
+    def test_pageid_view_with_name(self):
+        # The view.__name__ is used when it exists.
+        self.view.__name__ = '+snarf'
+        self.assertEqual(
+            'FakeContext:+snarf',
+            self.publication.constructPageID(self.view, self.context))
+
+    def test_pageid_context_is_view_from_template(self):
+        # When the context is a dynamic view class of a page template,
+        # such as adapting a form view to ++model++, the method recurses
+        # the views to locate the true context.
+        class FakeView2(FakeView):
+            pass
+
+        class FakeViewView(FakeView):
+            __name__ = '++model++'
+
+            def __init__(self):
+                self.request = FakeRequest()
+                self.context = FakeView2()
+
+        self.view = FakeViewView()
+        self.context = self.view.context
+        self.context.__name__ = '+bugs'
+        self.context.__class__.__name__ = 'SimpleViewClass from template.pt'
+        self.assertEqual(
+            'FakeContext:+bugs:++model++',
+            self.publication.constructPageID(self.view, self.context))
+
+
 class TestWebServicePageIDs(TestCase):
     """Ensure that the web service enhances the page ID correctly."""