Skip to content
This repository has been archived by the owner on Mar 21, 2019. It is now read-only.

api: add optional resource count to list endpoints #358

Closed
wants to merge 5 commits into from

Conversation

rjw57
Copy link
Member

@rjw57 rjw57 commented Oct 12, 2018

As sketched in #350, we need an ability to get a count of resources matching a search. This PR extends the resource list endpoints in the following ways:

  1. They now take an "include_count" parameter in the query. If "true", the response will include a total count of resource which match the other query options.
  2. They new take a "page_size" parameter which allows the page size to be customised. If the caller only wants the count of the number of resources (which they will for the search page design in Search results page #350) the page size can just be set to "1".

The page size cannot yet be limited to "0". Looking around the internets, this appears to be intentional; if the page size is zero the "next" links returned by the CursorPagination paginator become an infinite loop. This may or may not be a problem for us but, in any case, it is easy to add support for a zero page size later on if needs be.

@rjw57 rjw57 requested a review from a team October 12, 2018 14:02
@rjw57 rjw57 self-assigned this Oct 12, 2018
rjw57 added 3 commits October 12, 2018 15:35
Add a custom DRF paginator which is based on CursorPaginator byt can
optionally add a count of the number of results. This is disabled by
default because doing a full count can be inefficient and isn't always
needed.

Add appropriate magic to make sure that the Swagger API documentation
reflects the new change.
Make use of the new pagination class allowing all resource list views to
optionally include a total resource count.
Allow the page size of list responses to be specified. This is to allow
the case where one doesn't really care about the resources but one
wants the count. In this case, one can just set page_size to 1.

DRF diasallows setting page_size to 0.
@rjw57 rjw57 force-pushed the add-count-to-list-results branch from 3f31cbc to 8cbda3a Compare October 12, 2018 14:35
@codecov
Copy link

codecov bot commented Oct 15, 2018

Codecov Report

Merging #358 into master will decrease coverage by 0.38%.
The diff coverage is 75.55%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #358      +/-   ##
=========================================
- Coverage   91.28%   90.9%   -0.39%     
=========================================
  Files          43      44       +1     
  Lines        1882    1924      +42     
=========================================
+ Hits         1718    1749      +31     
- Misses        164     175      +11
Impacted Files Coverage Δ
api/views.py 94.73% <100%> (+0.09%) ⬆️
api/pagination.py 71.05% <71.05%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fce9f8f...e7db1f2. Read the comment docs.

@abrahammartin
Copy link
Member

Copy link
Member

@abrahammartin abrahammartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't work

msb
msb previously approved these changes Oct 15, 2018
Add tests which exercise include_count when the user is logged in.
@rjw57
Copy link
Member Author

rjw57 commented Oct 15, 2018

@abrahammartin I think this is related to a Django bug. I've added a (failing) test case which triggers the bug.

@rjw57
Copy link
Member Author

rjw57 commented Oct 15, 2018

I've moved the page size functionality from this PR to #373.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants