Skip to content

(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
wants to merge 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
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 Apr 8, 2025
3652c22
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 8, 2025
966a337
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 8, 2025
2e91681
feat: validate concept answers and display inline error for invalid e…
Bharath-K-Shetty Apr 8, 2025
c98fa96
Merge branch 'main' into fix/O3-4171
NethmiRodrigo Apr 9, 2025
3126298
Merge branch 'main' into fix/O3-4171
NethmiRodrigo Apr 10, 2025
65090f6
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 10, 2025
fd39b04
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 12, 2025
bf6db8c
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty Apr 8, 2025
46afb69
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty Apr 12, 2025
b1c5303
Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…
Bharath-K-Shetty Apr 12, 2025
92aa0c4
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 14, 2025
7529b29
Added warning icon for invalid concept answers along the concept name
Bharath-K-Shetty Apr 15, 2025
694b17d
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty Apr 15, 2025
875de46
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty Apr 16, 2025
c69562e
Moved api call to concept resource
Bharath-K-Shetty Apr 16, 2025
47a3981
Merge branch 'main' of https://github.com/Bharath-K-Shetty/openmrs-es…
Bharath-K-Shetty Apr 17, 2025
3281dd9
Added the warning icon inside the decorator prop
Bharath-K-Shetty Apr 18, 2025
72857db
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 18, 2025
cc84df6
Added the warning icon inside the decorator prop
Bharath-K-Shetty Apr 18, 2025
62bb341
Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…
Bharath-K-Shetty Apr 18, 2025
e7bbadb
Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…
Bharath-K-Shetty Apr 18, 2025
c913dbd
Merge branch 'fix/O3-4171' of https://github.com/Bharath-K-Shetty/ope…
Bharath-K-Shetty Apr 18, 2025
674b839
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 19, 2025
e03def6
Merge branch 'main' into fix/O3-4171
NethmiRodrigo Apr 21, 2025
f4a5028
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 22, 2025
0be5c7d
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 22, 2025
f25ee77
Merge branch 'main' into fix/O3-4171
NethmiRodrigo Apr 23, 2025
43bf43b
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 24, 2025
8d6f4c4
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 24, 2025
cb472c3
Merge branch 'main' into fix/O3-4171
Bharath-K-Shetty Apr 25, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import { Tag, MultiSelect, Stack } from '@carbon/react';
import { Tag, MultiSelect, Stack, InlineNotification } from '@carbon/react';
import { WarningAltFilled } from '@carbon/react/icons';
import { useTranslation } from 'react-i18next';
import ConceptSearch from '../../../common/concept-search/concept-search.component';
import { useFormField } from '../../../../form-field-context';
import { fetchConceptById } from '@resources/concept.resource';
import type { Concept } from '@types';
import styles from './select-answers.scss';

interface AnswerItem {
id: string;
text: string;
Expand All @@ -15,6 +16,7 @@ const SelectAnswers: React.FC = () => {
const { t } = useTranslation();
const { formField, concept, setFormField } = useFormField();
const [addedAnswers, setAddedAnswers] = useState<AnswerItem[]>([]);
const [invalidAnswerIds, setInvalidAnswerIds] = useState<string[]>([]);

useEffect(() => {
if (!concept) {
Expand Down Expand Up @@ -130,6 +132,43 @@ const SelectAnswers: React.FC = () => {
return [...answersFromConceptWithLabelsFromFormField, ...additionalAnswers];
}, [concept?.answers, formField.questionOptions?.answers]);

const validateAnswers = useCallback(async () => {
if (!answerItems.length) {
setInvalidAnswerIds([]);
return;
}

const originalAnswersMap = new Map((concept?.answers || []).map((ans) => [ans.uuid, ans.display]));

const invalidIds: string[] = [];
const uniqueAnswers = Array.from(new Map(answerItems.map((a) => [a.id, a])).values());

for (const answer of uniqueAnswers) {
if (originalAnswersMap.has(answer.id)) {
const expectedName = originalAnswersMap.get(answer.id);
if (answer.text !== expectedName) {
invalidIds.push(answer.id);
}
Comment on lines +148 to +151
Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@NethmiRodrigo NethmiRodrigo Apr 21, 2025

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.

Copy link
Contributor Author

@Bharath-K-Shetty Bharath-K-Shetty Apr 21, 2025

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.

Copy link
Collaborator

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.

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.

Copy link
Contributor Author

@Bharath-K-Shetty Bharath-K-Shetty Apr 22, 2025

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.

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

Copy link
Collaborator

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry i guess i misunderstood what conceptNameLookupError means..!

If you are referring to

yeah that variable should be named differently to something like conceptIdLookupError because that hook, as defined here -
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.

Copy link
Contributor Author

@Bharath-K-Shetty Bharath-K-Shetty Apr 23, 2025

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.

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)

Copy link
Contributor Author

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

} else {
try {
const res = await fetchConceptById(answer.id);
if (!(res?.data?.uuid === answer.id && res?.data?.display === answer.text)) {
invalidIds.push(answer.id);
}
} catch (error) {
invalidIds.push(answer.id);
}
}
}
setInvalidAnswerIds(invalidIds);
}, [answerItems, concept]);

useEffect(() => {
if (concept?.answers?.length) {
validateAnswers();
}
}, [answerItems, concept, validateAnswers]);

const convertAnswerItemsToString = useCallback((item: AnswerItem) => item.text, []);

return (
Expand All @@ -151,13 +190,29 @@ const SelectAnswers: React.FC = () => {
{selectedAnswers.length > 0 && (
<div>
{selectedAnswers.map((answer) => (
<Tag className={styles.tag} key={answer.id} type={'blue'}>
<Tag
className={styles.tag}
key={answer.id}
type={invalidAnswerIds.includes(answer.id) ? 'red' : 'blue'}
renderIcon={invalidAnswerIds.includes(answer.id) ? WarningAltFilled : undefined}
>
{answer.text}
</Tag>
))}
</div>
)}

{/* Display an inline notification if any answer fails validation */}
{invalidAnswerIds.length > 0 && (
<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. ')}
/>
)}

{concept && concept.datatype?.name === 'Coded' && (
<>
<ConceptSearch
Expand Down
6 changes: 6 additions & 0 deletions src/resources/concept.resource.ts
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);
}