(refactor) Remove the dependency of using patientUuid from the URL
#2080
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -373 kB (-2.3%) Total Size: 15.9 MB
ℹ️ View Unchanged
|
Merged
4 tasks
vasharma05
force-pushed
the
refactor/remove-patientuuid-dependency
branch
2 times, most recently
from
4f8d631 to
40bc433
Compare
usamaidrsk
reviewed
Oct 31, 2024
There was a problem hiding this comment.
Thanks @vasharma05 , generally looks great.
I just have a few comments.
| export function getPatientUuidFromUrl(): string { | ||
| const match = /\/patient\/([a-zA-Z0-9\-]+)\/?/.exec(location.pathname); | ||
| return match && match[1]; | ||
| const patientUuidFromUrl = match && match[1]; | ||
| return patientUuidFromUrl || getPatientChartStore().getState().patientUuid; |
There was a problem hiding this comment.
The function name getPatientUuidFromUrl suggests it only retrieves the UUID from the URL, but it actually has a fallback behavior that gets the UUID from the store state. I suggest either:
- Rename the function to better reflect its complete behavior, e.g.,
getActivePatientUuidorresolvePatientUuid, since it resolves the patient UUID from either the URL or store - OR split this into two separate concerns if these use cases need to be handled differently
This would make the function's behavior more predictable from its name
There was a problem hiding this comment.
Yes @usamaidrsk!
I agree with you on this, and am seeking @denniskigen's advice too. But do note that this is an API in the Patient Common Lib, and changing the function will require updating all the places this function is being used, and hence will become a breaking change, since this will also affect other implementations to update their code when released.
One thing is, we can deprecate this function and in the message we can ask them to update the API in their respective code. This way, it'll not be a breaking change.
Thanks!
| const patientChartStoreName = 'patient-chart-global-store'; | ||
|
|
||
| const patientChartStore = createGlobalStore<PatientChartStore>(patientChartStoreName, { | ||
| patientUuid: '', |
There was a problem hiding this comment.
Shouldn't this be patientUuid: null
vasharma05
force-pushed
the
refactor/remove-patientuuid-dependency
branch
from
041ee42 to
8673aa8
Compare
usamaidrsk
approved these changes
Nov 1, 2024
There was a problem hiding this comment.
Thanks @vasharma05 LGTM.
Could you check why the E2E tests are failing
vasharma05
force-pushed
the
refactor/remove-patientuuid-dependency
branch
from
7071e79 to
d7eadaf
Compare
Merged
3 tasks
senthil-athiban
pushed a commit
to senthil-athiban/openmrs-esm-patient-chart
that referenced
this pull request
Nov 19, 2024
…penmrs#2080) * Introduce patient chart store for handling patient chart related state * Replace instances of getPatientUuidFromUrl * Update function name
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Summary
This PR aims to refactor the code locations that retrieve the patient UUID from the URL of the patient chart. The primary motivation for this refactor is that the extensions, widgets, and workspaces utilized in the patient chart are no longer guaranteed to be present in the patient chart. For instance, the Patient chart summary workspaces in the IPD. To reuse these widgets/workspaces in the IPD’s patient summary workspace and potentially in other future locations, I have implemented a global Patient Chart Store, which must be accessible by all components in the patient chart.
I also modified the
getPatientUuidFromUrlfunction to get the patient UUID from the URL, but if not present it will look into the store.I also removed the instances where
patientUuidwas being fetched from theusePatient, which should now be fetched from theusePatientChartStorehook.Screenshots
None
Related Issue
None
Other