-
Notifications
You must be signed in to change notification settings - Fork 107
(fix) O3-4320: Remove Irrelevant Properties When Changing Question Type #391
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
base: main
Are you sure you want to change the base?
Changes from 13 commits
5630256
5775d3a
430a94c
c6d72bf
ad00001
1a4b060
4b48aa2
86b3dca
a360ece
94aaba8
2df050e
710f21d
5a35b81
59b0f64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,42 @@ | ||||
import { cleanFormFieldForType } from '../../question-utils'; | ||||
import type { FormField } from '@openmrs/esm-form-engine-lib'; | ||||
|
||||
describe('cleanFormFieldForType', () => { | ||||
it('should remove irrelevant top-level and nested properties when changing the question type', () => { | ||||
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, | ||||
}; | ||||
|
||||
// Change type from 'obs' to 'control' and update rendering to 'markdown' | ||||
const newType = 'control'; | ||||
const cleaned = cleanFormFieldForType( | ||||
{ ...formField, type: newType, questionOptions: { rendering: 'markdown' } }, | ||||
newType, | ||||
); | ||||
|
||||
// Assert that the type is updated. | ||||
expect(cleaned.type).toBe('control'); | ||||
|
||||
// Assert that id remains. | ||||
expect(cleaned.id).toBe('testQuestion'); | ||||
|
||||
// In our design, label is required for all question types, so it should remain. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, the
Suggested change
|
||||
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'); | ||||
Comment on lines
+35
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of asserting property by property, can't we have a result object and assert that the
|
||||
}); | ||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import type { FormField, RenderType } from '@openmrs/esm-form-engine-lib'; | ||
import { allowedPropertiesMapping, allowedQuestionOptionsMapping, allowedRenderingOptionsMapping } from '@constants'; | ||
|
||
/** | ||
* Cleans the given questionOptions object by retaining only allowed keys for the new type. | ||
* It combines allowed keys from both the question type and the rendering type. | ||
*/ | ||
function cleanQuestionOptionsForType(options: any, questionType: string, rendering: RenderType): any { | ||
const allowedByType = allowedQuestionOptionsMapping[questionType] || []; | ||
const allowedByRendering = allowedRenderingOptionsMapping[rendering] || []; | ||
const allowedOpts = new Set(['rendering', ...allowedByType, ...allowedByRendering]); | ||
const cleanedOpts = Object.fromEntries(Object.entries(options).filter(([key]) => allowedOpts.has(key))); | ||
cleanedOpts.rendering = options.rendering; | ||
return cleanedOpts; | ||
} | ||
|
||
/** | ||
* Cleans the given form field by retaining only allowed top‑level properties for the new type. | ||
* Also cleans nested questionOptions using the combined mappings for question type and rendering type. | ||
*/ | ||
export function cleanFormFieldForType(field: FormField, newType: string): FormField { | ||
const allowedKeys = allowedPropertiesMapping[newType] || []; | ||
const cleaned: Partial<FormField> = {}; | ||
|
||
allowedKeys.forEach((key) => { | ||
if (key in field) { | ||
(cleaned as any)[key] = field[key as keyof FormField]; | ||
} | ||
}); | ||
|
||
// If questionOptions is allowed and exists, clean it using the nested mapping. | ||
if (cleaned.questionOptions && typeof cleaned.questionOptions === 'object') { | ||
cleaned.questionOptions = cleanQuestionOptionsForType( | ||
cleaned.questionOptions, | ||
newType, | ||
cleaned.questionOptions.rendering as RenderType, | ||
); | ||
} | ||
cleaned.type = newType; | ||
return cleaned as FormField; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||
import type { RenderType } from '@openmrs/esm-form-engine-lib'; | ||||||
import type { FormField, RenderType } from '@openmrs/esm-form-engine-lib'; | ||||||
|
||||||
export const questionTypes = [ | ||||||
'control', | ||||||
|
@@ -54,3 +54,81 @@ export const renderTypeOptions: Record<Exclude<QuestionType, 'obs'>, Array<Rende | |||||
patientIdentifier: ['text'], | ||||||
programState: ['select'], | ||||||
}; | ||||||
|
||||||
/** | ||||||
Required properties for all question types. | ||||||
*/ | ||||||
export const requiredProperties: Array<keyof FormField> = ['id', 'label', 'type', 'questionOptions']; | ||||||
|
||||||
/** | ||||||
Type-specific properties for each question type. | ||||||
*/ | ||||||
export const typeSpecificProperties: Record<string, Array<keyof FormField>> = { | ||||||
control: [], | ||||||
encounterDatetime: ['datePickerFormat'], | ||||||
encounterLocation: [], | ||||||
encounterProvider: [], | ||||||
encounterRole: [], | ||||||
obs: ['required'], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 'required` is a common property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see this comment again |
||||||
obsGroup: ['questions'], | ||||||
patientIdentifier: [], | ||||||
testOrder: [], | ||||||
programState: [], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
}; | ||||||
|
||||||
/** | ||||||
Merge required properties with type-specific ones. | ||||||
*/ | ||||||
export const allowedPropertiesMapping: Record<string, string[]> = Object.fromEntries( | ||||||
Object.entries(typeSpecificProperties).map(([type, props]) => { | ||||||
const mergedProps = new Set<string>([...requiredProperties, ...props]); | ||||||
return [type, Array.from(mergedProps)]; | ||||||
}), | ||||||
); | ||||||
|
||||||
/** | ||||||
Mapping of allowed keys for the nested questionOptions object per question type. | ||||||
*/ | ||||||
export const allowedQuestionOptionsMapping: Record<string, string[]> = { | ||||||
control: ['rendering'], | ||||||
encounterDatetime: ['rendering'], | ||||||
encounterLocation: ['rendering'], | ||||||
encounterProvider: ['rendering'], | ||||||
encounterRole: ['rendering'], | ||||||
obs: ['rendering', 'concept', 'answers'], | ||||||
obsGroup: ['rendering', 'concept'], | ||||||
patientIdentifier: ['rendering', 'identifierType', 'minLength', 'maxLength'], | ||||||
testOrder: ['rendering'], | ||||||
programState: ['rendering', 'programUuid', 'workflowUuid', 'answers'], | ||||||
}; | ||||||
|
||||||
/** | ||||||
Mapping of allowed keys for questionOptions based on the rendering type. | ||||||
*/ | ||||||
export const allowedRenderingOptionsMapping: Record<RenderType, string[]> = { | ||||||
checkbox: ['rendering', 'concept', 'answers'], | ||||||
'checkbox-searchable': ['rendering', 'concept'], | ||||||
'content-switcher': ['rendering', 'concept'], | ||||||
date: ['rendering'], | ||||||
datetime: ['rendering'], | ||||||
drug: ['rendering', 'concept'], | ||||||
'encounter-location': ['rendering', 'concept'], | ||||||
'encounter-provider': ['rendering', 'concept'], | ||||||
'encounter-role': ['rendering', 'concept', 'isSearchable'], | ||||||
'fixed-value': ['rendering', 'concept'], | ||||||
file: ['rendering', 'concept'], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
group: ['rendering', 'concept'], | ||||||
number: ['rendering', 'concept', 'min', 'max'], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also relevant type: |
||||||
problem: ['rendering', 'concept'], | ||||||
radio: ['rendering', 'concept', 'answers'], | ||||||
repeating: ['rendering', 'concept'], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Relevant to this: |
||||||
select: ['rendering', 'concept', 'answers'], | ||||||
text: ['rendering', 'concept', 'minLength', 'maxLength'], | ||||||
textarea: ['rendering', 'concept', 'rows'], | ||||||
toggle: ['rendering', 'concept', 'toggleOptions'], | ||||||
'ui-select-extended': ['rendering', 'concept', 'isSearchable'], | ||||||
'workspace-launcher': ['rendering', 'concept'], | ||||||
markdown: ['rendering', 'concept'], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
'extension-widget': ['rendering', 'concept'], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The following are types relevant to this:
|
||||||
'select-concept-answers': ['rendering', 'concept', 'answers'], | ||||||
}; |
NethmiRodrigo marked this conversation as resolved.
Show resolved
Hide resolved
|
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.
Since these test cases are explicitly for testing the functions within the
question-utils
file and doesn't test anything within thequestion.component
itself, it would be better to add these to aquestion-utils.test
file. A test case that you could write for thequestion.component
would be something like loading a formField like:and then rendering the question component, and then clicking on the
Select Type
and changing the type and then checking ifsetFormField
gets called with the unnecessary properties removed