launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/bug-752153 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #752153 in Launchpad itself: " batch navigation uses high OFFSET rather than db constraints"
  https://bugs.launchpad.net/launchpad/+bug/752153

For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-752153/+merge/56505

Update our batchnavigator dependency to 1.2.3 which supports database friendly batching (including stable batchs that don't skip/repeat things when folk remove or add items).

The FUGLY bit in the google class that subclasses BatchNavigator is because its not calling the constructor - A small future iteration in batchnavigator will allow that to be cleaned up, but I noticed that very late in the piece, and its not really related to the thrust of this work.
-- 
https://code.launchpad.net/~lifeless/launchpad/bug-752153/+merge/56505
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-752153 into lp:launchpad.

=== modified file 'lib/lp/app/browser/root.py'
--- lib/lp/app/browser/root.py	2011-03-07 16:32:12 +0000
+++ lib/lp/app/browser/root.py	2011-04-06 05:04:27 +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.schema.interfaces import TooLong
@@ -560,6 +561,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
@@ -567,7 +569,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
@@ -583,20 +587,68 @@
         # 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:
+        local = (self.batch_variable_name, self.start_variable_name,
+            self.memo_variable_name, self.direction_variable_name)
+        self.transient_parameters = set(local)
+        if transient_parameters is not None:
+            self.transient_parameters.update(transient_parameters)
+
+        # For backwards compatibility (as in the past a work-around has been
+        # to include the url batch params in hidden fields within posted
+        # forms), if the request is a POST request, and either the 'start'
+        # or 'batch' params are included then revert to the default behaviour
+        # of using the request (which automatically gets the params from the
+        # request.form dict).
+        if request.method == 'POST' and (
+            self.start_variable_name in request.form or
+            self.batch_variable_name in request.form):
+            batch_params_source = request
+        else:
+            # We grab the request variables directly from the requests
+            # query_string_parameters so that they will be recognized
+            # even during post operations.
+            batch_params_source = dict(
+                (k, v[0]) for k, v
+                in self.query_string_parameters.items() if k in local)
+
+        # In this code we ignore invalid request variables since it
+        # probably means the user finger-fumbled it in the request. We
+        # could raise UnexpectedFormData, but is there a good reason?
+        def param_var(name):
+            return batch_params_source.get(name, None)
+        # -- start
+        request_start = param_var(self.start_variable_name)
+        if force_start or request_start is None:
             self.start = start
         else:
             try:
                 self.start = int(request_start)
             except (ValueError, TypeError):
                 self.start = start
-
+        # -- size
         self.default_size = 20
-
-        self.transient_parameters = [self.start_variable_name]
-
-        self.batch = WindowedListBatch(
-            results, start=self.start, size=self.default_size)
+        size = self.default_size
+        # -- direction
+        direction = param_var(self.direction_variable_name)
+        if direction == 'backwards':
+            direction = False
+        else:
+            direction = None
+        # -- memo
+        memo = param_var(self.memo_variable_name)
+        if direction is not None and memo is None:
+            # Walking backwards from the end - the only case where we generate
+            # a url with no memo but a direction (and the only case where we
+            # need it: from the start with no memo is equivalent to a simple
+            # list slice anyway).
+            memo = ''
+
+        if range_factory is None:
+            range_factory = ListRangeFactory(results)
+        self.batch = self._batch_factory(results, range_factory,
+            start=self.start, size=size, range_forwards=direction,
+            range_memo=memo)
+        if callback is not None:
+            callback(self, self.batch)
         self.setHeadings(
             self.default_singular_heading, self.default_plural_heading)

=== modified file 'lib/lp/app/browser/tests/launchpad-search-pages.txt'
--- lib/lp/app/browser/tests/launchpad-search-pages.txt	2011-03-07 16:32:12 +0000
+++ lib/lp/app/browser/tests/launchpad-search-pages.txt	2011-04-06 05:04:27 +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/services/features/browser/tests/test_changelog.py'
--- lib/lp/services/features/browser/tests/test_changelog.py	2011-02-18 21:51:56 +0000
+++ lib/lp/services/features/browser/tests/test_changelog.py	2011-04-06 05:04:27 +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 'versions.cfg'
--- versions.cfg	2011-04-05 12:10:01 +0000
+++ versions.cfg	2011-04-06 05:04:27 +0000
@@ -29,7 +29,7 @@
 keyring = 0.5.1
 launchpadlib = 1.9.3
 lazr.authentication = 0.1.1
-lazr.batchnavigator = 1.2.2
+lazr.batchnavigator = 1.2.3
 lazr.config = 1.1.3
 lazr.delegates = 1.2.0
 lazr.enum = 1.1.2