From 5e17a4e958db858bc7eee8c4806ee51ce46ff00b Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Tue, 30 Nov 2021 13:27:16 +0100 Subject: [PATCH 1/4] 85262: Always enable deposit button --- .../submission-form-footer.component.html | 1 - .../submission-form-footer.component.spec.ts | 9 ---- .../submission-objects.effects.spec.ts | 53 ------------------- .../objects/submission-objects.effects.ts | 4 +- 4 files changed, 1 insertion(+), 66 deletions(-) diff --git a/src/app/submission/form/footer/submission-form-footer.component.html b/src/app/submission/form/footer/submission-form-footer.component.html index 4964eb56a2..203f7e2afa 100644 --- a/src/app/submission/form/footer/submission-form-footer.component.html +++ b/src/app/submission/form/footer/submission-form-footer.component.html @@ -41,7 +41,6 @@ diff --git a/src/app/submission/form/footer/submission-form-footer.component.spec.ts b/src/app/submission/form/footer/submission-form-footer.component.spec.ts index dd47dad444..dbeb4ee00d 100644 --- a/src/app/submission/form/footer/submission-form-footer.component.spec.ts +++ b/src/app/submission/form/footer/submission-form-footer.component.spec.ts @@ -201,15 +201,6 @@ describe('SubmissionFormFooterComponent Component', () => { }); }); - it('should have deposit button disabled when submission is not valid', () => { - comp.showDepositAndDiscard = observableOf(true); - compAsAny.submissionIsInvalid = observableOf(true); - fixture.detectChanges(); - const depositBtn: any = fixture.debugElement.query(By.css('.btn-success')); - - expect(depositBtn.nativeElement.disabled).toBeTruthy(); - }); - it('should not have deposit button disabled when submission is valid', () => { comp.showDepositAndDiscard = observableOf(true); compAsAny.submissionIsInvalid = observableOf(false); diff --git a/src/app/submission/objects/submission-objects.effects.spec.ts b/src/app/submission/objects/submission-objects.effects.spec.ts index 122ebd90ac..8e763fd94b 100644 --- a/src/app/submission/objects/submission-objects.effects.spec.ts +++ b/src/app/submission/objects/submission-objects.effects.spec.ts @@ -879,59 +879,6 @@ describe('SubmissionObjectEffects test suite', () => { expect(submissionObjectEffects.saveAndDeposit$).toBeObservable(expected); }); - it('should not allow to deposit when there are errors', () => { - store.nextState({ - submission: { - objects: submissionState - } - } as any); - - actions = hot('--a-', { - a: { - type: SubmissionObjectActionTypes.SAVE_AND_DEPOSIT_SUBMISSION, - payload: { - submissionId: submissionId - } - } - }); - - const response = [Object.assign({}, mockSubmissionRestResponse[0], { - sections: mockSectionsData, - errors: mockSectionsErrors - })]; - - submissionJsonPatchOperationsServiceStub.jsonPatchByResourceType.and.returnValue(observableOf(response)); - - const errorsList = parseSectionErrors(mockSectionsErrors); - const expected = cold('--b-', { - b: [ - new UpdateSectionDataAction( - submissionId, - 'traditionalpageone', - mockSectionsData.traditionalpageone as any, - errorsList.traditionalpageone || [], - errorsList.traditionalpageone || [] - ), - new UpdateSectionDataAction( - submissionId, - 'license', - mockSectionsData.license as any, - errorsList.license || [], - errorsList.license || [] - ), - new UpdateSectionDataAction( - submissionId, - 'upload', - mockSectionsData.upload as any, - errorsList.upload || [], - errorsList.upload || [] - ) - ] - }); - - expect(submissionObjectEffects.saveAndDeposit$).toBeObservable(expected); - }); - it('should catch errors and return a SAVE_SUBMISSION_FORM_ERROR', () => { actions = hot('--a-', { a: { diff --git a/src/app/submission/objects/submission-objects.effects.ts b/src/app/submission/objects/submission-objects.effects.ts index b4ba1c2480..535e662922 100644 --- a/src/app/submission/objects/submission-objects.effects.ts +++ b/src/app/submission/objects/submission-objects.effects.ts @@ -206,9 +206,7 @@ export class SubmissionObjectEffects { if (this.canDeposit(response)) { return new DepositSubmissionAction(action.payload.submissionId); } else { - this.notificationsService.warning(null, this.translate.get('submission.sections.general.sections_not_valid')); - return this.parseSaveResponse((currentState.submission as SubmissionState).objects[action.payload.submissionId], - response, action.payload.submissionId, currentState.forms); + return new SaveSubmissionFormSuccessAction(action.payload.submissionId, response); } }), catchError(() => observableOf(new SaveSubmissionFormErrorAction(action.payload.submissionId)))); From 99af22b6216cfd67fffffc0ab5f7f7883584ab1b Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Tue, 30 Nov 2021 13:44:08 +0100 Subject: [PATCH 2/4] 85262: Re-disable deposit button during save/deposit --- .../submission/form/footer/submission-form-footer.component.html | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/submission/form/footer/submission-form-footer.component.html b/src/app/submission/form/footer/submission-form-footer.component.html index 203f7e2afa..e954fab34c 100644 --- a/src/app/submission/form/footer/submission-form-footer.component.html +++ b/src/app/submission/form/footer/submission-form-footer.component.html @@ -41,6 +41,7 @@ From ed2c774d86b71ca09c3e286a448650311bf97aca Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Thu, 2 Dec 2021 12:39:07 +0100 Subject: [PATCH 3/4] 85262: Allow empty patch requests --- .../json-patch-operations.service.ts | 16 ++++++---- .../submission-form-footer.component.spec.ts | 9 ++++++ .../submission-objects.effects.spec.ts | 30 +++++++++++++++++++ .../objects/submission-objects.effects.ts | 3 +- 4 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/app/core/json-patch/json-patch-operations.service.ts b/src/app/core/json-patch/json-patch-operations.service.ts index c3363f4db4..f160376837 100644 --- a/src/app/core/json-patch/json-patch-operations.service.ts +++ b/src/app/core/json-patch/json-patch-operations.service.ts @@ -40,13 +40,15 @@ export abstract class JsonPatchOperationsService * observable of response */ - protected submitJsonPatchOperations(hrefObs: Observable, resourceType: string, resourceId?: string): Observable { + protected submitJsonPatchOperations(hrefObs: Observable, resourceType: string, resourceId?: string, allowEmptyRequest = false): Observable { const requestId = this.requestService.generateRequestId(); let startTransactionTime = null; - const [patchRequest$, emptyRequest$] = partition((request: PatchRequestDefinition) => isNotEmpty(request.body))(hrefObs.pipe( + const [patchRequest$, emptyRequest$] = partition((request: PatchRequestDefinition) => allowEmptyRequest || isNotEmpty(request.body))(hrefObs.pipe( mergeMap((endpointURL: string) => { return this.store.select(jsonPatchOperationsByResourceType(resourceType)).pipe( take(1), @@ -79,11 +81,11 @@ export abstract class JsonPatchOperationsService isEmpty(request.body)), + filter((request: PatchRequestDefinition) => !allowEmptyRequest && isEmpty(request.body)), tap(() => startTransactionTime = null), map(() => null)), patchRequest$.pipe( - filter((request: PatchRequestDefinition) => isNotEmpty(request.body)), + filter((request: PatchRequestDefinition) => allowEmptyRequest || isNotEmpty(request.body)), tap(() => this.store.dispatch(new StartTransactionPatchOperationsAction(resourceType, resourceId, startTransactionTime))), tap((request: PatchRequestDefinition) => this.requestService.send(request)), mergeMap(() => { @@ -141,16 +143,18 @@ export abstract class JsonPatchOperationsService * observable of response */ - public jsonPatchByResourceType(linkPath: string, scopeId: string, resourceType: string): Observable { + public jsonPatchByResourceType(linkPath: string, scopeId: string, resourceType: string, allowEmptyRequest = false): Observable { const href$ = this.halService.getEndpoint(linkPath).pipe( filter((href: string) => isNotEmpty(href)), distinctUntilChanged(), map((endpointURL: string) => this.getEndpointByIDHref(endpointURL, scopeId))); - return this.submitJsonPatchOperations(href$, resourceType); + return this.submitJsonPatchOperations(href$, resourceType, undefined, allowEmptyRequest); } /** diff --git a/src/app/submission/form/footer/submission-form-footer.component.spec.ts b/src/app/submission/form/footer/submission-form-footer.component.spec.ts index dbeb4ee00d..072f826deb 100644 --- a/src/app/submission/form/footer/submission-form-footer.component.spec.ts +++ b/src/app/submission/form/footer/submission-form-footer.component.spec.ts @@ -201,6 +201,15 @@ describe('SubmissionFormFooterComponent Component', () => { }); }); + it('should not have deposit button disabled when submission is not valid', () => { + comp.showDepositAndDiscard = observableOf(true); + compAsAny.submissionIsInvalid = observableOf(true); + fixture.detectChanges(); + const depositBtn: any = fixture.debugElement.query(By.css('.btn-success')); + + expect(depositBtn.nativeElement.disabled).toBeFalsy(); + }); + it('should not have deposit button disabled when submission is valid', () => { comp.showDepositAndDiscard = observableOf(true); compAsAny.submissionIsInvalid = observableOf(false); diff --git a/src/app/submission/objects/submission-objects.effects.spec.ts b/src/app/submission/objects/submission-objects.effects.spec.ts index 8e763fd94b..e760f71941 100644 --- a/src/app/submission/objects/submission-objects.effects.spec.ts +++ b/src/app/submission/objects/submission-objects.effects.spec.ts @@ -879,6 +879,36 @@ describe('SubmissionObjectEffects test suite', () => { expect(submissionObjectEffects.saveAndDeposit$).toBeObservable(expected); }); + it('should return a SAVE_SUBMISSION_FORM_SUCCESS action when there are errors', () => { + store.nextState({ + submission: { + objects: submissionState + } + } as any); + + actions = hot('--a-', { + a: { + type: SubmissionObjectActionTypes.SAVE_AND_DEPOSIT_SUBMISSION, + payload: { + submissionId: submissionId + } + } + }); + + const response = [Object.assign({}, mockSubmissionRestResponse[0], { + sections: mockSectionsData, + errors: mockSectionsErrors + })]; + + submissionJsonPatchOperationsServiceStub.jsonPatchByResourceType.and.returnValue(observableOf(response)); + + const expected = cold('--b-', { + b: new SaveSubmissionFormSuccessAction(submissionId, response as any[]) + }); + + expect(submissionObjectEffects.saveAndDeposit$).toBeObservable(expected); + }); + it('should catch errors and return a SAVE_SUBMISSION_FORM_ERROR', () => { actions = hot('--a-', { a: { diff --git a/src/app/submission/objects/submission-objects.effects.ts b/src/app/submission/objects/submission-objects.effects.ts index 535e662922..5432aa7a4c 100644 --- a/src/app/submission/objects/submission-objects.effects.ts +++ b/src/app/submission/objects/submission-objects.effects.ts @@ -201,7 +201,8 @@ export class SubmissionObjectEffects { return this.operationsService.jsonPatchByResourceType( this.submissionService.getSubmissionObjectLinkName(), action.payload.submissionId, - 'sections').pipe( + 'sections', + true).pipe( map((response: SubmissionObject[]) => { if (this.canDeposit(response)) { return new DepositSubmissionAction(action.payload.submissionId); From 549529c8896b5e14e4ce6798fb30d52d35b56f8f Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Fri, 10 Dec 2021 14:18:51 +0100 Subject: [PATCH 4/4] 85262: Get submission object instead of sending empty patch + revert empty patch code --- .../json-patch-operations.service.ts | 16 +++++--------- .../submission-objects.effects.spec.ts | 7 ++++++ .../objects/submission-objects.effects.ts | 22 +++++++++++++------ 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/app/core/json-patch/json-patch-operations.service.ts b/src/app/core/json-patch/json-patch-operations.service.ts index f160376837..c3363f4db4 100644 --- a/src/app/core/json-patch/json-patch-operations.service.ts +++ b/src/app/core/json-patch/json-patch-operations.service.ts @@ -40,15 +40,13 @@ export abstract class JsonPatchOperationsService * observable of response */ - protected submitJsonPatchOperations(hrefObs: Observable, resourceType: string, resourceId?: string, allowEmptyRequest = false): Observable { + protected submitJsonPatchOperations(hrefObs: Observable, resourceType: string, resourceId?: string): Observable { const requestId = this.requestService.generateRequestId(); let startTransactionTime = null; - const [patchRequest$, emptyRequest$] = partition((request: PatchRequestDefinition) => allowEmptyRequest || isNotEmpty(request.body))(hrefObs.pipe( + const [patchRequest$, emptyRequest$] = partition((request: PatchRequestDefinition) => isNotEmpty(request.body))(hrefObs.pipe( mergeMap((endpointURL: string) => { return this.store.select(jsonPatchOperationsByResourceType(resourceType)).pipe( take(1), @@ -81,11 +79,11 @@ export abstract class JsonPatchOperationsService !allowEmptyRequest && isEmpty(request.body)), + filter((request: PatchRequestDefinition) => isEmpty(request.body)), tap(() => startTransactionTime = null), map(() => null)), patchRequest$.pipe( - filter((request: PatchRequestDefinition) => allowEmptyRequest || isNotEmpty(request.body)), + filter((request: PatchRequestDefinition) => isNotEmpty(request.body)), tap(() => this.store.dispatch(new StartTransactionPatchOperationsAction(resourceType, resourceId, startTransactionTime))), tap((request: PatchRequestDefinition) => this.requestService.send(request)), mergeMap(() => { @@ -143,18 +141,16 @@ export abstract class JsonPatchOperationsService * observable of response */ - public jsonPatchByResourceType(linkPath: string, scopeId: string, resourceType: string, allowEmptyRequest = false): Observable { + public jsonPatchByResourceType(linkPath: string, scopeId: string, resourceType: string): Observable { const href$ = this.halService.getEndpoint(linkPath).pipe( filter((href: string) => isNotEmpty(href)), distinctUntilChanged(), map((endpointURL: string) => this.getEndpointByIDHref(endpointURL, scopeId))); - return this.submitJsonPatchOperations(href$, resourceType, undefined, allowEmptyRequest); + return this.submitJsonPatchOperations(href$, resourceType); } /** diff --git a/src/app/submission/objects/submission-objects.effects.spec.ts b/src/app/submission/objects/submission-objects.effects.spec.ts index e760f71941..b2bc054287 100644 --- a/src/app/submission/objects/submission-objects.effects.spec.ts +++ b/src/app/submission/objects/submission-objects.effects.spec.ts @@ -54,6 +54,8 @@ import { Item } from '../../core/shared/item.model'; import { WorkspaceitemDataService } from '../../core/submission/workspaceitem-data.service'; import { WorkflowItemDataService } from '../../core/submission/workflowitem-data.service'; import { HALEndpointService } from '../../core/shared/hal-endpoint.service'; +import { SubmissionObjectDataService } from '../../core/submission/submission-object-data.service'; +import { mockSubmissionObjectDataService } from '../../shared/testing/submission-oject-data-service.mock'; describe('SubmissionObjectEffects test suite', () => { let submissionObjectEffects: SubmissionObjectEffects; @@ -63,6 +65,7 @@ describe('SubmissionObjectEffects test suite', () => { let notificationsServiceStub; let submissionServiceStub; let submissionJsonPatchOperationsServiceStub; + let submissionObjectDataServiceStub; const collectionId: string = mockSubmissionCollectionId; const submissionId: string = mockSubmissionId; const submissionDefinitionResponse: any = mockSubmissionDefinitionResponse; @@ -75,6 +78,9 @@ describe('SubmissionObjectEffects test suite', () => { notificationsServiceStub = new NotificationsServiceStub(); submissionServiceStub = new SubmissionServiceStub(); submissionJsonPatchOperationsServiceStub = new SubmissionJsonPatchOperationsServiceStub(); + submissionObjectDataServiceStub = mockSubmissionObjectDataService; + + submissionServiceStub.hasUnsavedModification.and.returnValue(observableOf(true)); TestBed.configureTestingModule({ imports: [ @@ -99,6 +105,7 @@ describe('SubmissionObjectEffects test suite', () => { { provide: WorkflowItemDataService, useValue: {} }, { provide: WorkflowItemDataService, useValue: {} }, { provide: HALEndpointService, useValue: {} }, + { provide: SubmissionObjectDataService, useValue: submissionObjectDataServiceStub }, ], }); diff --git a/src/app/submission/objects/submission-objects.effects.ts b/src/app/submission/objects/submission-objects.effects.ts index 5432aa7a4c..257854f027 100644 --- a/src/app/submission/objects/submission-objects.effects.ts +++ b/src/app/submission/objects/submission-objects.effects.ts @@ -196,13 +196,21 @@ export class SubmissionObjectEffects { */ @Effect() saveAndDeposit$ = this.actions$.pipe( ofType(SubmissionObjectActionTypes.SAVE_AND_DEPOSIT_SUBMISSION), - withLatestFrom(this.store$), - switchMap(([action, currentState]: [SaveAndDepositSubmissionAction, any]) => { - return this.operationsService.jsonPatchByResourceType( - this.submissionService.getSubmissionObjectLinkName(), - action.payload.submissionId, - 'sections', - true).pipe( + withLatestFrom(this.submissionService.hasUnsavedModification()), + switchMap(([action, hasUnsavedModification]: [SaveAndDepositSubmissionAction, boolean]) => { + let response$: Observable; + if (hasUnsavedModification) { + response$ = this.operationsService.jsonPatchByResourceType( + this.submissionService.getSubmissionObjectLinkName(), + action.payload.submissionId, + 'sections') as Observable; + } else { + response$ = this.submissionObjectService.findById(action.payload.submissionId).pipe( + getFirstSucceededRemoteDataPayload(), + map((submissionObject: SubmissionObject) => [submissionObject]) + ); + } + return response$.pipe( map((response: SubmissionObject[]) => { if (this.canDeposit(response)) { return new DepositSubmissionAction(action.payload.submissionId);