Roundup Tracker - Issues

Issue 2550678

classification
Allow pagesize=-1 which returns all results
Type: behavior Severity: normal
Components: Web interface Versions: 1.4
process
Status: closed accepted
:
: : ThomasAH, ber, jerrykan, satchit
Priority: normal : patch

Created on 2010-10-29 05:44 by jerrykan, last changed 2011-09-03 20:13 by ber.

Files
File name Uploaded Description Edit Remove
pagesize.patch jerrykan, 2010-10-29 05:44
pagesize_v2.patch jerrykan, 2010-11-01 01:46
pagesize_v3.patch jerrykan, 2011-07-28 01:38
pagesize_v4.patch jerrykan, 2011-09-02 05:03
Messages
msg4184 Author: [hidden] (jerrykan) Date: 2010-10-29 05:44
Allow specifying a pagesize=-1 which will allow 'request/batch' to
return all results, instead of picking a "random high number" that might
achieve the same thing (unreliably).
msg4185 Author: [hidden] (ber) Date: 2010-10-29 09:29
I like the idea.
(Because I do not remember another method of doing so.)
msg4187 Author: [hidden] (ThomasAH) Date: 2010-10-30 13:40
I propose to use pagesize=0, because it is easier to type  (and maybe
even more intuitive, but I only asked my own intuition) and leaves -n
for future expansions, e.g. -20 for pagesize 20 and reverse order of
pages or whatever.
msg4190 Author: [hidden] (jerrykan) Date: 2010-11-01 01:46
I was having a bit of a think about this on the weekend and came the the
conclusion that pagesize=0 would also probably be a better fit.

I have attached a second page that reflects this change.


I have also made a couple of changes to the Batch class. 'self.size' was
being set to the 'pagesize' specified and not being used again anywhere.
'self._size' was being set to the actual size being calculated, which
meant that the previous and next links where using a hard-coded
'pagesize=##' in the url (when the actual size may have changed in the
mean time).

I have removed 'self._size', and set 'self.size' to be assigned the
'pagesize' value. The means the the correct 'pagesize=##' will be set
(even if it is 0). One issue might be if someone is using 'batch/size'
in their template for some reason. Maybe they should use 'batch/length'
instead.
msg4349 Author: [hidden] (satchit) Date: 2011-07-18 23:23
John,


Functionally, the patch works as expected. But I've the following
question and comment:

In your most recent update ( msg4190 ) to this issue, you said:

"'self._size' was being set to the actual size being calculated, which
meant that the previous and next links where using a hard-coded
'pagesize=##' in the url (when the actual size may have changed in the
mean time)."

When you say mean time, do you mean after a search query has been saved?
Could you elaborate on that point (perhaps with a use-case for such a
scenario?)

Also, there is three places in Batch.py where "_size" is referred to
(lines: 21, 22, 25). For completeness these references should be with
"size" as well. Can you upload a new patch with these changes?
msg4350 Author: [hidden] (satchit) Date: 2011-07-18 23:26
Noticed the word "replaced" was missing in my last comment. The last
paragraph in msg4349 should read:

Also, there are three places in Batch.py where "_size" is still referred
to (lines: 21, 22, 25). For completeness these references should be
replaced with "size" as well. Can you upload a new patch with these changes?
msg4355 Author: [hidden] (jerrykan) Date: 2011-07-26 01:36
It was a while ago since I looked a this, so my memory is a bit hazy...
I believe what I mean is something like the following scenario:

* assume there are 10 issues in the tracker
* I am looking at a page that contains next/previous links containing
'pagesize=10'
* While I am looking at this page, someone else creates a new issue
(meaning 11 issues in total)
* I then click on a next/previous link, the 'pagesize=10' value is
propogated to the following page that loads and any next/previous links
loaded on the 'following' page still contain 'pagesize=10' instead of
'pagesize=11' (which is the new total).

I just realised that for most people this might be confusing as there
probably won't be any next/previous links (because it is showing all
possible issues already). I have set up some custom views/reports where
the next/previous links actually mean next/previous month (not pagination).

The example of a saved query (like you mentioned) would  indeed seem to
be another instance where the inability to distinguish between 'all
issues' and '# issues' would be an issue.

I still may not be explaining myself clearly, so feel free to query me
further.


Also, thanks for picking up on the errant '_size' references. I'll sort
another patch to fix these.
msg4357 Author: [hidden] (jerrykan) Date: 2011-07-28 01:38
3rd attempt at the patch (which fixes the errant _size references) is
attached
msg4363 Author: [hidden] (ber) Date: 2011-08-10 15:24
Thanks John for the new patch and the comments.
Satchit: Did John address all the issue you had?
Again, thanks for both of your feedback!
msg4369 Author: [hidden] (satchit) Date: 2011-08-11 13:21
Bernhard: I had a brief look at John's most recent patch and response, soon after he posted it. But I couldn't look at it in detail because of some work here. I'll look at it and respond shortly.
msg4370 Author: [hidden] (ber) Date: 2011-08-11 13:28
Am Donnerstag, 11. August 2011 15:21:50 schrieb satchit:
> I'll look at it and respond shortly.

Cool! I'm looking forward to it.
msg4372 Author: [hidden] (satchit) Date: 2011-08-11 22:38
The pagesize=0 component of the patch works as expected.

Regarding the replacement of the "_size" with "size", I couldn't find a way to reproduce different behaviors before and after applying the path. That said, I'll note that:

- I didn't notice any breakage because of this change and all tests pass.

- But I couldn't find a way to test the effect of this specific component in the patch.

John: 

Since there aren't any unit-tests for this code, can you provide a series of steps that I can follow in the demo instance to reproduce an issue that is fixed by the "_size" component of your patch?

If the above is not easy, then I would suggest the following: 

If the "_size" to "size" change is not needed for the "pagesize=0" component to work, break these into two separate patches, so that at least the "pagesize=0" component, which is a very useful feature, can be merged into the trunk.
msg4402 Author: [hidden] (jerrykan) Date: 2011-09-02 05:03
Sorry this has taken so long to get back to, it has been a busy few
weeks at work.

satchit: thanks for the suggestion about breaking the patch up into two
parts, it made me realise I was probably tackling the two issues in the
reverse order (and not necessarily coming up with the best solution).

I created (and attached) a newer and much simpler patch (v4).

I have also create a separate issue for the @pagesize propagation stuff
(issue2550723) which should make that issue a bit easier to follow.
msg4403 Author: [hidden] (ber) Date: 2011-09-02 07:16
On Friday 02 September 2011 07:03:59 John Kristensen wrote:
> I created (and attached) a newer and much simpler patch (v4).
>
> I have also create a separate issue for the @pagesize propagation stuff
> (issue2550723) which should make that issue a bit easier to follow.

John, thanks for following up on this!
msg4404 Author: [hidden] (satchit) Date: 2011-09-02 13:51
On Sep 2, 2011, at 3:16 AM, Bernhard Reiter wrote:

> 
> Bernhard Reiter <bernhard@intevation.de> added the comment:
> 
> On Friday 02 September 2011 07:03:59 John Kristensen wrote:
>> I created (and attached) a newer and much simpler patch (v4).
>> 
>> I have also create a separate issue for the @pagesize propagation stuff
>> (issue2550723) which should make that issue a bit easier to follow.
> 
> John, thanks for following up on this!

John: Thanks for the new patch.

Bernhard: I've reviewed the latest patch and it's good to go. 

Satchit
msg4405 Author: [hidden] (ber) Date: 2011-09-02 14:29
Thanks Satchit!
msg4406 Author: [hidden] (ber) Date: 2011-09-03 20:13
Commited with rev4645 .
Thanks again!
History
Date User Action Args
2011-09-03 20:13:45bersetstatus: new -> closed
priority: normal
resolution: accepted
messages: + msg4406
2011-09-02 14:29:46bersetmessages: + msg4405
2011-09-02 13:51:38satchitsetmessages: + msg4404
2011-09-02 07:16:55bersetmessages: + msg4403
2011-09-02 05:03:59jerrykansetfiles: + pagesize_v4.patch
messages: + msg4402
2011-08-11 22:38:48satchitsetmessages: + msg4372
2011-08-11 13:28:58bersetmessages: + msg4370
2011-08-11 13:21:50satchitsetmessages: + msg4369
2011-08-10 15:24:46bersetmessages: + msg4363
2011-07-28 01:38:29jerrykansetfiles: + pagesize_v3.patch
messages: + msg4357
2011-07-26 01:36:34jerrykansetmessages: + msg4355
2011-07-18 23:26:17satchitsetmessages: + msg4350
2011-07-18 23:23:04satchitsetnosy: + satchit
messages: + msg4349
2010-11-01 01:46:59jerrykansetfiles: + pagesize_v2.patch
messages: + msg4190
2010-10-30 13:40:36ThomasAHsetnosy: + ThomasAH
messages: + msg4187
2010-10-29 09:29:33bersetnosy: + ber
messages: + msg4185
2010-10-29 05:44:57jerrykancreate