From 8a6396ba2ad1fd8bbc57b78a1bef7b398dc7b21e Mon Sep 17 00:00:00 2001 From: Sebastian Leidig Date: Tue, 2 Apr 2024 19:28:51 +0200 Subject: [PATCH] fix(forms): reset anonymized fields immediately fixes #2285 --- .../entity-form/entity-form.service.spec.ts | 21 +++++++++++++++++++ .../entity-form/entity-form.service.ts | 9 ++++++-- .../entity-anonymize.service.spec.ts | 20 +++++++++++++----- .../entity-anonymize.service.ts | 4 +++- .../schema/entity-schema.service.spec.ts | 19 +++++++++++++++++ .../entity/schema/entity-schema.service.ts | 14 +++++++++++-- 6 files changed, 77 insertions(+), 10 deletions(-) diff --git a/src/app/core/common-components/entity-form/entity-form.service.spec.ts b/src/app/core/common-components/entity-form/entity-form.service.spec.ts index 2f6a0e3b0c..723e2509e5 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.spec.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.spec.ts @@ -29,6 +29,8 @@ import { User } from "../../user/user"; import { TEST_USER } from "../../user/demo-user-generator.service"; import { CurrentUserSubject } from "../../session/current-user-subject"; import moment from "moment"; +import { EntityMapperService } from "../../entity/entity-mapper/entity-mapper.service"; +import { MockEntityMapperService } from "../../entity/entity-mapper/mock-entity-mapper-service"; describe("EntityFormService", () => { let service: EntityFormService; @@ -306,6 +308,25 @@ describe("EntityFormService", () => { Entity.schema.delete("test"); }); + it("should not save 'null' as value from empty form fields", async () => { + Entity.schema.set("test", { dataType: "string" }); + + const entity = new Entity(); + const form = service.createFormGroup([{ id: "test" }], entity); + form.get("test").reset(); + expect(form.get("test").getRawValue()).toEqual(null); + + await service.saveChanges(form, entity); + + const entityMapper = TestBed.inject( + EntityMapperService, + ) as MockEntityMapperService; + const actualSaved = entityMapper.get(entity.getType(), entity.getId()); + expect(actualSaved).toEqual(entity); + // service should remove 'null' value, which are the default for empty form fields + expect(actualSaved["test"]).not.toEqual(null); + }); + it("should add column definitions from property schema", () => { class Test extends Child { @DatabaseField({ diff --git a/src/app/core/common-components/entity-form/entity-form.service.ts b/src/app/core/common-components/entity-form/entity-form.service.ts index 9946d53330..779ccc4d29 100644 --- a/src/app/core/common-components/entity-form/entity-form.service.ts +++ b/src/app/core/common-components/entity-form/entity-form.service.ts @@ -237,10 +237,15 @@ export class EntityFormService { entity: T, ): Promise { this.checkFormValidity(form); + const updatedEntity = entity.copy() as T; - Object.assign(updatedEntity, form.getRawValue()); - updatedEntity.assertValid(); + for (const [key, value] of Object.entries(form.getRawValue())) { + if (value !== null) { + updatedEntity[key] = value; + } + } + updatedEntity.assertValid(); this.assertPermissionsToSave(entity, updatedEntity); return this.entityMapper diff --git a/src/app/core/entity/entity-actions/entity-anonymize.service.spec.ts b/src/app/core/entity/entity-actions/entity-anonymize.service.spec.ts index 1befc74733..ee109b7203 100644 --- a/src/app/core/entity/entity-actions/entity-anonymize.service.spec.ts +++ b/src/app/core/entity/entity-actions/entity-anonymize.service.spec.ts @@ -98,7 +98,7 @@ describe("EntityAnonymizeService", () => { } } - it("should anonymize and only keep properties marked to be retained", async () => { + it("should anonymize and only keep properties marked to be retained, setting others to 'null'", async () => { const entity = new AnonymizableEntity(); entity.defaultField = "test"; entity.retainedField = "test"; @@ -107,13 +107,12 @@ describe("EntityAnonymizeService", () => { AnonymizableEntity.expectAnonymized( entity.getId(), - AnonymizableEntity.create({ retainedField: "test" }), + AnonymizableEntity.create({ retainedField: "test", defaultField: null }), ); }); it("should anonymize and keep empty record without any fields", async () => { const entity = new AnonymizableEntity(); - entity.defaultField = "test"; await service.anonymizeEntity(entity); @@ -140,6 +139,7 @@ describe("EntityAnonymizeService", () => { AnonymizableEntity.create({ inactive: true, anonymized: true, + defaultField: null, ...entityProperties, }), true, @@ -154,7 +154,11 @@ describe("EntityAnonymizeService", () => { AnonymizableEntity.expectAnonymized( entity.getId(), - AnonymizableEntity.create({ inactive: true, anonymized: true }), + AnonymizableEntity.create({ + inactive: true, + anonymized: true, + defaultField: null, + }), true, ); }); @@ -187,7 +191,7 @@ describe("EntityAnonymizeService", () => { AnonymizableEntity.expectAnonymized( entity.getId(), - AnonymizableEntity.create({}), + AnonymizableEntity.create({ file: null }), ); expect(mockFileService.removeFile).toHaveBeenCalled(); }); @@ -234,6 +238,12 @@ describe("EntityAnonymizeService", () => { expectedAnonymizedEntity.inactive = true; expectedAnonymizedEntity.anonymized = true; + for (const [k, v] of Object.entries(actualEntity)) { + if (v === null) { + expectedAnonymizedEntity[k] = null; + } + } + expect(comparableEntityData(actualEntity)).toEqual( comparableEntityData(expectedAnonymizedEntity), ); diff --git a/src/app/core/entity/entity-actions/entity-anonymize.service.ts b/src/app/core/entity/entity-actions/entity-anonymize.service.ts index 731e5757e6..252f20cf21 100644 --- a/src/app/core/entity/entity-actions/entity-anonymize.service.ts +++ b/src/app/core/entity/entity-actions/entity-anonymize.service.ts @@ -92,7 +92,9 @@ export class EntityAnonymizeService extends CascadingEntityAction { await firstValueFrom(this.fileService.removeFile(entity, key)); } - delete entity[key]; + if (entity[key] !== undefined) { + entity[key] = null; + } } private async keepEntityUnchanged(e: Entity): Promise { diff --git a/src/app/core/entity/schema/entity-schema.service.spec.ts b/src/app/core/entity/schema/entity-schema.service.spec.ts index bd5f59455d..3868265e62 100644 --- a/src/app/core/entity/schema/entity-schema.service.spec.ts +++ b/src/app/core/entity/schema/entity-schema.service.spec.ts @@ -58,6 +58,25 @@ describe("EntitySchemaService", () => { expect(rawData.aString).toEqual("192"); }); + it("should keep 'null' as value if explicitly set", () => { + class TestEntity extends Entity { + @DatabaseField() aString: string; + } + + const entity = new TestEntity(); + + const data = { + _id: entity.getId(), + aString: null, + }; + service.loadDataIntoEntity(entity, data); + + expect(entity.aString).toEqual(null); + + const rawData = service.transformEntityToDatabaseFormat(entity); + expect(rawData.aString).toEqual(null); + }); + it("should return the directly defined component name for viewing and editing a property", () => { class Test extends Entity { @DatabaseField({ diff --git a/src/app/core/entity/schema/entity-schema.service.ts b/src/app/core/entity/schema/entity-schema.service.ts index 4fe5075420..4a5ad4a7e8 100644 --- a/src/app/core/entity/schema/entity-schema.service.ts +++ b/src/app/core/entity/schema/entity-schema.service.ts @@ -104,7 +104,7 @@ export class EntitySchemaService { for (const key of schema.keys()) { const schemaField: EntitySchemaField = schema.get(key); - if (data[key] === undefined || data[key] === null) { + if (data[key] === undefined) { continue; } @@ -153,7 +153,7 @@ export class EntitySchemaService { let value = entity[key]; const schemaField: EntitySchemaField = schema.get(key); - if (value === undefined || value === null) { + if (value === undefined) { // skip and keep undefined continue; } @@ -218,6 +218,11 @@ export class EntitySchemaService { schemaField: EntitySchemaField, entity?: Entity, ) { + if (value === null) { + // keep 'null' to be able to explicitly mark a value as being reset + return null; + } + return this.getDatatypeOrDefault( schemaField.dataType, ).transformToDatabaseFormat(value, schemaField, entity); @@ -234,6 +239,11 @@ export class EntitySchemaService { schemaField: EntitySchemaField, dataObject?: any, ) { + if (value === null) { + // keep 'null' to be able to explicitly mark a value as being reset + return null; + } + return this.getDatatypeOrDefault( schemaField.dataType, ).transformToObjectFormat(value, schemaField, dataObject);