Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion e2e/pages/form-builder-page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export class FormBuilderPage {
name: /rendering type/i,
});
readonly conceptSearchInput = () => this.page.getByPlaceholder(/search using a concept name or uuid/i);
readonly selectAnswersDropdown = () => this.page.getByText(/select answers to display/i);
readonly selectAnswersDropdown = () => this.page.getByRole('combobox', { name: /select answers to display/i });
readonly answer = () => this.page.getByRole('menuitem', { name: /tested for covid 19/i });
readonly questionIdInput = () => this.page.getByRole('textbox', { name: /question id/i });
readonly questionCreatedMessage = () => this.page.getByText(/new question created/i);
Expand Down
4 changes: 2 additions & 2 deletions e2e/specs/create-form-with-interactive-builder.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@ test('Create a form using the interactive builder', async ({ page, context }) =>

await test.step('And then I select `Yes` and `No` as the answers to display', async () => {
await formBuilderPage.selectAnswersDropdown().click();
await formBuilderPage.page.getByRole('option', { name: 'No' }).click();
await formBuilderPage.page.getByRole('option', { name: 'Yes' }).click();
await formBuilderPage.page.getByRole('option', { name: /no/i }).click();
await formBuilderPage.page.getByRole('option', { name: /yes/i }).click();
});

await test.step('And then I click on `Save`', async () => {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"url": "https://github.com/openmrs/openmrs-esm-form-builder/issues"
},
"dependencies": {
"@carbon/react": "~1.49.0",
"@carbon/react": "^1.75.0",
"@openmrs/esm-form-engine-lib": "latest",
"ajv": "^8.17.1",
"dotenv": "^16.4.5",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,7 @@ describe('PatientIdentifierTypeQuestion', () => {
}),
).toBeInTheDocument();
await user.click(menuBox);
expect(
screen.getByRole('listbox', {
name: /choose an item/i,
}),
).toBeInTheDocument();
expect(screen.getByRole('listbox')).toBeInTheDocument();
expect(screen.getByText(/type 1/i)).toBeInTheDocument();
expect(screen.getByText(/type 2/i)).toBeInTheDocument();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ const ProgramStateTypeQuestion: React.FC = () => {
) : (
programStates?.length > 0 && (
<MultiSelect
label={t('programState', 'Program state')}
titleText={t('programState', 'Program state')}
id="programState"
items={programStates}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ describe('ProgramStateTypeQuestion', () => {

await user.click(programWorkflowSelectionOption);
expect(screen.getByRole('combobox', { name: /^program workflow$/i })).toHaveDisplayValue(/program 1 workflow 1/i);
expect(screen.getByText(/program state/i)).toBeInTheDocument();
expect(screen.getByText(/program state/i, { selector: 'span' })).toBeInTheDocument();

const programStateMenu = screen.getByRole('combobox', {
name: /program state/i,
Expand Down Expand Up @@ -185,7 +185,10 @@ describe('ProgramStateTypeQuestion', () => {
}),
).toHaveDisplayValue(/program 1 workflow 1/i);
expect(
screen.getByText(/total items selected: 1,to clear selection, press delete or backspace/i),
screen.getByText(
(content, element) =>
content.includes('Total items selected') && content.includes('1') && content.includes('To clear selection'),
),
).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

For improved readability and maintainability, consider using an array with .every() for validating all expected texts. This way, it's easier to scale if more substrings are added in the future, just like below :

const expectedTexts = ['Total items selected', '1', 'To clear selection'];
screen.getByText((content) => expectedTexts.every((text) => content.includes(text)));

expect(screen.getByText(/program 1 state 1/i)).toBeInTheDocument();
});
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be :

  1. Add a state variable isSelectAllDisabled or something on that line
  2. Update handleSelect to accommodate the changes

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import React, { useCallback, useEffect, useMemo, useState, useRef } from 'react';
import { Tag, MultiSelect, Stack } from '@carbon/react';
import { useTranslation } from 'react-i18next';
import ConceptSearch from '../../../common/concept-search/concept-search.component';
Expand All @@ -22,6 +22,9 @@ const SelectAnswers: React.FC = () => {
}
}, [concept]);

// Storing the initial questionOptions answers
const initialAnswers = useRef(formField.questionOptions?.answers ?? []);

Comment on lines +25 to +27
Copy link
Collaborator

Choose a reason for hiding this comment

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

Were these changes from a bad rebase?

const selectedAnswers = useMemo(
() =>
formField.questionOptions?.answers?.map((answer) => ({
Expand All @@ -33,24 +36,28 @@ const SelectAnswers: React.FC = () => {

const handleSelectAnswers = useCallback(
({ selectedItems }: { selectedItems: Array<AnswerItem> }) => {
const mappedAnswers = selectedItems.map((answer) => ({
concept: answer.id,
label: answer.text,
}));

setFormField((prevField) => {
const currentAnswers = prevField.questionOptions?.answers || [];
if (JSON.stringify(currentAnswers) === JSON.stringify(mappedAnswers)) {
return prevField;
}
return {
...prevField,
questionOptions: {
...prevField.questionOptions,
answers: mappedAnswers,
},
};
});
const mappedAnswers = selectedItems
.filter((item) => item.id !== 'select-all')
.map((answer) => ({
concept: answer.id,
label: answer.text,
}));

setTimeout(() => {
setFormField((prevField) => {
const currentAnswers = prevField.questionOptions?.answers || [];
if (JSON.stringify(currentAnswers) === JSON.stringify(mappedAnswers)) {
return prevField;
}
return {
...prevField,
questionOptions: {
...prevField.questionOptions,
answers: mappedAnswers,
},
};
});
}, 0);
},
[setFormField],
);
Expand Down Expand Up @@ -103,14 +110,17 @@ const SelectAnswers: React.FC = () => {
text: answer.display,
})) ?? [];

const formFieldAnswers = formField.questionOptions?.answers ?? [];
const formFieldAnswers = initialAnswers.current;

// If no answers from concept but we have form field answers, use those
if (conceptAnswerItems.length === 0 && formFieldAnswers.length > 0) {
return formFieldAnswers.map((answer) => ({
id: answer.concept,
text: answer.label,
}));
return [
{ id: 'select-all', text: 'Select All', isSelectAll: true },
...formFieldAnswers.map((answer) => ({
id: answer.concept,
text: answer.label,
})),
];
}

// Merge concept answers with any additional form field answers
Expand All @@ -122,7 +132,7 @@ const SelectAnswers: React.FC = () => {
}));

return [...conceptAnswerItems, ...additionalAnswers];
}, [concept?.answers, formField.questionOptions?.answers]);
}, [concept?.answers]);

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

Expand All @@ -131,6 +141,7 @@ const SelectAnswers: React.FC = () => {
{answerItems.length > 0 && (
<MultiSelect
className={styles.multiSelect}
label={t('selectAnswersToDisplay', 'Select answers to display')}
direction="top"
id="selectAnswers"
items={answerItems}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,11 @@ describe('Select answers component', () => {
});
it('renders', () => {
renderComponent();
expect(screen.getByText(/select answers to display/i)).toBeInTheDocument();
expect(
screen.getByRole('combobox', {
name: /select answers to display/i,
}),
).toBeInTheDocument();
expect(screen.getByText(/search for a concept to add as an answer/i)).toBeInTheDocument();
expect(
screen.getByRole('searchbox', {
Expand All @@ -72,7 +76,7 @@ describe('Select answers component', () => {
const user = userEvent.setup();
renderComponent();
const answersMenu = screen.getByRole('combobox', {
name: /select answers to display/i,
name: /^select answers to display/i,
});
expect(answersMenu).toBeInTheDocument();

Expand All @@ -85,7 +89,7 @@ describe('Select answers component', () => {
expect(screen.getByTitle(/answer 1/i)).toBeInTheDocument();
expect(
screen.getByRole('combobox', {
name: /select answers to display total items selected: 1,to clear selection, press delete or backspace/i,
name: /select answers to display total items selected: 1 answer 1,to clear selection, press delete or backspace/i,
}),
).toBeInTheDocument();
});
Expand All @@ -110,7 +114,7 @@ describe('Select answers component', () => {

expect(screen.getByTitle(/concept 2/i)).toBeInTheDocument();
expect(
screen.getByText(/total items selected: 1,to clear selection, press delete or backspace/i),
screen.getByText(/total items selected: 1 concept 2,to clear selection, press delete or backspace/i),
).toBeInTheDocument();
expect(
screen.getByRole('button', {
Expand Down
Loading