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
O3-3067: (fix) Incorrect parameters passed to react hook when filtering queues by service uuid #1095
base: main
Are you sure you want to change the base?
Conversation
Size Change: -153 kB (-5%) ✅ Total Size: 3.24 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @donaldkibet. I find this good already. I have left just one comment to be addressed.
@@ -61,7 +61,7 @@ const AddProviderQueueRoom: React.FC<AddProviderQueueRoomProps> = ({ providerUui | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are passing the same parameter. Is there a possibility to filter by service as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
@@ -92,10 +92,10 @@ interface PaginationData { | |||
} | |||
|
|||
function ActiveVisitsTable() { | |||
const selectedQueueUuid = useSelectedServiceUuid(); | |||
const selectedServiceQueueUuid = useSelectedServiceUuid(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
171ffd2
to
55bd1be
Compare
@chibongho i have updated the function to pick correct parameters |
8373bff
to
076b084
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -26,6 +27,7 @@ function ClinicMetrics() { | |||
|
|||
const currentQueueLocation = useSelectedQueueLocationUuid(); | |||
const { queues } = useQueues(currentQueueLocation); | |||
const uniqueQueues = unionBy(queues ?? [], ({ service }) => service?.uuid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing we need this because there can be multiple queues for the same service?
@@ -2,7 +2,7 @@ import { openmrsFetch, restBaseUrl } from '@openmrs/esm-framework'; | |||
import useSWR from 'swr'; | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This technically works, but we should use null
to denote absence of a value instead of empty string.
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