(fix) O3-4320: Remove Irrelevant Properties When Changing Question Type by Bharath-K-Shetty · Pull Request #391 · openmrs/openmrs-esm-form-builder

Bharath-K-Shetty · 2025-02-10T13:42:44Z

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

This PR implements functionality to clean up form field data when a user changes a question's type in the form builder. A mapping of allowed properties for each question type is defined, and a helper function (cleanFormFieldForType) ensures that only the properties relevant to the newly selected question type are retained. The solution also cleans the nested questionOptions object based on a separate mapping, ensuring required keys like "rendering" remain intact.

Screenshots

Screen.Recording.2025-02-10.183846.mp4

Related Issue

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

Other

Bharath-K-Shetty · 2025-02-10T13:43:47Z

@NethmiRodrigo review required..!

src/components/interactive-builder/modals/question/question.modal.tsx

NethmiRodrigo

Thanks @Bharath-K-Shetty! A couple of suggestions:

src/components/interactive-builder/modals/question/question.modal.tsx

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

Bharath-K-Shetty · 2025-02-21T18:33:16Z

@NethmiRodrigo updated the pr..!

NethmiRodrigo

@Bharath-K-Shetty left a few comments. Please add unit tests to cover this scenario

src/components/interactive-builder/modals/question/question.modal.tsx

...components/interactive-builder/modals/question/question-form/question/question.component.tsx

src/components/interactive-builder/modals/question/question-form/question/question.test.tsx

src/components/interactive-builder/modals/question/question-utils.ts

src/components/interactive-builder/modals/question/question.modal.tsx

translations/en.json

...components/interactive-builder/modals/question/question-form/question/question.component.tsx

NethmiRodrigo

@Bharath-K-Shetty looks like this isn't as simple as what I had initially thought. While this is halfway there, we have to consider the propeties specific for renderingTypes as well (as per https://openmrs.atlassian.net/wiki/spaces/docs/pages/68681754/Field+Types+Reference), which means there needs to be a bit more rework that needs to be done on this PR. You can do it and I can guide you through it, but if its a hassle, I can do it. What do you prefer?

Bharath-K-Shetty · 2025-03-11T11:10:58Z

Hey @NethmiRodrigo i think for cleanup of rendering Type there is a separate ticket focusing on that can you please clarify once.Ticket:https://openmrs.atlassian.net/browse/O3-4319

NethmiRodrigo · 2025-03-11T11:33:35Z

Hey @NethmiRodrigo i think for cleanup of rendering Type there is a separate ticket focusing on that can you please clarify once.Ticket:https://openmrs.atlassian.net/browse/O3-4319

Yeah, I thought we could handle it separately, but now seeing it in action, I don't think we can do them seperately

NethmiRodrigo

@Bharath-K-Shetty you'd have to reconsider the properties you added for questionType since some of them are for renderingType

NethmiRodrigo · 2025-03-11T11:35:47Z

src/constants.ts

+  encounterLocation: [],
+  encounterProvider: [],
+  encounterRole: [],
+  obs: ['required'],


'required` is a common property

Please see this comment again

NethmiRodrigo · 2025-03-11T11:36:32Z

src/constants.ts

+   Mapping of allowed keys for the nested questionOptions object per question type.
+ */
+export const allowedQuestionOptionsMapping: Record<string, string[]> = {
+  control: ['rendering', 'minLength', 'maxLength'],


'minLength', 'maxLength' are properties relevant to text rendering type and not specific to the control question type

NethmiRodrigo · 2025-03-11T11:37:07Z

src/constants.ts

+  encounterDatetime: ['rendering'],
+  encounterLocation: ['rendering'],
+  encounterProvider: ['rendering'],
+  encounterRole: ['rendering', 'isSearchable'],


Similarly, isSearchable is for ui-select-extended rendering type

NethmiRodrigo · 2025-03-11T11:46:59Z

@Bharath-K-Shetty So what I do to find the correct properties is, I go through the FormField type definition - https://github.com/openmrs/openmrs-esm-form-engine-lib/blob/d28f37f92cff6c6e97b3339113d921e880400855/src/types/schema.ts#L60 because most often than not they are pretty self explanatory. Similarly, - https://openmrs.atlassian.net/wiki/spaces/docs/pages/68681754/Field+Types+Reference for renderingType. Most of what you have put for questionType you'd see is actually what should be under renderingType

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

NethmiRodrigo

@Bharath-K-Shetty I've left a few comments, but the more I think about this, the more this approach seems to be unscalable, especially because I was thinking of all the possible combinations of questionType to renderingType we'd have to consider. Seems like the best way forward is to rework this to:

When changing the question type or rendering type, we just remove all the properties except id, label and required
Show a info tooltip next to the question type and rendering type that on hover would show a warning that changing the type would remove properties.
Over time, we'll probably evolve this into a better solution, but I think this proposed approach would be a much better and less complex task (and easier to test).
Sorry about the iterative changes, this is a ticket that I hadn't thought about in such depth until now

NethmiRodrigo · 2025-03-14T07:04:27Z

src/components/interactive-builder/modals/question/question-form/question/question.test.tsx

Since these test cases are explicitly for testing the functions within the question-utils file and doesn't test anything within the question.component itself, it would be better to add these to a question-utils.test file. A test case that you could write for the question.component would be something like loading a formField like:

const formField: FormField = { id: 'testQuestion', label: 'Test Question', type: 'obs', questionOptions: { rendering: 'radio', concept: '12345', answers: [ { concept: '111', label: 'Yes' }, { concept: '222', label: 'No' }, ], }, required: true, };

and then rendering the question component, and then clicking on the Select Type and changing the type and then checking if setFormField gets called with the unnecessary properties removed

NethmiRodrigo · 2025-03-14T07:06:27Z

src/components/interactive-builder/modals/question/question-form/question/question.test.tsx

+    // Assert that id remains.
+    expect(cleaned.id).toBe('testQuestion');
+
+    // In our design, label is required for all question types, so it should remain.


Actually, the label property is not required for questions that have the renderingType markdown

Suggested change

// In our design, label is required for all question types, so it should remain.

NethmiRodrigo · 2025-03-14T07:11:16Z

src/components/interactive-builder/modals/question/question-form/question/question.test.tsx

+    expect(cleaned.label).toBe('Test Question');
+
+    // Irrelevant nested properties should be removed.
+    expect(cleaned.questionOptions).toHaveProperty('rendering', 'markdown');
+    expect(cleaned.questionOptions).not.toHaveProperty('concept');
+    expect(cleaned.questionOptions).not.toHaveProperty('answers');


Instead of asserting property by property, can't we have a result object and assert that the cleaned object is equal to that, i.e:

const expectedObjectAfterClean = { id: 'testQuestion', type: 'control', questionOptions: { rendering: 'markdown', concept: '12345', }, required: true, } expect(cleaned).toEqual(expectedObjectAfterClean);

NethmiRodrigo · 2025-03-14T07:12:28Z

src/constants.ts

+  encounterLocation: [],
+  encounterProvider: [],
+  encounterRole: [],
+  obs: ['required'],


Please see this comment again

NethmiRodrigo · 2025-03-14T07:14:24Z

src/constants.ts

+  toggle: ['rendering', 'concept', 'toggleOptions'],
+  'ui-select-extended': ['rendering', 'concept', 'isSearchable'],
+  'workspace-launcher': ['rendering', 'concept'],
+  markdown: ['rendering', 'concept'],


Suggested change

markdown: ['rendering', 'concept'],

markdown: ['rendering'],

NethmiRodrigo · 2025-03-14T07:16:00Z

src/constants.ts

+  'ui-select-extended': ['rendering', 'concept', 'isSearchable'],
+  'workspace-launcher': ['rendering', 'concept'],
+  markdown: ['rendering', 'concept'],
+  'extension-widget': ['rendering', 'concept'],


The following are types relevant to this:

extensionId?: string; extensionSlotName?: string;

NethmiRodrigo · 2025-03-14T07:16:41Z

src/constants.ts

+  'fixed-value': ['rendering', 'concept'],
+  file: ['rendering', 'concept'],
+  group: ['rendering', 'concept'],
+  number: ['rendering', 'concept', 'min', 'max'],


Also relevant type: step?: number;

NethmiRodrigo · 2025-03-14T07:17:31Z

src/constants.ts

+  number: ['rendering', 'concept', 'min', 'max'],
+  problem: ['rendering', 'concept'],
+  radio: ['rendering', 'concept', 'answers'],
+  repeating: ['rendering', 'concept'],


Relevant to this: repeatOptions?: RepeatOptions;

NethmiRodrigo · 2025-03-14T07:17:56Z

src/constants.ts

+  'encounter-provider': ['rendering', 'concept'],
+  'encounter-role': ['rendering', 'concept', 'isSearchable'],
+  'fixed-value': ['rendering', 'concept'],
+  file: ['rendering', 'concept'],


allowedFileTypes?: Array<string>;

NethmiRodrigo · 2025-03-14T07:19:18Z

src/constants.ts

+  obsGroup: ['questions'],
+  patientIdentifier: [],
+  testOrder: [],
+  programState: [],


programUuid?: string; workflowUuid?: string;

anjula-sack · 2025-08-13T04:04:15Z

What's the status of this @Bharath-K-Shetty?

Allow users to change question type when editing a question

5630256

NethmiRodrigo self-requested a review February 11, 2025 11:57

Bharath-K-Shetty added 4 commits February 11, 2025 20:17

Merge branch 'main' into fix/O3-4320

5775d3a

Merge branch 'main' into fix/O3-4320

430a94c

Merge branch 'main' into fix/O3-4320

c6d72bf

Update question.modal.tsx

ad00001

harshthakkr suggested changes Feb 17, 2025

View reviewed changes

src/components/interactive-builder/modals/question/question.modal.tsx Outdated Show resolved Hide resolved

Bharath-K-Shetty added 2 commits February 17, 2025 18:48

Merge branch 'main' into fix/O3-4320

1a4b060

Update question.modal.tsx

4b48aa2

Bharath-K-Shetty requested a review from harshthakkr February 18, 2025 16:18

NethmiRodrigo requested changes Feb 21, 2025

View reviewed changes

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

86b3dca

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

Bharath-K-Shetty force-pushed the fix/O3-4320 branch from 613401b to e4b491f Compare February 21, 2025 16:06

Bharath-K-Shetty requested a review from NethmiRodrigo February 21, 2025 16:06

Refactor question type cleaning to remove irrelevant properties

a360ece

Bharath-K-Shetty force-pushed the fix/O3-4320 branch from e4b491f to a360ece Compare February 21, 2025 16:28

NethmiRodrigo requested changes Feb 26, 2025

View reviewed changes

src/components/interactive-builder/modals/question/question.modal.tsx Outdated Show resolved Hide resolved

src/components/interactive-builder/modals/question/question.modal.tsx Outdated Show resolved Hide resolved

Refactor question type cleaning & added unit tests

94aaba8

Bharath-K-Shetty force-pushed the fix/O3-4320 branch from 8462b2d to 94aaba8 Compare February 28, 2025 18:16

Bharath-K-Shetty requested a review from NethmiRodrigo February 28, 2025 18:17

NethmiRodrigo requested changes Mar 6, 2025

View reviewed changes

Bharath-K-Shetty requested a review from NethmiRodrigo March 7, 2025 09:12

NethmiRodrigo requested changes Mar 7, 2025

View reviewed changes

Use a memoized handler for question type changes

2df050e

Bharath-K-Shetty force-pushed the fix/O3-4320 branch from e399f65 to 2df050e Compare March 7, 2025 10:54

Bharath-K-Shetty requested a review from NethmiRodrigo March 7, 2025 10:59

NethmiRodrigo reviewed Mar 11, 2025

View reviewed changes

Bharath-K-Shetty requested a review from NethmiRodrigo March 11, 2025 11:11

NethmiRodrigo reviewed Mar 11, 2025

View reviewed changes

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

710f21d

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

Bharath-K-Shetty force-pushed the fix/O3-4320 branch from 1d30f27 to 9269ac7 Compare March 14, 2025 06:02

Allowed users to change rendering type when editing a question

5a35b81

Bharath-K-Shetty force-pushed the fix/O3-4320 branch from 9269ac7 to 5a35b81 Compare March 14, 2025 06:51

Bharath-K-Shetty requested a review from NethmiRodrigo March 14, 2025 06:52

NethmiRodrigo reviewed Mar 14, 2025

View reviewed changes

Merge branch 'main' into fix/O3-4320

59b0f64

NethmiRodrigo changed the title ~~(fix)O3-4320: Remove Irrelevant Properties When Changing Question Type~~ (fix) O3-4320: Remove Irrelevant Properties When Changing Question Type Mar 24, 2025

NethmiRodrigo mentioned this pull request Apr 29, 2025

(fix) O3-4613 Removing previous sub properties from Form json after changing Question and Rendering Type #446

Open

3 tasks

Conversation

Bharath-K-Shetty commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Requirements

Summary

Screenshots

Related Issue

Other

Uh oh!

Bharath-K-Shetty commented Feb 10, 2025

Uh oh!

Uh oh!

NethmiRodrigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bharath-K-Shetty commented Feb 21, 2025

Uh oh!

NethmiRodrigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NethmiRodrigo left a comment

Choose a reason for hiding this comment

Uh oh!

Bharath-K-Shetty commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NethmiRodrigo commented Mar 11, 2025

Uh oh!

NethmiRodrigo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NethmiRodrigo commented Mar 11, 2025

Uh oh!

NethmiRodrigo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjula-sack commented Aug 13, 2025

Uh oh!

Bharath-K-Shetty commented Feb 10, 2025 •

edited

Loading

Bharath-K-Shetty commented Mar 11, 2025 •

edited

Loading