Pagination integration proof of concept

[rtibbles]

Summary

Description of the change(s) you made

After numerous issues arising because of pagination, this creates a prototype to ensure full integration of both the ValuesViewset and the frontend resource layer with both ordering and pagination.

This means that we can use standard Django Rest Framework ordering syntax for specifying the ordering of results from the API (https://www.django-rest-framework.org/api-guide/filtering/#orderingfilter) and this will be properly interpreted by both the backend and the IndexedDB where wrapper command.

In addition, this adds support in the backend for PageNumberPagination to be added to any ValuesViewset class when needed, and to leverage the efficiency gains that were added specifically to the AdminChannel and AdminUser viewsets.

In the frontend it adds support for PageNumberPagination and LimitOffsetPagination for IndexedDB queries through where.

Manual verification steps performed

Checked all places that ordering and pagination are currently used:

• Admin channel and user viewsets
• Content node search viewsets
• Catalog listing for a logged in user (changed to using where and verified that pagination worked as expected)

Screenshots (if applicable)

Does this introduce any tech-debt items?

---

Reviewer guidance

How can a reviewer test these changes?

Are there any risky areas that deserve extra testing?

References

Comments

---

Contributor's Checklist

PR process:

• If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
• If this includes an internal dependency change, a link to the diff is provided
• The docs label has been added if this introduces a change that needs to be updated in the user docs?
• If any Python requirements have changed, the updated requirements.txt files also included in this PR
• Opportunities for using Google Analytics here are noted
• Migrations are safe for a large db

Studio-specifc:

• All user-facing strings are translated properly
• The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
• All UI components are LTR and RTL compliant
• Views are organized into pages, components, and layouts directories as described in the docs
• Users' storage used is recalculated properly on any changes to main tree files
• If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

• Code is clean and well-commented
• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical user journeys are covered by Gherkin stories
• Any new interactions have been added to the QA Sheet
• Critical and brittle code paths are covered by unit tests

---

Reviewer's Checklist

This section is for reviewers to fill out.

• Automated test coverage is satisfactory
• PR is fully functional
• PR has been tested for accessibility regressions
• External dependency files were updated if necessary (yarn and pip)
• Documentation is updated
• Contributor is in AUTHORS.md

[codecov]

Codecov Report

Merging #3086 (7522854) into unstable (7558536) will decrease coverage by 0.29%.
The diff coverage is 60.95%.

❗ Current head 7522854 differs from pull request most recent head 9eee591. Consider uploading reports for the commit 9eee591 to get more accurate results

@@             Coverage Diff              @@
##           unstable    #3086      +/-   ##
============================================
- Coverage     86.13%   85.84%   -0.30%     
============================================
  Files           305      298       -7     
  Lines         16455    15855     -600     
============================================
- Hits          14173    13610     -563     
+ Misses         2282     2245      -37     

Impacted Files	Coverage Δ
...uration/contentcuration/viewsets/assessmentitem.py	100.00% <ø> (ø)
...entcuration/contentcuration/viewsets/channelset.py	95.89% <ø> (-0.11%)	⬇️
...tentcuration/contentcuration/viewsets/clipboard.py	92.85% <ø> (-0.20%)	⬇️
...ntcuration/contentcuration/viewsets/contentnode.py	85.95% <ø> (-0.25%)	⬇️
contentcuration/contentcuration/viewsets/file.py	71.42% <ø> (-3.58%)	⬇️
...entcuration/contentcuration/viewsets/invitation.py	97.14% <ø> (-0.08%)	⬇️
contentcuration/contentcuration/viewsets/task.py	73.91% <ø> (-21.01%)	⬇️
contentcuration/search/viewsets/savedsearch.py	77.77% <ø> (-1.54%)	⬇️
contentcuration/search/viewsets/contentnode.py	59.04% <33.33%> (+6.05%)	⬆️
contentcuration/contentcuration/viewsets/base.py	85.31% <40.54%> (-4.23%)	⬇️
... and 46 more

---

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 6fd9392...9eee591. Read the comment docs.

[bjester]

Does this cover sorting by annotated fields? I see pagination is now before annotation, but I don't see how sorting by an annotated field would get picked up. I noticed this use case in the AdminUser viewset where it annotates a field prior to pagination if the sorting requires that field. That appears to be removed here

[rtibbles]

Does this cover sorting by annotated fields?

To some extent, and it is now parallel to filtering in that regard. Any annotations that are required for filtering or sorting now need to be applied in the get_queryset method - this is best seen in the AdminChannelViewset where the annotation of the modified and primary token happen in get_queryset.

It's very possible that I missed something in my messing with the AdminUserViewset, can correct if some of the ordering is not working there now.

I definitely think some improved documentation of the values viewset life cycle is a necessary corollary to this.

[bjester]

I think it would be useful to have the paginator conditionally annotate the queryset if it detected sorting by an annotated field, to avoid unnecessary annotations which could slow down the un-LIMIT-ed query during the pagination calculation. That would maintain the behavior that I see removed in a few places here. The paginator should have access to the viewset so it may not be difficult. I do see that in the AdminUser viewset it is no longer annotating the view_count or edit_count fields prior to pagination, which are sortable fields.

[rtibbles]

I think it would be useful to have the paginator conditionally annotate the queryset if it detected sorting by an annotated field, to avoid unnecessary annotations which could slow down the un-LIMIT-ed query during the pagination calculation. That would maintain the behavior that I see removed in a few places here. The paginator should have access to the viewset so it may not be difficult. I do see that in the AdminUser viewset it is no longer annotating the view_count or edit_count fields prior to pagination, which are sortable fields.

I did check for that - the annotations only get put into the eventual query if they are part of the resulting values call. For the un-LIMIT-ed query, we do a values_list("pk") call, so the annotations are only used if they have been used for ordering or filtering prior to the pagination.

[bjester]

so the annotations are only used if they have been used for ordering or filtering prior to the pagination.

Ah yes, that's Django magic not really performing what your code looks like it's doing. Thanks!

[marcellamaki]

Manual QA looks good!

[bjester]

Nice! Mostly just comments with only one likely important change for LIMIT_OFFSET_KEYS