Merge lp:~lifeless/launchpad/malone into lp:launchpad
- malone
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Robert Collins | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 11582 | ||||
Proposed branch: | lp:~lifeless/launchpad/malone | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
598 lines (+223/-101) 13 files modified
lib/canonical/launchpad/webapp/batching.py (+18/-0) lib/lp/blueprints/browser/tests/test_specificationtarget.py (+6/-27) lib/lp/bugs/browser/bugtracker.py (+29/-16) lib/lp/bugs/browser/tests/test_bugtracker_views.py (+51/-17) lib/lp/bugs/interfaces/bugtracker.py (+8/-0) lib/lp/bugs/model/bugtracker.py (+15/-1) lib/lp/bugs/stories/bugtracker/bugtrackers-index.txt (+6/-4) lib/lp/bugs/templates/bugtrackers-index.pt (+6/-4) lib/lp/bugs/tests/test_bugtracker.py (+28/-1) lib/lp/registry/browser/person.py (+5/-19) lib/lp/registry/browser/team.py (+0/-2) lib/lp/registry/browser/tests/test_team.py (+4/-10) lib/lp/testing/matchers.py (+47/-0) |
||||
To merge this branch: | bzr merge lp:~lifeless/launchpad/malone | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Hudson-Doyle | Approve | ||
Review via email: mp+35941@code.launchpad.net |
Commit message
Batch /bugs/bugtrackers
Description of the change
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).
Robert Collins (lifeless) wrote : | # |
Thanks for the feedback will do most of it.
I have a few quirks though:
- matchers inline - i agree, but the noddy style guide line wrapping
rules make it ugly. If you agree that inline is better even so, I'll
move it inline and wrap it the way I would in every other python
project.
- the flavor comment is there because its a deprecated API. I think
the deprecation is a mistake until we actually don't need it.
Thanks,
Rob
Michael Hudson-Doyle (mwhudson) wrote : | # |
For the first, I don't think:
is too bad, but *shrug*, indent it however you like :)
For the second, fine.
Preview Diff
1 | === modified file 'lib/canonical/launchpad/webapp/batching.py' |
2 | --- lib/canonical/launchpad/webapp/batching.py 2010-09-06 18:52:45 +0000 |
3 | +++ lib/canonical/launchpad/webapp/batching.py 2010-09-20 05:15:57 +0000 |
4 | @@ -77,6 +77,24 @@ |
5 | return self.batch.total() > self.batch.size |
6 | |
7 | |
8 | +class ActiveBatchNavigator(BatchNavigator): |
9 | + """A paginator for active items. |
10 | + |
11 | + Used when a view needs to display more than one BatchNavigator of items. |
12 | + """ |
13 | + start_variable_name = 'active_start' |
14 | + batch_variable_name = 'active_batch' |
15 | + |
16 | + |
17 | +class InactiveBatchNavigator(BatchNavigator): |
18 | + """A paginator for inactive items. |
19 | + |
20 | + Used when a view needs to display more than one BatchNavigator of items. |
21 | + """ |
22 | + start_variable_name = 'inactive_start' |
23 | + batch_variable_name = 'inactive_batch' |
24 | + |
25 | + |
26 | class TableBatchNavigator(BatchNavigator): |
27 | """See canonical.launchpad.interfaces.ITableBatchNavigator.""" |
28 | implements(ITableBatchNavigator) |
29 | |
30 | === modified file 'lib/lp/blueprints/browser/tests/test_specificationtarget.py' |
31 | --- lib/lp/blueprints/browser/tests/test_specificationtarget.py 2010-09-17 14:04:29 +0000 |
32 | +++ lib/lp/blueprints/browser/tests/test_specificationtarget.py 2010-09-20 05:15:57 +0000 |
33 | @@ -3,11 +3,9 @@ |
34 | |
35 | __metaclass__ = type |
36 | |
37 | -import unittest |
38 | |
39 | from zope.security.proxy import removeSecurityProxy |
40 | |
41 | -from canonical.launchpad.webapp.batching import BatchNavigator |
42 | from canonical.launchpad.testing.pages import find_tag_by_id |
43 | from canonical.testing.layers import DatabaseFunctionalLayer |
44 | from lp.blueprints.interfaces.specificationtarget import ( |
45 | @@ -21,6 +19,7 @@ |
46 | login_person, |
47 | TestCaseWithFactory, |
48 | ) |
49 | +from lp.testing.matchers import IsConfiguredBatchNavigator |
50 | from lp.testing.views import ( |
51 | create_view, |
52 | create_initialized_view, |
53 | @@ -103,27 +102,17 @@ |
54 | '<div id="involvement" class="portlet involvement">' in view()) |
55 | |
56 | def test_specs_batch(self): |
57 | - # Some pages turn up in very large contexts and patch. E.g. |
58 | + # Some pages turn up in very large contexts and so batch. E.g. |
59 | # Distro:+assignments which uses SpecificationAssignmentsView, a |
60 | # subclass. |
61 | person = self.factory.makePerson() |
62 | view = create_initialized_view(person, name='+assignments') |
63 | - self.assertIsInstance(view.specs_batched, BatchNavigator) |
64 | - |
65 | - def test_batch_headings(self): |
66 | - person = self.factory.makePerson() |
67 | - view = create_initialized_view(person, name='+assignments') |
68 | - navigator = view.specs_batched |
69 | - self.assertEqual('specification', navigator._singular_heading) |
70 | - self.assertEqual('specifications', navigator._plural_heading) |
71 | - |
72 | - def test_batch_size(self): |
73 | # Because +assignments is meant to provide an overview, we default to |
74 | # 500 as the default batch size. |
75 | - person = self.factory.makePerson() |
76 | - view = create_initialized_view(person, name='+assignments') |
77 | - navigator = view.specs_batched |
78 | - self.assertEqual(500, view.specs_batched.default_size) |
79 | + self.assertThat( |
80 | + view.specs_batched, |
81 | + IsConfiguredBatchNavigator( |
82 | + 'specification', 'specifications', batch_size=500)) |
83 | |
84 | |
85 | class TestAssignments(TestCaseWithFactory): |
86 | @@ -243,13 +232,3 @@ |
87 | login_person(project_group.owner) |
88 | view = create_initialized_view(project_group, '+specs') |
89 | self.assertEqual(False, view.can_configure_blueprints) |
90 | - |
91 | - |
92 | -def test_suite(): |
93 | - suite = unittest.TestSuite() |
94 | - suite.addTest(unittest.TestLoader().loadTestsFromName(__name__)) |
95 | - return suite |
96 | - |
97 | - |
98 | -if __name__ == '__main__': |
99 | - unittest.TextTestRunner().run(test_suite()) |
100 | |
101 | === modified file 'lib/lp/bugs/browser/bugtracker.py' |
102 | --- lib/lp/bugs/browser/bugtracker.py 2010-09-03 15:02:39 +0000 |
103 | +++ lib/lp/bugs/browser/bugtracker.py 2010-09-20 05:15:57 +0000 |
104 | @@ -54,7 +54,11 @@ |
105 | structured, |
106 | ) |
107 | from canonical.launchpad.webapp.authorization import check_permission |
108 | -from canonical.launchpad.webapp.batching import BatchNavigator |
109 | +from canonical.launchpad.webapp.batching import ( |
110 | + ActiveBatchNavigator, |
111 | + BatchNavigator, |
112 | + InactiveBatchNavigator, |
113 | + ) |
114 | from canonical.launchpad.webapp.breadcrumb import Breadcrumb |
115 | from canonical.launchpad.webapp.menu import NavigationMenu |
116 | from canonical.lazr.utils import smartquote |
117 | @@ -154,21 +158,30 @@ |
118 | pillar_limit = 3 |
119 | |
120 | def initialize(self): |
121 | - # Sort the bug trackers into active and inactive lists so that |
122 | - # we can display them separately. |
123 | - all_bug_trackers = list(self.context) |
124 | - self.active_bug_trackers = [ |
125 | - bug_tracker for bug_tracker in all_bug_trackers |
126 | - if bug_tracker.active] |
127 | - self.inactive_bug_trackers = [ |
128 | - bug_tracker for bug_tracker in all_bug_trackers |
129 | - if not bug_tracker.active] |
130 | - |
131 | - bugtrackerset = getUtility(IBugTrackerSet) |
132 | - # The caching of bugtracker pillars here avoids us hitting the |
133 | - # database multiple times for each bugtracker. |
134 | - self._pillar_cache = bugtrackerset.getPillarsForBugtrackers( |
135 | - all_bug_trackers) |
136 | + # eager load related pillars. In future we should do this for |
137 | + # just the rendered trackers, and also use group by to get |
138 | + # bug watch counts per tracker. However the batching makes |
139 | + # the inefficiency tolerable for now. Robert Collins 20100919. |
140 | + self._pillar_cache = self.context.getPillarsForBugtrackers( |
141 | + list(self.context.trackers())) |
142 | + |
143 | + @property |
144 | + def inactive_tracker_count(self): |
145 | + return self.inactive_trackers.currentBatch().listlength |
146 | + |
147 | + @cachedproperty |
148 | + def active_trackers(self): |
149 | + results = self.context.trackers(active=True) |
150 | + navigator = ActiveBatchNavigator(results, self.request) |
151 | + navigator.setHeadings('tracker', 'trackers') |
152 | + return navigator |
153 | + |
154 | + @cachedproperty |
155 | + def inactive_trackers(self): |
156 | + results = self.context.trackers(active=False) |
157 | + navigator = InactiveBatchNavigator(results, self.request) |
158 | + navigator.setHeadings('tracker', 'trackers') |
159 | + return navigator |
160 | |
161 | def getPillarData(self, bugtracker): |
162 | """Return dict of pillars and booleans indicating ellipsis. |
163 | |
164 | === modified file 'lib/lp/bugs/browser/tests/test_bugtracker_views.py' |
165 | --- lib/lp/bugs/browser/tests/test_bugtracker_views.py 2010-08-26 20:08:43 +0000 |
166 | +++ lib/lp/bugs/browser/tests/test_bugtracker_views.py 2010-09-20 05:15:57 +0000 |
167 | @@ -3,27 +3,61 @@ |
168 | |
169 | """Tests for BugTracker views.""" |
170 | |
171 | +from __future__ import with_statement |
172 | + |
173 | __metaclass__ = type |
174 | |
175 | -import unittest |
176 | - |
177 | -from datetime import datetime, timedelta |
178 | -from pytz import utc |
179 | - |
180 | from zope.component import getUtility |
181 | -from zope.security.interfaces import Unauthorized |
182 | |
183 | -from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities |
184 | +from canonical.launchpad.testing.pages import find_tag_by_id |
185 | +from canonical.launchpad.webapp import canonical_url |
186 | from canonical.testing.layers import DatabaseFunctionalLayer |
187 | - |
188 | -from lp.bugs.browser.bugtracker import ( |
189 | - BugTrackerEditView) |
190 | +from lp.bugs.interfaces.bugtracker import IBugTrackerSet |
191 | from lp.registry.interfaces.person import IPersonSet |
192 | -from lp.testing import login, TestCaseWithFactory |
193 | -from lp.testing.sampledata import ADMIN_EMAIL, NO_PRIVILEGE_EMAIL |
194 | +from lp.testing import ( |
195 | + login, |
196 | + TestCaseWithFactory, |
197 | + ) |
198 | from lp.testing.views import create_initialized_view |
199 | - |
200 | - |
201 | - |
202 | -def test_suite(): |
203 | - return unittest.TestLoader().loadTestsFromName(__name__) |
204 | +from lp.testing.matchers import IsConfiguredBatchNavigator |
205 | +from lp.testing import ( |
206 | + person_logged_in, |
207 | + TestCaseWithFactory, |
208 | + ) |
209 | +from lp.testing.sampledata import ADMIN_EMAIL |
210 | + |
211 | + |
212 | +class TestBugTrackerSetView(TestCaseWithFactory): |
213 | + |
214 | + layer = DatabaseFunctionalLayer |
215 | + |
216 | + def test_trackers_are_batch_navigators(self): |
217 | + trackers = getUtility(IBugTrackerSet) |
218 | + view = create_initialized_view(trackers, name='+index') |
219 | + matcher = IsConfiguredBatchNavigator('tracker', 'trackers') |
220 | + self.assertThat(view.active_trackers, matcher) |
221 | + self.assertThat(view.inactive_trackers, matcher) |
222 | + |
223 | + def test_page_is_batched(self): |
224 | + active_tracker1 = self.factory.makeBugTracker() |
225 | + active_tracker2 = self.factory.makeBugTracker() |
226 | + inactive_tracker1 = self.factory.makeBugTracker() |
227 | + inactive_tracker2 = self.factory.makeBugTracker() |
228 | + admin = getUtility(IPersonSet).find(ADMIN_EMAIL).any() |
229 | + with person_logged_in(admin): |
230 | + inactive_tracker1.active = False |
231 | + inactive_tracker2.active = False |
232 | + trackers = getUtility(IBugTrackerSet) |
233 | + url = (canonical_url(trackers) + |
234 | + "/+index?active_batch=1&inactive_batch=1") |
235 | + browser = self.getUserBrowser(url) |
236 | + content = browser.contents |
237 | + # XXX RobertCollns 20100919 bug=642504. The support for multiple batches |
238 | + # isn't complete and the id for the nav links gets duplicated. |
239 | + #self.assertEqual('next', |
240 | + # find_tag_by_id(content, 'upper-batch-nav-batchnav-next')['class']) |
241 | + #self.assertEqual('next', |
242 | + # find_tag_by_id(content, 'lower-batch-nav-batchnav-next')['class']) |
243 | + # Instead we check the string appears. |
244 | + self.assertTrue('upper-batch-nav-batchnav-next' in content) |
245 | + |
246 | |
247 | === modified file 'lib/lp/bugs/interfaces/bugtracker.py' |
248 | --- lib/lp/bugs/interfaces/bugtracker.py 2010-08-26 20:08:43 +0000 |
249 | +++ lib/lp/bugs/interfaces/bugtracker.py 2010-09-20 05:15:57 +0000 |
250 | @@ -432,6 +432,14 @@ |
251 | def getPillarsForBugtrackers(bug_trackers): |
252 | """Return dict mapping bugtrackers to lists of pillars.""" |
253 | |
254 | + def trackers(active=None): |
255 | + """Return a ResultSet of bugtrackers. |
256 | + |
257 | + :param active: If True, only active trackers are returned, if False |
258 | + only inactive trackers are returned. All trackers are returned |
259 | + by default. |
260 | + """ |
261 | + |
262 | |
263 | class IBugTrackerAlias(Interface): |
264 | """Another URL for a remote bug system. |
265 | |
266 | === modified file 'lib/lp/bugs/model/bugtracker.py' |
267 | --- lib/lp/bugs/model/bugtracker.py 2010-08-26 20:08:43 +0000 |
268 | +++ lib/lp/bugs/model/bugtracker.py 2010-09-20 05:15:57 +0000 |
269 | @@ -8,7 +8,8 @@ |
270 | 'BugTracker', |
271 | 'BugTrackerAlias', |
272 | 'BugTrackerAliasSet', |
273 | - 'BugTrackerSet'] |
274 | + 'BugTrackerSet', |
275 | + ] |
276 | |
277 | |
278 | from datetime import datetime |
279 | @@ -66,6 +67,8 @@ |
280 | IBugTrackerSet, |
281 | SINGLE_PRODUCT_BUGTRACKERTYPES, |
282 | ) |
283 | +from canonical.launchpad.webapp.interfaces import ( |
284 | + DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE) |
285 | from lp.bugs.interfaces.bugtrackerperson import BugTrackerPersonAlreadyExists |
286 | from lp.bugs.model.bug import Bug |
287 | from lp.bugs.model.bugmessage import BugMessage |
288 | @@ -577,6 +580,17 @@ |
289 | """See `IBugTrackerSet`.""" |
290 | return BugTracker.select() |
291 | |
292 | + def trackers(self, active=None): |
293 | + # Without context, cannot tell what store flavour is desirable. |
294 | + store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) |
295 | + if active is not None: |
296 | + clauses = [BugTracker.active==active] |
297 | + else: |
298 | + clauses = [] |
299 | + results = store.find(BugTracker, *clauses) |
300 | + results.order_by(BugTracker.name) |
301 | + return results |
302 | + |
303 | def ensureBugTracker(self, baseurl, owner, bugtrackertype, |
304 | title=None, summary=None, contactdetails=None, name=None): |
305 | """See `IBugTrackerSet`.""" |
306 | |
307 | === modified file 'lib/lp/bugs/stories/bugtracker/bugtrackers-index.txt' |
308 | --- lib/lp/bugs/stories/bugtracker/bugtrackers-index.txt 2010-07-26 13:30:57 +0000 |
309 | +++ lib/lp/bugs/stories/bugtracker/bugtrackers-index.txt 2010-09-20 05:15:57 +0000 |
310 | @@ -39,11 +39,11 @@ |
311 | Email Address |
312 | 0 |
313 | ------------------------ |
314 | - GNU Savannah Bug Tracker |
315 | - http://savannah.gnu.org/ |
316 | - --> http://savannah.gnu.org/ |
317 | + T'other Gnome GBugGTracker |
318 | + http://bugzilla.gnome.org/ |
319 | + --> http://bugzilla.gnome.org/ |
320 | — |
321 | - Savane |
322 | + Bugzilla |
323 | 0 |
324 | ------------------------ |
325 | ... |
326 | @@ -68,6 +68,8 @@ |
327 | — |
328 | Email Address |
329 | 0 |
330 | + ------------------------ |
331 | + ... |
332 | |
333 | The watch counts match the number of bugs listed, of course: |
334 | |
335 | |
336 | === modified file 'lib/lp/bugs/templates/bugtrackers-index.pt' |
337 | --- lib/lp/bugs/templates/bugtrackers-index.pt 2009-09-01 16:24:52 +0000 |
338 | +++ lib/lp/bugs/templates/bugtrackers-index.pt 2010-09-20 05:15:57 +0000 |
339 | @@ -13,6 +13,7 @@ |
340 | </tal:comment> |
341 | <metal:macros fill-slot="bogus"> |
342 | <metal:macro define-macro="tracker-table"> |
343 | + <tal:navigation content="structure trackers/@@+navigation-links-upper" /> |
344 | <table class="sortable listing" tal:attributes="id id"> |
345 | <thead> |
346 | <tr> |
347 | @@ -24,7 +25,7 @@ |
348 | </tr> |
349 | </thead> |
350 | <tbody> |
351 | - <tr tal:repeat="tracker trackers"> |
352 | + <tr tal:repeat="tracker trackers/currentBatch"> |
353 | <td> |
354 | <a tal:replace="structure tracker/fmt:link" /> |
355 | </td> |
356 | @@ -53,6 +54,7 @@ |
357 | </tr> |
358 | </tbody> |
359 | </table> |
360 | + <tal:navigation content="structure trackers/@@+navigation-links-lower" /> |
361 | </metal:macro> |
362 | </metal:macros> |
363 | |
364 | @@ -70,7 +72,7 @@ |
365 | the external status changes. |
366 | </p> |
367 | <tal:table define="id string:trackers; |
368 | - trackers view/active_bug_trackers"> |
369 | + trackers view/active_trackers"> |
370 | <metal:table use-macro="template/macros/tracker-table" /> |
371 | </tal:table> |
372 | <p style="margin-top: 1em" |
373 | @@ -81,11 +83,11 @@ |
374 | </p> |
375 | </div> |
376 | </div> |
377 | - <div class="yui-u" tal:condition="view/inactive_bug_trackers"> |
378 | + <div class="yui-u" tal:condition="view/inactive_tracker_count"> |
379 | <div class="portlet"> |
380 | <h2>Inactive bug trackers</h2> |
381 | <tal:table define="id string:inactive-trackers; |
382 | - trackers view/inactive_bug_trackers"> |
383 | + trackers view/inactive_trackers"> |
384 | <metal:table use-macro="template/macros/tracker-table" /> |
385 | </tal:table> |
386 | </div> |
387 | |
388 | === modified file 'lib/lp/bugs/tests/test_bugtracker.py' |
389 | --- lib/lp/bugs/tests/test_bugtracker.py 2010-08-26 20:08:43 +0000 |
390 | +++ lib/lp/bugs/tests/test_bugtracker.py 2010-09-20 05:15:57 +0000 |
391 | @@ -28,7 +28,10 @@ |
392 | |
393 | from canonical.launchpad.ftests import login_person |
394 | from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities |
395 | -from canonical.testing import LaunchpadFunctionalLayer |
396 | +from canonical.testing import ( |
397 | + DatabaseFunctionalLayer, |
398 | + LaunchpadFunctionalLayer, |
399 | + ) |
400 | from lp.bugs.externalbugtracker import ( |
401 | BugTrackerConnectError, |
402 | Mantis, |
403 | @@ -38,12 +41,36 @@ |
404 | BugTrackerType, |
405 | IBugTracker, |
406 | ) |
407 | +from lp.bugs.model.bugtracker import BugTrackerSet |
408 | from lp.bugs.tests.externalbugtracker import Urlib2TransportTestHandler |
409 | from lp.registry.interfaces.person import IPersonSet |
410 | from lp.testing import login, login_person, TestCaseWithFactory |
411 | from lp.testing.sampledata import ADMIN_EMAIL |
412 | |
413 | |
414 | +class TestBugTrackerSet(TestCaseWithFactory): |
415 | + |
416 | + layer = DatabaseFunctionalLayer |
417 | + |
418 | + def test_trackers(self): |
419 | + tracker = self.factory.makeBugTracker() |
420 | + trackers = BugTrackerSet() |
421 | + # Active trackers are in all trackers, |
422 | + self.assertTrue(tracker in trackers.trackers()) |
423 | + # and active, |
424 | + self.assertTrue(tracker in trackers.trackers(active=True)) |
425 | + # But not inactive. |
426 | + self.assertFalse(tracker in trackers.trackers(active=False)) |
427 | + login(ADMIN_EMAIL) |
428 | + tracker.active = False |
429 | + # Inactive trackers are in all trackers |
430 | + self.assertTrue(tracker in trackers.trackers()) |
431 | + # and inactive, |
432 | + self.assertTrue(tracker in trackers.trackers(active=False)) |
433 | + # but not in active. |
434 | + self.assertFalse(tracker in trackers.trackers(active=True)) |
435 | + |
436 | + |
437 | class BugTrackerTestCase(TestCaseWithFactory): |
438 | """Unit tests for the `BugTracker` class.""" |
439 | |
440 | |
441 | === modified file 'lib/lp/registry/browser/person.py' |
442 | --- lib/lp/registry/browser/person.py 2010-09-15 16:44:41 +0000 |
443 | +++ lib/lp/registry/browser/person.py 2010-09-20 05:15:57 +0000 |
444 | @@ -194,7 +194,11 @@ |
445 | structured, |
446 | ) |
447 | from canonical.launchpad.webapp.authorization import check_permission |
448 | -from canonical.launchpad.webapp.batching import BatchNavigator |
449 | +from canonical.launchpad.webapp.batching import ( |
450 | + ActiveBatchNavigator, |
451 | + BatchNavigator, |
452 | + InactiveBatchNavigator, |
453 | + ) |
454 | from canonical.launchpad.webapp.breadcrumb import Breadcrumb |
455 | from canonical.launchpad.webapp.interfaces import ( |
456 | ILaunchBag, |
457 | @@ -1385,24 +1389,6 @@ |
458 | links = ['profile', 'polls', 'members', 'ppas'] |
459 | |
460 | |
461 | -class ActiveBatchNavigator(BatchNavigator): |
462 | - """A paginator for active items. |
463 | - |
464 | - Used when a view needs to display more than one BatchNavigator of items. |
465 | - """ |
466 | - start_variable_name = 'active_start' |
467 | - batch_variable_name = 'active_batch' |
468 | - |
469 | - |
470 | -class InactiveBatchNavigator(BatchNavigator): |
471 | - """A paginator for inactive items. |
472 | - |
473 | - Used when a view needs to display more than one BatchNavigator of items. |
474 | - """ |
475 | - start_variable_name = 'inactive_start' |
476 | - batch_variable_name = 'inactive_batch' |
477 | - |
478 | - |
479 | class TeamMembershipView(LaunchpadView): |
480 | """The view behind ITeam/+members.""" |
481 | |
482 | |
483 | === modified file 'lib/lp/registry/browser/team.py' |
484 | --- lib/lp/registry/browser/team.py 2010-09-17 18:32:23 +0000 |
485 | +++ lib/lp/registry/browser/team.py 2010-09-20 05:15:57 +0000 |
486 | @@ -829,8 +829,6 @@ |
487 | """ |
488 | results = self.mailing_list.getReviewableMessages() |
489 | navigator = BatchNavigator(results, self.request) |
490 | - # Subclasses often set the singular and plural headings, |
491 | - # but we can use the generic class too. |
492 | navigator.setHeadings('message', 'messages') |
493 | return navigator |
494 | |
495 | |
496 | === modified file 'lib/lp/registry/browser/tests/test_team.py' |
497 | --- lib/lp/registry/browser/tests/test_team.py 2010-09-14 00:21:04 +0000 |
498 | +++ lib/lp/registry/browser/tests/test_team.py 2010-09-20 05:15:57 +0000 |
499 | @@ -3,10 +3,10 @@ |
500 | |
501 | __metaclass__ = type |
502 | |
503 | -from canonical.launchpad.webapp.batching import BatchNavigator |
504 | from canonical.testing import DatabaseFunctionalLayer |
505 | from lp.registry.browser.person import TeamOverviewMenu |
506 | from lp.testing import TestCaseWithFactory |
507 | +from lp.testing.matchers import IsConfiguredBatchNavigator |
508 | from lp.testing.menu import check_menu_links |
509 | from lp.testing.views import create_initialized_view |
510 | |
511 | @@ -47,12 +47,6 @@ |
512 | team = self.factory.makeTeam() |
513 | self.factory.makeMailingList(team, team.teamowner) |
514 | view = create_initialized_view(team, name='+mailinglist-moderate') |
515 | - self.assertIsInstance(view.held_messages, BatchNavigator) |
516 | - |
517 | - def test_held_message_headings(self): |
518 | - team = self.factory.makeTeam() |
519 | - self.factory.makeMailingList(team, team.teamowner) |
520 | - view = create_initialized_view(team, name='+mailinglist-moderate') |
521 | - navigator = view.held_messages |
522 | - self.assertEqual('message', navigator._singular_heading) |
523 | - self.assertEqual('messages', navigator._plural_heading) |
524 | + self.assertThat( |
525 | + view.held_messages, |
526 | + IsConfiguredBatchNavigator('message', 'messages')) |
527 | |
528 | === modified file 'lib/lp/testing/matchers.py' |
529 | --- lib/lp/testing/matchers.py 2010-08-25 20:04:40 +0000 |
530 | +++ lib/lp/testing/matchers.py 2010-09-20 05:15:57 +0000 |
531 | @@ -17,9 +17,12 @@ |
532 | from testtools.content import Content |
533 | from testtools.content_type import UTF8_TEXT |
534 | from testtools.matchers import ( |
535 | + Equals, |
536 | Matcher, |
537 | Mismatch, |
538 | + MismatchesAll, |
539 | ) |
540 | +from testtools import matchers |
541 | from zope.interface.exceptions import ( |
542 | BrokenImplementation, |
543 | BrokenMethodImplementation, |
544 | @@ -31,6 +34,8 @@ |
545 | Proxy, |
546 | ) |
547 | |
548 | +from canonical.launchpad.webapp.batching import BatchNavigator |
549 | + |
550 | |
551 | class DoesNotProvide(Mismatch): |
552 | """An object does not provide an interface.""" |
553 | @@ -220,3 +225,45 @@ |
554 | if not matchee.startswith(self.expected): |
555 | return DoesNotStartWith(matchee, self.expected) |
556 | return None |
557 | + |
558 | + |
559 | +class IsConfiguredBatchNavigator(Matcher): |
560 | + """Check that an object is a batch navigator.""" |
561 | + |
562 | + def __init__(self, singular, plural, batch_size=None): |
563 | + """Create a ConfiguredBatchNavigator. |
564 | + |
565 | + :param singular: The singular header the batch should be using. |
566 | + :param plural: The plural header the batch should be using. |
567 | + :param batch_size: The batch size that should be configured by default. |
568 | + """ |
569 | + self._single = Equals(singular) |
570 | + self._plural = Equals(plural) |
571 | + self._batch = None |
572 | + if batch_size: |
573 | + self._batch = Equals(batch_size) |
574 | + self.matchers = dict( |
575 | + _singular_heading=self._single, _plural_heading=self._plural) |
576 | + if self._batch: |
577 | + self.matchers['default_size'] = self._batch |
578 | + |
579 | + def __str__(self): |
580 | + if self._batch: |
581 | + batch = ", %r" % self._batch.expected |
582 | + else: |
583 | + batch = '' |
584 | + return "ConfiguredBatchNavigator(%r, %r%s)" % ( |
585 | + self._single.expected, self._plural.expected, batch) |
586 | + |
587 | + def match(self, matchee): |
588 | + if not isinstance(matchee, BatchNavigator): |
589 | + # Testtools doesn't have an IsInstanceMismatch yet. |
590 | + return matchers._BinaryMismatch( |
591 | + BatchNavigator, 'isinstance', matchee) |
592 | + mismatches = [] |
593 | + for attrname, matcher in self.matchers.items(): |
594 | + mismatch = matcher.match(getattr(matchee, attrname)) |
595 | + if mismatch is not None: |
596 | + mismatches.append(mismatch) |
597 | + if mismatches: |
598 | + return MismatchesAll(mismatches) |
The change is basically fine, and clearly a good idea. I have a number of code clarity comments though, see below.
> === modified file 'lib/canonical/ launchpad/ webapp/ batching. py' launchpad/ webapp/ batching. py 2010-09-06 18:52:45 +0000 launchpad/ webapp/ batching. py 2010-09-19 22:21:05 +0000 gator(BatchNavi gator): vigator( BatchNavigator) :
> --- lib/canonical/
> +++ lib/canonical/
> @@ -77,6 +77,24 @@
> return self.batch.total() > self.batch.size
>
>
> +class ActiveBatchNavi
> + """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 InactiveBatchNa
> + """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'
> +
> +
These are fine and clear enough -- are they generic enough to go in a
generic enough location though? Later: oh, I see you moved them from
other code. It's a shame you have to subclass to change these.
> class TableBatchNavig ator(BatchNavig ator): launchpad. interfaces. ITableBatchNavi gator." "" ITableBatchNavi gator)
> """See canonical.
> implements(
> === modified file 'lib/lp/ blueprints/ browser/ tests/test_ specificationta rget.py' blueprints/ browser/ tests/test_ specificationta rget.py 2010-09-17 14:04:29 +0000 blueprints/ browser/ tests/test_ specificationta rget.py 2010-09-19 22:21:05 +0000 launchpad. webapp. batching import BatchNavigator launchpad. testing. pages import find_tag_by_id testing. layers import DatabaseFunctio nalLayer interfaces. specificationta rget import ( tory, chNavigator initialized_ view, makePerson( ) initialized_ view(person, name='+ assignments' )
> --- lib/lp/
> +++ lib/lp/
> @@ -3,11 +3,9 @@
>
> __metaclass__ = type
>
> -import unittest
>
> from zope.security.proxy import removeSecurityProxy
>
> -from canonical.
> from canonical.
> from canonical.
> from lp.blueprints.
> @@ -21,6 +19,7 @@
> login_person,
> TestCaseWithFac
> )
> +from lp.testing.matchers import IsConfiguredBat
> from lp.testing.views import (
> create_view,
> create_
> @@ -108,22 +107,11 @@
> # subclass.
> person = self.factory.
> view = create_
Just up from here there's a comment:
# Some pages turn up in very large contexts and patch. E.g. signmentsView, a
# Distro:+assignments which uses SpecificationAs
# subclass.
Is patch a typo for batch here?
> - self.assertIsIn stance( view.specs_ batched, BatchNavigator) headings( self): makePerson( ) initialized_ view(person, name='+ assignments' ) l('specificatio n', navigator. _singular_ heading) l('specificatio ns', navigator. _plural_ heading) size(self) :
> -
> - def test_batch_
> - person = self.factory.
> - view = create_
> - navigator = view.specs_batched
> - self.assertEqua
> - self.assertEqua
> -
> - def test_batch_
> # Because +assignments is meant to provide an overview, we default to
> # 500 as the default batch size.
> - ...