O3-3067: (fix) Incorrect parameters passed to react hook when filtering queues by service uuid #1095
Conversation
|
Size Change: -153 kB (-5%) ✅ Total Size: 3.24 MB
ℹ️ View Unchanged
|
ojwanganto
left a comment
ojwanganto
left a comment
There was a problem hiding this comment.
Thanks @donaldkibet. I find this good already. I have left just one comment to be addressed.
| const { mutate } = useProvidersQueueRoom(providerUuid); | ||
| const { queues } = useQueues(currentLocationUuid); | ||
| const { rooms } = useQueueRooms(currentLocationUuid, currentServiceUuid); | ||
| const { rooms } = useQueueRooms(currentLocationUuid, currentLocationUuid); |
There was a problem hiding this comment.
we are passing the same parameter. Is there a possibility to filter by service as well?
|
|
chibongho
left a comment
chibongho
left a comment
There was a problem hiding this comment.
We have had a bad naming for updateSelectedServiceUuid and updateSelectedServiceName to be set to queueUuid / queueName instead of serviceUuid / serviceName, This PR addresses that, which is great.
However, if we go with this, we must update all places that call updateSelectedServiceUuid to take in the serviceUuid instead of queueUuid. There are places missed in this PR. Also, we should change updateSelectedServiceName to also take in the service name instead of queue name.
It might be less dangerous to just rename useSelectedServiceUuid / updateSelectedServiceName to updateSelectedQueueUuid / updateSelectedQueueName. If we want to be selecting on a particular queue, I think selecting by queueUuid is preferable to selecting by serviceUuid (even if there is a one-to-one relationship between queue and service). Then fix the queue-entry-metrics call to use the right params.
|
|
||
| function ActiveVisitsTable() { | ||
| const selectedQueueUuid = useSelectedServiceUuid(); | ||
| const selectedServiceQueueUuid = useSelectedServiceUuid(); |
There was a problem hiding this comment.
Let's name this variable selectedServiceUuid, not selectedServiceQueueUuid
| const visitQueueEntries = queueEntries | ||
| .map((entry) => mapVisitQueueEntryProperties(entry, visitQueueNumberAttributeUuid)) | ||
| .filter((entry) => (currentServiceUuid ? entry.queue.service.uuid === currentServiceUuid : true)); | ||
|
|
There was a problem hiding this comment.
I wouldn't think this filter is needed at all. We should just be able to use queueEntries as-is, since it is already filtering by both the selected service and location. So let's get rid of this visitQueueEntries and just use the queueEntries returned from the hook.
…ng queues by service uuid
donaldkibet
force-pushed
the
fix/queueFiltering
branch
from
171ffd2 to
55bd1be
Compare
@chibongho i have updated the function to pick correct parameters |
donaldkibet
force-pushed
the
fix/queueFiltering
branch
2 times, most recently
from
8373bff to
076b084
Compare
chibongho
left a comment
chibongho
left a comment
There was a problem hiding this comment.
There are still places using the updateSelectedServiceUuid() function that needs updating, namely in add-provider-queue-room.component.tsx and default-queue-table.component.tsx
| const visitQueueEntries = queueEntries.map((entry) => | ||
| mapVisitQueueEntryProperties(entry, visitQueueNumberAttributeUuid), | ||
| ); | ||
| const queues = uniqBy(queueEntries?.flatMap((entry) => entry.queue) ?? [], 'uuid'); |
There was a problem hiding this comment.
I think this will make the dropdown list display only queues that have at least one queue entry. Are we sure we want that? The order of those queues can be unpredictable based on the order of the returned queue entries.
|
|
||
| const currentQueueLocation = useSelectedQueueLocationUuid(); | ||
| const { queues } = useQueues(currentQueueLocation); | ||
| const uniqueQueues = unionBy(queues ?? [], ({ service }) => service?.uuid); |
There was a problem hiding this comment.
I'm guessing we need this because there can be multiple queues for the same service?
| import { type ProvidersQueueRoom, type QueueRoom } from '../types'; | ||
|
|
||
| export function useQueueRooms(location: string, queueUuid: string) { | ||
| export function useQueueRooms(location: string, queueUuid: string = '') { |
There was a problem hiding this comment.
This technically works, but we should use null to denote absence of a value instead of empty string.
|
Closing as stale. |
Requirements
Summary
PR description here https://openmrs.atlassian.net/browse/O3-3067
Screenshots
Kapture.2024-04-11.at.20.34.01.mp4
Related Issue
Other