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

PRN prescriptions table set upto latest administration #6345

Closed
wants to merge 3 commits into from

Conversation

print-Sathvik
Copy link
Contributor

@print-Sathvik print-Sathvik commented Sep 25, 2023

WHAT

🤖 Generated by Copilot at 07a6035

This pull request improves the functionality and appearance of the PrescriptionAdministrationsTable component by using the latest administration dates and avoiding data conflicts. It also modifies the listAdministrations action in src/Redux/actions.tsx to support multiple keys for concurrent requests.

Proposed Changes

  • Fixes Bug: Medicine Administrations table starts with an additional hour offset to the future #6323 and one more issue described in the last point
  • The PRN prescription table shows time slots up to the latest administration of all medicines
  • The first administration was not visible when user has to click on the right arrow of the table. To reproduce /medicines by clicking here. Then go to the right most part of the PRN prescriptions table by clicking on the right arrow button. You can see that earliest time starts at 9:00 AM of 23/09 but I have administered a medicine at 8:51 AM of 23/09. So I fixed that and you can see 8-9 slot also for 23/09

@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers

HOW

🤖 Generated by Copilot at 07a6035

  • Add a state variable latestAdministration to store the latest administration date of all the prescriptions in the table (link)
  • Fetch the administrations for each prescription and update the state with the latest administration date (link, link)
  • Use the latestAdministration as a fallback value for the end date of the time bounds for the table (link)
  • Pass a unique key to the listAdministrations action creator to avoid conflicts with other requests (link, link, link, link)
  • Subtract one hour from the start date of the time bounds to ensure the first slot of the table is not empty (link)

@print-Sathvik print-Sathvik requested a review from a team September 25, 2023 17:48
@print-Sathvik print-Sathvik requested a review from a team as a code owner September 25, 2023 17:48
@vercel
Copy link

vercel bot commented Sep 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
care-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2023 1:11pm

@netlify
Copy link

netlify bot commented Sep 25, 2023

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit 48bd060
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/651c12ce831e550008db34f0
😎 Deploy Preview https://deploy-preview-6345--care-egov-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

  1. You need not compute the latest administration date manually by fetching the administrations. The prescription record itself has last_administered_on field that will give u the latest date without fetching administrations.

  2. We are now querying administrations list api two times now for each prescription.

N+1 queries are bad to begin with, we now have 2N+1 queries :) which is even more bad 😅

@rithviknishad
Copy link
Member

You need not create additional state either to store last administered. The problem is within how the slots are generated by the useRangePagination hook alone.

@print-Sathvik
Copy link
Contributor Author

print-Sathvik commented Sep 26, 2023

The prescription record itself has last_administered_on field that will give u the latest date without fetching administrations.

The last_administered_on is storing the time at which the administration was made by the user instead of storing the latest administered time given by the user. If a medicine is administered right now(current time: 8:50 AM) to a previous time, let's say to 4:30 AM then 5-8 slots will also be displayed even though latest administration was at. 4:30 AM

@print-Sathvik
Copy link
Contributor Author

You need not create additional state either to store last administered. The problem is within how the slots are generated by the useRangePagination hook alone.

So the issue is to remove only 1 additional column that is ahead of the last_administered_on time.

At 8:50 AM if I administer a medicine and set the administration time as 4:30 AM then currently we can see columns upto 9-10 slot as latest_administered_on is 8:50 AM.
I changed this to show only upto slot 4-5
But what I need to do is to display slots upto 8-9 only right?

@rithviknishad
Copy link
Member

Understood, if that's the case then isn't it a backend issue since the backend is giving the wrong value for last_administered_on? We are returning created_date instead of administered_date in backend.

image

@print-Sathvik
Copy link
Contributor Author

I will remove that N additional queries part now and also the additional state as we can use the last_administered_on due to above fix on backend

@print-Sathvik
Copy link
Contributor Author

At 8:50 AM if I administer a medicine and set the administration time as 4:30 AM then currently we can see columns upto 9-10 slot as latest_administered_on is 8:50 AM.

@rithviknishad I fixed it as you suggested by modifying in useRangePagination hook but "last modified" part on the top of the table is shown as 4:30 AM as per the above example. But it should be shown as 8:50AM. There is another attribute modified_date in the response of the endpoint /api/v1/consultation/{consultation_external_id}/prescriptions/ which I think should actually contain the last time the prescription is updated which is 8:50 AM as per above example but the modified_date contains the same value as created_date. I think this should also be fixed on the backend only.

Previously before the above fix on backend we used last_administered_on for this part.

@rithviknishad
Copy link
Member

That can be a separate issue

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM

@nihal467
Copy link
Member

nihal467 commented Oct 3, 2023

LGTM

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Oct 3, 2023
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

👋 Hi, @print-Sathvik,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@print-Sathvik
Copy link
Contributor Author

I am closing this PR as the issue is fixed by this commit: b7fc53c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cypress passed merge conflict pull requests with merge conflict tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Medicine Administrations table starts with an additional hour offset to the future
4 participants