(feat) O3-4171 : Validate concept answers and show error for non-existing answers #445

Bharath-K-Shetty · 2025-04-08T14:38:27Z

Requirements

This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
My work includes tests or is validated by existing tests.

Summary

Checks each answer concept against the original concept's answer list
Performs a concept lookup only for non-original answers
Shows an inline notification when one or more answers are invalid.

Screenshots

Screen.Recording.2025-04-08.190713.mp4

Related Issue

https://openmrs.atlassian.net/browse/O3-4171

Other

…ntries

Bharath-K-Shetty · 2025-04-08T14:46:53Z

Hey @NethmiRodrigo should i stick with this endpoint ${restBaseUrl}/concept?q=${conceptId}&v=full for additional concept answers validation or current end point is good?
Also, I'm planning to extend the validation to check for invalid concept answer names as well. I haven’t implemented that part yet since there are a few existing issues with editing concept answer names that I want to avoid interfering with for now.Thanks..!

…ntries

NethmiRodrigo · 2025-04-10T06:03:58Z

...der/modals/question/question-form/rendering-types/inputs/select/select-answers.component.tsx

+      setInvalidAnswerFound(!allValid);
+    };
+
+    validateAnswers();


Since you are calling this function inside useEffect, the function should be defined outside the useEffect hook.

NethmiRodrigo · 2025-04-10T06:07:43Z

...der/modals/question/question-form/rendering-types/inputs/select/select-answers.component.tsx

+        if (originalAnswerIds.has(answer.id)) {
+          continue;
+        } else {
+          const url = `${restBaseUrl}/concept/${answer.id}?v=full`;


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.

@NethmiRodrigo But we can't directly use the hook inside the loop right??Can you please elaborate how exactly i can do that..!

NethmiRodrigo · 2025-04-10T06:09:09Z

...der/modals/question/question-form/rendering-types/inputs/select/select-answers.component.tsx

+      {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. ')}
+        />
+      )}


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.

…m-form-builder into fix/O3-4171

…nmrs-esm-form-builder into fix/O3-4171

…m-form-builder into fix/O3-4171

Bharath-K-Shetty · 2025-04-15T12:25:37Z

Added Warning icon for invalid concept answers..!

…m-form-builder into fix/O3-4171

Bharath-K-Shetty · 2025-04-17T12:37:07Z

Updated one..!

NethmiRodrigo · 2025-04-18T06:06:15Z

...der/modals/question/question-form/rendering-types/inputs/select/select-answers.component.tsx

+        const expectedName = originalAnswersMap.get(answer.id);
+        if (answer.text !== expectedName) {
+          invalidIds.push(answer.id);
+        }


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?

Yes @NethmiRodrigo

Is there a reason behind that? If the concept UUID is correct, that is sufficient, no? Since we fetch concepts using the UUID only.

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.

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.

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.

Then is it correct to add a concept answer that has invalid label but with a valid UUID..?

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.

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..?

Mind pointing me to where we're doing that?

Sorry i guess i misunderstood what conceptNameLookupError means..!

Then is it correct to add a concept answer that has invalid label but with a valid UUID..?

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.

Sorry i guess i misunderstood what conceptNameLookupError means..!

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

conceptNameLookupError,

yeah that variable should be named differently to something like conceptIdLookupError because that hook, as defined here -

openmrs-esm-form-builder/src/hooks/useConceptId.ts

Lines 12 to 23 in 4873e34

export function useConceptId(conceptId: string | undefined): UseConceptNameReturnType {

const url = `${restBaseUrl}/concept/${conceptId}?v=full`;

const { data, error } = useSWR<{ data: Concept }, Error>(conceptId ? url : null, openmrsFetch);

return {

concept: data?.data ?? null,

conceptName: data?.data?.display ?? null,

conceptNameLookupError: error,

isLoadingConcept: (conceptId && !data && !error) || false,

};

}

actually searches for the concept using the concept id and not the concept name.

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.

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)

@NethmiRodrigo should i remove the validation done on labels..?

NethmiRodrigo

Any progress on fixing the test?

Bharath-K-Shetty · 2025-04-18T06:15:05Z

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..!

Bharath-K-Shetty · 2025-04-18T07:07:48Z

Hey @NethmiRodrigo & @denniskigen i think i got the solution instead of explicitly adding the icon.In <Tag> we have a prop called decorator where we can add a warning icon..!

denniskigen · 2025-04-18T08:17:59Z

Isn't that experimental though? It looks like you can use the renderIcon prop to achieve the same goal.

Bharath-K-Shetty · 2025-04-18T09:18:49Z

Hey @denniskigen Yeah i understand that it is experimental.But if i use renderIcon prop instead of decorator then the icon will be rendered first before the child..!

NethmiRodrigo · 2025-04-18T10:08:35Z

But if i use renderIcon prop instead of decorator then the icon will be rendered first before the child..!

@Bharath-K-Shetty that's actually fine

Bharath-K-Shetty · 2025-04-18T10:40:05Z

@NethmiRodrigo it is updated now..!

…nmrs-esm-form-builder into fix/O3-4171

gracepotma · 2025-04-24T14:40:08Z

Discussed on today's design call with myself, Bharath, and Veronica. We decided on this approach:

Sketches file:
https://docs.google.com/presentation/d/1h7O5OAyBrS8YEeE_KXntEqB8HVZ9XofNFolWEMXqIPk/edit?usp=sharing

Bharath-K-Shetty added 3 commits April 8, 2025 19:52

feat: validate concept answers and display inline error for invalid e…

6364699

…ntries

Merge branch 'main' into fix/O3-4171

3652c22

Merge branch 'main' into fix/O3-4171

966a337

Bharath-K-Shetty and others added 3 commits April 8, 2025 20:19

feat: validate concept answers and display inline error for invalid e…

2e91681

…ntries

Merge branch 'main' into fix/O3-4171

c98fa96

Merge branch 'main' into fix/O3-4171

3126298

NethmiRodrigo requested changes Apr 10, 2025

View reviewed changes

Bharath-K-Shetty added 6 commits April 10, 2025 20:33

Merge branch 'main' into fix/O3-4171

65090f6

Merge branch 'main' into fix/O3-4171

fd39b04

Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…

bf6db8c

…m-form-builder into fix/O3-4171

Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…

46afb69

…m-form-builder into fix/O3-4171

Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…

b1c5303

…nmrs-esm-form-builder into fix/O3-4171

Merge branch 'main' into fix/O3-4171

92aa0c4

Bharath-K-Shetty requested a review from NethmiRodrigo April 15, 2025 11:23

Bharath-K-Shetty added 2 commits April 15, 2025 17:49

Added warning icon for invalid concept answers along the concept name

7529b29

Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…

694b17d

…m-form-builder into fix/O3-4171

Bharath-K-Shetty added 3 commits April 16, 2025 14:53

Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…

875de46

…m-form-builder into fix/O3-4171

Moved api call to concept resource

c69562e

Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…

47a3981

…m-form-builder into fix/O3-4171

NethmiRodrigo reviewed Apr 18, 2025

View reviewed changes

NethmiRodrigo approved these changes Apr 18, 2025

View reviewed changes

Added the warning icon inside the decorator prop

3281dd9

Bharath-K-Shetty requested a review from NethmiRodrigo April 18, 2025 07:11

Merge branch 'main' into fix/O3-4171

72857db

Bharath-K-Shetty and others added 10 commits April 18, 2025 16:13

Added the warning icon inside the decorator prop

cc84df6

Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…

62bb341

…nmrs-esm-form-builder into fix/O3-4171

Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…

e7bbadb

…nmrs-esm-form-builder into fix/O3-4171

Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…

c913dbd

…nmrs-esm-form-builder into fix/O3-4171

Merge branch 'main' into fix/O3-4171

674b839

Merge branch 'main' into fix/O3-4171

e03def6

Merge branch 'main' into fix/O3-4171

f4a5028

Merge branch 'main' into fix/O3-4171

0be5c7d

Merge branch 'main' into fix/O3-4171

f25ee77

Merge branch 'main' into fix/O3-4171

43bf43b

Bharath-K-Shetty added 2 commits April 24, 2025 20:11

Merge branch 'main' into fix/O3-4171

8d6f4c4

Merge branch 'main' into fix/O3-4171

cb472c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) O3-4171 : Validate concept answers and show error for non-existing answers #445

(feat) O3-4171 : Validate concept answers and show error for non-existing answers #445

Bharath-K-Shetty commented Apr 8, 2025

Bharath-K-Shetty commented Apr 8, 2025 •

edited

Loading

NethmiRodrigo Apr 10, 2025

NethmiRodrigo Apr 10, 2025

Bharath-K-Shetty Apr 10, 2025 •

edited

Loading

NethmiRodrigo Apr 10, 2025

Bharath-K-Shetty commented Apr 15, 2025

Bharath-K-Shetty commented Apr 17, 2025

NethmiRodrigo Apr 18, 2025

Bharath-K-Shetty Apr 18, 2025

NethmiRodrigo Apr 21, 2025 •

edited

Loading

Bharath-K-Shetty Apr 21, 2025 •

edited

Loading

NethmiRodrigo Apr 22, 2025

Bharath-K-Shetty Apr 22, 2025 •

edited

Loading

NethmiRodrigo Apr 22, 2025

NethmiRodrigo Apr 22, 2025

Bharath-K-Shetty Apr 23, 2025 •

edited

Loading

Bharath-K-Shetty Apr 25, 2025

NethmiRodrigo left a comment

Bharath-K-Shetty commented Apr 18, 2025 •

edited

Loading

Bharath-K-Shetty commented Apr 18, 2025

denniskigen commented Apr 18, 2025 •

edited

Loading

Bharath-K-Shetty commented Apr 18, 2025

NethmiRodrigo commented Apr 18, 2025

Bharath-K-Shetty commented Apr 18, 2025 •

edited

Loading

gracepotma commented Apr 24, 2025

	export function useConceptId(conceptId: string \| undefined): UseConceptNameReturnType {
	const url = `${restBaseUrl}/concept/${conceptId}?v=full`;

	const { data, error } = useSWR<{ data: Concept }, Error>(conceptId ? url : null, openmrsFetch);

	return {
	concept: data?.data ?? null,
	conceptName: data?.data?.display ?? null,
	conceptNameLookupError: error,
	isLoadingConcept: (conceptId && !data && !error) \|\| false,
	};
	}

(feat) O3-4171 : Validate concept answers and show error for non-existing answers #445

Are you sure you want to change the base?

(feat) O3-4171 : Validate concept answers and show error for non-existing answers #445

Conversation

Bharath-K-Shetty commented Apr 8, 2025

Requirements

Summary

Screenshots

Related Issue

Other

Bharath-K-Shetty commented Apr 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharath-K-Shetty Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharath-K-Shetty commented Apr 15, 2025

Bharath-K-Shetty commented Apr 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NethmiRodrigo Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Bharath-K-Shetty Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharath-K-Shetty Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharath-K-Shetty Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NethmiRodrigo left a comment

Choose a reason for hiding this comment

Bharath-K-Shetty commented Apr 18, 2025 • edited Loading

Bharath-K-Shetty commented Apr 18, 2025

denniskigen commented Apr 18, 2025 • edited Loading

Bharath-K-Shetty commented Apr 18, 2025

NethmiRodrigo commented Apr 18, 2025

Bharath-K-Shetty commented Apr 18, 2025 • edited Loading

gracepotma commented Apr 24, 2025

Bharath-K-Shetty commented Apr 8, 2025 •

edited

Loading

Bharath-K-Shetty Apr 10, 2025 •

edited

Loading

NethmiRodrigo Apr 21, 2025 •

edited

Loading

Bharath-K-Shetty Apr 21, 2025 •

edited

Loading

Bharath-K-Shetty Apr 22, 2025 •

edited

Loading

Bharath-K-Shetty Apr 23, 2025 •

edited

Loading

Bharath-K-Shetty commented Apr 18, 2025 •

edited

Loading

denniskigen commented Apr 18, 2025 •

edited

Loading

Bharath-K-Shetty commented Apr 18, 2025 •

edited

Loading