Skip to content
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

Feature/895 pagination #358

Merged
merged 4 commits into from
Dec 5, 2022
Merged

Feature/895 pagination #358

merged 4 commits into from
Dec 5, 2022

Conversation

annashamray
Copy link
Contributor

@annashamray annashamray commented Dec 2, 2022

Since we have to filter cases on client side (separate open cases from closed) we can't use page query param to Zaken API for pagination and we have to implement it also on the client side.

There were 3 options considered to implement pagination:

  1. Each time request all cases, filter them and then paginate
    + We show client all the data
    - Performance-heavy, especially on staging environment, where we have thousands cases for 1 person
  2. Use ordering query param when requesting cases from Zaken API.
    + It would be an optimal solution: we could order cases on the end date and limit the request to only open ones or closed ones.
    - Too bas this query param doesn't work in eSuite
  3. Set the maximum amount of requests we do to cases resource in Zaken API
    + Improve performance
    - We don't show all cases
    - We limit requests, not the amount of cases, and then we separate open and closed ones, it means it is possible to show the user 3 closed cases and hide 30 older ones. But for regular user in production it is very unusual situation

@alextreme and I decided to stick to the 3rd option for now, because it won't affect users in production much, since it's rare for a user to have hundreds of cases, but it will improve our performance on staging env

  • only 100 cases can be retrieved and processed, e-Suite returns 20 cases per page, so no more than 5 requests to cases could be done
  • these 100 cases are then separated on the open and closed ones, each of them we limit to 50 cases. Not sure if this step is necessary, I'd like to hear your opinion Upd. removed thanks to @alextreme
  • these open or closed cases are paginated. We request related resources (statuses, documents. etc) only for the cases shown on the page

@annashamray annashamray marked this pull request as ready for review December 2, 2022 11:42
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2022

Codecov Report

Merging #358 (49ed2b0) into develop (243b4c7) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #358      +/-   ##
===========================================
- Coverage    96.42%   96.40%   -0.02%     
===========================================
  Files          442      442              
  Lines        13932    13971      +39     
===========================================
+ Hits         13434    13469      +35     
- Misses         498      502       +4     
Impacted Files Coverage Δ
src/open_inwoner/accounts/views/cases.py 97.26% <100.00%> (+0.06%) ⬆️
src/open_inwoner/openzaak/api_models.py 98.93% <100.00%> (+0.17%) ⬆️
src/open_inwoner/openzaak/cases.py 64.78% <100.00%> (-4.23%) ⬇️
...rc/open_inwoner/openzaak/tests/test_case_detail.py 100.00% <100.00%> (ø)
src/open_inwoner/openzaak/tests/test_cases.py 100.00% <100.00%> (ø)
...rc/open_inwoner/openzaak/tests/test_cases_cache.py 100.00% <100.00%> (ø)
src/open_inwoner/openzaak/utils.py 93.50% <0.00%> (+2.59%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

I agree that the best approach is the 3rd one. It looks nice (CI fails though) but I cannot test it completely in my local machine as I fetch only a few cases using the bsn we usually use. I assume we can see that in the right way when we deploy.

Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

My only observation is the paginator shows when there aren't any cases but that might be common for all list views.

@alextreme alextreme merged commit 6fde2bd into develop Dec 5, 2022
@alextreme alextreme deleted the feature/895-pagination branch December 5, 2022 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants