Merge lp:~bac/launchpad/getnewcache into lp:launchpad

[Brad Crittenden]

Provide namespace ++model++ for retrieving a view's JSON cache.

= Summary =

Add a namespace ++model++ so that using it returns a fresh JSON cache for the object or view referenced.

== Pre-implementation notes ==

Lots of conferring with Aaron and Gary.

== Tests ==

bin/test -vvt '(test_publisher|test_view_model)'

== Demo and Q/A ==

Go to any Launchpad page and append ++model++, e.g.
https://launchpad.net/~bac/++model++
https://launchpad.net/~bac/+edit/++model++

[Benji York (benji)]

This branch looks good. I have a couple of concerns that I'd like you
to take under consideration.

We should do a quick (but not overly taxing) audit to identify existing
views that populate the JSON cache post-initialization and attempt to
fix them.

We should also consider a mechanism to "close" the JSON cache after view
initialization to prevent people from accidentally putting data in it
too late.

In launchpad-model.pt (line 121 of the diff): we probably want more
mellow header text.

review: Approve (code)

[Brad Crittenden (bac)]

Thanks for the review Benji.

I did a quick look at our use of IJSONRequestCache and see that all uses are within view.initialize() ... except for one. The new ILongPollSubscriber adapter does not work within initialize and seems adamant about not doing it.

Gary and I talked about ways to force compliance but decided against it reasoning that an unenforced contract is sufficient. An explanation of expectations should be added to the namespace definition so that developers will understand the restriction.

Does that sound reasonable to you? If we run into problems in practice we can investigate a more robust approach.

[Benji York (benji)]

On Wed, Jul 27, 2011 at 11:22 AM, Brad Crittenden <email address hidden> wrote:
> Gary and I talked about ways to force compliance but decided against
> it reasoning that an unenforced contract is sufficient.  An
> explanation of expectations should be added to the namespace
> definition so that developers will understand the restriction.

That would be good.

> Does that sound reasonable to you?  If we run into problems in
> practice we can investigate a more robust approach.

It is reasonable. I am a little worried that with only a hand full of
users of the JSON cache we already have one that's incompatible with the
new view, but it's entirely possible that it will never rise up and hurt
us. Tie goes to the runner.
--
Benji York

=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
--- lib/canonical/launchpad/webapp/configure.zcml	2011-06-08 18:39:38 +0000
+++ lib/canonical/launchpad/webapp/configure.zcml	2011-07-27 15:26:31 +0000
@@ -440,6 +440,25 @@
         factory="canonical.launchpad.webapp.namespace.FormNamespaceView"
         />
 
+    <!-- Expose LaunchpadView methods. -->
+    <class class="canonical.launchpad.webapp.publisher.LaunchpadView">
+      <allow attributes="getCacheJson initialize" />
+    </class>
+
+    <!-- Create a namespace to render the model of any LaunchpadView-->
+    <view
+        name="model" type="*"
+        provides="zope.traversing.interfaces.ITraversable" for="*"
+        factory="canonical.launchpad.webapp.namespace.JsonModelNamespaceView"
+        permission="zope.Public"
+        />
+
+    <class class="canonical.launchpad.webapp.namespace.JsonModelNamespaceView">
+        <allow
+          attributes="__call__"
+          interface="zope.publisher.interfaces.browser.IBrowserPublisher" />
+    </class>
+
     <!-- Registrations to support +haproxy status url. -->
     <browser:page
         for="canonical.launchpad.webapp.interfaces.ILaunchpadRoot"

=== modified file 'lib/canonical/launchpad/webapp/namespace.py'
--- lib/canonical/launchpad/webapp/namespace.py	2011-05-27 21:03:22 +0000
+++ lib/canonical/launchpad/webapp/namespace.py	2011-07-27 15:26:31 +0000
@@ -1,11 +1,23 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+__metaclass__ = type
+
+__all__ = [
+    'FormNamespaceView',
+    'JsonModelNamespaceView',
+    ]
+
+
 from z3c.ptcompat import ViewPageTemplateFile
 from zope.app.pagetemplate.viewpagetemplatefile import BoundPageTemplate
+from zope.app.publisher.browser import getDefaultViewName
+from zope.component import getMultiAdapter
 from zope.security.proxy import removeSecurityProxy
 from zope.traversing.interfaces import TraversalError
 from zope.traversing.namespace import view
+from zope.interface import implements
+from zope.publisher.interfaces.browser import IBrowserPublisher
 
 from lp.app.browser.launchpadform import LaunchpadFormView
 
@@ -28,7 +40,7 @@
         context = removeSecurityProxy(self.context)
 
         if isinstance(context, LaunchpadFormView):
-            # Note: without explicitely creating the BoundPageTemplate here
+            # Note: without explicitly creating the BoundPageTemplate here
             # the view fails to render.
             context.index = BoundPageTemplate(FormNamespaceView.template,
                                               context)
@@ -36,3 +48,42 @@
             raise TraversalError("The URL does not correspond to a form.")
 
         return self.context
+
+
+class JsonModelNamespaceView(view):
+    """A namespace view to handle traversals with ++model++.
+
+    Use of this namespace is only guaranteed to work if it is fully populated
+    by a view's `initialize` method.  Any objects added after the call to
+    initialize will not be presented by the namespace.
+    """
+
+    implements(IBrowserPublisher)
+
+    def traverse(self, name, ignored):
+        """Model traversal adapter.
+
+        This adapter allows any LaunchpadView to render its JSON cache.
+        """
+        return self
+
+    def browserDefault(self, request):
+        # Tell traversal to stop, already.
+        return self, None
+
+    def __call__(self):
+        """Return the JSON cache."""
+        if IBrowserPublisher.providedBy(self.context):
+            view = self.context
+        else:
+            defaultviewname = getDefaultViewName(
+                self.context, self.request)
+            view = getMultiAdapter(
+                (self.context, self.request), name=defaultviewname)
+        if view is None:
+            return
+        naked_view = removeSecurityProxy(view)
+        naked_view.initialize()
+        cache = naked_view.getCacheJSON()
+        self.request.response.setHeader('content-type', 'application/json')
+        return cache

=== added file 'lib/canonical/launchpad/webapp/templates/launchpad-model.pt'
--- lib/canonical/launchpad/webapp/templates/launchpad-model.pt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/templates/launchpad-model.pt	2011-07-27 15:26:31 +0000
@@ -0,0 +1,13 @@
+<div
+  xmlns="http://www.w3.org/1999/xhtml"
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+  xml:lang="en"
+  lang="en"
+  dir="ltr"
+  i18n:domain="launchpad">
+
+  <tal:json replace="structure context/getCacheJSON" />
+
+</div>

=== added file 'lib/canonical/launchpad/webapp/tests/test_view_model.py'
--- lib/canonical/launchpad/webapp/tests/test_view_model.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_view_model.py	2011-07-27 15:26:31 +0000
@@ -0,0 +1,167 @@
+# Copyright 2011 Canonical Ltd.  All rights reserved.
+
+"""Tests for the user requested oops using ++oops++ traversal."""
+
+__metaclass__ = type
+
+
+from lazr.restful.interfaces import IJSONRequestCache
+from lazr.restful.utils import get_current_browser_request
+from simplejson import loads
+from testtools.matchers import KeysEqual
+from zope.configuration import xmlconfig
+
+from canonical.launchpad.webapp import LaunchpadView
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.launchpad.webapp.namespace import JsonModelNamespaceView
+import canonical.launchpad.webapp.tests
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.browser.launchpadform import LaunchpadFormView
+from lp.testing import (
+    ANONYMOUS,
+    BrowserTestCase,
+    login,
+    logout,
+    TestCaseWithFactory,
+    )
+
+
+class FakeView:
+    """A view object that just has a fake context and request."""
+    def __init__(self):
+        self.context = object()
+        self.request = object()
+
+
+class TestJsonModelNamespace(TestCaseWithFactory):
+    """Test that traversal to ++model++ returns a namespace."""
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        login(ANONYMOUS)
+
+    def tearDown(self):
+        logout()
+        TestCaseWithFactory.tearDown(self)
+
+    def test_JsonModelNamespace_traverse_non_LPview(self):
+        # Test traversal for JSON model namespace,
+        # ++model++ for a non-LaunchpadView context.
+        request = get_current_browser_request()
+        context = object()
+        view = FakeView()
+        namespace = JsonModelNamespaceView(context, request)
+        result = namespace.traverse(view, None)
+        self.assertEqual(result, namespace)
+
+    def test_JsonModelNamespace_traverse_LPView(self):
+        # Test traversal for JSON model namespace,
+        # ++model++ for a non-LaunchpadView context.
+        request = get_current_browser_request()
+        context = object()
+        view = LaunchpadView(context, request)
+        namespace = JsonModelNamespaceView(view, request)
+        result = namespace.traverse(view, None)
+        self.assertEqual(result, namespace)
+
+    def test_JsonModelNamespace_traverse_LPFormView(self):
+        # Test traversal for JSON model namespace,
+        # ++model++ for a non-LaunchpadView context.
+        request = get_current_browser_request()
+        context = object()
+        view = LaunchpadFormView(context, request)
+        namespace = JsonModelNamespaceView(view, request)
+        result = namespace.traverse(view, None)
+        self.assertEqual(result, namespace)
+
+
+class BaseProductModelTestView(LaunchpadView):
+    def initialize(self):
+        # Ensure initialize does not put anything in the cache.
+        pass
+
+
+class TestJsonModelView(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        login(ANONYMOUS)
+        self.product = self.factory.makeProduct(name="test-product")
+        self.url = canonical_url(self.product) + '/+modeltest/++model++'
+
+    def tearDown(self):
+        logout()
+        TestCaseWithFactory.tearDown(self)
+
+    def configZCML(self):
+        # Register the ZCML for our test view.  Note the view class must be
+        # registered first.
+        xmlconfig.string("""
+          <configure
+              xmlns:browser="http://namespaces.zope.org/browser">
+              <include package="canonical.launchpad.webapp"
+                  file="meta.zcml" />
+              <include package="zope.app.zcmlfiles" file="meta.zcml" />
+              <browser:page
+                name="+modeltest"
+                for="lp.registry.interfaces.product.IProduct"
+                class="canonical.launchpad.webapp.tests.ProductModelTestView"
+                permission="zope.Public"
+                />
+          </configure>""")
+
+    def test_JsonModel_default_cache(self):
+        # If nothing is added to the class by the view, the cache will only
+        # have the context.
+        class ProductModelTestView(BaseProductModelTestView):
+            pass
+        canonical.launchpad.webapp.tests.ProductModelTestView = \
+            ProductModelTestView
+        self.configZCML()
+        browser = self.getUserBrowser(self.url)
+        cache = loads(browser.contents)
+        self.assertEqual(['context'], cache.keys())
+
+    def test_JsonModel_custom_cache(self):
+        # Adding an item to the cache in the initialize method results in it
+        # being in the cache.
+        class ProductModelTestView(BaseProductModelTestView):
+            def initialize(self):
+                request = get_current_browser_request()
+                target_info = {}
+                target_info['title'] = "The Title"
+                cache = IJSONRequestCache(request).objects
+                cache['target_info'] = target_info
+        canonical.launchpad.webapp.tests.ProductModelTestView = \
+            ProductModelTestView
+        self.configZCML()
+        browser = self.getUserBrowser(self.url)
+        cache = loads(browser.contents)
+        self.assertThat(cache, KeysEqual('context', 'target_info'))
+
+    def test_JsonModel_custom_cache_wrong_method(self):
+        # Adding an item to the cache in some other method is not recognized,
+        # even if it called as part of normal rendering.
+        class ProductModelTestView(BaseProductModelTestView):
+            def initialize(self):
+                request = get_current_browser_request()
+                target_info = {}
+                target_info['title'] = "The Title"
+                cache = IJSONRequestCache(request).objects
+                cache['target_info'] = target_info
+
+            def render(self):
+                request = get_current_browser_request()
+                other_info = {}
+                other_info['spaz'] = "Stuff"
+                IJSONRequestCache(request).objects['other_info'] = other_info
+
+        canonical.launchpad.webapp.tests.ProductModelTestView = \
+            ProductModelTestView
+        self.configZCML()
+        browser = self.getUserBrowser(self.url)
+        cache = loads(browser.contents)
+        self.assertThat(cache, KeysEqual('context', 'target_info'))

=== modified file 'lib/lp/translations/browser/tests/test_sharing_details.py'
--- lib/lp/translations/browser/tests/test_sharing_details.py	2011-07-27 15:26:29 +0000
+++ lib/lp/translations/browser/tests/test_sharing_details.py	2011-07-27 15:26:31 +0000
@@ -251,7 +251,7 @@
         # If the packaging link is set and if an upstream series
         # uses Launchpad translations but if the other conditions
         # are not fulfilled, is_configuration_complete is False.
-        self.configureSharing(translations_usage=ServiceUsage.LAUNCHPAD)
+        self.configureSharing(translations_usage = ServiceUsage.LAUNCHPAD)
         self.assertFalse(self.view.is_configuration_complete)
 
     def test_is_configuration_complete__no_auto_sync(self):
@@ -262,8 +262,8 @@
         # but if the upstream series does not synchronize translations
         # then is_configuration_complete is False.
         self.configureSharing(
-            set_upstream_branch=True,
-            translations_usage=ServiceUsage.LAUNCHPAD)
+            set_upstream_branch = True,
+            translations_usage = ServiceUsage.LAUNCHPAD)
         self.assertFalse(self.view.is_configuration_complete)
 
     def test_is_configuration_complete__all_conditions_fulfilled(self):
@@ -274,9 +274,9 @@
         #   - the upstream series synchronizes translations
         # then is_configuration_complete is True.
         self.configureSharing(
-            set_upstream_branch=True,
-            translations_usage=ServiceUsage.LAUNCHPAD,
-            translation_import_mode=
+            set_upstream_branch = True,
+            translations_usage = ServiceUsage.LAUNCHPAD,
+            translation_import_mode =
                 TranslationsBranchImportMode.IMPORT_TRANSLATIONS)
         self.assertTrue(self.view.is_configuration_complete)