From 042d2e71f0e01cb39fbbd49b996ca3d7da449597 Mon Sep 17 00:00:00 2001 From: Alessandro Martelli Date: Tue, 15 Dec 2020 11:38:46 +0100 Subject: [PATCH] [835] Auto-save in new Item Submission form breaks the form Improved form touched state in ngrx store. --- .../form/builder/form-builder.service.ts | 2 +- src/app/shared/form/form.actions.ts | 25 ++++---- src/app/shared/form/form.component.spec.ts | 2 +- src/app/shared/form/form.reducer.spec.ts | 62 +++++++------------ src/app/shared/form/form.reducer.ts | 35 +++++------ src/app/shared/form/form.service.spec.ts | 2 +- src/app/shared/form/form.service.ts | 16 ++--- .../submission-objects.effects.spec.ts | 6 +- .../objects/submission-objects.effects.ts | 14 +++-- 9 files changed, 75 insertions(+), 89 deletions(-) diff --git a/src/app/shared/form/builder/form-builder.service.ts b/src/app/shared/form/builder/form-builder.service.ts index a6100e7ce7..c621fe2aa7 100644 --- a/src/app/shared/form/builder/form-builder.service.ts +++ b/src/app/shared/form/builder/form-builder.service.ts @@ -384,7 +384,7 @@ export class FormBuilderService extends DynamicFormService { const result = iterateControlModels([model]); - return result; + return Object.keys(result); } } diff --git a/src/app/shared/form/form.actions.ts b/src/app/shared/form/form.actions.ts index 76d9fef253..5733cb90bc 100644 --- a/src/app/shared/form/form.actions.ts +++ b/src/app/shared/form/form.actions.ts @@ -13,7 +13,7 @@ import { type } from '../ngrx/type'; export const FormActionTypes = { FORM_INIT: type('dspace/form/FORM_INIT'), FORM_CHANGE: type('dspace/form/FORM_CHANGE'), - FORM_ADDITIONAL: type('dspace/form/FORM_ADDITIONAL'), + FORM_ADD_TOUCHED: type('dspace/form/FORM_ADD_TOUCHED'), FORM_REMOVE: type('dspace/form/FORM_REMOVE'), FORM_STATUS_CHANGE: type('dspace/form/FORM_STATUS_CHANGE'), FORM_ADD_ERROR: type('dspace/form/FORM_ADD_ERROR'), @@ -28,7 +28,6 @@ export class FormInitAction implements Action { formId: string; formData: any; valid: boolean; - formAdditional: any; }; /** @@ -41,8 +40,8 @@ export class FormInitAction implements Action { * @param valid * the Form validation status */ - constructor(formId: string, formData: any, valid: boolean, formAdditional?: any) { - this.payload = {formId, formData, valid, formAdditional}; + constructor(formId: string, formData: any, valid: boolean) { + this.payload = {formId, formData, valid}; } } @@ -66,23 +65,23 @@ export class FormChangeAction implements Action { } } -export class FormSetAdditionalAction implements Action { - type = FormActionTypes.FORM_ADDITIONAL; +export class FormAddTouchedAction implements Action { + type = FormActionTypes.FORM_ADD_TOUCHED; payload: { formId: string; - additionalData: any; + touched: string[]; }; /** - * Create a new FormSetAdditionalAction + * Create a new FormAddTouchedAction * * @param formId * the Form's ID - * @param additionalData - * the additionalData Object + * @param touched + * the array containing new touched fields */ - constructor(formId: string, additionalData: any) { - this.payload = {formId, additionalData}; + constructor(formId: string, touched: string[]) { + this.payload = {formId, touched}; } } @@ -169,7 +168,7 @@ export class FormClearErrorsAction implements Action { */ export type FormAction = FormInitAction | FormChangeAction - | FormSetAdditionalAction + | FormAddTouchedAction | FormRemoveAction | FormStatusChangeAction | FormAddError diff --git a/src/app/shared/form/form.component.spec.ts b/src/app/shared/form/form.component.spec.ts index e7193246fb..bda2b3e38c 100644 --- a/src/app/shared/form/form.component.spec.ts +++ b/src/app/shared/form/form.component.spec.ts @@ -120,7 +120,7 @@ function init() { }, valid: false, errors: [], - additional: {} + touched: {} } }; diff --git a/src/app/shared/form/form.reducer.spec.ts b/src/app/shared/form/form.reducer.spec.ts index ff89ae27cb..5547eee7a4 100644 --- a/src/app/shared/form/form.reducer.spec.ts +++ b/src/app/shared/form/form.reducer.spec.ts @@ -6,7 +6,7 @@ import { FormInitAction, FormRemoveAction, FormRemoveErrorAction, - FormSetAdditionalAction, + FormAddTouchedAction, FormStatusChangeAction } from './form.actions'; @@ -23,7 +23,7 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: {} + touched: {} } }; const formId = 'testForm'; @@ -51,7 +51,7 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: {} + touched: {} } }; const formId = 'testForm'; @@ -71,7 +71,7 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: {} + touched: {} } }; @@ -93,7 +93,7 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: {} + touched: {} } }; const state = { @@ -106,7 +106,7 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: {} + touched: {} } }; const formId = 'testForm'; @@ -134,7 +134,7 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: {} + touched: {} } }; const state = { @@ -147,7 +147,7 @@ describe('formReducer', () => { }, valid: true, errors: [], - additional: {} + touched: {} } }; const formId = 'testForm'; @@ -169,7 +169,7 @@ describe('formReducer', () => { }, valid: true, errors: [], - additional: {} + touched: {} } }; @@ -214,7 +214,7 @@ describe('formReducer', () => { message: 'error.validation.required' } ], - additional: {} + touched: {} } }; @@ -247,7 +247,7 @@ describe('formReducer', () => { }, valid: true, errors: [], - additional: {} + touched: {} } }; @@ -276,7 +276,7 @@ describe('formReducer', () => { message: 'error.validation.required' } ], - additional: {} + touched: {} } }; @@ -299,7 +299,7 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: {} + touched: {} } }; const state = { @@ -312,21 +312,15 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: { - touched: { - title: true - } + touched: { + title: true } } }; const formId = 'testForm'; - const additionalData = { - touched: { - title: true - } - }; + const touched = ['title']; - const action = new FormSetAdditionalAction(formId, additionalData); + const action = new FormAddTouchedAction(formId, touched); const newState = formReducer(initState, action); expect(newState).toEqual(state); @@ -343,10 +337,8 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: { - touched: { - title: true - } + touched: { + title: true } } }; @@ -360,22 +352,16 @@ describe('formReducer', () => { }, valid: false, errors: [], - additional: { - touched: { - title: true, - author: true - } + touched: { + title: true, + author: true } } }; const formId = 'testForm'; - const additionalData = { - touched: { - author: true - } - }; + const touched = ['author']; - const action = new FormSetAdditionalAction(formId, additionalData); + const action = new FormAddTouchedAction(formId, touched); const newState = formReducer(initState, action); expect(newState).toEqual(state); diff --git a/src/app/shared/form/form.reducer.ts b/src/app/shared/form/form.reducer.ts index 2a2b3cd2de..b5b4090ef9 100644 --- a/src/app/shared/form/form.reducer.ts +++ b/src/app/shared/form/form.reducer.ts @@ -4,9 +4,8 @@ import { FormAddError, FormChangeAction, FormClearErrorsAction, FormInitAction, - FormRemoveAction, - FormRemoveErrorAction, FormSetAdditionalAction, - FormStatusChangeAction + FormRemoveAction, FormRemoveErrorAction, + FormStatusChangeAction, FormAddTouchedAction } from './form.actions'; import { hasValue } from '../empty.util'; import { isEqual, uniqWith } from 'lodash'; @@ -17,11 +16,15 @@ export interface FormError { fieldIndex: number; } +export interface FormTouchedState { + [key: string]: boolean +} + export interface FormEntry { data: any; valid: boolean; errors: FormError[]; - additional: any; + touched: FormTouchedState; } export interface FormState { @@ -41,8 +44,8 @@ export function formReducer(state = initialState, action: FormAction): FormState return changeDataForm(state, action as FormChangeAction); } - case FormActionTypes.FORM_ADDITIONAL: { - return additionalData(state, action as FormSetAdditionalAction); + case FormActionTypes.FORM_ADD_TOUCHED: { + return changeTouchedState(state, action as FormAddTouchedAction); } case FormActionTypes.FORM_REMOVE: { @@ -132,8 +135,8 @@ function initForm(state: FormState, action: FormInitAction): FormState { const formState = { data: action.payload.formData, valid: action.payload.valid, + touched: {}, errors: [], - additional: action.payload.formAdditional ? action.payload.formAdditional : {} }; if (!hasValue(state[action.payload.formId])) { return Object.assign({}, state, { @@ -220,25 +223,19 @@ function removeForm(state: FormState, action: FormRemoveAction): FormState { } /** - * Compute the additional data state of the form. New touched fields are merged with the previous ones. + * Compute the touched state of the form. New touched fields are merged with the previous ones. * @param state * @param action */ -function additionalData(state: FormState, action: FormSetAdditionalAction): FormState { +function changeTouchedState(state: FormState, action: FormAddTouchedAction): FormState { if (hasValue(state[action.payload.formId])) { - const newState = Object.assign({}, state); - const newAdditional = newState[action.payload.formId].additional ? {...newState[action.payload.formId].additional} : {}; + const newForm = Object.assign({}, newState[action.payload.formId]); + newState[action.payload.formId] = newForm; - const newTouchedValue = newAdditional.touched ? {...newAdditional.touched, - ...action.payload.additionalData.touched} : { ...action.payload.additionalData.touched}; - newAdditional.touched = newTouchedValue; - - newState[action.payload.formId] = Object.assign({}, newState[action.payload.formId], { - additional: newAdditional - } - ); + newForm.touched = { ... newForm.touched}; + action.payload.touched.forEach((field) => newForm[field] = true); return newState; } else { diff --git a/src/app/shared/form/form.service.spec.ts b/src/app/shared/form/form.service.spec.ts index 031c83e457..a0f7f9980b 100644 --- a/src/app/shared/form/form.service.spec.ts +++ b/src/app/shared/form/form.service.spec.ts @@ -85,7 +85,7 @@ describe('FormService test suite', () => { data: formData, valid: false, errors: [], - additional: {} + touched: {} } }; diff --git a/src/app/shared/form/form.service.ts b/src/app/shared/form/form.service.ts index 8983bade44..b895a188a4 100644 --- a/src/app/shared/form/form.service.ts +++ b/src/app/shared/form/form.service.ts @@ -1,5 +1,5 @@ import { map, distinctUntilChanged, filter } from 'rxjs/operators'; -import { Inject, Injectable } from '@angular/core'; +import { Injectable } from '@angular/core'; import { AbstractControl, FormArray, FormControl, FormGroup } from '@angular/forms'; import { Observable } from 'rxjs'; import { select, Store } from '@ngrx/store'; @@ -7,16 +7,16 @@ import { select, Store } from '@ngrx/store'; import { AppState } from '../../app.reducer'; import { formObjectFromIdSelector } from './selectors'; import { FormBuilderService } from './builder/form-builder.service'; -import {DynamicFormControlEvent, DynamicFormControlModel} from '@ng-dynamic-forms/core'; +import { DynamicFormControlEvent, DynamicFormControlModel } from '@ng-dynamic-forms/core'; import { isEmpty, isNotUndefined } from '../empty.util'; import { uniqueId } from 'lodash'; import { FormChangeAction, FormInitAction, - FormRemoveAction, FormRemoveErrorAction, FormSetAdditionalAction, + FormRemoveAction, FormRemoveErrorAction, FormAddTouchedAction, FormStatusChangeAction } from './form.actions'; -import { FormEntry } from './form.reducer'; +import { FormEntry, FormTouchedState } from './form.reducer'; import { environment } from '../../../environments/environment'; @Injectable() @@ -52,13 +52,13 @@ export class FormService { } /** - * Method to retrieve form's additional data from state + * Method to retrieve form's touched state */ - public getFormAdditionalData(formId: string): Observable { + public getFormTouchedState(formId: string): Observable { return this.store.pipe( select(formObjectFromIdSelector(formId)), filter((state) => isNotUndefined(state)), - map((state) => state.additional), + map((state) => state.touched), distinctUntilChanged() ); } @@ -183,7 +183,7 @@ export class FormService { public setTouched(formId: string, model: DynamicFormControlModel[], event: DynamicFormControlEvent) { const ids = this.formBuilderService.getMetadataIdsFromEvent(event); - this.store.dispatch(new FormSetAdditionalAction(formId, { touched: ids})); + this.store.dispatch(new FormAddTouchedAction(formId, ids)); } public removeError(formId: string, eventModelId: string, fieldIndex: number) { diff --git a/src/app/submission/objects/submission-objects.effects.spec.ts b/src/app/submission/objects/submission-objects.effects.spec.ts index 0f0f2aa7d7..477a84531e 100644 --- a/src/app/submission/objects/submission-objects.effects.spec.ts +++ b/src/app/submission/objects/submission-objects.effects.spec.ts @@ -384,10 +384,8 @@ describe('SubmissionObjectEffects test suite', () => { }, forms: { '2_traditionalpageone': { - additional: { - touched: { - 'dc.title': true - } + touched: { + 'dc.title': true } } } diff --git a/src/app/submission/objects/submission-objects.effects.ts b/src/app/submission/objects/submission-objects.effects.ts index 9e2db13776..a4b3a94ff8 100644 --- a/src/app/submission/objects/submission-objects.effects.ts +++ b/src/app/submission/objects/submission-objects.effects.ts @@ -59,6 +59,7 @@ import { getFirstSucceededRemoteDataPayload } from '../../core/shared/operators' import { SubmissionObjectDataService } from '../../core/submission/submission-object-data.service'; import { followLink } from '../../shared/utils/follow-link-config.model'; import parseSectionErrorPaths, {SectionErrorPath} from '../utils/parseSectionErrorPaths'; +import { FormState } from '../../shared/form/form.reducer'; @Injectable() export class SubmissionObjectEffects { @@ -431,24 +432,29 @@ function getForm(forms, currentState, sectionId) { /** * Filter sectionErrors accordingly to this rules: * 1. if notifications are enabled return all errors - * 2. if sectionType is different from submission-form return all errors + * 2. if sectionType is different from 'submission-form' return all errors * 3. otherwise return errors only for those fields marked as touched inside the section form * @param sectionForm + * The form related to the section * @param sectionErrors + * The section errors array + * @param sectionType + * The section type * @param notify + * Whether notifications are enabled */ -function filterErrors(sectionForm, sectionErrors, sectionType, notify): any { +function filterErrors(sectionForm: FormState, sectionErrors: SubmissionSectionError[], sectionType: string, notify: boolean): any { if (notify || sectionType !== SectionsType.SubmissionForm) { return sectionErrors; } - if (!sectionForm || !sectionForm.additional || !sectionForm.additional.touched) { + if (!sectionForm || !sectionForm.touched) { return []; } const filteredErrors = []; sectionErrors.forEach((error: SubmissionSectionError) => { const errorPaths: SectionErrorPath[] = parseSectionErrorPaths(error.path); errorPaths.forEach((path: SectionErrorPath) => { - if (path.fieldId && sectionForm.additional.touched[path.fieldId]) { + if (path.fieldId && sectionForm.touched[path.fieldId]) { filteredErrors.push(error); } });