New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the user experience of pagination. #5

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@puck
Copy link

puck commented Jun 29, 2018

Collection results which require pagination now include "pages",
"next_page" and "prev_page". The later two only if there are next
or previous pages. For example:

https://10.0.19.168/REST/2.0/queues/all?per_page=5\&page=3

{
"count" : 5,
"pages" : 8,
"page" : 3,
"per_page" : 5,
"prev_page" : "https://10.0.19.168/REST/2.0/queues/all?per_page=5&page=2",
"next_page" : "https://10.0.19.168/REST/2.0/queues/all?per_page=5&page=4",
"total" : 37,
"items" : [ ... ]
}

Andrew Ruthven added some commits Jun 29, 2018

Andrew Ruthven
Improve the user experience of pagination.
Collection results which require pagination now include "pages",
"next_page" and "prev_page". The later two only if there are next
or previous pages. For example:

https://10.0.19.168/REST/2.0/queues/all?per_page=5\&page=3

{
   "count" : 5,
   "pages" : 8,
   "page" : 3,
   "per_page" : 5,
   "prev_page" : "https://10.0.19.168/REST/2.0/queues/all?per_page=5&page=2",
   "next_page" : "https://10.0.19.168/REST/2.0/queues/all?per_page=5&page=4",
   "total" : 37,
   "items" : [ ... ]
}
$self->collection->RowsPerPage($per_page);

my $max_page = ceil($self->collection->CountAll / $self->collection->RowsPerPage);

This comment has been minimized.

@gibus

gibus Oct 10, 2018

Note that at this point, any Limit passed in the request's JSON payload hasn't be applied yet. CountAll() will return the number of total (unlimited) records. So max_page is only an approximation here. The side effect is that CountAll() cache this (unlimited) value and next call to CountAll after the limit as been applied (in serialize) will still return the same (unlimited value). I'm not sure it's really useful to force the page passed in parameter not being greater than the max_page.

@@ -59,13 +65,37 @@ sub serialize {
# TODO: Allow selection of desired fields
push @results, expand_uid( $item->UID );
}
return {

my %results = (

This comment has been minimized.

@gibus

gibus Oct 10, 2018

Since CountAll() has already been called in setup_paging before the collection has been limited, to avoid getting an outdated cache value a recount has to be forced with: $collection->{count_all} = 0; or maybe avoid calling CountAll in setup_paging ?

@gibus

This comment has been minimized.

Copy link

gibus commented Oct 10, 2018

Hello @puck! Thanks for this pull request, I hope it will me merged soon…

Nevertheless I've stumbled on a bug caused by DBIx::SearchBuilder::CountAll() being called twice: once for computing $max_page in RT::Extension::REST2::Ressource::Collection::setup_paging(), before applying any limit passed in the request's JSON payload; and twice for computing total attribute in RT::Extension::REST2::Ressource::Collection::serialize(), after the limit has been applied.

I've added some comment in the code of your pull request to fix this: either by forcing a recount before calling DBIx::SearchBuilder::CountAll() when the collection has been limited; or by simply dropping $max_page computation and the idea to force the page to the maximum (which is not accurate anyway since it is computed before applying any limit).

Here's a test that can be added at the end of t/pagination.t:

# Test with limit
{
    my $alphabis = RT::Test->load_or_create_queue( Name => 'Alphabis' );
    my $res = $mech->post_json("$rest_base_path/queues/all?per_page=1",
        [{field => 'Name', operator => 'LIKE', value => 'Alp'}],
        'Authorization' => $auth,
    );
    is($res->code, 200);

    my $content = $mech->json_response;
    is($content->{count}, 1);
    is($content->{page}, 1);
    is($content->{pages}, 2);
    is($content->{per_page}, 1);
    is($content->{total}, 2);
    is($content->{prev_page}, undef);
    like($content->{next_page}, qr{/queues/all\?per_page=1&page=2$});
    is(scalar @{$content->{items}}, 1);
}

without the fix $collection->{count_all} = 0; the test fails for the number of pages and total:


#   Failed test at t/pagination.t line 161.
#          got: '4'
#     expected: '2'

#   Failed test at t/pagination.t line 163.
#          got: '4'
#     expected: '2'
# Some tests failed or we bailed out, tmp directory '/srv/rt/rt-extension-rest2/t/tmp/pagination.t-bUBpl6A6' is not cleaned
# Looks like you failed 2 tests of 111.
t/pagination.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/111 subtests 

Test Summary Report
-------------------
t/pagination.t (Wstat: 512 Tests: 111 Failed: 2)
  Failed tests:  105, 107
  Non-zero exit status: 2
Files=1, Tests=111, 13 wallclock secs ( 0.02 usr  0.01 sys +  5.09 cusr  0.17 csys =  5.29 CPU)
Result: FAIL
Failed 1/1 test programs. 2/111 subtests failed.

gibus and others added some commits Oct 15, 2018

Andrew Ruthven
Don't limit the page param by a guess of the number of pages.
Because we haven't applied the limits to the query yet, the call
to CountAll returns the total number of results. Therefore, using
that to limit the number of available of pages is useless.
@puck

This comment has been minimized.

Copy link
Author

puck commented Feb 2, 2019

Hey @gibus , thanks for that, given that CountAll will just result in a guesstimate, I've removed the flawed attempt to limit the number of pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment