Skip to content
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

Validate Unique ID for All Layouts in a Layout-Set #12487

Merged
merged 9 commits into from
Mar 13, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,21 @@ import { EditComponentIdRow, type EditComponentIdRowProps } from './EditComponen
import userEvent from '@testing-library/user-event';
import { ComponentType } from 'app-shared/types/ComponentType';
import { textMock } from '../../../../../../../testing/mocks/i18nMock';
import { queryClientMock } from 'app-shared/mocks/queryClientMock';
import { formDesignerMock } from '../../../../testing/stateMocks';
import type { IFormLayouts } from '../../../../types/global';
import { QueryKey } from 'app-shared/types/QueryKey';
import { layout1NameMock, layoutMock } from '../../../../testing/layoutMock';

const studioRender = async (props: Partial<EditComponentIdRowProps>) => {
const org = 'org';
const app = 'app';
const layoutSetName = formDesignerMock.layout.selectedLayoutSet;
const layouts: IFormLayouts = {
[layout1NameMock]: layoutMock,
};

const studioRender = async (props: Partial<EditComponentIdRowProps> = {}) => {
queryClientMock.setQueryData([QueryKey.FormLayouts, org, app, layoutSetName], layouts);
return renderWithMockStore({})(
<EditComponentIdRow
component={{
Expand All @@ -27,14 +40,14 @@ describe('EditComponentIdRow', () => {
});

it('should render button ', async () => {
await studioRender({});
await studioRender();
const testIdButton = screen.getByRole('button', { name: 'ID: test' });
expect(testIdButton).toBeInTheDocument();
});

it('should render textField when the button is clicked', async () => {
const user = userEvent.setup();
await studioRender({});
await studioRender();
const testIdButton = screen.getByRole('button', { name: 'ID: test' });
await act(() => user.click(testIdButton));
const textField = screen.getByRole('textbox', {
Expand All @@ -45,7 +58,7 @@ describe('EditComponentIdRow', () => {

it('should not render the textfield when changing from edit mode to view mode ', async () => {
const user = userEvent.setup();
await studioRender({});
await studioRender();
const testIdButton = screen.getByRole('button', { name: 'ID: test' });
await act(() => user.click(testIdButton));
const textField = screen.getByRole('textbox', {
Expand All @@ -71,7 +84,7 @@ describe('EditComponentIdRow', () => {

it('should show error required error message when id is empty', async () => {
const user = userEvent.setup();
await studioRender({});
await studioRender();
const testIdButton = screen.getByRole('button', { name: 'ID: test' });
await act(() => user.click(testIdButton));
const textField = screen.getByRole('textbox', {
Expand All @@ -80,4 +93,19 @@ describe('EditComponentIdRow', () => {
await act(() => user.clear(textField));
expect(screen.getByText(textMock('validation_errors.required'))).toBeInTheDocument();
});

it('should show error message when id is not unique', async () => {
const user = userEvent.setup();
await studioRender();
const testIdButton = screen.getByRole('button', { name: 'ID: test' });
await act(() => user.click(testIdButton));
const textField = screen.getByRole('textbox', {
name: textMock('ux_editor.modal_properties_component_change_id'),
});
await act(() => user.clear(textField));
await act(() => user.type(textField, 'fileUploadComponentIdMock'));
expect(
screen.getByText(textMock('ux_editor.modal_properties_component_id_not_unique_error')),
).toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ import React, { useState } from 'react';
import { StudioToggleableTextfieldSchema, type SchemaValidationError } from '@studio/components';
import { KeyVerticalIcon } from '@navikt/aksel-icons';
import classes from './EditComponentIdRow.module.css';
import { idExists } from '../../../../utils/formLayoutUtils';
import { useSelectedFormLayout } from '../../../../hooks';
import { idExists } from '../../../../utils/formLayoutsUtils';
import { useTranslation } from 'react-i18next';
import type { FormItem } from '../../../../types/FormItem';
import { useLayoutSchemaQuery } from '../../../../hooks/queries/useLayoutSchemaQuery';
import { useFormLayouts } from '../../../../hooks/useFormLayoutsSelector';

export interface EditComponentIdRowProps {
handleComponentUpdate: (component: FormItem) => void;
Expand All @@ -18,8 +18,7 @@ export const EditComponentIdRow = ({
component,
handleComponentUpdate,
}: EditComponentIdRowProps) => {
const { components, containers } = useSelectedFormLayout();

const formLayouts = useFormLayouts();
const { t } = useTranslation();
const [{ data: layoutSchema }, , { data: expressionSchema }, { data: numberFormatSchema }] =
useLayoutSchemaQuery();
Expand All @@ -39,7 +38,7 @@ export const EditComponentIdRow = ({
if (value?.length === 0) {
return t('validation_errors.required');
}
if (value !== component.id && idExists(value, components, containers)) {
if (value !== component.id && idExists(value, formLayouts)) {
return t('ux_editor.modal_properties_component_id_not_unique_error');
}
return '';
Expand Down
13 changes: 0 additions & 13 deletions frontend/packages/ux-editor/src/utils/formLayoutUtils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import type {
IFormDesignerComponents,
IFormDesignerContainers,
IInternalLayout,
InternalLayoutComponents,
InternalLayoutData,
Expand All @@ -27,17 +25,6 @@ export const mapComponentToToolbarElement = <T extends ComponentType>(
type: c.name,
});

export function idExists(
id: string,
components: IFormDesignerComponents,
containers: IFormDesignerContainers,
): boolean {
return (
Object.keys(containers || {}).findIndex((key) => key.toUpperCase() === id.toUpperCase()) > -1 ||
Object.keys(components || {}).findIndex((key) => key.toUpperCase() === id.toUpperCase()) > -1
);
}

/**
* Checks if a layout has navigation buttons.
* @param layout The layout to check.
Expand Down
44 changes: 44 additions & 0 deletions frontend/packages/ux-editor/src/utils/formLayoutsUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ import {
addOrRemoveNavigationButtons,
convertExternalLayoutsToInternalFormat,
firstAvailableLayout,
idExists,
} from './formLayoutsUtils';
import { ComponentType } from 'app-shared/types/ComponentType';
import { createEmptyLayout } from './formLayoutUtils';
import { BASE_CONTAINER_ID, DEFAULT_SELECTED_LAYOUT_NAME } from 'app-shared/constants';
import { externalLayoutsMock, layout1NameMock, layout2NameMock } from '../testing/layoutMock';
import type { FormButtonComponent } from '../types/FormComponent';
import { componentMocks } from '../testing/componentMocks';

describe('formLayoutsUtils', () => {
describe('addOrRemoveNavigationButtons', () => {
Expand Down Expand Up @@ -295,4 +297,46 @@ describe('formLayoutsUtils', () => {
expect(layout).toBe(DEFAULT_SELECTED_LAYOUT_NAME);
});
});

describe('idExists', () => {
const layoutId = 'layout1';
const layoutId2 = 'layout2';
const navButtonsId = 'navButtons';
const groupId = 'group1';
const groupComponent = componentMocks[ComponentType.Group];

const formLayouts: IFormLayouts = {
[layoutId]: {
components: { [navButtonsId]: componentMocks[ComponentType.NavigationButtons] },
containers: undefined,
order: { [BASE_CONTAINER_ID]: [navButtonsId] },
customRootProperties: {},
customDataProperties: {},
},
[layoutId2]: {
components: undefined,
containers: {
[groupId]: groupComponent,
},
order: { [BASE_CONTAINER_ID]: [groupId] },
customRootProperties: {},
customDataProperties: {},
},
};

it('returns true when a container has the same id', () => {
const exists = idExists(groupId, formLayouts);
expect(exists).toBe(true);
});

it('returns true if when a component has the same id', () => {
const exists = idExists(navButtonsId, formLayouts);
expect(exists).toBe(true);
});

it('Returns false if id does not exist in any of the layouts', () => {
const exists = idExists('unique', formLayouts);
expect(exists).toBe(false);
});
});
});
21 changes: 21 additions & 0 deletions frontend/packages/ux-editor/src/utils/formLayoutsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,24 @@ export const firstAvailableLayout = (deletedLayoutName: string, layoutPagesOrder

return DEFAULT_SELECTED_LAYOUT_NAME;
};

/**
* Checks if a layout-set with the given id exists in the given list of layouts
* @param id The id of the component/container to check for
* @param formLayouts The list of layouts to check
* @returns True if the id exists in any of the layouts, false otherwise
*/
export function idExists(id: string, formLayouts: IFormLayouts): boolean {
return Object.values(formLayouts).some((layout) => {
const idMatchesContainers =
Object.keys(layout.containers || {}).findIndex(
(key) => key.toUpperCase() === id.toUpperCase(),
) > -1;
const idMatchesComponents =
Object.keys(layout.components || {}).findIndex(
(key) => key.toUpperCase() === id.toUpperCase(),
) > -1;

return idMatchesContainers || idMatchesComponents;
});
}
Comment on lines +119 to +132
Copy link
Contributor

Choose a reason for hiding this comment

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

Her kan vi også bruke some i stedet for findIndex. Men ville det ikke vært ryddigere å beholde den gamle idExists og referere til den her?

export function idExists(id: string, formLayouts: IFormLayouts): boolean {
  return Object.values(formLayouts).some((layout) => idExistsInLayout(id, layout));
}

@lassopicasso

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Det var et godt forslag! Jeg skal fikse det

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomasEng, jeg har laget en PR her: #12502

Loading