launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/bug-page-oops-1027797 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1027797 in Launchpad itself: "OOPS rendering ++model++ page in a context less bug page "
  https://bugs.launchpad.net/launchpad/+bug/1027797

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bug-page-oops-1027797/+merge/123673

== Implementation ==

A url like https://bugs.launchpad.net/bugs/5/++model++ oopes because normally such a url without the ++model++ bit loads a RedirectionView which redirects to the bugtask url. This view has none of the methods called by JsonModelNamespaceView to fetch the model, hence the oops.

I at first extended RedirectionView just to provide empty stubs for the required methods. But, with a little extra work, it was possible to refactor things so that the RedirectionView held on to enough state so that it could, if asked, provide the correct ++model++ data for the redirected view. This requires that RedirectionView subclasses be modified to support this; the default is just to return an empty model. I've fixed the 2 bug view subclasses. Others can be done as and when required.

There's also a bit of lint fixed.

== Tests ==

Provide tests for the RedirectionView ++model++ with and without a cache_view instance.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bug.py
  lib/lp/services/webapp/publisher.py
  lib/lp/services/webapp/tests/test_publisher.py


-- 
https://code.launchpad.net/~wallyworld/launchpad/bug-page-oops-1027797/+merge/123673
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bug-page-oops-1027797 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2012-09-10 06:45:22 +0000
+++ lib/lp/bugs/browser/bug.py	2012-09-11 02:20:24 +0000
@@ -2,6 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """IBug related view classes."""
+from zope.publisher.defaultview import getDefaultViewName
 
 __metaclass__ = type
 
@@ -705,8 +706,12 @@
     """
 
     def __init__(self, context, request):
+        redirected_context = context.default_bugtask
+        viewname = getDefaultViewName(redirected_context, request)
+        cache_view = getMultiAdapter(
+            (redirected_context, request), name=viewname)
         super(BugWithoutContextView, self).__init__(
-            canonical_url(context.default_bugtask), request)
+            canonical_url(redirected_context), request, cache_view=cache_view)
 
 
 class BugEditViewBase(LaunchpadEditFormView):
@@ -980,15 +985,15 @@
     """
 
     def __init__(self, context, request):
-        self.context = context
-        self.request = request
+        redirected_context = getUtility(ILaunchBag).user
+        viewname = '+assignedbugs'
+        cache_view = getMultiAdapter(
+            (redirected_context, request), name=viewname)
+        target = canonical_url(redirected_context, view_name='+assignedbugs')
+        super(DeprecatedAssignedBugsView, self).__init__(
+            target, request, cache_view=cache_view)
         self.status = 303
 
-    def __call__(self):
-        self.target = canonical_url(
-            getUtility(ILaunchBag).user, view_name='+assignedbugs')
-        super(DeprecatedAssignedBugsView, self).__call__()
-
 
 normalize_mime_type = re.compile(r'\s+')
 

=== modified file 'lib/lp/services/webapp/publisher.py'
--- lib/lp/services/webapp/publisher.py	2012-06-29 08:40:05 +0000
+++ lib/lp/services/webapp/publisher.py	2012-09-11 02:20:24 +0000
@@ -982,7 +982,7 @@
                                     text=stepthrough_text)
                                 self.request.traversed_objects.append(
                                     stepthrough_breadcrumb)
-                                
+
                         return self._handle_next_object(nextobj, request,
                             nextstep)
 
@@ -1018,10 +1018,21 @@
 class RedirectionView:
     implements(IBrowserPublisher)
 
-    def __init__(self, target, request, status=None):
+    def __init__(self, target, request, status=None, cache_view=None):
         self.target = target
         self.request = request
         self.status = status
+        self.cache_view = cache_view
+
+    def initialize(self):
+        if self.cache_view:
+            self.cache_view.initialize()
+
+    def getCacheJSON(self):
+        if self.cache_view:
+            return self.cache_view.getCacheJSON()
+        else:
+            return simplejson.dumps({})
 
     def __call__(self):
         self.request.response.redirect(self.target, status=self.status)

=== modified file 'lib/lp/services/webapp/tests/test_publisher.py'
--- lib/lp/services/webapp/tests/test_publisher.py	2012-05-22 17:00:08 +0000
+++ lib/lp/services/webapp/tests/test_publisher.py	2012-09-11 02:20:24 +0000
@@ -22,6 +22,7 @@
 from lp.services.webapp.publisher import (
     FakeRequest,
     LaunchpadView,
+    RedirectionView,
     )
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.services.worlddata.interfaces.country import ICountrySet
@@ -117,6 +118,28 @@
         self.assertIs(None, view.user)
         self.assertNotIn('user@domain', view.getCacheJSON())
 
+    def test_getCache_redirected_view_default(self):
+        # A redirection view by default provides no json cache data.
+        request = LaunchpadTestRequest()
+        view = RedirectionView(None, request)
+        json_dict = simplejson.loads(view.getCacheJSON())
+        self.assertEqual({}, json_dict)
+
+    def test_getCache_redirected_view(self):
+        # A redirection view may be provided with a target view instance from
+        # which json cache data is obtained.
+
+        class TestView(LaunchpadView):
+            pass
+
+        request = LaunchpadTestRequest()
+        test_view = TestView(self.getCanada(), request)
+        view = RedirectionView(None, request, cache_view=test_view)
+        IJSONRequestCache(request).objects['my_bool'] = True
+        json_dict = simplejson.loads(view.getCacheJSON())
+        self.assertIsCanada(json_dict['context'])
+        self.assertIn('my_bool', json_dict)
+
     def test_related_feature_info__default(self):
         # By default, LaunchpadView.related_feature_info is empty.
         request = LaunchpadTestRequest()
@@ -310,7 +333,7 @@
 
         view = LaunchpadView(PrivateObject(False), FakeRequest())
         self.assertFalse(view.private)
-    
+
     def test_view_beta_features_simple(self):
         class TestView(LaunchpadView):
             related_features = ['test_feature']
@@ -322,7 +345,7 @@
         view = TestView(object(), request)
         expected_beta_features = [{
             'url': 'http://wiki.lp.dev/LEP/sample', 'is_beta': True,
-            'value': u'on', 'title': 'title'}]         
+            'value': u'on', 'title': 'title'}]
         self.assertEqual(expected_beta_features, view.beta_features)
 
     def test_view_beta_features_mixed(self):
@@ -342,9 +365,10 @@
         view = TestView(object(), request)
         expected_beta_features = [{
             'url': 'http://wiki.lp.dev/LEP/sample', 'is_beta': True,
-            'value': u'on', 'title': 'title'}]         
+            'value': u'on', 'title': 'title'}]
         self.assertEqual(expected_beta_features, view.beta_features)
 
+
 def test_suite():
     suite = TestSuite()
     suite.addTest(DocTestSuite(publisher, optionflags=ELLIPSIS))