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

Add pagination to surveys lists #12

Merged
merged 6 commits into from
Dec 20, 2023
Merged

Conversation

davidbgomes
Copy link
Contributor

📌 References

📝 Implementation

  • Added pagination to all surveys lists

📹 Screenshots/Screen capture

image

🔥 Notes to the tester

Copy link
Contributor

@9sneha-n 9sneha-n left a comment

Choose a reason for hiding this comment

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

@davidbgomes. We need changes in the PR to reflect the following :

  1. There is a filter logic applied on survey list based on the type of survey. The page details do not refelect this and show incorrect count
Screenshot 2023-12-14 at 9 45 12 AM 2. Looks like the page details like count is not refreshed in country list, it shows the count of PPS Survey list Screenshot 2023-12-14 at 9 43 55 AM
  1. The PPS Survey list has 2 filter by options, when the filter is applied only the first page is filtered. I do not think this is acceptable behaviour. All the pages need to be filtered.

I would apply the paging logic only to lists that would actually be large in number, which I am guessing is only patient list @MiquelAdell Please provide your inputs here. And for the lists that have paging enabled, we will provide only search functionality, where we will go directly to the backend, so we wont be effected by the pagination.

@@ -41,7 +47,10 @@ export class GetAllSurveysUseCase {
.compact()
.value();

return Future.success(filteredSurveys);
return Future.success({
Copy link
Contributor

Choose a reason for hiding this comment

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

@davidbgomes see the code above, the survey list is getting filtered. The page details need to reflect this filtering.

@davidbgomes
Copy link
Contributor Author

Thanks Sneha! Oh I didn't account for the filtering outside the API. But in this case we can't use the API's pagination altogether, it will always be wrong on those cases. I'd need to redo and get all data at once to do our own pagination with the cached data, which performace-wise makes the pagination useless. Even if it's only for the Patients List the problem is there as well. What do you think @9sneha-n @MiquelAdell ?

@9sneha-n
Copy link
Contributor

Thanks Sneha! Oh I didn't account for the filtering outside the API. But in this case we can't use the API's pagination altogether, it will always be wrong on those cases. I'd need to redo and get all data at once to do our own pagination with the cached data, which performace-wise makes the pagination useless. Even if it's only for the Patients List the problem is there as well. What do you think @9sneha-n @MiquelAdell ?

@davidbgomes We can solve this issue in 3 ways

  1. Save UI page numbers/count and server page numbers/count separately and update the UI page numbers/count only based on filter.
  2. Do not have page numbers in UI, have infinite scrolling. Fetch , a page from server, when the user scrolls down, show a loader and fetch the next page.
  3. Do not have pagination at UI at all. Just fetch the surveys in a paginated manner from server - which would cause performance issue but ensure we never crash.
    I would strongly recommend the second option, i think it would be the quickest and most performant solution. @davidbgomes @MiquelAdell Do you see any concerns with option 2?

@davidbgomes
Copy link
Contributor Author

Thanks Sneha! Oh I didn't account for the filtering outside the API. But in this case we can't use the API's pagination altogether, it will always be wrong on those cases. I'd need to redo and get all data at once to do our own pagination with the cached data, which performace-wise makes the pagination useless. Even if it's only for the Patients List the problem is there as well. What do you think @9sneha-n @MiquelAdell ?

@davidbgomes We can solve this issue in 3 ways

  1. Save UI page numbers/count and server page numbers/count separately and update the UI page numbers/count only based on filter.
  2. Do not have page numbers in UI, have infinite scrolling. Fetch , a page from server, when the user scrolls down, show a loader and fetch the next page.
  3. Do not have pagination at UI at all. Just fetch the surveys in a paginated manner from server - which would cause performance issue but ensure we never crash.
    I would strongly recommend the second option, i think it would be the quickest and most performant solution. @davidbgomes @MiquelAdell Do you see any concerns with option 2?

@9sneha-n the problem is that you could be fetching a page from the API that would then return empty or with fewer results because in-between we would filter them. In my opinion the infinite scroll would add complexity to it, but the base problem would remain. Option 1 would be too complex to handle two different pagers I think. I don't think I understood the option 3, but I think we could do a in-between solution, the other one you said in the meeting, where we fetch all and do our pagination. In this case where the API call isn't the final result is what is quickest to do and most reliable I think. Let me know what you think

* development:
  feat(SurveryListFilters): change "none" option for "all", as the result of no filtering is all data
  feat: add loader for delete functionality
  feat(PPSProgramsHelper): add delete option for all surveys
  feat: add correct type for refreshSurveys
  feat(ActionOutcome): add new interface and reuse it in several hooks
  feat(SurveryFormD2Repository): set eventId type as Id
  chore(PPSProgramsHelper): update copy accidentally reverted by merge
  feat: make the list refresh and show snackbar depending on status
  feat: add delete functionality

# Conflicts:
#	src/webapp/components/survey-list/SurveyList.tsx
#	src/webapp/components/survey-list/SurveyListTable.tsx
#	src/webapp/hooks/useSurveys.ts
@MiquelAdell MiquelAdell merged commit 9d25631 into development Dec 20, 2023
1 check passed
@MiquelAdell MiquelAdell deleted the feature/list-pagination branch December 20, 2023 08:07
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.

3 participants