Merge lp:~adeuring/lazr.batchnavigator/lazr.batchnavigator-no-start-param-for-first-last-url into lp:lazr.batchnavigator

[Abel Deuring]

BatchNavigator.firstBatchURL() and BatchNavigator.lastBatchURL()
use the parameter start to determine if a "real" URL is returned
or if an empty URL is returned.

This works mostly fine when BatchNavigator can use a
ListRangeFactory, but it can lead to some confusion when a
StormRangeFactory is used. And even with a ListRangeFactory it
is possible to manually change the URL so that firstBatchURL()
and lastBatchURL() return an empty string: Just visit any
batched Launchpad view, click on the "next" link and then change
the query parameter "start=N" to "start=0". The page displayed
for this URL still shows the second batch, because BatchNavigator
and ListRangeFactory use the query parameter "memo" instead of
"start" to determine what to display. But the the "first batch"
on the page is just grey text instead of a "real" link.

For StormRangeFactory, this becomes a more serious problem, for
at least two reasons:

1:
StormRangeFactory does need to know the exact length of the
result set, which allows us to avoid expensive calls of
ResultSet.count(); we can use estimates of the row count
instead. This makes expressions like

   batch_start + batch_size > length_of_result_set

unreliable.

2:
* A user visits the first page of a batched list and clicks
   on the "next" link
* For some reason, the result set changes so that another
  element appears at the start of the result set.
* When the user now clicks the "previous" link, a view
  using StormRangeFactory will return that part of the result
  set coming directly before the current batch. This means
  that there another previous batch, starting with the newly
  created element. But the parameter "start" will still be
  zero, thus disabing the "first batch" link. (The text
  "previous batch" will still be a proper link.)
  (ListRangeFactory will return the first batch, now starting
  with the new element)

The code changes are simple:

_Batch.prevBatch() and _Batch.nextBatch() already had all
the logic needed to reliably determine if the current batch
is the first/last batch. I moced this logic into two new
properties, _Batch.has_previous_batch and _Batch.has_next_batch.

While prevBatch() and nextBatch() return None if the current
batch is the first/last batch, firstBatch()/lastBatch() always
return a _Batch instance, and firstBatchURL()/lastbatchURL()
determine if a real link or an empty string should be returned.
This decision was based to the value of batch.start, but is now
based on the the new properties.

I also noticed that an existing test did not test what it was
supposed to test -- this required a range factory which works
similar to StormRangeFactory, as described above. This test
is also about the behaviour of _Batch when the the result set
changes, as described above.)

[Graham Binns (gmb)]

Hi Abel,

Nice branch, just a few nitpicky things that need to be taken care of
before landing. r=me.

[1]

30 + def test_lastBatchURL_not_empty_for_bogus_start_value(self):
31 + # lastBatchURL() is correct even when the start parameter
32 + # is bogus.
33 + batchnav = sample_batchnav(start=9, batch=3, memo="6")
34 + self.assertThat(batchnav.lastBatchURL(),
35 + EqualsQuery(start=9, batch=3, direction='backwards'))
36 +

Why is the start parameter bogus here? Is that explained elsewhere? If
not, I think this needs a comment to explain why start=9 is bogus for
the batchnav you've created.

[2]

41 + def test_firstBatchURL_does_not_depend_on_start_parameter(self):
42 + # nextBatchURL() is correct even when start has the (incorrect)
43 + # value 0.
44 + batchnav = sample_batchnav(start=0, batch=3, memo='3')
45 + self.assertThat(batchnav.firstBatchURL(), EqualsQuery(batch=3))

Same again, really. If these tests were failing and I had to look at
them I wouldn't understand why the values you're passing are incorrect.
Explaining it here saves time down the road.

[3]

106 + def test_has_next_batch__forwards_with_memo_not_at_end(self):

111 + def test_has_next_batch__forwards_with_memo_at_end(self):

116 + def test_has_next_batch__forwards_without_memo_not_at_end(self):

120 + def test_has_next_batch__forwards_without_memo_at_end(self):

124 + def test_has_next_batch__backwards_with_memo_not_at_end(self):

129 + def test_has_next_batch__backforwards_with_memo_at_end(self):

I know it's obvious to you what these mean, but the act of truncating
the method names to fit within our character limits has stripped them of
context for someone who has not to start with (i.e. me). I'd appreciate
even a short comment at the start of each test explaining the expected
behaviour.

[4]

166 + return (
167 + (self.range_forwards and len(self.sliced_list) > self.size) or
168 + (not self.range_forwards and self.range_memo != ''))

That which is concise is often hard to parse. How about:

    if self.range_forwards:
        return len(self.sliced_list) > self.size
    else:
        return self.range_memo != ''

to make it clearer to chumps like me?

review: Approve (code)

[lp:~adeuring/lazr.batchnavigator/lazr.batchnavigator-no-start-param-for-first-last-url]

52. By Abel Deuring on 2011-09-06

implemented reviewer's comments

53. By Abel Deuring on 2011-09-07

Deuring: never commit without first running bin/test: now fix the two syntax errors.

=== modified file 'src/lazr/batchnavigator/_batchnavigator.py'
--- src/lazr/batchnavigator/_batchnavigator.py	2011-08-19 18:53:54 +0000
+++ src/lazr/batchnavigator/_batchnavigator.py	2011-09-07 09:29:23 +0000
@@ -274,7 +274,7 @@
 
     def firstBatchURL(self):
         batch = self.batch.firstBatch()
-        if self.start == 0:
+        if not self.batch.has_previous_batch:
             # We are already on the first batch.
             batch = None
         return self.generateBatchURL(batch)
@@ -287,7 +287,7 @@
 
     def lastBatchURL(self):
         batch = self.batch.lastBatch()
-        if self.start == batch.start:
+        if not self.batch.has_next_batch:
             # We are already on the last batch.
             batch = None
         return self.generateBatchURL(batch, backwards=True)

=== modified file 'src/lazr/batchnavigator/tests/test_batchnavigator.py'
--- src/lazr/batchnavigator/tests/test_batchnavigator.py	2011-08-19 18:53:54 +0000
+++ src/lazr/batchnavigator/tests/test_batchnavigator.py	2011-09-07 09:29:23 +0000
@@ -149,10 +149,27 @@
         self.assertThat(batchnav.lastBatchURL(),
             EqualsQuery(start=9, batch=3, direction='backwards'))
 
+    def test_lastBatchURL_not_empty_for_bogus_start_value(self):
+        # lastBatchURL() is correct even when the start parameter
+        # is bogus: memo says that the batch begins at offset
+        # 6, while start points to the last element of the
+        # of the batch.
+        batchnav = sample_batchnav(start=9, batch=3, memo="6")
+        self.assertThat(batchnav.lastBatchURL(),
+            EqualsQuery(start=9, batch=3, direction='backwards'))
+
     def test_firstBatchURL_is_trivial(self):
         batchnav = sample_batchnav(start=3, batch=3, memo='3')
         self.assertThat(batchnav.firstBatchURL(), EqualsQuery(batch=3))
 
+    def test_firstBatchURL_does_not_depend_on_start_parameter(self):
+        # nextBatchURL() is correct even when start has the (incorrect)
+        # value 0. Start = 0 implies that we are at the first batch.
+        # But this value may be wrong (see _Batch.__init__()), and
+        # firstBatchURL() does not rely on it.
+        batchnav = sample_batchnav(start=0, batch=3, memo='3')
+        self.assertThat(batchnav.firstBatchURL(), EqualsQuery(batch=3))
+
     def test_nextBatchURL_has_memo(self):
         batchnav = sample_batchnav(start=3, batch=3, memo='3')
         self.assertThat(batchnav.nextBatchURL(),

=== modified file 'src/lazr/batchnavigator/tests/test_z3batching.py'
--- src/lazr/batchnavigator/tests/test_z3batching.py	2011-08-24 15:40:20 +0000
+++ src/lazr/batchnavigator/tests/test_z3batching.py	2011-09-07 09:29:23 +0000
@@ -581,16 +581,17 @@
         # The start parameter of a backwards batch may reach zero
         # even when the real begin of a result set is not yet reached.
         data = [1, 2, 3, 4, 5, 6]
+        range_factory = RangeFactoryWithValueBasedEndpointMemos(data)
         # We create the first backwards.
         batch = _Batch(
-            data, range_factory=ListRangeFactory(data),
-            size=3, range_memo='', start=3, range_forwards=False)
+            data, range_factory=range_factory, size=3, range_memo='',
+            start=3, range_forwards=False)
         self.assertEqual([4, 5, 6], batch.sliced_list)
         self.assertEqual(3, batch.start)
         # The previous batch is now the first batch.
         batch = batch.prevBatch()
         self.assertEqual(0, batch.start)
-        self.assertEqual("3", batch.range_memo)
+        self.assertEqual(4, batch.range_memo)
         self.assertEqual([1, 2, 3, 4], batch.sliced_list)
         self.assertTrue(batch.prevBatch() is None)
         # If we now insert another value at the start of the result set,
@@ -598,14 +599,20 @@
         # we get the same data slice, but also a prevBtach()
         # that is not None.
         data = [0, 1, 2, 3, 4, 5, 6]
+        range_factory = RangeFactoryWithValueBasedEndpointMemos(data)
         new_batch = _Batch(
-            data, range_factory=ListRangeFactory(data),
-            size=3, range_memo=batch.range_memo, start=batch.start,
+            data, range_factory=range_factory, size=3,
+            range_memo=batch.range_memo, start=batch.start,
             range_forwards=False)
-        # the contents of the batch is still correct.
-        self.assertEqual([1, 2, 3, 4], batch.sliced_list)
+        # the contents of the batch is still correct. (Note that the
+        # value of the last does not matter, see _Batch.sliced_list().)
+        self.assertEqual([1, 2, 3, 0], new_batch.sliced_list)
         # And we get a previous batch.
         self.assertEqual([0, 1, 2, 3], new_batch.prevBatch().sliced_list)
+        # Note that both new_batch and its previous batch claim to start
+        # at 0.
+        self.assertEqual(0, new_batch.start)
+        self.assertEqual(0, new_batch.prevBatch().start)
 
     def test_last_backwards_batch_with_value_based_range_factory(self):
         # Another slice is added in _Batch.sliced_list() when the
@@ -647,4 +654,49 @@
         # startNumber, endNumber() are implemented
         self.assertEqual(1, partial.startNumber())
         self.assertEqual(4, partial.endNumber())
-        
+
+    def test_has_next_batch__forwards_with_memo_not_at_end(self):
+        # If a batch is not the last batch for a result set,
+        # has_next_batch is True. When the parameter range_memo
+        # is given, the value of start does not matter.
+        batch = self.getBatch(
+            start=7, size=3, range_forwards=True, range_memo="6")
+        self.assertTrue(batch.has_next_batch)
+
+    def test_has_next_batch__forwards_with_memo_at_end(self):
+        # If a batch is the last batch for a result set,
+        # has_next_batch is False. When the parameter range_memo
+        # is given, the value of start does not matter.
+        batch = self.getBatch(
+            start=6, size=3, range_forwards=True, range_memo="7")
+        self.assertFalse(batch.has_next_batch)
+
+    def test_has_next_batch__forwards_without_memo_not_at_end(self):
+        # If a batch is the last batch for a result set,
+        # has_next_batch is False. When the parameter range_memo
+        # is not given, the value of start is used.
+        batch = self.getBatch(start=6, size=3)
+        self.assertTrue(batch.has_next_batch)
+
+    def test_has_next_batch__forwards_without_memo_at_end(self):
+        # If a batch is the last batch for a result set,
+        # has_next_batch is False. When the parameter range_memo
+        # is not given, the value of start is used.
+        batch = self.getBatch(start=7, size=3)
+        self.assertFalse(batch.has_next_batch)
+
+    def test_has_previous_batch__backwards_with_memo_not_at_end(self):
+        # If a batch is not the first batch for a result set,
+        # has_previous_batch is True. The value of start does not
+        # matter.
+        batch = self.getBatch(
+            start=0, size=3, range_forwards=False, range_memo="4")
+        self.assertTrue(batch.has_previous_batch)
+
+    def test_has_previous_batch__backwards_with_memo_at_end(self):
+        # If a batch is not the first batch for a result set,
+        # has_previous_batch is False.The value of start does not
+        # matter.
+        batch = self.getBatch(
+            start=1, size=3, range_forwards=False, range_memo="3")
+        self.assertFalse(batch.has_previous_batch)

=== modified file 'src/lazr/batchnavigator/z3batching/batch.py'
--- src/lazr/batchnavigator/z3batching/batch.py	2011-08-24 15:40:20 +0000
+++ src/lazr/batchnavigator/z3batching/batch.py	2011-09-07 09:29:23 +0000
@@ -109,6 +109,10 @@
         """See `IBatch`."""
         return len(self.sliced_list) + 1
 
+    # The values do not matter at all. Just make verifyObject() happy.
+    has_previous_batch = None
+    has_next_batch = None
+
 
 class _Batch(object):
     implements(IBatch)
@@ -287,17 +291,22 @@
     def __contains__(self, item):
         return item in self.__iter__()
 
-    def nextBatch(self):
+    @property
+    def has_next_batch(self):
+        """See `IBatch`."""
         # self.sliced_list tries to return one more object than the
         # batch size. If it returns the batch size, or fewer, then
         # this batch encompasses the end of the list, for forward
         # batching.
-        if self.range_forwards and len(self.sliced_list) <= self.size:
-            return None
-
         # In the case of backward batching, an empty memo value
         # indicates that this is the last batch.
-        if not self.range_forwards and self.range_memo == '':
+        if self.range_forwards:
+            return len(self.sliced_list) > self.size
+        else:
+            return self.range_memo != ''
+
+    def nextBatch(self):
+        if not self.has_next_batch:
             return None
 
         start = self.start + self.size
@@ -306,10 +315,26 @@
             range_memo=memos[1], range_forwards=True,
             _listlength=self._listlength)
 
+    @property
+    def has_previous_batch(self):
+        """See `IBatch`."""
+        if self.range_memo is None:
+            # If no range memo is specified, sliced_list() falls back
+            # to slicing by index. This happens for old URLs, for example.
+            return self.start > 0
+        if self.range_forwards:
+            # For forward batching, we are at the first batch if the memo
+            # value is empty.
+            return self.range_memo != ''
+        else:
+            # For backwards batching, we can rely on the flag set in
+            # sliced_list. sliced_list is a @Lazy property, so make
+            # sure that it has been evaluated.
+            self.sliced_list
+            return not self.is_first_batch
+
     def prevBatch(self):
-        if self.range_forwards and self.range_memo == '':
-            return None
-        if not self.range_forwards and self.is_first_batch:
+        if not self.has_previous_batch:
             return None
 
         # The only case in which we should /not/ offer a previous batch

=== modified file 'src/lazr/batchnavigator/z3batching/interfaces.py'
--- src/lazr/batchnavigator/z3batching/interfaces.py	2011-08-24 15:40:20 +0000
+++ src/lazr/batchnavigator/z3batching/interfaces.py	2011-09-07 09:29:23 +0000
@@ -69,3 +69,9 @@
         "A sliced list as returned by IRangeFactory.sliced_list.")
 
     trueSize = Attribute("The actual size of this batch.")
+
+    has_previous_batch = Attribute(
+        "True, if this batch has a predecessor, else False.")
+
+    has_next_batch = Attribute(
+        "True, if this batch has a successor, else False.")