launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/advanced-bug-search-model-915880 into lp:launchpad.

Commit message:
Do not populate bug model cache for advanced search form.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #915880 in Launchpad itself: "bug search model cache is populated for 'advanced bug search form'"
  https://bugs.launchpad.net/launchpad/+bug/915880

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/advanced-bug-search-model-915880/+merge/128401

== Implementation ==

Add a check to the BugTaskSearchListingView initiaisation so that if the advanced form is being used, do not render the bug model in the json cache.

== Tests ==

Refacator some common tests to a mixin and add a new test for the new advanced for behaviour.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/browser/tests/test_bugtask.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/advanced-bug-search-model-915880/+merge/128401
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/advanced-bug-search-model-915880 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-10-04 21:57:37 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-10-08 00:53:26 +0000
@@ -2641,7 +2641,9 @@
             self.context, self.request, self.user)
         can_view = (IPrivacy(self.context, None) is None
             or check_permission('launchpad.View', self.context))
-        if can_view and not FeedsLayer.providedBy(self.request):
+        if (can_view and
+            not FeedsLayer.providedBy(self.request) and
+            not self.shouldShowAdvancedForm()):
             cache = IJSONRequestCache(self.request)
             view_names = set(reg.name for reg
                 in iter_view_registrations(self.__class__))

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-10-05 15:17:11 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-10-08 00:53:26 +0000
@@ -42,10 +42,7 @@
     BugTasksAndNominationsView,
     )
 from lp.bugs.enums import BugNotificationLevel
-from lp.bugs.feed.bug import (
-    BugTargetBugsFeed,
-    PersonBugsFeed,
-    )
+from lp.bugs.feed.bug import PersonBugsFeed
 from lp.bugs.interfaces.bugactivity import IBugActivitySet
 from lp.bugs.interfaces.bugnomination import IBugNomination
 from lp.bugs.interfaces.bugtask import (
@@ -1435,7 +1432,35 @@
         self.assertEqual(0, len(notifications))
 
 
-class TestPersonBugs(TestCaseWithFactory):
+class BugTaskViewTestMixin():
+
+    def _assert_shouldShowStructuralSubscriberWidget(self, show=True):
+        view = create_initialized_view(
+            self.target, name=u'+bugs', rootsite='bugs')
+        self.assertEqual(show, view.shouldShowStructuralSubscriberWidget())
+
+    def _assert_structural_subscriber_label(self, label):
+        view = create_initialized_view(
+            self.target, name=u'+bugs', rootsite='bugs')
+        self.assertEqual(label, 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'))
+
+    def test_mustache_cache_is_none_for_advanced_form(self):
+        """The mustache model should not be added to JSON cache for feeds."""
+        form = {
+            'advanced': 1,
+            }
+        view = create_initialized_view(
+            self.target, name=u'+bugs', rootsite='bugs', form=form)
+        cache = IJSONRequestCache(view.request)
+        self.assertIsNone(cache.objects.get('mustache_model'))
+
+
+class TestPersonBugs(TestCaseWithFactory, BugTaskViewTestMixin):
     """Test the bugs overview page for distributions."""
 
     layer = DatabaseFunctionalLayer
@@ -1445,24 +1470,14 @@
         self.target = self.factory.makePerson()
 
     def test_shouldShowStructuralSubscriberWidget(self):
-        view = create_initialized_view(
-            self.target, name=u'+bugs', rootsite='bugs')
-        self.assertTrue(view.shouldShowStructuralSubscriberWidget())
+        self._assert_shouldShowStructuralSubscriberWidget()
 
     def test_structural_subscriber_label(self):
-        view = create_initialized_view(
-            self.target, name=u'+bugs', rootsite='bugs')
-        self.assertEqual(
-            '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):
+        self._assert_structural_subscriber_label(
+            'Project, distribution, package, or series subscriber')
+
+
+class TestDistributionBugs(TestCaseWithFactory, BugTaskViewTestMixin):
     """Test the bugs overview page for distributions."""
 
     layer = DatabaseFunctionalLayer
@@ -1471,24 +1486,15 @@
         super(TestDistributionBugs, self).setUp()
         self.target = self.factory.makeDistribution()
 
+    def test_structural_subscriber_label(self):
+        self._assert_structural_subscriber_label(
+            'Package or series subscriber')
+
     def test_shouldShowStructuralSubscriberWidget(self):
-        view = create_initialized_view(
-            self.target, name=u'+bugs', rootsite='bugs')
-        self.assertTrue(view.shouldShowStructuralSubscriberWidget())
-
-    def test_structural_subscriber_label(self):
-        view = create_initialized_view(
-            self.target, name=u'+bugs', rootsite='bugs')
-        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):
+        self._assert_shouldShowStructuralSubscriberWidget()
+
+
+class TestDistroSeriesBugs(TestCaseWithFactory, BugTaskViewTestMixin):
     """Test the bugs overview page for distro series."""
 
     layer = DatabaseFunctionalLayer
@@ -1498,23 +1504,14 @@
         self.target = self.factory.makeDistroSeries()
 
     def test_shouldShowStructuralSubscriberWidget(self):
-        view = create_initialized_view(
-            self.target, name=u'+bugs', rootsite='bugs')
-        self.assertTrue(view.shouldShowStructuralSubscriberWidget())
+        self._assert_shouldShowStructuralSubscriberWidget()
 
     def test_structural_subscriber_label(self):
-        view = create_initialized_view(
-            self.target, name=u'+bugs', rootsite='bugs')
-        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):
+        self._assert_structural_subscriber_label('Package subscriber')
+
+
+class TestDistributionSourcePackageBugs(TestCaseWithFactory,
+                                        BugTaskViewTestMixin):
     """Test the bugs overview page for distribution source packages."""
 
     layer = DatabaseFunctionalLayer
@@ -1524,17 +1521,11 @@
         self.target = self.factory.makeDistributionSourcePackage()
 
     def test_shouldShowStructuralSubscriberWidget(self):
-        view = create_initialized_view(
-            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):
+        self._assert_shouldShowStructuralSubscriberWidget(show=False)
+
+
+class TestDistroSeriesSourcePackageBugs(TestCaseWithFactory,
+                                        BugTaskViewTestMixin):
     """Test the bugs overview page for distro series source packages."""
 
     layer = DatabaseFunctionalLayer
@@ -1544,17 +1535,10 @@
         self.target = self.factory.makeSourcePackage()
 
     def test_shouldShowStructuralSubscriberWidget(self):
-        view = create_initialized_view(
-            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):
+        self._assert_shouldShowStructuralSubscriberWidget(show=False)
+
+
+class TestProductBugs(TestCaseWithFactory, BugTaskViewTestMixin):
     """Test the bugs overview page for projects."""
 
     layer = DatabaseFunctionalLayer
@@ -1564,23 +1548,13 @@
         self.target = self.factory.makeProduct()
 
     def test_shouldShowStructuralSubscriberWidget(self):
-        view = create_initialized_view(
-            self.target, name=u'+bugs', rootsite='bugs')
-        self.assertTrue(view.shouldShowStructuralSubscriberWidget())
+        self._assert_shouldShowStructuralSubscriberWidget()
 
     def test_structural_subscriber_label(self):
-        view = create_initialized_view(
-            self.target, name=u'+bugs', rootsite='bugs')
-        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):
+        self._assert_structural_subscriber_label('Series subscriber')
+
+
+class TestProductSeriesBugs(TestCaseWithFactory, BugTaskViewTestMixin):
     """Test the bugs overview page for project series."""
 
     layer = DatabaseFunctionalLayer
@@ -1589,18 +1563,8 @@
         super(TestProductSeriesBugs, self).setUp()
         self.target = self.factory.makeProductSeries()
 
-    def test_shouldShowStructuralSubscriberWidget(self):
-        view = create_initialized_view(
-            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):
+
+class TestProjectGroupBugs(TestCaseWithFactory, BugTaskViewTestMixin):
     """Test the bugs overview page for project groups."""
 
     layer = DatabaseFunctionalLayer
@@ -1699,20 +1663,11 @@
         self.assertIs(None, help_link)
 
     def test_shouldShowStructuralSubscriberWidget(self):
-        view = create_initialized_view(
-            self.target, name=u'+bugs', rootsite='bugs')
-        self.assertTrue(view.shouldShowStructuralSubscriberWidget())
+        self._assert_shouldShowStructuralSubscriberWidget()
 
     def test_structural_subscriber_label(self):
-        view = create_initialized_view(
-            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'))
+        self._assert_structural_subscriber_label(
+            'Project or series subscriber')
 
 
 class TestBugActivityItem(TestCaseWithFactory):