launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04261
[Merge] lp:~lifeless/launchpad/batchnav into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/batchnav into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/batchnav/+merge/67791
This is https://code.launchpad.net/~stub/launchpad/batch-navigator/+merge/67233 updated to grab 1.2.5 and fix the regression we found in it (discussed on that bug). The upstream change is trivial so I'm just self-reviewing this and shooting to ec2. I've made a placeholder bug to permit a QA step.
--
https://code.launchpad.net/~lifeless/launchpad/batchnav/+merge/67791
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/batchnav into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py 2010-10-14 23:03:41 +0000
+++ lib/canonical/launchpad/webapp/batching.py 2011-07-13 06:27:29 +0000
@@ -98,19 +98,17 @@
class ActiveBatchNavigator(BatchNavigator):
"""A paginator for active items.
- Used when a view needs to display more than one BatchNavigator of items.
+ Used when a view needs to display two BatchNavigators.
"""
- start_variable_name = 'active_start'
- batch_variable_name = 'active_batch'
+ variable_name_prefix = 'active'
class InactiveBatchNavigator(BatchNavigator):
"""A paginator for inactive items.
- Used when a view needs to display more than one BatchNavigator of items.
+ Used when a view needs to display two Batchavigators.
"""
- start_variable_name = 'inactive_start'
- batch_variable_name = 'inactive_batch'
+ variable_name_prefix = 'inactive'
class TableBatchNavigator(BatchNavigator):
=== modified file 'lib/lp/app/browser/root.py'
--- lib/lp/app/browser/root.py 2011-07-12 01:42:11 +0000
+++ lib/lp/app/browser/root.py 2011-07-13 06:27:29 +0000
@@ -13,6 +13,7 @@
import time
import feedparser
+from lazr.batchnavigator import ListRangeFactory
from lazr.batchnavigator.z3batching import batch
from zope.component import getUtility
from zope.interface import Interface
@@ -566,6 +567,7 @@
class GoogleBatchNavigator(BatchNavigator):
"""A batch navigator with a fixed size of 20 items per batch."""
+ _batch_factory = WindowedListBatch
# Searches generally don't show the 'Last' link when there is a
# good chance of getting over 100,000 results.
show_last_link = False
@@ -573,7 +575,9 @@
singular_heading = 'page'
plural_heading = 'pages'
- def __init__(self, results, request, start=0, size=20, callback=None):
+ def __init__(self, results, request, start=0, size=20, callback=None,
+ transient_parameters=None, force_start=False,
+ range_factory=None):
"""See `BatchNavigator`.
:param results: A `PageMatches` object that contains the matching
@@ -584,25 +588,13 @@
:param size: The batch size is fixed to 20, The param is not used.
:param callback: Not used.
"""
- # We do not want to call super() because it will use the batch
- # size from the URL.
- # pylint: disable-msg=W0231
results = WindowedList(results, start, results.total)
- self.request = request
- request_start = request.get(self.start_variable_name, None)
- if request_start is None:
- self.start = start
- else:
- try:
- self.start = int(request_start)
- except (ValueError, TypeError):
- self.start = start
+ super(GoogleBatchNavigator, self).__init__(results, request,
+ start=start, size=size, callback=callback,
+ transient_parameters=transient_parameters, force_start=force_start,
+ range_factory=range_factory)
+ def determineSize(self, size, batch_params_source):
+ # Force the default and users requested sizes to 20.
self.default_size = 20
-
- self.transient_parameters = [self.start_variable_name]
-
- self.batch = WindowedListBatch(
- results, start=self.start, size=self.default_size)
- self.setHeadings(
- self.default_singular_heading, self.default_plural_heading)
+ return 20
=== modified file 'lib/lp/app/browser/tests/launchpad-search-pages.txt'
--- lib/lp/app/browser/tests/launchpad-search-pages.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/app/browser/tests/launchpad-search-pages.txt 2011-07-13 06:27:29 +0000
@@ -444,8 +444,6 @@
provides a link to the next batch, which also happens to be the last
batch.
- >>> len(search_view.pages.getBatches())
- 2
>>> search_view.pages.nextBatchURL()
'...start=20'
>>> search_view.pages.lastBatchURL()
=== modified file 'lib/lp/app/doc/batch-navigation.txt'
--- lib/lp/app/doc/batch-navigation.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/app/doc/batch-navigation.txt 2011-07-13 06:27:29 +0000
@@ -47,88 +47,18 @@
... 'Cupid', 'Donner', 'Blitzen', 'Rudolph']
-Performance with SQLObject
-==========================
-
-This section demonstrates that batching generates sensible SQL queries when
-used with SQLObject, i.e. that it puts appropriate LIMIT clauses on queries.
-
-Imports and initialization:
-
- >>> from lp.testing.pgsql import CursorWrapper
- >>> from canonical.launchpad.database.emailaddress import EmailAddress
- >>> from canonical.launchpad.interfaces.lpstorm import IStore
- >>> ignore = IStore(EmailAddress) # Prime the database connection.
- ... # (Priming is important if this test is run in isolation).
- >>> CursorWrapper.record_sql = True
-
-Prepare a query, and create a batch of the results:
-
- >>> select_results = EmailAddress.select(orderBy='id')
- >>> batch_nav = BatchNavigator(select_results, build_request(), size=10)
- >>> email_batch = batch_nav.currentBatch()
- >>> batch_items = list(email_batch)
-
-Because we're only looking at the first batch, the database is only
-asked for the first 11 rows. (lazr.batchnavigator asks for 11 instead
-of 10 so that it can reliably detect the end of the dataset).
-
- >>> len(CursorWrapper.last_executed_sql)
- 1
- >>> print CursorWrapper.last_executed_sql[0]
- SELECT ... FROM EmailAddress ... LIMIT 11...
-
-Get the next 10. The database is only asked for the next 11 rows:
-
- >>> CursorWrapper.last_executed_sql = []
- >>> email_batch2 = email_batch.nextBatch()
- >>> batch_items = list(email_batch2)
- >>> len(CursorWrapper.last_executed_sql)
- 1
- >>> CursorWrapper.last_executed_sql[0].endswith('LIMIT 11 OFFSET 10')
- True
-
-As seen above, simply accessing the batch doesn't trigger a SQL query
-asking for the length of the entire resultset. But explicitly asking
-for the length will trigger a SQL query in most circumstances.
-
- >>> CursorWrapper.last_executed_sql = []
- >>> ignored = email_batch.total()
- >>> print CursorWrapper.last_executed_sql[0]
- SELECT COUNT(*) FROM EmailAddress
-
-There are exceptions. When the current batch is the last one in the
-list, it's possible to get the length of the entire resultset without
-triggering a COUNT query.
-
- >>> CursorWrapper.last_executed_sql = []
- >>> batch_nav = BatchNavigator(select_results, build_request(), size=50)
- >>> final_batch = batch_nav.currentBatch().nextBatch()
- >>> batch_items = list(final_batch)
- >>> ignored = final_batch.total()
- >>> print "\n".join(CursorWrapper.last_executed_sql)
- SELECT ... FROM EmailAddress ... OFFSET 0
- SELECT ... FROM EmailAddress ... OFFSET 50
-
-When the current batch contains the entire resultset, it's possible to
-get the length of the resultset without triggering a COUNT query.
-
- >>> CursorWrapper.last_executed_sql = []
- >>> one_page_nav = BatchNavigator(select_results, build_request(), size=200)
- >>> only_batch = one_page_nav.currentBatch()
- >>> batch_items = list(only_batch)
- >>> ignored = only_batch.total()
- >>> print "\n".join(CursorWrapper.last_executed_sql)
- SELECT ... FROM EmailAddress ... OFFSET 0
-
Multiple pages
==============
The batch navigator tells us whether multiple pages will be used.
+ >>> from canonical.launchpad.database.emailaddress import EmailAddress
+ >>> select_results = EmailAddress.select(orderBy='id')
+ >>> batch_nav = BatchNavigator(select_results, build_request(), size=50)
>>> batch_nav.has_multiple_pages
True
+ >>> one_page_nav = BatchNavigator(select_results, build_request(), size=200)
>>> one_page_nav.has_multiple_pages
False
=== modified file 'lib/lp/bugs/stories/webservice/xx-bug-tracker.txt'
--- lib/lp/bugs/stories/webservice/xx-bug-tracker.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug-tracker.txt 2011-07-13 06:27:29 +0000
@@ -10,7 +10,7 @@
>>> bug_tracker_collection = anon_webservice.get(
... '/bugs/bugtrackers').jsonBody()
>>> pprint_collection(bug_tracker_collection)
- next_collection_link: u'http://.../bugs/bugtrackers?ws.start=5&ws.size=5'
+ next_collection_link: u'http://.../bugs/bugtrackers?ws.size=5&memo=5&ws.start=5'
resource_type_link: u'http://.../#bug_trackers'
start: 0
total_size: 8
=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt 2011-07-13 06:27:29 +0000
@@ -1841,7 +1841,7 @@
>>> cves = webservice.get("/bugs/cve").jsonBody()
>>> pprint_collection(cves)
- next_collection_link: u'http://.../bugs/cve?ws.start=5&ws.size=5'
+ next_collection_link: u'http://.../bugs/cve?ws.size=5&memo=5&ws.start=5'
resource_type_link: u'http://.../#cves'
start: 0
total_size: 10
@@ -2130,7 +2130,7 @@
>>> activity = webservice.get(
... bug_one['activity_collection_link']).jsonBody()
>>> pprint_collection(activity)
- next_collection_link: u'http://.../bugs/1/activity?ws.start=5&ws.size=5'
+ next_collection_link: u'http://.../bugs/1/activity?ws.size=5&memo=5&ws.start=5'
resource_type_link: u'http://.../#bug_activity-page-resource'
start: 0
total_size: 25
=== modified file 'lib/lp/services/features/browser/tests/test_changelog.py'
--- lib/lp/services/features/browser/tests/test_changelog.py 2011-07-12 01:42:11 +0000
+++ lib/lp/services/features/browser/tests/test_changelog.py 2011-07-13 06:27:29 +0000
@@ -80,7 +80,7 @@
self.assertEqual('change', batch._singular_heading)
self.assertEqual('changes', batch._plural_heading)
self.assertEqual(10, batch.default_size)
- self.assertEqual(2, len(batch.getBatches()))
+ self.assertEqual(None, batch.currentBatch().nextBatch().nextBatch())
def test_page_batched_changes(self):
self.makeFeatureFlagChanges()
=== modified file 'lib/lp/soyuz/stories/ppa/xx-ppa-navigation.txt'
--- lib/lp/soyuz/stories/ppa/xx-ppa-navigation.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/soyuz/stories/ppa/xx-ppa-navigation.txt 2011-07-13 06:27:29 +0000
@@ -179,8 +179,8 @@
The 'First' and 'Previous' links, however, are now active.
>>> anon_browser.getLink('First').url
- 'http://launchpad.dev/%7Ecprov/+archive/ppa/+index?start=0&batch=1'
+ 'http://launchpad.dev/%7Ecprov/+archive/ppa/+index?batch=1'
>>> anon_browser.getLink('Previous').url
- 'http://launchpad.dev/%7Ecprov/+archive/ppa/+index?start=1&batch=1'
+ 'http://launchpad.dev/%7Ecprov/+archive/ppa/+index?batch=1&direction=backwards&memo=2&start=1'
=== modified file 'lib/lp/translations/browser/tests/pofile-views.txt'
--- lib/lp/translations/browser/tests/pofile-views.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/translations/browser/tests/pofile-views.txt 2011-07-13 06:27:29 +0000
@@ -241,7 +241,7 @@
Also, we get redirected to the next batch.
>>> pofile_view.request.response.getHeader('Location')
- 'http://127.0.0.1?start=10'
+ 'http://127.0.0.1?memo=10&start=10'
The messsage's sequence is the position of that message in latest imported
template. We are going to test now what happens when we submit a potmsgset
=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py 2011-07-12 01:42:11 +0000
+++ lib/lp/translations/browser/translationmessage.py 2011-07-13 06:27:29 +0000
@@ -128,6 +128,9 @@
results is an iterable of results. request is the web request
being processed. size is a default batch size which the callsite
can choose to provide.
+
+ Why a custom BatchNavigator is required is a great mystery and
+ should be documented here.
"""
schema, netloc, path, parameters, query, fragment = (
urlparse(str(request.URL)))
@@ -164,7 +167,7 @@
BatchNavigator.__init__(self, results, request, start_value, size)
- def generateBatchURL(self, batch):
+ def generateBatchURL(self, batch, backwards=False):
"""Return a custom batch URL for `ITranslationMessage`'s views."""
url = ""
if batch is None:
=== modified file 'lib/lp/translations/stories/importqueue/xx-translation-import-queue-filtering.txt'
--- lib/lp/translations/stories/importqueue/xx-translation-import-queue-filtering.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/translations/stories/importqueue/xx-translation-import-queue-filtering.txt 2011-07-13 06:27:29 +0000
@@ -56,7 +56,7 @@
>>> browser.getLink('Next')
<Link
text='Next'
- url='http://.../+imports/+index?start=5&batch=5'>
+ url='http://.../+imports/+index?batch=5&memo=5&start=5'>
== Target filter ==
=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate-empty-strings-without-validation.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-translate-empty-strings-without-validation.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-translate-empty-strings-without-validation.txt 2011-07-13 06:27:29 +0000
@@ -27,5 +27,5 @@
it as an error.
>>> print browser.url
- http://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+pots/evolution-2.2/es/+translate?batch=1&start=13
+ http://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+pots/evolution-2.2/es/+translate?batch=1&memo=13&start=13
=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate-html-tags-escape.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-translate-html-tags-escape.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-translate-html-tags-escape.txt 2011-07-13 06:27:29 +0000
@@ -20,7 +20,7 @@
We are in next form page.
>>> print user_browser.url
- http://translations.launchpad.dev/ubuntu/hoary/+source/pmount/+pots/pmount/hr/+translate?start=10
+ http://translations.launchpad.dev/ubuntu/hoary/+source/pmount/+pots/pmount/hr/+translate?memo=10&start=10
Let's go back to the modified message.
=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate-newlines-check.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-translate-newlines-check.txt 2011-07-12 01:42:11 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-translate-newlines-check.txt 2011-07-13 06:27:29 +0000
@@ -82,7 +82,7 @@
We were redirected to the next form, the translation was accepted.
>>> print browser.url
- http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/es/+translate?batch=1&start=0
+ http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/es/+translate?batch=1
Get previous page to check that the save translation is the right one.
@@ -112,7 +112,7 @@
We were redirected to the next form, the translation was accepted.
>>> print browser.url
- http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/es/+translate?batch=1&start=0
+ http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/es/+translate?batch=1
Get previous page to check that the save translation is the right one.
@@ -142,7 +142,7 @@
We were redirected to the next form, the translation was accepted.
>>> print browser.url
- http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/es/+translate?batch=1&start=0
+ http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/es/+translate?batch=1
Get previous page to check that the save translation is the right one.
=== modified file 'versions.cfg'
--- versions.cfg 2011-07-13 01:37:43 +0000
+++ versions.cfg 2011-07-13 06:27:29 +0000
@@ -30,7 +30,7 @@
launchpadlib = 1.9.3
lazr.amqp = 0.1
lazr.authentication = 0.1.1
-lazr.batchnavigator = 1.2.2
+lazr.batchnavigator = 1.2.5
lazr.config = 1.1.3
lazr.delegates = 1.2.0
lazr.enum = 1.1.3
