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.

Sign up for GitHub

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

Fix add subform race condition #2995

Merged
merged 1 commit into from
Feb 11, 2025
Merged

Fix add subform race condition #2995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 25 additions & 26 deletions src/layout/CustomButton/CustomButtonComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ import { Lang } from 'src/features/language/Lang';
import { useCurrentLanguage } from 'src/features/language/LanguageProvider';
import { useIsSubformPage, useNavigationParam } from 'src/features/routing/AppRoutingContext';
import { useOnPageNavigationValidation } from 'src/features/validation/callbacks/onPageNavigationValidation';
import { useIsProcessing } from 'src/hooks/useIsProcessing';
import { useNavigatePage } from 'src/hooks/useNavigatePage';
import { ComponentStructureWrapper } from 'src/layout/ComponentStructureWrapper';
import { isSpecificClientAction } from 'src/layout/CustomButton/typeHelpers';
import { NodesInternal } from 'src/utils/layout/NodesContext';
import { useNodeItem } from 'src/utils/layout/useNodeItem';
import { promisify } from 'src/utils/promisify';
import type { ButtonColor, ButtonVariant } from 'src/app-components/Button/Button';
import type { BackendValidationIssueGroups } from 'src/features/validation';
import type { PropsFromGenericComponent } from 'src/layout';
Expand Down Expand Up @@ -72,10 +72,10 @@ function useHandleClientActions(): UseHandleClientActions {

const frontendActions: ClientActionHandlers = useMemo(
() => ({
nextPage: promisify(navigateToNextPage),
previousPage: promisify(navigateToPreviousPage),
navigateToPage: promisify<ClientActionHandlers['navigateToPage']>(async ({ page }) => navigateToPage(page)),
closeSubform: promisify(exitSubform),
nextPage: navigateToNextPage,
previousPage: navigateToPreviousPage,
navigateToPage: async ({ page }) => navigateToPage(page),
closeSubform: exitSubform,
}),
[exitSubform, navigateToNextPage, navigateToPage, navigateToPreviousPage],
);
Expand Down Expand Up @@ -238,8 +238,9 @@ export const CustomButtonComponent = ({ node }: Props) => {
const lockTools = FD.useLocking(id);
const { isAuthorized } = useActionAuthorization();
const { handleClientActions } = useHandleClientActions();
const { handleServerAction, isPending } = useHandleServerActionMutation(lockTools);
const { handleServerAction } = useHandleServerActionMutation(lockTools);
const onPageNavigationValidation = useOnPageNavigationValidation();
const [isProcessing, processing] = useIsProcessing<'action'>();

const getScrollPosition = React.useCallback(
() => document.querySelector(`[data-componentid="${id}"]`)?.getClientRects().item(0)?.y,
Expand All @@ -250,7 +251,7 @@ export const CustomButtonComponent = ({ node }: Props) => {
const isPermittedToPerformActions = actions
.filter((action) => action.type === 'ServerAction')
.reduce((acc, action) => acc && isAuthorized(action.id), true);
const disabled = !isPermittedToPerformActions || isPending;
const disabled = !isPermittedToPerformActions || !!isProcessing;

const isSubformCloseButton = actions.filter((action) => action.id === 'closeSubform').length > 0;
let interceptedButtonStyle = buttonStyle ?? 'secondary';
Expand All @@ -264,27 +265,25 @@ export const CustomButtonComponent = ({ node }: Props) => {
buttonText = 'general.done';
}

const onClick = async () => {
if (disabled) {
return;
}
for (const action of actions) {
if (action.validation) {
const prevScrollPosition = getScrollPosition();
const hasErrors = await onPageNavigationValidation(node.page, action.validation);
if (hasErrors) {
resetScrollPosition(prevScrollPosition);
return;
const onClick = () =>
processing('action', async () => {
for (const action of actions) {
if (action.validation) {
const prevScrollPosition = getScrollPosition();
const hasErrors = await onPageNavigationValidation(node.page, action.validation);
if (hasErrors) {
resetScrollPosition(prevScrollPosition);
return;
}
}
}

if (isClientAction(action)) {
await handleClientActions([action]);
} else if (isServerAction(action)) {
await handleServerAction({ action, buttonId: id });
if (isClientAction(action)) {
await handleClientActions([action]);
} else if (isServerAction(action)) {
await handleServerAction({ action, buttonId: id });
}
}
}
};
});

const style = buttonStyles[interceptedButtonStyle];

Expand All @@ -297,7 +296,7 @@ export const CustomButtonComponent = ({ node }: Props) => {
size={toShorthandSize(buttonSize)}
color={buttonColor ?? style.color}
variant={style.variant}
isLoading={isPending}
isLoading={!!isProcessing}
>
<Lang id={buttonText} />
</Button>
Expand Down
8 changes: 6 additions & 2 deletions src/layout/Subform/SubformComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { Button } from 'src/app-components/Button/Button';
import { Flex } from 'src/app-components/Flex/Flex';
import { Caption } from 'src/components/form/caption/Caption';
import { useDataTypeFromLayoutSet } from 'src/features/form/layout/LayoutsContext';
import { FD } from 'src/features/formData/FormDataWrite';
import { useFormDataQuery } from 'src/features/formData/useFormDataQuery';
import { useStrictDataElements, useStrictInstanceId } from 'src/features/instance/InstanceContext';
import { Lang } from 'src/features/language/Lang';
Expand Down Expand Up @@ -52,20 +53,22 @@ export function SubformComponent({ node }: PropsFromGenericComponent<'Subform'>)
const addEntryMutation = useAddEntryMutation(dataType);
const dataElements = useStrictDataElements(dataType);
const navigate = useNavigate();
const { lock, unlock } = FD.useLocking(id);
const [isAdding, setIsAdding] = useState(false);
const [subformEntries, updateSubformEntries] = useState(dataElements);

const subformIdsWithError = useComponentValidationsForNode(node).find(isSubformValidation)?.subformDataElementIds;

const addEntry = async () => {
setIsAdding(true);

try {
setIsAdding(true);
await lock();
const result = await addEntryMutation.mutateAsync({});
navigate(`${node.id}/${result.id}`);
} catch {
// NOTE: Handled by useAddEntryMutation
} finally {
unlock();
setIsAdding(false);
}
};
Expand Down Expand Up @@ -146,6 +149,7 @@ export function SubformComponent({ node }: PropsFromGenericComponent<'Subform'>)
id={`subform-${id}-add-button`}
size='md'
disabled={isAdding}
isLoading={isAdding}
onClick={async () => await addEntry()}
onKeyUp={async (event: React.KeyboardEvent<HTMLButtonElement>) => {
const allowedKeys = ['enter', ' ', 'spacebar'];
Expand Down
12 changes: 0 additions & 12 deletions src/utils/promisify.ts
View file
Open in desktop

This file was deleted.

24 changes: 12 additions & 12 deletions test/e2e/integration/subform-test/subform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,14 @@ describe('Subform test', () => {
cy.get(appFrontend.fieldValidation('subform-mopeder')).should('contain.text', 'Minst 1 moped oppføring er påkrevd');

// Test that save is blocked by validation
cy.findByRole('button', { name: /legg til moped/i }).click();
cy.findByRole('button', { name: /legg til moped/i }).clickAndGone();
cy.findByRole('button', { name: /ferdig/i }).should('be.visible');
cy.get(appFrontend.errorReport).should('not.exist');
cy.findByRole('button', { name: /ferdig/i }).click();
cy.get(appFrontend.errorReport).should('be.visible');

// Test validation of subform content
cy.findByRole('button', { name: /avbryt/i }).click();
cy.findByRole('button', { name: /avbryt/i }).clickAndGone();
cy.findByRole('button', { name: /neste/i }).click();
cy.get(appFrontend.errorReport).should('be.visible');
cy.get(appFrontend.fieldValidation('subform-mopeder')).should(
Expand All @@ -127,12 +127,12 @@ describe('Subform test', () => {
);

// Test that editing a subform with visible validations shows validations upon entering
cy.findByRole('button', { name: /endre/i }).click();
cy.findByRole('button', { name: /endre/i }).clickAndGone();
cy.findByRole('button', { name: /ferdig/i }).should('be.visible');
cy.get(appFrontend.errorReport).should('be.visible');

// Test that main form still shows the same validations as before upon exiting subform
cy.findByRole('button', { name: /avbryt/i }).click();
cy.findByRole('button', { name: /avbryt/i }).clickAndGone();
cy.findByRole('button', { name: /neste/i }).should('be.visible');
cy.get(appFrontend.errorReport).should('be.visible');
cy.get(appFrontend.fieldValidation('subform-mopeder')).should(
Expand All @@ -142,10 +142,10 @@ describe('Subform test', () => {

// Test that main form still shows the same validations as before upon exiting a newly created subform
// The reason for this case is that this did not work initially
cy.findByRole('button', { name: /legg til moped/i }).click();
cy.findByRole('button', { name: /legg til moped/i }).clickAndGone();
cy.findByRole('button', { name: /ferdig/i }).should('be.visible');
cy.get(appFrontend.errorReport).should('not.exist');
cy.findByRole('button', { name: /avbryt/i }).click();
cy.findByRole('button', { name: /avbryt/i }).clickAndGone();
cy.findByRole('button', { name: /neste/i }).should('be.visible');
cy.get(appFrontend.errorReport).should('be.visible');
cy.get(appFrontend.fieldValidation('subform-mopeder')).should(
Expand All @@ -155,15 +155,15 @@ describe('Subform test', () => {
cy.findAllByRole('button', { name: /slett/i }).last().clickAndGone();

// Test that fixing the validations works
cy.findByRole('button', { name: /endre/i }).click();
cy.findByRole('button', { name: /endre/i }).clickAndGone();
cy.findByRole('button', { name: /ferdig/i }).should('be.visible');
cy.get(appFrontend.errorReport).should('be.visible');
cy.findByRole('textbox', { name: /registreringsnummer/i }).type('ABC123');
cy.findByRole('textbox', { name: /merke/i }).type('Digdir');
cy.findByRole('textbox', { name: /modell/i }).type('Scooter2000');
cy.findByRole('textbox', { name: /produksjonsår/i }).type('2024');
cy.get(appFrontend.errorReport).should('not.exist');
cy.findByRole('button', { name: /ferdig/i }).click();
cy.findByRole('button', { name: /ferdig/i }).clickAndGone();
cy.findByRole('button', { name: /neste/i }).should('be.visible');
cy.get(appFrontend.errorReport).should('not.exist');
});
Expand All @@ -172,19 +172,19 @@ describe('Subform test', () => {
cy.findByRole('textbox', { name: /navn/i }).type('Per');
cy.findByRole('textbox', { name: /alder/i }).type('28');

cy.findByRole('button', { name: /legg til moped/i }).click();
cy.findByRole('button', { name: /legg til moped/i }).clickAndGone();
cy.findByRole('textbox', { name: /registreringsnummer/i }).type('ABC123');
cy.findByRole('textbox', { name: /merke/i }).type('Digdir');
cy.findByRole('textbox', { name: /modell/i }).type('Scooter2000');
cy.findByRole('textbox', { name: /produksjonsår/i }).type('2024');
cy.findByRole('button', { name: /ferdig/i }).click();
cy.findByRole('button', { name: /ferdig/i }).clickAndGone();

cy.findByRole('button', { name: /legg til moped/i }).click();
cy.findByRole('button', { name: /legg til moped/i }).clickAndGone();
cy.findByRole('textbox', { name: /registreringsnummer/i }).type('XYZ987');
cy.findByRole('textbox', { name: /merke/i }).type('Altinn');
cy.findByRole('textbox', { name: /modell/i }).type('3.0');
cy.findByRole('textbox', { name: /produksjonsår/i }).type('2030');
cy.findByRole('button', { name: /ferdig/i }).click();
cy.findByRole('button', { name: /ferdig/i }).clickAndGone();

cy.testPdf({
snapshotName: 'subform',
Expand Down
Loading