(feat) O3-4171 : Validate concept answers and show error for non-existing answers #445
Conversation
|
Hey @NethmiRodrigo should i stick with this endpoint |
| setInvalidAnswerFound(!allValid); | ||
| }; | ||
|
|
||
| validateAnswers(); |
There was a problem hiding this comment.
Since you are calling this function inside useEffect, the function should be defined outside the useEffect hook.
| if (originalAnswerIds.has(answer.id)) { | ||
| continue; | ||
| } else { | ||
| const url = `${restBaseUrl}/concept/${answer.id}?v=full`; |
There was a problem hiding this comment.
Please use the useConceptId hook.
For future reference, we don't usually do API calls within the component like this. We make hooks - https://o3-docs.openmrs.org/docs/coding-conventions/data-fetching.
There was a problem hiding this comment.
@NethmiRodrigo But we can't directly use the hook inside the loop right??Can you please elaborate how exactly i can do that..!
| {invalidAnswerFound && ( | ||
| <InlineNotification | ||
| kind="error" | ||
| lowContrast | ||
| className={styles.error} | ||
| title={t('invalidAnswerConcept', 'Invalid Answer Concept Detected')} | ||
| subtitle={t('answerConceptValidation', 'One or more selected answer concepts do not exist in the system. ')} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
This is good, maybe we can show a not found icon within the blue tag of the answer that is invalid. Otherwise we'd have to guess which ones are invalid.
|
|
|
Updated one..! |
| const expectedName = originalAnswersMap.get(answer.id); | ||
| if (answer.text !== expectedName) { | ||
| invalidIds.push(answer.id); | ||
| } |
There was a problem hiding this comment.
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.
Yes @NethmiRodrigo
There was a problem hiding this comment.
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.
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.
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.
Also, since label validation is already used for validating concepts
Mind pointing me to where we're doing that?
Regarding the fetching part—it also returns the labels of the valid concepts, which we can use to identify and validate any invalid ones.
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.
@NethmiRodrigo should i remove the validation done on labels..?
There was a problem hiding this comment.
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)
I don't understand. What do you mean main selected concept answer? What is a concept answer, and what is a main selected concept answer?
There was a problem hiding this comment.
If on patient chart you didn't get an error saving a form that had a answer whose label you you altered, then yes you should remove the validation done on labels.
There was a problem hiding this comment.
I don't understand. What do you mean main selected concept answer? What is a concept answer, and what is a main selected concept answer?
What I meant here is it is not showing error for both invalid id and label of a concept answer.(But it is showing error for the main backing concept which we selected if it has invalid id)
There was a problem hiding this comment.
If on patient chart you didn't get an error saving a form that had a answer whose label you you altered, then yes you should remove the validation done on labels.
Ok @NethmiRodrigo..!
|
Hey @NethmiRodrigo Dennis mentioned like without adding warning icon inside the Tag component he will have some better approach so that we don't need to change that robust test case..! He also mentioned like adding a icon inside the Tag component is not a good approach because previously the Tag element had the single child which it was considering it as a title.But now it is changed and we don't have any attribute by which we can add a title to it and make the test case work..! |
|
Hey @NethmiRodrigo & @denniskigen i think i got the solution instead of explicitly adding the icon.In |
|
Isn't that experimental though? It looks like you can use the |
|
Discussed on today's design call with myself, Bharath, and Veronica. We decided on this approach: Sketches file: |
|
Updated @NethmiRodrigo ..! |
There was a problem hiding this comment.
@Bharath-K-Shetty is the plan to merge this in and then work on updating the UI for the answer items in #403?
...der/modals/question/question-form/rendering-types/inputs/select/select-answers.component.tsx
Outdated
Show resolved
Hide resolved
…rm/rendering-types/inputs/select/select-answers.component.tsx Co-authored-by: Nethmi Rodrigo <[email protected]>
yes @NethmiRodrigo ..! |
Requirements
Summary
Screenshots
Screen.Recording.2025-04-08.190713.mp4
Related Issue
https://openmrs.atlassian.net/browse/O3-4171
Other