-
Notifications
You must be signed in to change notification settings - Fork 99
(feat) O3-4171 : Validate concept answers and show error for non-existing answers #445
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
Open
Bharath-K-Shetty
wants to merge
31
commits into
openmrs:main
Choose a base branch
from
Bharath-K-Shetty:fix/O3-4171
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+64
−3
Open
Changes from all commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
6364699
feat: validate concept answers and display inline error for invalid e…
Bharath-K-Shetty 3652c22
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty 966a337
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty 2e91681
feat: validate concept answers and display inline error for invalid e…
Bharath-K-Shetty c98fa96
Merge branch 'main' into fix/O3-4171
NethmiRodrigo 3126298
Merge branch 'main' into fix/O3-4171
NethmiRodrigo 65090f6
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty fd39b04
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty bf6db8c
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty 46afb69
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty b1c5303
Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…
Bharath-K-Shetty 92aa0c4
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty 7529b29
Added warning icon for invalid concept answers along the concept name
Bharath-K-Shetty 694b17d
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty 875de46
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty c69562e
Moved api call to concept resource
Bharath-K-Shetty 47a3981
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty 3281dd9
Added the warning icon inside the decorator prop
Bharath-K-Shetty 72857db
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty cc84df6
Added the warning icon inside the decorator prop
Bharath-K-Shetty 62bb341
Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…
Bharath-K-Shetty e7bbadb
Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…
Bharath-K-Shetty c913dbd
Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…
Bharath-K-Shetty 674b839
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty e03def6
Merge branch 'main' into fix/O3-4171
NethmiRodrigo f4a5028
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty 0be5c7d
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty f25ee77
Merge branch 'main' into fix/O3-4171
NethmiRodrigo 43bf43b
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty 8d6f4c4
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty cb472c3
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import { openmrsFetch, restBaseUrl } from '@openmrs/esm-framework'; | ||
|
||
export async function fetchConceptById(uuid: string) { | ||
const url = `${restBaseUrl}/concept/${uuid}?v=full`; | ||
return await openmrsFetch(url); | ||
} |
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.
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.
Hey @Bharath-K-Shetty, does this mean that if the label of the answer is not the same as the label of the original answer from the concept, it will be flagged as invalid?
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.
Yes @NethmiRodrigo
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.
Is there a reason behind that? If the concept UUID is correct, that is sufficient, no? Since we fetch concepts using the UUID only.
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.
Here, I added validation for labels as well, because in the schema, it's possible to edit a concept answer to a name that isn't present in the system (even if the UUID is valid), which technically isn't incorrect, right? Also, since label validation is already used for validating concepts, I thought it would be better to validate concept answers through labels too. Regarding the fetching part—it also returns the labels of the valid concepts, which we can use to identify and validate any invalid ones.
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.
How I was thinking about it is, if changing the label of a concept doesn't cause a failure when saving the form, then the form builder shouldn't flag an error saying concept not found.
Mind pointing me to where we're doing that?
Yes, but we aren't using the label to fetch concepts, so it feels wrong to invalidate saying
concept not found
, when it can be found.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.
Then is it correct to add a concept answer that has invalid label but with a valid UUID..?
If it feels wrong.We can add a different notification saying that concept answer label is invalid.Is this be good enough or should removing it be better..?
Sorry i guess i misunderstood what
conceptNameLookupError
means..!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.
Can you test this out and see? Change just the label of an answer in the form, and submit the form in the patient chart for a patient, and see if it causes an error? If it doesn't then we shouldn't show any error for it.
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.
If you are referring to
openmrs-esm-form-builder/src/components/interactive-builder/modals/question/question-form/common/concept-search/concept-search.component.tsx
Line 34 in 4873e34
conceptIdLookupError
because that hook, as defined here -openmrs-esm-form-builder/src/hooks/useConceptId.ts
Lines 12 to 23 in 4873e34
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.
but in the patient chart it is not showing error for both invalid id and label of concept answer..?(It is showing error if main selected concept answer has invalid id)
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.
@NethmiRodrigo should i remove the validation done on labels..?