Merge lp:~deryck/launchpad/person-latest-bugs-listings-921366 into lp:launchpad

[Deryck Hodge]

Do not add bug listing data to JSON cache for bug feeds.

This branch fixes bug 921366, which notes that we have an OOPS for Person bugs feeds. This happens because feeds initialize a delegate view, which causes the bug task listing code to not be able to get at the view name in the same way as a normal view. I discussed this bug in pre-imp discussions with both abentley and flacoste and settled on completely avoiding adding bug list data to the cache in the case of feeds. Populating the cache is extra work we really don't need to do for feeds, regardless of the view name ambiguity it introduces.

So this branch checks to see if we're in a feed view or not, and if not, then we add the data to the cache. I've also added a number of tests to ensure this works for all bug targets. browser/tests/test_bugtask already had a test class for each target type, so I added a test_mustache_cache_is_none_for_feed test to each target test class. I also added a BugsFeedsTestMixin class, which provides a test helper method to get the cache for a feed's delegate view.

The bugs feed classes had been creating this delegate view inside another method, and I pulled view creation out to a separate method to make this easier to test. Finally, the project group test assigned the target to self.projectgroup rather than self.target, and I changed this test to match the other tests.

make lint doesn't complain.

[Aaron Bentley (abentley)]

I'd prefer to have getFeedViewCache as a function so that we don't need an extra class, but this is good to land either way.

review: Approve

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-03-14 17:30:30 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-03-29 14:38:22 +0000
@@ -219,6 +219,7 @@
 from lp.bugs.interfaces.malone import IMaloneApplication
 from lp.bugs.model.bugtasksearch import orderby_expression
 from lp.code.interfaces.branchcollection import IAllBranches
+from lp.layers import FeedsLayer
 from lp.registry.interfaces.distribution import (
     IDistribution,
     IDistributionSet,
@@ -2695,38 +2696,39 @@
         expose_structural_subscription_data_to_js(
             self.context, self.request, self.user)
         if getFeatureFlag('bugs.dynamic_bug_listings.enabled'):
-            cache = IJSONRequestCache(self.request)
-            view_names = set(reg.name for reg
-                in iter_view_registrations(self.__class__))
-            if len(view_names) != 1:
-                raise AssertionError("Ambiguous view name.")
-            cache.objects['view_name'] = view_names.pop()
-            batch_navigator = self.search()
-            cache.objects['mustache_model'] = batch_navigator.model
-            cache.objects['field_visibility'] = (
-                batch_navigator.field_visibility)
-            cache.objects['field_visibility_defaults'] = (
-                batch_navigator.field_visibility_defaults)
-            cache.objects['cbl_cookie_name'] = batch_navigator.getCookieName()
-
-            def _getBatchInfo(batch):
-                if batch is None:
-                    return None
-                return {'memo': batch.range_memo,
-                        'start': batch.startNumber() - 1}
-
-            next_batch = batch_navigator.batch.nextBatch()
-            cache.objects['next'] = _getBatchInfo(next_batch)
-            prev_batch = batch_navigator.batch.prevBatch()
-            cache.objects['prev'] = _getBatchInfo(prev_batch)
-            cache.objects['total'] = batch_navigator.batch.total()
-            cache.objects['order_by'] = ','.join(
-                get_sortorder_from_request(self.request))
-            cache.objects['forwards'] = batch_navigator.batch.range_forwards
-            last_batch = batch_navigator.batch.lastBatch()
-            cache.objects['last_start'] = last_batch.startNumber() - 1
-            cache.objects.update(_getBatchInfo(batch_navigator.batch))
-            cache.objects['sort_keys'] = SORT_KEYS
+            if not FeedsLayer.providedBy(self.request):
+                cache = IJSONRequestCache(self.request)
+                view_names = set(reg.name for reg
+                    in iter_view_registrations(self.__class__))
+                if len(view_names) != 1:
+                    raise AssertionError("Ambiguous view name.")
+                cache.objects['view_name'] = view_names.pop()
+                batch_navigator = self.search()
+                cache.objects['mustache_model'] = batch_navigator.model
+                cache.objects['field_visibility'] = (
+                    batch_navigator.field_visibility)
+                cache.objects['field_visibility_defaults'] = (
+                    batch_navigator.field_visibility_defaults)
+                cache.objects['cbl_cookie_name'] = batch_navigator.getCookieName()
+
+                def _getBatchInfo(batch):
+                    if batch is None:
+                        return None
+                    return {'memo': batch.range_memo,
+                            'start': batch.startNumber() - 1}
+
+                next_batch = batch_navigator.batch.nextBatch()
+                cache.objects['next'] = _getBatchInfo(next_batch)
+                prev_batch = batch_navigator.batch.prevBatch()
+                cache.objects['prev'] = _getBatchInfo(prev_batch)
+                cache.objects['total'] = batch_navigator.batch.total()
+                cache.objects['order_by'] = ','.join(
+                    get_sortorder_from_request(self.request))
+                cache.objects['forwards'] = batch_navigator.batch.range_forwards
+                last_batch = batch_navigator.batch.lastBatch()
+                cache.objects['last_start'] = last_batch.startNumber() - 1
+                cache.objects.update(_getBatchInfo(batch_navigator.batch))
+                cache.objects['sort_keys'] = SORT_KEYS
 
     @property
     def show_config_portlet(self):

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-03-19 17:16:32 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-03-29 14:38:22 +0000
@@ -41,6 +41,10 @@
     BugTaskListingItem,
     BugTasksAndNominationsView,
     )
+from lp.bugs.feed.bug import (
+    BugTargetBugsFeed,
+    PersonBugsFeed,
+    )
 from lp.bugs.interfaces.bugactivity import IBugActivitySet
 from lp.bugs.interfaces.bugnomination import IBugNomination
 from lp.bugs.interfaces.bugtask import (
@@ -49,6 +53,10 @@
     IBugTaskSet,
     )
 from lp.bugs.model.bugtasksearch import orderby_expression
+from lp.layers import (
+    FeedsLayer,
+    setFirstLayer,
+    )
 from lp.registry.interfaces.person import PersonVisibility
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
@@ -90,6 +98,18 @@
 from lp.testing.views import create_initialized_view
 
 
+def getFeedViewCache(target, feed_cls):
+    """Return JSON cache for a feed's delegate view."""
+    with dynamic_listings():
+        request = LaunchpadTestRequest(
+            SERVER_URL='http://feeds.example.com/latest-bugs.atom')
+        setFirstLayer(request, FeedsLayer)
+        feed = feed_cls(target, request)
+        delegate_view = feed._createView()
+        delegate_view.initialize()
+        return IJSONRequestCache(delegate_view.request)
+
+
 class TestBugTaskView(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
@@ -1425,6 +1445,12 @@
             'Project, distribution, package, or series subscriber',
             view.structural_subscriber_label)
 
+    def test_mustache_cache_is_none_for_feed(self):
+        """The mustache model should not be added to JSON cache for feeds."""
+        cache = getFeedViewCache(self.target, PersonBugsFeed)
+        self.assertIsNone(cache.objects.get('mustache_model'))
+
+
 
 class TestDistributionBugs(TestCaseWithFactory):
     """Test the bugs overview page for distributions."""
@@ -1446,6 +1472,11 @@
         self.assertEqual(
             'Package or series subscriber', view.structural_subscriber_label)
 
+    def test_mustache_cache_is_none_for_feed(self):
+        """The mustache model should not be added to JSON cache for feeds."""
+        cache = getFeedViewCache(self.target, BugTargetBugsFeed)
+        self.assertIsNone(cache.objects.get('mustache_model'))
+
 
 class TestDistroSeriesBugs(TestCaseWithFactory):
     """Test the bugs overview page for distro series."""
@@ -1467,6 +1498,11 @@
         self.assertEqual(
             'Package subscriber', view.structural_subscriber_label)
 
+    def test_mustache_cache_is_none_for_feed(self):
+        """The mustache model should not be added to JSON cache for feeds."""
+        cache = getFeedViewCache(self.target, BugTargetBugsFeed)
+        self.assertIsNone(cache.objects.get('mustache_model'))
+
 
 class TestDistributionSourcePackageBugs(TestCaseWithFactory):
     """Test the bugs overview page for distribution source packages."""
@@ -1482,6 +1518,11 @@
             self.target, name=u'+bugs', rootsite='bugs')
         self.assertFalse(view.shouldShowStructuralSubscriberWidget())
 
+    def test_mustache_cache_is_none_for_feed(self):
+        """The mustache model should not be added to JSON cache for feeds."""
+        cache = getFeedViewCache(self.target, BugTargetBugsFeed)
+        self.assertIsNone(cache.objects.get('mustache_model'))
+
 
 class TestDistroSeriesSourcePackageBugs(TestCaseWithFactory):
     """Test the bugs overview page for distro series source packages."""
@@ -1497,6 +1538,11 @@
             self.target, name=u'+bugs', rootsite='bugs')
         self.assertFalse(view.shouldShowStructuralSubscriberWidget())
 
+    def test_mustache_cache_is_none_for_feed(self):
+        """The mustache model should not be added to JSON cache for feeds."""
+        cache = getFeedViewCache(self.target, BugTargetBugsFeed)
+        self.assertIsNone(cache.objects.get('mustache_model'))
+
 
 class TestProductBugs(TestCaseWithFactory):
     """Test the bugs overview page for projects."""
@@ -1518,6 +1564,11 @@
         self.assertEqual(
             'Series subscriber', view.structural_subscriber_label)
 
+    def test_mustache_cache_is_none_for_feed(self):
+        """The mustache model should not be added to JSON cache for feeds."""
+        cache = getFeedViewCache(self.target, BugTargetBugsFeed)
+        self.assertIsNone(cache.objects.get('mustache_model'))
+
 
 class TestProductSeriesBugs(TestCaseWithFactory):
     """Test the bugs overview page for project series."""
@@ -1533,6 +1584,11 @@
             self.target, name=u'+bugs', rootsite='bugs')
         self.assertFalse(view.shouldShowStructuralSubscriberWidget())
 
+    def test_mustache_cache_is_none_for_feed(self):
+        """The mustache model should not be added to JSON cache for feeds."""
+        cache = getFeedViewCache(self.target, BugTargetBugsFeed)
+        self.assertIsNone(cache.objects.get('mustache_model'))
+
 
 class TestProjectGroupBugs(TestCaseWithFactory):
     """Test the bugs overview page for project groups."""
@@ -1542,38 +1598,38 @@
     def setUp(self):
         super(TestProjectGroupBugs, self).setUp()
         self.owner = self.factory.makePerson(name='bob')
-        self.projectgroup = self.factory.makeProject(name='container',
+        self.target = self.factory.makeProject(name='container',
                                                      owner=self.owner)
 
     def makeSubordinateProduct(self, tracks_bugs_in_lp):
         """Create a new product and add it to the project group."""
         product = self.factory.makeProduct(official_malone=tracks_bugs_in_lp)
         with person_logged_in(product.owner):
-            product.project = self.projectgroup
+            product.project = self.target
 
     def test_empty_project_group(self):
         # An empty project group does not use Launchpad for bugs.
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs')
-        self.assertFalse(self.projectgroup.hasProducts())
+            self.target, name=u'+bugs', rootsite='bugs')
+        self.assertFalse(self.target.hasProducts())
         self.assertFalse(view.should_show_bug_information)
 
     def test_project_group_with_subordinate_not_using_launchpad(self):
         # A project group with all subordinates not using Launchpad
         # will itself be marked as not using Launchpad for bugs.
         self.makeSubordinateProduct(False)
-        self.assertTrue(self.projectgroup.hasProducts())
+        self.assertTrue(self.target.hasProducts())
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs')
+            self.target, name=u'+bugs', rootsite='bugs')
         self.assertFalse(view.should_show_bug_information)
 
     def test_project_group_with_subordinate_using_launchpad(self):
         # A project group with one subordinate using Launchpad
         # will itself be marked as using Launchpad for bugs.
         self.makeSubordinateProduct(True)
-        self.assertTrue(self.projectgroup.hasProducts())
+        self.assertTrue(self.target.hasProducts())
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs')
+            self.target, name=u'+bugs', rootsite='bugs')
         self.assertTrue(view.should_show_bug_information)
 
     def test_project_group_with_mixed_subordinates(self):
@@ -1581,9 +1637,9 @@
         # will itself be marked as using Launchpad for bugs.
         self.makeSubordinateProduct(False)
         self.makeSubordinateProduct(True)
-        self.assertTrue(self.projectgroup.hasProducts())
+        self.assertTrue(self.target.hasProducts())
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs')
+            self.target, name=u'+bugs', rootsite='bugs')
         self.assertTrue(view.should_show_bug_information)
 
     def test_project_group_has_no_portlets_if_not_using_LP(self):
@@ -1591,7 +1647,7 @@
         # bug portlets.
         self.makeSubordinateProduct(False)
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs',
+            self.target, name=u'+bugs', rootsite='bugs',
             current_request=True)
         self.assertFalse(view.should_show_bug_information)
         contents = view.render()
@@ -1603,7 +1659,7 @@
         # portlets.
         self.makeSubordinateProduct(True)
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs',
+            self.target, name=u'+bugs', rootsite='bugs',
             current_request=True)
         self.assertTrue(view.should_show_bug_information)
         contents = view.render()
@@ -1615,7 +1671,7 @@
         # a 'Getting started' help link.
         self.makeSubordinateProduct(False)
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs',
+            self.target, name=u'+bugs', rootsite='bugs',
             current_request=True)
         contents = view.render()
         help_link = find_tag_by_id(contents, 'getting-started-help')
@@ -1626,7 +1682,7 @@
         # a 'Getting started' help link.
         self.makeSubordinateProduct(True)
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs',
+            self.target, name=u'+bugs', rootsite='bugs',
             current_request=True)
         contents = view.render()
         help_link = find_tag_by_id(contents, 'getting-started-help')
@@ -1634,15 +1690,20 @@
 
     def test_shouldShowStructuralSubscriberWidget(self):
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs')
+            self.target, name=u'+bugs', rootsite='bugs')
         self.assertTrue(view.shouldShowStructuralSubscriberWidget())
 
     def test_structural_subscriber_label(self):
         view = create_initialized_view(
-            self.projectgroup, name=u'+bugs', rootsite='bugs')
+            self.target, name=u'+bugs', rootsite='bugs')
         self.assertEqual(
             'Project or series subscriber', view.structural_subscriber_label)
 
+    def test_mustache_cache_is_none_for_feed(self):
+        """The mustache model should not be added to JSON cache for feeds."""
+        cache = getFeedViewCache(self.target, BugTargetBugsFeed)
+        self.assertIsNone(cache.objects.get('mustache_model'))
+
 
 class TestBugActivityItem(TestCaseWithFactory):
 

=== modified file 'lib/lp/bugs/feed/bug.py'
--- lib/lp/bugs/feed/bug.py	2012-02-17 21:00:21 +0000
+++ lib/lp/bugs/feed/bug.py	2012-03-29 14:38:22 +0000
@@ -221,6 +221,10 @@
         if 'bugtargetdisplayname' in self.show_column:
             del self.show_column['bugtargetdisplayname']
 
+    def _createView(self):
+        """Create the delegate view used by this feed."""
+        return BugTargetView(self.context, self.request)
+
     @property
     def title(self):
         """See `IFeed`."""
@@ -245,7 +249,7 @@
 
     def _getRawItems(self):
         """Get the raw set of items for the feed."""
-        delegate_view = BugTargetView(self.context, self.request)
+        delegate_view = self._createView()
         # XXX: BradCrittenden 2008-03-25 bug=206811:
         # The feed should have `self.quantity` entries, each representing a
         # bug.  Our query returns bugtasks, not bugs.  We then work backward
@@ -270,10 +274,14 @@
         """See `IFeed`."""
         return "Bugs for %s" % self.context.displayname
 
-    def _getRawItems(self):
-        """Perform the search."""
-        delegate_view = PersonRelatedBugTaskSearchListingView(
+    def _createView(self):
+        """Create the delegate view used by this feed."""
+        return PersonRelatedBugTaskSearchListingView(
             self.context, self.request)
+
+    def _getRawItems(self):
+        """Perform the search."""
+        delegate_view = self._createView()
         # Since the delegate_view derives from LaunchpadFormView the view must
         # be initialized to setup the widgets.
         delegate_view.initialize()