(fix) O3-3015: Fetch all the queue entries to support filters on the frontend #1124
Conversation
|
Size Change: -1.31 kB (-0.04%) Total Size: 3.52 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
I tested this locally and found a couple issues.
- The assumption that the chunkSize == 50 is incorrect for PIH distros, and it causes the number of fetched entries to be incorrect. (Some of them get double-fetched)
- The subsequent fetches to get the next chunk does not seem to get triggered immediately after the first fetch. I was able to trigger it by switching focus out of the page and back in. See video. (Note that the correctly patient count should be 603, as seen in the metrics card)
OpenMRS.10.-.-.Microsoft.Edge.2024-05-07.16-09-23.mp4
| if (value != null) { | ||
| searchParam.append(key, value?.toString()); | ||
| const [totalCount, setTotalCount] = useState<number>(); | ||
| const chunkSize = 50; |
There was a problem hiding this comment.
I'm not sure if we can assume the chunk size to be 50 for everyone. I believe PIH's distro configured the chunk size to be 500. Maybe we can determine the chunk size after the first fetch?
There was a problem hiding this comment.
I wasn't aware of this scenario, thank you @chibongho for pointing this out!
| if (data?.[0]?.data?.totalCount && data[0].data.totalCount !== totalCount) { | ||
| setTotalCount(data[0].data.totalCount); | ||
| } | ||
| }, [data?.[0], totalCount, setTotalCount]); |
There was a problem hiding this comment.
Is there a reason to put the setter function in the dependency array? I've seen this pattern elsewhere in this code base, but I don't understand why.
There was a problem hiding this comment.
Both the value and the setter are put in the dependency array, AFAIK
There was a problem hiding this comment.
@chibongho It's mostly about consistency. We've had issues with deps that should've be included being omitted, leading to inconsistent behavior, so its easiest across a code-base developed by a large number of contributors with varying degrees of familiarity with React, etc., just to include everything in the deps array.
Technically, setters from useState() don't need to be in the dependency array because part of the contract of useState() is that the setter remain consistent across renders. However, that's not necessarily the case for functions returned from other hooks
| @@ -29,9 +30,8 @@ const ClearQueueEntries: React.FC<ClearQueueEntriesProps> = ({ queueEntries }) = | |||
|
|
|||
| return ( | |||
| <Button | |||
| size="sm" | |||
| size={isDesktop(layout) ? 'sm' : 'lg'} | |||
|
In this PR, the It would be nice for use cases like this to have a usePaginate hook where the totalCount and the first batch of results are loaded on first fetch, and it delays fetching subsequent entries until the user paginates to the corresponding page in the DataTable. Thoughts? @ibacher @denniskigen @mseaton |
|
I agree. That sounds like a useful utility to have. That said it wouldn't fix this part: "the filters only work on those top 50 results". |
Yes, this is exactly what we want, but if you see, the main action of the table is to
Since this is not yet supported by the REST endpoint, we have to fetch all the entries and then handle the sorting and searching the whole results. |
|
Hi @chibongho ! |
|
I will say that making the queue table sortable, in my view, is a mistake. Queues are supposed to be implicitly ordered. I'd rather we applied filtering to allow the users to zoom in on particular statuses, etc. |
@vasharma05 just DM'd you the server + credentials
We have no sorting in the queue table (and plan to keep it that way). Vineet's point stands regarding the search filtering though. |
|
Hi @chibongho! |
Requirements
Summary
Currently, the queue entries only fetch the top 50 results and the filters only work on those top 50 results. Now, we will be fetching all the queue entries using
useSWRInfiniteto fetch all the results and display them accordingly.This PR also includes some UI improvements on the Table Toolbar.
Screenshots
Before
After
Related Issue
https://issues.openmrs.org/browse/O3-3015
Other