launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01106
[Merge] lp:~lifeless/launchpad/malone into lp:launchpad/devel
Robert Collins has proposed merging lp:~lifeless/launchpad/malone into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#497386 ScopedCollection:CollectionResource:#message-page-resource timeouts for bugs with many messages
https://bugs.launchpad.net/bugs/497386
#607776 +filebug-show-similar FTI query timeouts
https://bugs.launchpad.net/bugs/607776
#618375 BugTrackerSet:+index consistently slower than 10 seconds
https://bugs.launchpad.net/bugs/618375
Batch the bug tracker lists - theres no point showing 1000 by default; the batching will also shrink the time massively; we can actually tune it later (this was sensible to do regardless and will give more headroom than just tuning it in the short term).
--
https://code.launchpad.net/~lifeless/launchpad/malone/+merge/35941
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/malone into lp:launchpad/devel.
=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py 2010-09-06 18:52:45 +0000
+++ lib/canonical/launchpad/webapp/batching.py 2010-09-19 03:09:53 +0000
@@ -77,6 +77,24 @@
return self.batch.total() > self.batch.size
+class ActiveBatchNavigator(BatchNavigator):
+ """A paginator for active items.
+
+ Used when a view needs to display more than one BatchNavigator of items.
+ """
+ start_variable_name = 'active_start'
+ batch_variable_name = 'active_batch'
+
+
+class InactiveBatchNavigator(BatchNavigator):
+ """A paginator for inactive items.
+
+ Used when a view needs to display more than one BatchNavigator of items.
+ """
+ start_variable_name = 'inactive_start'
+ batch_variable_name = 'inactive_batch'
+
+
class TableBatchNavigator(BatchNavigator):
"""See canonical.launchpad.interfaces.ITableBatchNavigator."""
implements(ITableBatchNavigator)
=== modified file 'lib/lp/blueprints/browser/tests/test_specificationtarget.py'
--- lib/lp/blueprints/browser/tests/test_specificationtarget.py 2010-09-17 14:04:29 +0000
+++ lib/lp/blueprints/browser/tests/test_specificationtarget.py 2010-09-19 03:09:53 +0000
@@ -3,11 +3,14 @@
__metaclass__ = type
+<<<<<<< TREE
import unittest
from zope.security.proxy import removeSecurityProxy
from canonical.launchpad.webapp.batching import BatchNavigator
+=======
+>>>>>>> MERGE-SOURCE
from canonical.launchpad.testing.pages import find_tag_by_id
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.blueprints.interfaces.specificationtarget import (
@@ -21,6 +24,7 @@
login_person,
TestCaseWithFactory,
)
+from lp.testing.matchers import IsConfiguredBatchNavigator
from lp.testing.views import (
create_view,
create_initialized_view,
@@ -108,22 +112,11 @@
# subclass.
person = self.factory.makePerson()
view = create_initialized_view(person, name='+assignments')
- self.assertIsInstance(view.specs_batched, BatchNavigator)
-
- def test_batch_headings(self):
- person = self.factory.makePerson()
- view = create_initialized_view(person, name='+assignments')
- navigator = view.specs_batched
- self.assertEqual('specification', navigator._singular_heading)
- self.assertEqual('specifications', navigator._plural_heading)
-
- def test_batch_size(self):
# Because +assignments is meant to provide an overview, we default to
# 500 as the default batch size.
- person = self.factory.makePerson()
- view = create_initialized_view(person, name='+assignments')
- navigator = view.specs_batched
- self.assertEqual(500, view.specs_batched.default_size)
+ matcher = IsConfiguredBatchNavigator(
+ 'specification', 'specifications', batch_size=500)
+ self.assertThat(view.specs_batched, matcher)
class TestAssignments(TestCaseWithFactory):
=== modified file 'lib/lp/bugs/browser/bugtracker.py'
--- lib/lp/bugs/browser/bugtracker.py 2010-09-03 15:02:39 +0000
+++ lib/lp/bugs/browser/bugtracker.py 2010-09-19 03:09:53 +0000
@@ -54,7 +54,11 @@
structured,
)
from canonical.launchpad.webapp.authorization import check_permission
-from canonical.launchpad.webapp.batching import BatchNavigator
+from canonical.launchpad.webapp.batching import (
+ ActiveBatchNavigator,
+ BatchNavigator,
+ InactiveBatchNavigator,
+ )
from canonical.launchpad.webapp.breadcrumb import Breadcrumb
from canonical.launchpad.webapp.menu import NavigationMenu
from canonical.lazr.utils import smartquote
@@ -154,21 +158,30 @@
pillar_limit = 3
def initialize(self):
- # Sort the bug trackers into active and inactive lists so that
- # we can display them separately.
- all_bug_trackers = list(self.context)
- self.active_bug_trackers = [
- bug_tracker for bug_tracker in all_bug_trackers
- if bug_tracker.active]
- self.inactive_bug_trackers = [
- bug_tracker for bug_tracker in all_bug_trackers
- if not bug_tracker.active]
-
- bugtrackerset = getUtility(IBugTrackerSet)
- # The caching of bugtracker pillars here avoids us hitting the
- # database multiple times for each bugtracker.
- self._pillar_cache = bugtrackerset.getPillarsForBugtrackers(
- all_bug_trackers)
+ # eager load related pillars. In future we should do this for
+ # just the rendered trackers, and also use group by to get
+ # bug watch counts per tracker. However the batching makes
+ # the inefficiency tolerable for now. Robert Collins 20100919.
+ self._pillar_cache = self.context.getPillarsForBugtrackers(
+ list(self.context.trackers()))
+
+ @property
+ def inactive_tracker_count(self):
+ return self.inactive_trackers.currentBatch().listlength
+
+ @cachedproperty
+ def active_trackers(self):
+ results = self.context.trackers(active=True)
+ navigator = ActiveBatchNavigator(results, self.request)
+ navigator.setHeadings('tracker', 'trackers')
+ return navigator
+
+ @cachedproperty
+ def inactive_trackers(self):
+ results = self.context.trackers(active=False)
+ navigator = InactiveBatchNavigator(results, self.request)
+ navigator.setHeadings('tracker', 'trackers')
+ return navigator
def getPillarData(self, bugtracker):
"""Return dict of pillars and booleans indicating ellipsis.
=== modified file 'lib/lp/bugs/browser/tests/test_bugtracker_views.py'
--- lib/lp/bugs/browser/tests/test_bugtracker_views.py 2010-08-26 20:08:43 +0000
+++ lib/lp/bugs/browser/tests/test_bugtracker_views.py 2010-09-19 03:09:53 +0000
@@ -3,27 +3,61 @@
"""Tests for BugTracker views."""
+from __future__ import with_statement
+
__metaclass__ = type
-import unittest
-
-from datetime import datetime, timedelta
-from pytz import utc
-
from zope.component import getUtility
-from zope.security.interfaces import Unauthorized
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.testing.pages import find_tag_by_id
+from canonical.launchpad.webapp import canonical_url
from canonical.testing.layers import DatabaseFunctionalLayer
-
-from lp.bugs.browser.bugtracker import (
- BugTrackerEditView)
+from lp.bugs.interfaces.bugtracker import IBugTrackerSet
from lp.registry.interfaces.person import IPersonSet
-from lp.testing import login, TestCaseWithFactory
-from lp.testing.sampledata import ADMIN_EMAIL, NO_PRIVILEGE_EMAIL
+from lp.testing import (
+ login,
+ TestCaseWithFactory,
+ )
from lp.testing.views import create_initialized_view
-
-
-
-def test_suite():
- return unittest.TestLoader().loadTestsFromName(__name__)
+from lp.testing.matchers import IsConfiguredBatchNavigator
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+from lp.testing.sampledata import ADMIN_EMAIL
+
+
+class TestBugTrackerSetView(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_trackers_are_batch_navigators(self):
+ trackers = getUtility(IBugTrackerSet)
+ view = create_initialized_view(trackers, name='+index')
+ matcher = IsConfiguredBatchNavigator('tracker', 'trackers')
+ self.assertThat(view.active_trackers, matcher)
+ self.assertThat(view.inactive_trackers, matcher)
+
+ def test_page_is_batched(self):
+ active_tracker1 = self.factory.makeBugTracker()
+ active_tracker2 = self.factory.makeBugTracker()
+ inactive_tracker1 = self.factory.makeBugTracker()
+ inactive_tracker2 = self.factory.makeBugTracker()
+ admin = getUtility(IPersonSet).find(ADMIN_EMAIL).any()
+ with person_logged_in(admin):
+ inactive_tracker1.active = False
+ inactive_tracker2.active = False
+ trackers = getUtility(IBugTrackerSet)
+ url = (canonical_url(trackers) +
+ "/+index?active_batch=1&inactive_batch=1")
+ browser = self.getUserBrowser(url)
+ content = browser.contents
+ # XXX RobertCollns 20100919 bug=642504. The support for multiple batches
+ # isn't complete and the id for the nav links gets duplicated.
+ #self.assertEqual('next',
+ # find_tag_by_id(content, 'upper-batch-nav-batchnav-next')['class'])
+ #self.assertEqual('next',
+ # find_tag_by_id(content, 'lower-batch-nav-batchnav-next')['class'])
+ # Instead we check the string appears.
+ self.assertTrue('upper-batch-nav-batchnav-next' in content)
+
=== modified file 'lib/lp/bugs/interfaces/bugtracker.py'
--- lib/lp/bugs/interfaces/bugtracker.py 2010-08-26 20:08:43 +0000
+++ lib/lp/bugs/interfaces/bugtracker.py 2010-09-19 03:09:53 +0000
@@ -432,6 +432,13 @@
def getPillarsForBugtrackers(bug_trackers):
"""Return dict mapping bugtrackers to lists of pillars."""
+ def trackers(active=None):
+ """Return a ResultSet of bugtrackers.
+
+ :param active: If True, only active trackers are returned, if False
+ only inactive trackers are returned.
+ """
+
class IBugTrackerAlias(Interface):
"""Another URL for a remote bug system.
=== modified file 'lib/lp/bugs/model/bugtracker.py'
--- lib/lp/bugs/model/bugtracker.py 2010-08-26 20:08:43 +0000
+++ lib/lp/bugs/model/bugtracker.py 2010-09-19 03:09:53 +0000
@@ -8,7 +8,8 @@
'BugTracker',
'BugTrackerAlias',
'BugTrackerAliasSet',
- 'BugTrackerSet']
+ 'BugTrackerSet',
+ ]
from datetime import datetime
@@ -66,6 +67,8 @@
IBugTrackerSet,
SINGLE_PRODUCT_BUGTRACKERTYPES,
)
+from canonical.launchpad.webapp.interfaces import (
+ DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
from lp.bugs.interfaces.bugtrackerperson import BugTrackerPersonAlreadyExists
from lp.bugs.model.bug import Bug
from lp.bugs.model.bugmessage import BugMessage
@@ -577,6 +580,17 @@
"""See `IBugTrackerSet`."""
return BugTracker.select()
+ def trackers(self, active=None):
+ # Without context, cannot tell what store flavour is desirable.
+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+ if active is not None:
+ clauses = [BugTracker.active==active]
+ else:
+ clauses = []
+ results = store.find(BugTracker, *clauses)
+ results.order_by(BugTracker.name)
+ return results
+
def ensureBugTracker(self, baseurl, owner, bugtrackertype,
title=None, summary=None, contactdetails=None, name=None):
"""See `IBugTrackerSet`."""
=== modified file 'lib/lp/bugs/templates/bugtrackers-index.pt'
--- lib/lp/bugs/templates/bugtrackers-index.pt 2009-09-01 16:24:52 +0000
+++ lib/lp/bugs/templates/bugtrackers-index.pt 2010-09-19 03:09:53 +0000
@@ -13,6 +13,7 @@
</tal:comment>
<metal:macros fill-slot="bogus">
<metal:macro define-macro="tracker-table">
+ <tal:navigation content="structure trackers/@@+navigation-links-upper" />
<table class="sortable listing" tal:attributes="id id">
<thead>
<tr>
@@ -24,7 +25,7 @@
</tr>
</thead>
<tbody>
- <tr tal:repeat="tracker trackers">
+ <tr tal:repeat="tracker trackers/currentBatch">
<td>
<a tal:replace="structure tracker/fmt:link" />
</td>
@@ -53,6 +54,7 @@
</tr>
</tbody>
</table>
+ <tal:navigation content="structure trackers/@@+navigation-links-lower" />
</metal:macro>
</metal:macros>
@@ -70,7 +72,7 @@
the external status changes.
</p>
<tal:table define="id string:trackers;
- trackers view/active_bug_trackers">
+ trackers view/active_trackers">
<metal:table use-macro="template/macros/tracker-table" />
</tal:table>
<p style="margin-top: 1em"
@@ -81,11 +83,11 @@
</p>
</div>
</div>
- <div class="yui-u" tal:condition="view/inactive_bug_trackers">
+ <div class="yui-u" tal:condition="view/inactive_tracker_count">
<div class="portlet">
<h2>Inactive bug trackers</h2>
<tal:table define="id string:inactive-trackers;
- trackers view/inactive_bug_trackers">
+ trackers view/inactive_trackers">
<metal:table use-macro="template/macros/tracker-table" />
</tal:table>
</div>
=== modified file 'lib/lp/bugs/tests/test_bugtracker.py'
--- lib/lp/bugs/tests/test_bugtracker.py 2010-08-26 20:08:43 +0000
+++ lib/lp/bugs/tests/test_bugtracker.py 2010-09-19 03:09:53 +0000
@@ -28,7 +28,10 @@
from canonical.launchpad.ftests import login_person
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
-from canonical.testing import LaunchpadFunctionalLayer
+from canonical.testing import (
+ DatabaseFunctionalLayer,
+ LaunchpadFunctionalLayer,
+ )
from lp.bugs.externalbugtracker import (
BugTrackerConnectError,
Mantis,
@@ -38,12 +41,36 @@
BugTrackerType,
IBugTracker,
)
+from lp.bugs.model.bugtracker import BugTrackerSet
from lp.bugs.tests.externalbugtracker import Urlib2TransportTestHandler
from lp.registry.interfaces.person import IPersonSet
from lp.testing import login, login_person, TestCaseWithFactory
from lp.testing.sampledata import ADMIN_EMAIL
+class TestBugTrackerSet(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_trackers(self):
+ tracker = self.factory.makeBugTracker()
+ trackers = BugTrackerSet()
+ # Active trackers are in all trackers,
+ self.assertTrue(tracker in trackers.trackers())
+ # and active,
+ self.assertTrue(tracker in trackers.trackers(active=True))
+ # But not inactive.
+ self.assertFalse(tracker in trackers.trackers(active=False))
+ login(ADMIN_EMAIL)
+ tracker.active = False
+ # Inactive trackers are in all trackers
+ self.assertTrue(tracker in trackers.trackers())
+ # and inactive,
+ self.assertTrue(tracker in trackers.trackers(active=False))
+ # but not in active.
+ self.assertFalse(tracker in trackers.trackers(active=True))
+
+
class BugTrackerTestCase(TestCaseWithFactory):
"""Unit tests for the `BugTracker` class."""
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2010-09-15 16:44:41 +0000
+++ lib/lp/registry/browser/person.py 2010-09-19 03:09:53 +0000
@@ -194,7 +194,11 @@
structured,
)
from canonical.launchpad.webapp.authorization import check_permission
-from canonical.launchpad.webapp.batching import BatchNavigator
+from canonical.launchpad.webapp.batching import (
+ ActiveBatchNavigator,
+ BatchNavigator,
+ InactiveBatchNavigator,
+ )
from canonical.launchpad.webapp.breadcrumb import Breadcrumb
from canonical.launchpad.webapp.interfaces import (
ILaunchBag,
@@ -1385,24 +1389,6 @@
links = ['profile', 'polls', 'members', 'ppas']
-class ActiveBatchNavigator(BatchNavigator):
- """A paginator for active items.
-
- Used when a view needs to display more than one BatchNavigator of items.
- """
- start_variable_name = 'active_start'
- batch_variable_name = 'active_batch'
-
-
-class InactiveBatchNavigator(BatchNavigator):
- """A paginator for inactive items.
-
- Used when a view needs to display more than one BatchNavigator of items.
- """
- start_variable_name = 'inactive_start'
- batch_variable_name = 'inactive_batch'
-
-
class TeamMembershipView(LaunchpadView):
"""The view behind ITeam/+members."""
=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py 2010-09-17 18:32:23 +0000
+++ lib/lp/registry/browser/team.py 2010-09-19 03:09:53 +0000
@@ -829,8 +829,6 @@
"""
results = self.mailing_list.getReviewableMessages()
navigator = BatchNavigator(results, self.request)
- # Subclasses often set the singular and plural headings,
- # but we can use the generic class too.
navigator.setHeadings('message', 'messages')
return navigator
=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py 2010-09-14 00:21:04 +0000
+++ lib/lp/registry/browser/tests/test_team.py 2010-09-19 03:09:53 +0000
@@ -3,10 +3,10 @@
__metaclass__ = type
-from canonical.launchpad.webapp.batching import BatchNavigator
from canonical.testing import DatabaseFunctionalLayer
from lp.registry.browser.person import TeamOverviewMenu
from lp.testing import TestCaseWithFactory
+from lp.testing.matchers import IsConfiguredBatchNavigator
from lp.testing.menu import check_menu_links
from lp.testing.views import create_initialized_view
@@ -47,12 +47,6 @@
team = self.factory.makeTeam()
self.factory.makeMailingList(team, team.teamowner)
view = create_initialized_view(team, name='+mailinglist-moderate')
- self.assertIsInstance(view.held_messages, BatchNavigator)
-
- def test_held_message_headings(self):
- team = self.factory.makeTeam()
- self.factory.makeMailingList(team, team.teamowner)
- view = create_initialized_view(team, name='+mailinglist-moderate')
- navigator = view.held_messages
- self.assertEqual('message', navigator._singular_heading)
- self.assertEqual('messages', navigator._plural_heading)
+ self.assertThat(
+ view.held_messages,
+ IsConfiguredBatchNavigator('message', 'messages'))
=== modified file 'lib/lp/testing/matchers.py'
--- lib/lp/testing/matchers.py 2010-08-25 20:04:40 +0000
+++ lib/lp/testing/matchers.py 2010-09-19 03:09:53 +0000
@@ -17,9 +17,12 @@
from testtools.content import Content
from testtools.content_type import UTF8_TEXT
from testtools.matchers import (
+ Equals,
Matcher,
Mismatch,
+ MismatchesAll,
)
+from testtools import matchers
from zope.interface.exceptions import (
BrokenImplementation,
BrokenMethodImplementation,
@@ -31,6 +34,8 @@
Proxy,
)
+from canonical.launchpad.webapp.batching import BatchNavigator
+
class DoesNotProvide(Mismatch):
"""An object does not provide an interface."""
@@ -220,3 +225,44 @@
if not matchee.startswith(self.expected):
return DoesNotStartWith(matchee, self.expected)
return None
+
+
+class IsConfiguredBatchNavigator(Matcher):
+ """Check that an object is a batch navigator."""
+
+ def __init__(self, singular, plural, batch_size=None):
+ """Create a ConfiguredBatchNavigator.
+
+ :param singular: The singular header the batch should be using.
+ :param plural: The plural header the batch should be using.
+ """
+ self._single = Equals(singular)
+ self._plural = Equals(plural)
+ self._batch = None
+ if batch_size:
+ self._batch = Equals(batch_size)
+ self.matchers = dict(
+ _singular_heading=self._single, _plural_heading=self._plural)
+ if self._batch:
+ self.matchers['default_size'] = self._batch
+
+ def __str__(self):
+ if self._batch:
+ batch = ", %r" % self._batch.expected
+ else:
+ batch = ''
+ return "ConfiguredBatchNavigator(%r, %r%s)" % (
+ self._single.expected, self._plural.expected, batch)
+
+ def match(self, matchee):
+ if not isinstance(matchee, BatchNavigator):
+ # Testtools doesn't have an IsInstanceMismatch yet.
+ return matchers._BinaryMismatch(
+ BatchNavigator, 'isinstance', matchee)
+ mismatches = []
+ for attrname, matcher in self.matchers.items():
+ mismatches.append(matcher.match(getattr(matchee, attrname)))
+ if not mismatches[-1]:
+ mismatches.pop(-1)
+ if mismatches:
+ return MismatchesAll(mismatches)
=== modified file 'lib/lp/testing/views.py'
--- lib/lp/testing/views.py 2010-08-24 01:57:53 +0000
+++ lib/lp/testing/views.py 2010-09-19 03:09:53 +0000
@@ -12,6 +12,10 @@
import os
+from testtools.matchers import (
+ Matcher,
+ Mismatch,
+ )
from zope.component import (
getMultiAdapter,
getUtility,
