From 1aa659e6b7e9899bbccc6b4858a688d638163b19 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 23 Apr 2021 13:28:41 +0200 Subject: [PATCH 1/7] 78849: Fix cache/re-request issue & other improvements --- .../core/data/eperson-registration.service.ts | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/src/app/core/data/eperson-registration.service.ts b/src/app/core/data/eperson-registration.service.ts index fd55c031d8..b8614e62b5 100644 --- a/src/app/core/data/eperson-registration.service.ts +++ b/src/app/core/data/eperson-registration.service.ts @@ -3,7 +3,7 @@ import { RequestService } from './request.service'; import { HALEndpointService } from '../shared/hal-endpoint.service'; import { GetRequest, PostRequest } from './request.models'; import { Observable } from 'rxjs'; -import { filter, find, map, take } from 'rxjs/operators'; +import { filter, find, map, skipWhile, take } from 'rxjs/operators'; import { hasValue, isNotEmpty } from '../../shared/empty.util'; import { Registration } from '../shared/registration.model'; import { getFirstCompletedRemoteData, getFirstSucceededRemoteData } from '../shared/operators'; @@ -60,9 +60,9 @@ export class EpersonRegistrationService { const requestId = this.requestService.generateRequestId(); - const hrefObs = this.getRegistrationEndpoint(); + const href$ = this.getRegistrationEndpoint(); - hrefObs.pipe( + href$.pipe( find((href: string) => hasValue(href)), map((href: string) => { const request = new PostRequest(requestId, href, registration); @@ -82,27 +82,28 @@ export class EpersonRegistrationService { searchByToken(token: string): Observable { const requestId = this.requestService.generateRequestId(); - const hrefObs = this.getTokenSearchEndpoint(token); - - hrefObs.pipe( + const href$ = this.getTokenSearchEndpoint(token).pipe( find((href: string) => hasValue(href)), - map((href: string) => { - const request = new GetRequest(requestId, href); - Object.assign(request, { - getResponseParser(): GenericConstructor { - return RegistrationResponseParsingService; - } - }); - this.requestService.send(request, true); - }) - ).subscribe(); + ) - return this.rdbService.buildFromRequestUUID(requestId).pipe( + href$.subscribe((href: string) => { + const request = new GetRequest(requestId, href); + Object.assign(request, { + getResponseParser(): GenericConstructor { + return RegistrationResponseParsingService; + } + }); + this.requestService.send(request, true); + }); + + return this.rdbService.buildSingle(href$).pipe( + skipWhile((rd: RemoteData) => rd.isStale), getFirstSucceededRemoteData(), map((restResponse: RemoteData) => { - return Object.assign(new Registration(), {email: restResponse.payload.email, token: token, user: restResponse.payload.user}); + return Object.assign(new Registration(), { + email: restResponse.payload.email, token: token, user: restResponse.payload.user + }); }), - take(1), ); } From 7d0ea04b3e9d5b3bd75aa45ce70266aa2a4db5d1 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 23 Apr 2021 15:39:15 +0200 Subject: [PATCH 2/7] 78849: Add unit tests for EPerson registration caching --- .../data/eperson-registration.service.spec.ts | 61 +++++++++++++++++-- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/src/app/core/data/eperson-registration.service.spec.ts b/src/app/core/data/eperson-registration.service.spec.ts index fe3a1958eb..cffd3266e5 100644 --- a/src/app/core/data/eperson-registration.service.spec.ts +++ b/src/app/core/data/eperson-registration.service.spec.ts @@ -1,15 +1,18 @@ import { RequestService } from './request.service'; import { EpersonRegistrationService } from './eperson-registration.service'; import { RestResponse } from '../cache/response.models'; -import { RequestEntry } from './request.reducer'; +import { RequestEntry, RequestEntryState } from './request.reducer'; import { cold } from 'jasmine-marbles'; import { PostRequest } from './request.models'; import { Registration } from '../shared/registration.model'; import { HALEndpointServiceStub } from '../../shared/testing/hal-endpoint-service.stub'; -import { createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils'; +import { createPendingRemoteDataObject, createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils'; import { of as observableOf } from 'rxjs/internal/observable/of'; +import { TestScheduler } from 'rxjs/testing'; + +fdescribe('EpersonRegistrationService', () => { + let testScheduler; -describe('EpersonRegistrationService', () => { let service: EpersonRegistrationService; let requestService: RequestService; @@ -29,6 +32,12 @@ describe('EpersonRegistrationService', () => { rd = createSuccessfulRemoteDataObject(registrationWithUser); halService = new HALEndpointServiceStub('rest-url'); + testScheduler = new TestScheduler((actual, expected) => { + // asserting the two objects are equal + // e.g. using chai. + expect(actual).toEqual(expected); + }); + requestService = jasmine.createSpyObj('requestService', { generateRequestId: 'request-id', send: {}, @@ -36,7 +45,8 @@ describe('EpersonRegistrationService', () => { { a: Object.assign(new RequestEntry(), { response: new RestResponse(true, 200, 'Success') }) }) }); rdbService = jasmine.createSpyObj('rdbService', { - buildFromRequestUUID: observableOf(rd) + buildSingle: observableOf(rd), + buildFromRequestUUID: observableOf(rd), }); service = new EpersonRegistrationService( requestService, @@ -88,6 +98,49 @@ describe('EpersonRegistrationService', () => { })); }); + + it('should return the original registration if it was already cached', () => { + testScheduler.run(({ cold, expectObservable }) => { + rdbService.buildSingle.and.returnValue(cold('a-b-c', { + a: createSuccessfulRemoteDataObject(registrationWithUser), + b: createPendingRemoteDataObject(), + c: createSuccessfulRemoteDataObject(new Registration()) + })); + + expectObservable( + service.searchByToken('test-token') + ).toBe('(a|)', { + a: Object.assign(new Registration(), { + email: registrationWithUser.email, + token: 'test-token', + user: registrationWithUser.user + }) + }); + }); + }); + + it('should re-request the registration if it was already cached but stale', () => { + const rdCachedStale = createSuccessfulRemoteDataObject(new Registration()); + rdCachedStale.state = RequestEntryState.SuccessStale; + + testScheduler.run(({ cold, expectObservable }) => { + rdbService.buildSingle.and.returnValue(cold('a-b-c', { + a: rdCachedStale, + b: createPendingRemoteDataObject(), + c: createSuccessfulRemoteDataObject(registrationWithUser), + })); + + expectObservable( + service.searchByToken('test-token') + ).toBe('----(c|)', { + c: Object.assign(new Registration(), { + email: registrationWithUser.email, + token: 'test-token', + user: registrationWithUser.user + }) + }); + }); + }); }); }); From 0fe199a97c05a05f5f26847f778ffeab8acdc99f Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Mon, 26 Apr 2021 16:49:07 +0200 Subject: [PATCH 3/7] 78849: Fix unit test --- .../data/eperson-registration.service.spec.ts | 55 +++++-------------- 1 file changed, 15 insertions(+), 40 deletions(-) diff --git a/src/app/core/data/eperson-registration.service.spec.ts b/src/app/core/data/eperson-registration.service.spec.ts index cffd3266e5..cebe7ffa80 100644 --- a/src/app/core/data/eperson-registration.service.spec.ts +++ b/src/app/core/data/eperson-registration.service.spec.ts @@ -1,16 +1,16 @@ import { RequestService } from './request.service'; import { EpersonRegistrationService } from './eperson-registration.service'; import { RestResponse } from '../cache/response.models'; -import { RequestEntry, RequestEntryState } from './request.reducer'; +import { RequestEntry } from './request.reducer'; import { cold } from 'jasmine-marbles'; import { PostRequest } from './request.models'; import { Registration } from '../shared/registration.model'; import { HALEndpointServiceStub } from '../../shared/testing/hal-endpoint-service.stub'; -import { createPendingRemoteDataObject, createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils'; +import { createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils'; import { of as observableOf } from 'rxjs/internal/observable/of'; import { TestScheduler } from 'rxjs/testing'; -fdescribe('EpersonRegistrationService', () => { +describe('EpersonRegistrationService', () => { let testScheduler; let service: EpersonRegistrationService; @@ -96,51 +96,26 @@ fdescribe('EpersonRegistrationService', () => { user: registrationWithUser.user }) })); - }); - it('should return the original registration if it was already cached', () => { + it('should use cached responses and /registrations/search/findByToken?', () => { testScheduler.run(({ cold, expectObservable }) => { - rdbService.buildSingle.and.returnValue(cold('a-b-c', { - a: createSuccessfulRemoteDataObject(registrationWithUser), - b: createPendingRemoteDataObject(), - c: createSuccessfulRemoteDataObject(new Registration()) - })); + rdbService.buildSingle.and.returnValue(cold('a', { a: rd })); - expectObservable( - service.searchByToken('test-token') - ).toBe('(a|)', { - a: Object.assign(new Registration(), { - email: registrationWithUser.email, - token: 'test-token', - user: registrationWithUser.user - }) + service.searchByToken('test-token'); + + expect(requestService.send).toHaveBeenCalledWith( + jasmine.objectContaining({ + uuid: 'request-id', method: 'GET', + href: 'rest-url/registrations/search/findByToken?token=test-token', + }), true + ); + expectObservable(rdbService.buildSingle.calls.argsFor(0)[0]).toBe('(a|)', { + a: 'rest-url/registrations/search/findByToken?token=test-token' }); }); }); - it('should re-request the registration if it was already cached but stale', () => { - const rdCachedStale = createSuccessfulRemoteDataObject(new Registration()); - rdCachedStale.state = RequestEntryState.SuccessStale; - - testScheduler.run(({ cold, expectObservable }) => { - rdbService.buildSingle.and.returnValue(cold('a-b-c', { - a: rdCachedStale, - b: createPendingRemoteDataObject(), - c: createSuccessfulRemoteDataObject(registrationWithUser), - })); - - expectObservable( - service.searchByToken('test-token') - ).toBe('----(c|)', { - c: Object.assign(new Registration(), { - email: registrationWithUser.email, - token: 'test-token', - user: registrationWithUser.user - }) - }); - }); - }); }); }); From 781a88bc4c2a4478a99e31cb468486611e2e0289 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Thu, 29 Apr 2021 13:29:18 +0200 Subject: [PATCH 4/7] 78849: Fix double notification on submit --- .../forgot-password-form/forgot-password-form.component.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app/forgot-password/forgot-password-form/forgot-password-form.component.ts b/src/app/forgot-password/forgot-password-form/forgot-password-form.component.ts index 98188a07e8..707c70f19c 100644 --- a/src/app/forgot-password/forgot-password-form/forgot-password-form.component.ts +++ b/src/app/forgot-password/forgot-password-form/forgot-password-form.component.ts @@ -11,6 +11,7 @@ import { Store } from '@ngrx/store'; import { CoreState } from '../../core/core.reducers'; import { RemoteData } from '../../core/data/remote-data'; import { EPerson } from '../../core/eperson/models/eperson.model'; +import { getFirstCompletedRemoteData } from '../../core/shared/operators'; @Component({ selector: 'ds-forgot-password-form', @@ -70,7 +71,9 @@ export class ForgotPasswordFormComponent { */ submit() { if (!this.isInValid) { - this.ePersonDataService.patchPasswordWithToken(this.user, this.token, this.password).subscribe((response: RemoteData) => { + this.ePersonDataService.patchPasswordWithToken(this.user, this.token, this.password).pipe( + getFirstCompletedRemoteData() + ).subscribe((response: RemoteData) => { if (response.hasSucceeded) { this.notificationsService.success( this.translateService.instant(this.NOTIFICATIONS_PREFIX + '.success.title'), From 032231f10e563cf7a898ebdf731385c08fd48c49 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Thu, 29 Apr 2021 13:57:31 +0200 Subject: [PATCH 5/7] 78849: Fix lint issues --- src/app/core/data/eperson-registration.service.spec.ts | 5 +++-- src/app/core/data/eperson-registration.service.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/app/core/data/eperson-registration.service.spec.ts b/src/app/core/data/eperson-registration.service.spec.ts index cebe7ffa80..2860880803 100644 --- a/src/app/core/data/eperson-registration.service.spec.ts +++ b/src/app/core/data/eperson-registration.service.spec.ts @@ -99,8 +99,8 @@ describe('EpersonRegistrationService', () => { }); it('should use cached responses and /registrations/search/findByToken?', () => { - testScheduler.run(({ cold, expectObservable }) => { - rdbService.buildSingle.and.returnValue(cold('a', { a: rd })); + testScheduler.run(({ tscold, expectObservable }) => { + rdbService.buildSingle.and.returnValue(tscold('a', { a: rd })); service.searchByToken('test-token'); @@ -119,3 +119,4 @@ describe('EpersonRegistrationService', () => { }); }); +/**/ diff --git a/src/app/core/data/eperson-registration.service.ts b/src/app/core/data/eperson-registration.service.ts index b8614e62b5..8ea43b3c3f 100644 --- a/src/app/core/data/eperson-registration.service.ts +++ b/src/app/core/data/eperson-registration.service.ts @@ -84,7 +84,7 @@ export class EpersonRegistrationService { const href$ = this.getTokenSearchEndpoint(token).pipe( find((href: string) => hasValue(href)), - ) + ); href$.subscribe((href: string) => { const request = new GetRequest(requestId, href); From b2b077868e46b73636c7cbdcc99e816b475a0ed8 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Thu, 29 Apr 2021 14:26:33 +0200 Subject: [PATCH 6/7] 78994: Disable no-shadowed-variable --- src/app/core/data/eperson-registration.service.spec.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/app/core/data/eperson-registration.service.spec.ts b/src/app/core/data/eperson-registration.service.spec.ts index 2860880803..768d83c024 100644 --- a/src/app/core/data/eperson-registration.service.spec.ts +++ b/src/app/core/data/eperson-registration.service.spec.ts @@ -98,9 +98,10 @@ describe('EpersonRegistrationService', () => { })); }); + // tslint:disable:no-shadowed-variable it('should use cached responses and /registrations/search/findByToken?', () => { - testScheduler.run(({ tscold, expectObservable }) => { - rdbService.buildSingle.and.returnValue(tscold('a', { a: rd })); + testScheduler.run(({ cold, expectObservable }) => { + rdbService.buildSingle.and.returnValue(cold('a', { a: rd })); service.searchByToken('test-token'); From 60009144a117a526716e29226ad9df1331066727 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Thu, 29 Apr 2021 14:28:37 +0200 Subject: [PATCH 7/7] 78994: Remove unused import --- src/app/core/data/eperson-registration.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/core/data/eperson-registration.service.ts b/src/app/core/data/eperson-registration.service.ts index 8ea43b3c3f..adf01b0ce9 100644 --- a/src/app/core/data/eperson-registration.service.ts +++ b/src/app/core/data/eperson-registration.service.ts @@ -3,7 +3,7 @@ import { RequestService } from './request.service'; import { HALEndpointService } from '../shared/hal-endpoint.service'; import { GetRequest, PostRequest } from './request.models'; import { Observable } from 'rxjs'; -import { filter, find, map, skipWhile, take } from 'rxjs/operators'; +import { filter, find, map, skipWhile } from 'rxjs/operators'; import { hasValue, isNotEmpty } from '../../shared/empty.util'; import { Registration } from '../shared/registration.model'; import { getFirstCompletedRemoteData, getFirstSucceededRemoteData } from '../shared/operators';