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

fix(DataModel): Enables the deletion of a child node from a definition that is currently in use #12235

Merged
merged 11 commits into from
Feb 2, 2024
1 change: 1 addition & 0 deletions frontend/language/src/nb.json
Original file line number Diff line number Diff line change
Expand Up @@ -1068,6 +1068,7 @@
"schema_editor.custom_props_help": "Dette er egenskaper som ikke støttes av Altinn Studio, men som er satt inn på datamodellen ved hjelp av andre verktøy. Her kan man slette egenskepene eller endre deres verdier, men det er ikke mulig å legge til nye. Vær oppmerksom på at egenskaper som slettes fra denne listen ikke kan legges til igjen ved hjelp av Altinn Studio.",
"schema_editor.custom_props_unknown_format": "Ukjent format",
"schema_editor.custom_props_unknown_format_help": "Denne egenskapen har et format som ikke kan forhåndsvises eller redigeres i Altinn Studio.",
"schema_editor.datamodel_definition_field_deletion_text": "Feltet du vil slette tilhører en aktiv type og vil påvirke alle tilknyttede områder. Er du sikker på slettingen?",
"schema_editor.datamodel_field_deletion_confirm": "Ja, slett feltet",
"schema_editor.datamodel_field_deletion_info": "Forsikre deg om at feltet ikke er knyttet til noen oppsett eller regler før du sletter det.",
"schema_editor.datamodel_field_deletion_text": "Er du sikker på at du vil slette dette feltet?",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ const useDeleteNode = (pointer: string, savableModel: SavableSchemaModel) => {
const { setSelectedNodePointer } = useSchemaEditorAppContext();

return useCallback(() => {
if (confirm(t('schema_editor.datamodel_field_deletion_text'))) {
const confirmMessage = savableModel.areDefinitionParentsInUse(pointer)
? t('schema_editor.datamodel_definition_field_deletion_text')
: t('schema_editor.datamodel_field_deletion_text');
if (confirm(confirmMessage)) {
setSelectedNodePointer(null);
savableModel.deleteNode(pointer);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
import {
definitionNodeMock,
objectNodeMock,
referenceNodeMock,
referredNodeMock,
uiSchemaNodesMock,
} from '../../../../test/mocks/uiSchemaMock';
Expand Down Expand Up @@ -101,6 +102,17 @@ describe('SchemaNode', () => {
expect(deleteButton).toBeEnabled();
});

it('Enables the deletion of a child node from a definition that is currently in use', async () => {
const { pointer } = referenceNodeMock;
const save = jest.fn();
render({ pointer, save });

const deleteButton = screen.getByRole('button', { name: textMock('general.delete') });
jest.spyOn(window, 'confirm').mockImplementation(() => true);
await act(() => user.click(deleteButton));
expect(save).toHaveBeenCalledTimes(1);
});

it('Saves the model correctly when a node is deleted', async () => {
const { pointer } = objectNodeMock;
const schemaModel = setupSchemaModel();
Expand Down
27 changes: 15 additions & 12 deletions frontend/packages/schema-model/src/lib/SchemaModel.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
combinationNodeWithMultipleChildrenMock,
defNodeMock,
defNodeWithChildrenChildMock,
defNodeWithChildrenGrandchildMock,
defNodeWithChildrenMock,
enumNodeMock,
nodeWithSameNameAsStringNodeMock,
Expand Down Expand Up @@ -221,6 +220,18 @@ describe('SchemaModel', () => {
});
});

describe('areDefinitionParentsInUse', () => {
it('Returns false if definition parent not in use', () => {
const result = schemaModel.areDefinitionParentsInUse(unusedDefinitionMock.pointer);
expect(result).toBeFalsy();
});

it('Returns true if definition parent is in use', () => {
const result = schemaModel.areDefinitionParentsInUse(defNodeWithChildrenChildMock.pointer);
expect(result).toBeTruthy();
});
});

describe('getFinalNode', () => {
it('Returns the node itself when it is not a reference', () => {
const result = schemaModel.getFinalNode(parentNodeMock.pointer);
Expand Down Expand Up @@ -696,18 +707,10 @@ describe('SchemaModel', () => {
expect(model.asArray()).toEqual(schemaModel.asArray());
});

it('Throws an error and keeps the model unchanged if trying to delete a definition node that is in use', () => {
it('Should not throws an error if trying to delete a child node of a definition in use', () => {
framitdavid marked this conversation as resolved.
Show resolved Hide resolved
const model = schemaModel.deepClone();
expect(() => model.deleteNode(defNodeMock.pointer)).toThrowError();
expect(() => model.deleteNode(defNodeWithChildrenMock.pointer)).toThrowError();
expect(model.asArray()).toEqual(schemaModel.asArray());
});

it('Throws an error and keeps the model unchanged if trying to delete a child node of a definition in use', () => {
const model = schemaModel.deepClone();
expect(() => model.deleteNode(defNodeWithChildrenChildMock.pointer)).toThrowError();
expect(() => model.deleteNode(defNodeWithChildrenGrandchildMock.pointer)).toThrowError();
expect(model.asArray()).toEqual(schemaModel.asArray());
expect(() => model.deleteNode(defNodeWithChildrenChildMock.pointer)).not.toThrowError();
expect(model.asArray()).not.toEqual(schemaModel.asArray());
});
});

Expand Down
20 changes: 14 additions & 6 deletions frontend/packages/schema-model/src/lib/SchemaModel.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import type { NodePosition, UiSchemaNode, UiSchemaNodes } from '../types';
import { CombinationKind, FieldType } from '../types';
import {
CombinationKind,
FieldType,
type NodePosition,
type UiSchemaNode,
type UiSchemaNodes,
} from '../types';
import type { FieldNode } from '../types/FieldNode';
import type { CombinationNode } from '../types/CombinationNode';
import type { NodeMap } from '../types/NodeMap';
Expand Down Expand Up @@ -417,6 +422,7 @@ export class SchemaModel {
referringNodes.push(node);
}
}

return referringNodes;
}

Expand All @@ -432,9 +438,10 @@ export class SchemaModel {
}

public deleteNode(pointer: string): SchemaModel {
if (pointer === ROOT_POINTER) throw new Error('It is not possible to delete the root node.');
if (this.isDefinitionInUse(pointer))
throw new Error('Cannot delete a definition that is in use.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hva skjer nå hvis man kaller deleteNode på en type som er i bruk? Kunne det vært bedre å bytte ut denne med hasReferringNodes i stedet for å fjerne den helt?

Copy link
Collaborator Author

@framitdavid framitdavid Feb 2, 2024

Choose a reason for hiding this comment

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

Hva skjer nå hvis man kaller deleteNode på en type som er i bruk? Kunne det vært bedre å bytte ut denne med hasReferringNodes i stedet for å fjerne den helt?

Godt forslag. Jeg gjør et forsøk på det på mandag. Usikker på om det potensielt kan stoppe oss for å kunne slette noden på typen også?

Copy link
Contributor

@TomasEng TomasEng Feb 5, 2024

Choose a reason for hiding this comment

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

Det skal ikke være mulig. hasReferringNodes sjekker kun elementet som det pekes på, mens isDefinitionInUse er rekursiv nettopp fordi den også sjekker oppover i treet. Egentlig tror jeg ikke vi trenger isDefinitionInUse i det hele tatt.

if (pointer === ROOT_POINTER) {
throw new Error('It is not possible to delete the root node.');
}
This conversation was marked as resolved.
Show resolved Hide resolved

return this.deleteNodeWithChildrenRecursively(pointer);
}

Expand All @@ -448,6 +455,7 @@ export class SchemaModel {
private isDefinitionInUse(pointer: string): boolean {
const node = this.getNode(pointer);
if (!isDefinition(node)) return false;

return this.hasReferringNodes(pointer) || this.areDefinitionParentsInUse(pointer);
}

Expand All @@ -456,7 +464,7 @@ export class SchemaModel {
return !!referringNodes.length;
}

private areDefinitionParentsInUse(pointer: string): boolean {
public areDefinitionParentsInUse(pointer: string): boolean {
const parent = this.getParentNode(pointer);
return this.isDefinitionInUse(parent.pointer);
}
Expand Down
Loading