(feat) O3-4084: Implement numeric validation for lab results entry form #2061
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
donaldkibet
requested review from
CynthiaKamau,
denniskigen,
pirupius and
ibacher
and removed request for
denniskigen
|
Size Change: -6.97 kB (-0.05%) Total Size: 14.8 MB ℹ️ View Unchanged
|
|
Thanks, @donaldkibet for working on this. I support the approach you have implemented and I propose that we also update the displayed result ranges accordingly. Similar to the implementation here, we probably should show the hiAbsolute and lowAbsolute only if they are defined. The current implementation, IIRC, gets the minimum of the defined LOW ranges and maximum of the defined HIGH ranges. The gap is mostly when not all the ranges are defined resulting in false HIGH or LOW ranges. Borrowing from the PR description, we should:
These are just my thoughts and we can benefit more from other people's perspectives. |
pirupius
approved these changes
Oct 15, 2024
There was a problem hiding this comment.
Thanks @donaldkibet for adding this much needed validation. I have no objections to this PR because typically the ranges cover all the valid entries and will avoid entry of wrong data.
The only concern would be if typically the specified ranges configured accommodate edge cases as you also mentioned in the PR description. Wouldn't there be scenarios where the the entered value would be critically high/low and prevent data entry due to failing validation?
But if there ranges already have that in account then it's all good. Maybe the people with more medical knowledge like @gracepotma can advise on this.
ibacher
requested changes
Oct 15, 2024
Comment on lines
105
to
107
| if (lowerLimit === null && upperLimit === null) { | ||
| return baseSchema; | ||
| } |
There was a problem hiding this comment.
Shouldn't the check here be for undefined rather than null? The typings imply that these variables can never be null.
There was a problem hiding this comment.
I'm under the impression that what we accept here should be driven by what the backend actually returns, i.e., if we're always setting these values to null (as in the tests), then the types here should be number | null and I guess the code works as-is. If the backend doesn't return values for those, then the types should be as-written, but the code updated. The safest option would be to just explicitly support number | null | undefined and then be sure to check for undefined.
packages/esm-patient-orders-app/src/lab-results/useLabResultsFormSchema.tsx
Outdated
Show resolved
Hide resolved
ibacher
approved these changes
Oct 15, 2024
denniskigen
approved these changes
Oct 15, 2024
packages/esm-patient-orders-app/src/lab-results/useLabResultsFormSchema.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/lab-results/lab-results.resource.ts
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/lab-results/lab-results-form.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/lab-results/lab-results-form.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/lab-results/lab-results-form.test.tsx
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/lab-results/lab-results-form.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/lab-results/lab-results-form.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/lab-results/lab-results-form.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/lab-results/lab-results-form.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/lab-results/lab-results-form.component.tsx
Outdated
Show resolved
Hide resolved
denniskigen
changed the title
|
Merging this in since failing e2e test failing isn't associated with this change. |
senthil-athiban
pushed a commit
to senthil-athiban/openmrs-esm-patient-chart
that referenced
this pull request
Nov 19, 2024
…rm (openmrs#2061) * (feat) O3-4084 : implement numeric validation for lab results entry form * code reviews changes * additional code review changes
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
See https://openmrs.atlassian.net/browse/O3-4084
I have implemented the validation logic as follows:
hiAbsoluteandlowAbsoluteas they represent the max and min thresholds supported by the backend.I have not used the critical high or low values for validation because there are cases where values might fall outside of their ranges. should we leverage these values for validation. cc @ibacher @makombe @ojwanganto @denniskigen @CynthiaKamau
Screenshots
Related Issue
Other