launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06919
[Merge] lp:~deryck/launchpad/person-latest-bugs-listings-921366 into lp:launchpad
Deryck Hodge has proposed merging lp:~deryck/launchpad/person-latest-bugs-listings-921366 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #921366 in Launchpad itself: "Person:latest-bugs.atom broken with dynamic bug listings"
https://bugs.launchpad.net/launchpad/+bug/921366
For more details, see:
https://code.launchpad.net/~deryck/launchpad/person-latest-bugs-listings-921366/+merge/99800
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.
--
https://code.launchpad.net/~deryck/launchpad/person-latest-bugs-listings-921366/+merge/99800
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/person-latest-bugs-listings-921366 into lp:launchpad.
=== 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-28 18:38:23 +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-28 18:38:23 +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
@@ -1404,7 +1412,22 @@
self.assertEqual(0, len(notifications))
-class TestPersonBugs(TestCaseWithFactory):
+class BugsFeedsTestMixin:
+ """Mixin class to provide helper method for bugs feeds tests."""
+
+ def getFeedViewCache(self, 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(self.target, request)
+ delegate_view = feed._createView()
+ delegate_view.initialize()
+ return IJSONRequestCache(delegate_view.request)
+
+
+class TestPersonBugs(TestCaseWithFactory, BugsFeedsTestMixin):
"""Test the bugs overview page for distributions."""
layer = DatabaseFunctionalLayer
@@ -1425,8 +1448,14 @@
'Project, distribution, package, or series subscriber',
view.structural_subscriber_label)
-
-class TestDistributionBugs(TestCaseWithFactory):
+ def test_mustache_cache_is_none_for_feed(self):
+ """The mustache model should not be added to JSON cache for feeds."""
+ cache = self.getFeedViewCache(PersonBugsFeed)
+ self.assertIsNone(cache.objects.get('mustache_model'))
+
+
+
+class TestDistributionBugs(TestCaseWithFactory, BugsFeedsTestMixin):
"""Test the bugs overview page for distributions."""
layer = DatabaseFunctionalLayer
@@ -1446,8 +1475,13 @@
self.assertEqual(
'Package or series subscriber', view.structural_subscriber_label)
-
-class TestDistroSeriesBugs(TestCaseWithFactory):
+ def test_mustache_cache_is_none_for_feed(self):
+ """The mustache model should not be added to JSON cache for feeds."""
+ cache = self.getFeedViewCache(BugTargetBugsFeed)
+ self.assertIsNone(cache.objects.get('mustache_model'))
+
+
+class TestDistroSeriesBugs(TestCaseWithFactory, BugsFeedsTestMixin):
"""Test the bugs overview page for distro series."""
layer = DatabaseFunctionalLayer
@@ -1467,8 +1501,14 @@
self.assertEqual(
'Package subscriber', view.structural_subscriber_label)
-
-class TestDistributionSourcePackageBugs(TestCaseWithFactory):
+ def test_mustache_cache_is_none_for_feed(self):
+ """The mustache model should not be added to JSON cache for feeds."""
+ cache = self.getFeedViewCache(BugTargetBugsFeed)
+ self.assertIsNone(cache.objects.get('mustache_model'))
+
+
+class TestDistributionSourcePackageBugs(
+ TestCaseWithFactory, BugsFeedsTestMixin):
"""Test the bugs overview page for distribution source packages."""
layer = DatabaseFunctionalLayer
@@ -1482,8 +1522,14 @@
self.target, name=u'+bugs', rootsite='bugs')
self.assertFalse(view.shouldShowStructuralSubscriberWidget())
-
-class TestDistroSeriesSourcePackageBugs(TestCaseWithFactory):
+ def test_mustache_cache_is_none_for_feed(self):
+ """The mustache model should not be added to JSON cache for feeds."""
+ cache = self.getFeedViewCache(BugTargetBugsFeed)
+ self.assertIsNone(cache.objects.get('mustache_model'))
+
+
+class TestDistroSeriesSourcePackageBugs(
+ TestCaseWithFactory, BugsFeedsTestMixin):
"""Test the bugs overview page for distro series source packages."""
layer = DatabaseFunctionalLayer
@@ -1497,8 +1543,13 @@
self.target, name=u'+bugs', rootsite='bugs')
self.assertFalse(view.shouldShowStructuralSubscriberWidget())
-
-class TestProductBugs(TestCaseWithFactory):
+ def test_mustache_cache_is_none_for_feed(self):
+ """The mustache model should not be added to JSON cache for feeds."""
+ cache = self.getFeedViewCache(BugTargetBugsFeed)
+ self.assertIsNone(cache.objects.get('mustache_model'))
+
+
+class TestProductBugs(TestCaseWithFactory, BugsFeedsTestMixin):
"""Test the bugs overview page for projects."""
layer = DatabaseFunctionalLayer
@@ -1518,8 +1569,13 @@
self.assertEqual(
'Series subscriber', view.structural_subscriber_label)
-
-class TestProductSeriesBugs(TestCaseWithFactory):
+ def test_mustache_cache_is_none_for_feed(self):
+ """The mustache model should not be added to JSON cache for feeds."""
+ cache = self.getFeedViewCache(BugTargetBugsFeed)
+ self.assertIsNone(cache.objects.get('mustache_model'))
+
+
+class TestProductSeriesBugs(TestCaseWithFactory, BugsFeedsTestMixin):
"""Test the bugs overview page for project series."""
layer = DatabaseFunctionalLayer
@@ -1533,8 +1589,13 @@
self.target, name=u'+bugs', rootsite='bugs')
self.assertFalse(view.shouldShowStructuralSubscriberWidget())
-
-class TestProjectGroupBugs(TestCaseWithFactory):
+ def test_mustache_cache_is_none_for_feed(self):
+ """The mustache model should not be added to JSON cache for feeds."""
+ cache = self.getFeedViewCache(BugTargetBugsFeed)
+ self.assertIsNone(cache.objects.get('mustache_model'))
+
+
+class TestProjectGroupBugs(TestCaseWithFactory, BugsFeedsTestMixin):
"""Test the bugs overview page for project groups."""
layer = DatabaseFunctionalLayer
@@ -1542,38 +1603,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 +1642,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 +1652,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 +1664,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 +1676,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 +1687,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 +1695,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 = self.getFeedViewCache(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-28 18:38:23 +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()
