From 6360d5a88c9d38b128902eb1c7610c93b83760b9 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 28 Aug 2019 11:02:05 -0700 Subject: [PATCH 01/14] Modified the log-in component to set the redirectUrl in AuthState (so the user is returned to the same page after login). --- src/app/shared/log-in/log-in.component.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index 1291e6aa4c..6bdf7ad987 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -20,6 +20,7 @@ import { CoreState } from '../../core/core.reducers'; import { isNotEmpty } from '../empty.util'; import { fadeOut } from '../animations/fade'; import { AuthService } from '../../core/auth/auth.service'; +import {Router} from '@angular/router'; /** * /users/sign-in @@ -90,6 +91,7 @@ export class LogInComponent implements OnDestroy, OnInit { constructor( private authService: AuthService, private formBuilder: FormBuilder, + private router: Router, private store: Store ) { } @@ -182,6 +184,9 @@ export class LogInComponent implements OnDestroy, OnInit { email.trim(); password.trim(); + // add the current url to store for later redirect. + this.authService.setRedirectUrl(this.router.url); + // dispatch AuthenticationAction this.store.dispatch(new AuthenticateAction(email, password)); From db326a706cb5a8402012f7f974b884f69b49c203 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 28 Aug 2019 12:56:29 -0700 Subject: [PATCH 02/14] Fixed the unit test (and added a typedoc) --- .../shared/log-in/log-in.component.spec.ts | 29 +++++++++++++++++-- src/app/shared/log-in/log-in.component.ts | 1 + 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts index dd2aea35d5..38173a6a8b 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -3,7 +3,7 @@ import { async, ComponentFixture, inject, TestBed } from '@angular/core/testing' import { FormGroup, FormsModule, ReactiveFormsModule } from '@angular/forms'; import { By } from '@angular/platform-browser'; -import { Store, StoreModule } from '@ngrx/store'; +import { Store, StoreModule, select } from '@ngrx/store'; import { LogInComponent } from './log-in.component'; import { authReducer } from '../../core/auth/auth.reducer'; @@ -13,6 +13,9 @@ import { TranslateModule } from '@ngx-translate/core'; import { AuthService } from '../../core/auth/auth.service'; import { AuthServiceStub } from '../testing/auth-service-stub'; import { AppState } from '../../app.reducer'; +import {AppRoutingModule} from '../../app-routing.module'; +import {PageNotFoundComponent} from '../../pagenotfound/pagenotfound.component'; +import {APP_BASE_HREF} from '@angular/common'; describe('LogInComponent', () => { @@ -38,13 +41,16 @@ describe('LogInComponent', () => { FormsModule, ReactiveFormsModule, StoreModule.forRoot(authReducer), + AppRoutingModule, TranslateModule.forRoot() ], declarations: [ - LogInComponent + LogInComponent, + PageNotFoundComponent ], providers: [ - {provide: AuthService, useClass: AuthServiceStub} + {provide: AuthService, useClass: AuthServiceStub}, + {provide: APP_BASE_HREF, useValue: '/'} ], schemas: [ CUSTOM_ELEMENTS_SCHEMA @@ -95,6 +101,23 @@ describe('LogInComponent', () => { // verify Store.dispatch() is invoked expect(page.navigateSpy.calls.any()).toBe(true, 'Store.dispatch not invoked'); }); + + it('should set the redirect url', () => { + fixture.detectChanges(); + + // set FormControl values + component.form.controls.email.setValue('user'); + component.form.controls.password.setValue('password'); + + const authService: AuthService = TestBed.get(AuthService); + spyOn(authService, 'setRedirectUrl'); + + component.submit(); + + // the redirect url should be set upon submit + expect(authService.setRedirectUrl).toHaveBeenCalled(); + }); + }); /** diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index 6bdf7ad987..f6df9ee389 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -86,6 +86,7 @@ export class LogInComponent implements OnDestroy, OnInit { * @constructor * @param {AuthService} authService * @param {FormBuilder} formBuilder + * @param {Router} router * @param {Store} store */ constructor( From c6156c5cbe009f3af4ff370eac8c6dfb0cbf680d Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Thu, 29 Aug 2019 16:08:46 -0700 Subject: [PATCH 03/14] Modified log-in component to set the redirect url only if one has not been set already. --- .../shared/log-in/log-in.component.spec.ts | 17 +++++++++++++ src/app/shared/log-in/log-in.component.ts | 24 +++++++++++-------- src/app/shared/testing/auth-service-stub.ts | 7 +++++- 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts index 38173a6a8b..6992afa5ec 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -118,6 +118,23 @@ describe('LogInComponent', () => { expect(authService.setRedirectUrl).toHaveBeenCalled(); }); + it('should not set the redirect url because one already exists', () => { + fixture.detectChanges(); + + const authService: AuthService = TestBed.get(AuthService); + authService.setRedirectUrl('/submit') + + // set FormControl values + component.form.controls.email.setValue('user'); + component.form.controls.password.setValue('password'); + + spyOn(authService, 'setRedirectUrl'); + + component.submit(); + + expect(authService.setRedirectUrl).not.toHaveBeenCalled(); + }); + }); /** diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index f6df9ee389..13fa73bc4e 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -1,4 +1,4 @@ -import { filter, map, takeWhile } from 'rxjs/operators'; +import {filter, map, take, takeWhile, tap} from 'rxjs/operators'; import { Component, OnDestroy, OnInit } from '@angular/core'; import { FormBuilder, FormGroup, Validators } from '@angular/forms'; @@ -17,7 +17,7 @@ import { } from '../../core/auth/selectors'; import { CoreState } from '../../core/core.reducers'; -import { isNotEmpty } from '../empty.util'; +import {isEmpty, isNotEmpty} from '../empty.util'; import { fadeOut } from '../animations/fade'; import { AuthService } from '../../core/auth/auth.service'; import {Router} from '@angular/router'; @@ -185,13 +185,17 @@ export class LogInComponent implements OnDestroy, OnInit { email.trim(); password.trim(); - // add the current url to store for later redirect. - this.authService.setRedirectUrl(this.router.url); - - // dispatch AuthenticationAction - this.store.dispatch(new AuthenticateAction(email, password)); - - // clear form - this.form.reset(); + this.authService.getRedirectUrl().pipe( + take(1)). + subscribe((r) => { + // Set the redirect url if none exists. + if (isEmpty(r)) { + this.authService.setRedirectUrl(this.router.url) + } + // dispatch AuthenticationAction + this.store.dispatch(new AuthenticateAction(email, password)); + // clear form + this.form.reset(); + }); } } diff --git a/src/app/shared/testing/auth-service-stub.ts b/src/app/shared/testing/auth-service-stub.ts index a65923dcab..a6d24d5c8b 100644 --- a/src/app/shared/testing/auth-service-stub.ts +++ b/src/app/shared/testing/auth-service-stub.ts @@ -10,6 +10,7 @@ export class AuthServiceStub { token: AuthTokenInfo = new AuthTokenInfo('token_test'); private _tokenExpired = false; + private redirectUrl; constructor() { this.token.expires = Date.now() + (1000 * 60 * 60); @@ -88,7 +89,11 @@ export class AuthServiceStub { } setRedirectUrl(url: string) { - return; + this.redirectUrl = url; + } + + getRedirectUrl() { + return observableOf(this.redirectUrl); } public storeToken(token: AuthTokenInfo) { From fcaf01807cf3c4993244518337b180d76054bb70 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 3 Sep 2019 17:49:08 -0700 Subject: [PATCH 04/14] Modifed log-in component to use LOGIN_ROUTE when setting the redirect url. Also added check for mobile layout that forces setting of the redirect url. --- .../shared/log-in/log-in.component.spec.ts | 111 +++++++++++++++++- src/app/shared/log-in/log-in.component.ts | 55 ++++++--- src/app/shared/testing/router-events-stub.ts | 24 ++++ 3 files changed, 170 insertions(+), 20 deletions(-) create mode 100644 src/app/shared/testing/router-events-stub.ts diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts index 6992afa5ec..adb7187757 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -16,6 +16,12 @@ import { AppState } from '../../app.reducer'; import {AppRoutingModule} from '../../app-routing.module'; import {PageNotFoundComponent} from '../../pagenotfound/pagenotfound.component'; import {APP_BASE_HREF} from '@angular/common'; +import {HostWindowService} from '../host-window.service'; +import {HostWindowServiceStub} from '../testing/host-window-service-stub'; +import {RouterStub} from '../testing/router-stub'; +import {NavigationEnd, Router} from '@angular/router'; +import {Observable, of as observableOf} from 'rxjs'; +import {RouterEventsStub} from '../testing/router-events-stub'; describe('LogInComponent', () => { @@ -50,7 +56,9 @@ describe('LogInComponent', () => { ], providers: [ {provide: AuthService, useClass: AuthServiceStub}, - {provide: APP_BASE_HREF, useValue: '/'} + {provide: APP_BASE_HREF, useValue: '/'}, + {provide: Router, useClass: RouterStub}, + {provide: HostWindowService, useValue: new HostWindowServiceStub(900) } ], schemas: [ CUSTOM_ELEMENTS_SCHEMA @@ -118,11 +126,13 @@ describe('LogInComponent', () => { expect(authService.setRedirectUrl).toHaveBeenCalled(); }); - it('should not set the redirect url because one already exists', () => { + it('should not set the redirect url to /login', () => { fixture.detectChanges(); + const router: Router = TestBed.get(Router); + router.navigateByUrl('/login') + const authService: AuthService = TestBed.get(AuthService); - authService.setRedirectUrl('/submit') // set FormControl values component.form.controls.email.setValue('user'); @@ -135,6 +145,101 @@ describe('LogInComponent', () => { expect(authService.setRedirectUrl).not.toHaveBeenCalled(); }); + it('should not set the redirect url on init', () => { + + const authService: AuthService = TestBed.get(AuthService); + spyOn(authService, 'setRedirectUrl'); + + fixture.detectChanges(); + expect(authService.setRedirectUrl).not.toHaveBeenCalledWith(); + + }); + +}); + +describe('LogInComponent on small screen', () => { + + let component: LogInComponent; + let fixture: ComponentFixture; + let page: Page; + let user: EPerson; + + const navEvents = observableOf( + new NavigationEnd(0, 'http://localhost:3000/home', 'http://localhost:3000/home'), + new NavigationEnd(1, 'http://localhost:3000/login', 'http://localhost:3000/login') + ); + + const authState = { + authenticated: false, + loaded: false, + loading: false, + }; + + beforeEach(() => { + user = EPersonMock; + }); + + beforeEach(async(() => { + // refine the test module by declaring the test component + TestBed.configureTestingModule({ + imports: [ + FormsModule, + ReactiveFormsModule, + StoreModule.forRoot(authReducer), + AppRoutingModule, + TranslateModule.forRoot() + ], + declarations: [ + LogInComponent, + PageNotFoundComponent + ], + providers: [ + {provide: AuthService, useClass: AuthServiceStub}, + {provide: APP_BASE_HREF, useValue: '/'}, + {provide: Router, useValue: new RouterEventsStub(navEvents)}, + {provide: HostWindowService, useValue: new HostWindowServiceStub(300) } + ], + schemas: [ + CUSTOM_ELEMENTS_SCHEMA + ] + }) + .compileComponents(); + + })); + + beforeEach(inject([Store], (store: Store) => { + store + .subscribe((state) => { + (state as any).core = Object.create({}); + (state as any).core.auth = authState; + }); + + // create component and test fixture + fixture = TestBed.createComponent(LogInComponent); + + // get test component from the fixture + component = fixture.componentInstance; + + // create page + page = new Page(component, fixture); + + // verify the fixture is stable (no pending tasks) + fixture.whenStable().then(() => { + page.addPageElements(); + }); + + })); + + it('should set the redirect url on init', () => { + + const authService: AuthService = TestBed.get(AuthService); + spyOn(authService, 'setRedirectUrl'); + + fixture.detectChanges(); + expect(authService.setRedirectUrl).toHaveBeenCalledWith('http://localhost:3000/home'); + + }); + }); /** diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index 13fa73bc4e..0821ff1678 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -1,4 +1,4 @@ -import {filter, map, take, takeWhile, tap} from 'rxjs/operators'; +import {filter, map, pairwise, take, takeWhile, tap} from 'rxjs/operators'; import { Component, OnDestroy, OnInit } from '@angular/core'; import { FormBuilder, FormGroup, Validators } from '@angular/forms'; @@ -19,8 +19,9 @@ import { CoreState } from '../../core/core.reducers'; import {isEmpty, isNotEmpty} from '../empty.util'; import { fadeOut } from '../animations/fade'; -import { AuthService } from '../../core/auth/auth.service'; -import {Router} from '@angular/router'; +import {AuthService, LOGIN_ROUTE} from '../../core/auth/auth.service'; +import {NavigationEnd, Router, RoutesRecognized} from '@angular/router'; +import {HostWindowService} from '../host-window.service'; /** * /users/sign-in @@ -82,17 +83,21 @@ export class LogInComponent implements OnDestroy, OnInit { */ private alive = true; + private redirectUrl = LOGIN_ROUTE; + /** * @constructor * @param {AuthService} authService * @param {FormBuilder} formBuilder * @param {Router} router + * @param {HostWindowService} windowService * @param {Store} store */ constructor( private authService: AuthService, private formBuilder: FormBuilder, private router: Router, + private windowService: HostWindowService, private store: Store ) { } @@ -101,10 +106,22 @@ export class LogInComponent implements OnDestroy, OnInit { * Lifecycle hook that is called after data-bound properties of a directive are initialized. * @method ngOnInit */ - public ngOnInit() { - // set isAuthenticated + public ngOnInit() { // set isAuthenticated this.isAuthenticated = this.store.pipe(select(isAuthenticated)); + // for mobile login, set the redirect url to the previous route + this.windowService.isXs().pipe(take(1)) + .subscribe((isMobile) => { + if (isMobile) { + this.router.events.pipe( + filter((e: any) => e instanceof NavigationEnd), + pairwise() + ).subscribe((e: any) => { + this.setRedirectUrl(e[0].urlAfterRedirects); + }); + } + }); + // set formGroup this.form = this.formBuilder.group({ email: ['', Validators.required], @@ -185,17 +202,21 @@ export class LogInComponent implements OnDestroy, OnInit { email.trim(); password.trim(); - this.authService.getRedirectUrl().pipe( - take(1)). - subscribe((r) => { - // Set the redirect url if none exists. - if (isEmpty(r)) { - this.authService.setRedirectUrl(this.router.url) - } - // dispatch AuthenticationAction - this.store.dispatch(new AuthenticateAction(email, password)); - // clear form - this.form.reset(); - }); + this.setRedirectUrl(this.router.url); + // dispatch AuthenticationAction + this.store.dispatch(new AuthenticateAction(email, password)); + // clear form + this.form.reset(); + + } + + /** + * Sets the redirect url if not LOGIN_ROUTE + * @param url + */ + private setRedirectUrl(url: string) { + if (url !== this.redirectUrl) { + this.authService.setRedirectUrl(url) + } } } diff --git a/src/app/shared/testing/router-events-stub.ts b/src/app/shared/testing/router-events-stub.ts new file mode 100644 index 0000000000..313f6a2b4e --- /dev/null +++ b/src/app/shared/testing/router-events-stub.ts @@ -0,0 +1,24 @@ +import {Observable, of as observableOf} from 'rxjs'; +export class RouterEventsStub { + url: string; + routeReuseStrategy = {shouldReuseRoute: {}}; + //noinspection TypeScriptUnresolvedFunction + navigate = jasmine.createSpy('navigate'); + parseUrl = jasmine.createSpy('parseUrl'); + events = new Observable((observer) => { + this.eventArr.forEach((e) => { + observer.next(e); + }); + observer.complete(); + }); + eventArr: any; + + // Stub constructor takes array of event objects. + constructor( events: any = observableOf({})) { + this.eventArr = events; + } + + navigateByUrl(url): void { + this.url = url; + } +} From 54fc57d1f33cec34f1dd165d87c534651b72ff32 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 4 Sep 2019 12:56:37 -0700 Subject: [PATCH 05/14] Modified mobile log-in to use history as provided by the store. Minor typedoc and import updates. --- .../shared/log-in/log-in.component.spec.ts | 22 +++++++--------- src/app/shared/log-in/log-in.component.ts | 26 +++++++++++-------- src/app/shared/testing/route-service-stub.ts | 3 +++ src/app/shared/testing/router-events-stub.ts | 24 ----------------- 4 files changed, 28 insertions(+), 47 deletions(-) delete mode 100644 src/app/shared/testing/router-events-stub.ts diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts index adb7187757..43004b79c7 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -19,9 +19,9 @@ import {APP_BASE_HREF} from '@angular/common'; import {HostWindowService} from '../host-window.service'; import {HostWindowServiceStub} from '../testing/host-window-service-stub'; import {RouterStub} from '../testing/router-stub'; -import {NavigationEnd, Router} from '@angular/router'; -import {Observable, of as observableOf} from 'rxjs'; -import {RouterEventsStub} from '../testing/router-events-stub'; +import {Router} from '@angular/router'; +import {RouteService} from '../services/route.service'; +import {routeServiceStub} from '../testing/route-service-stub'; describe('LogInComponent', () => { @@ -58,6 +58,7 @@ describe('LogInComponent', () => { {provide: AuthService, useClass: AuthServiceStub}, {provide: APP_BASE_HREF, useValue: '/'}, {provide: Router, useClass: RouterStub}, + {provide: RouteService, useValue: routeServiceStub }, {provide: HostWindowService, useValue: new HostWindowServiceStub(900) } ], schemas: [ @@ -164,11 +165,6 @@ describe('LogInComponent on small screen', () => { let page: Page; let user: EPerson; - const navEvents = observableOf( - new NavigationEnd(0, 'http://localhost:3000/home', 'http://localhost:3000/home'), - new NavigationEnd(1, 'http://localhost:3000/login', 'http://localhost:3000/login') - ); - const authState = { authenticated: false, loaded: false, @@ -196,7 +192,8 @@ describe('LogInComponent on small screen', () => { providers: [ {provide: AuthService, useClass: AuthServiceStub}, {provide: APP_BASE_HREF, useValue: '/'}, - {provide: Router, useValue: new RouterEventsStub(navEvents)}, + {provide: Router, useClass: RouterStub}, + {provide: RouteService, useValue: routeServiceStub }, {provide: HostWindowService, useValue: new HostWindowServiceStub(300) } ], schemas: [ @@ -231,12 +228,13 @@ describe('LogInComponent on small screen', () => { })); it('should set the redirect url on init', () => { - const authService: AuthService = TestBed.get(AuthService); spyOn(authService, 'setRedirectUrl'); - fixture.detectChanges(); - expect(authService.setRedirectUrl).toHaveBeenCalledWith('http://localhost:3000/home'); + // set FormControl values + component.form.controls.email.setValue('user'); + component.form.controls.password.setValue('password'); + expect(authService.setRedirectUrl).toHaveBeenCalledWith('collection/123'); }); diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index 0821ff1678..5097b2a0c7 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -1,9 +1,9 @@ -import {filter, map, pairwise, take, takeWhile, tap} from 'rxjs/operators'; +import {filter, map, pairwise, take, takeUntil, takeWhile, tap} from 'rxjs/operators'; import { Component, OnDestroy, OnInit } from '@angular/core'; import { FormBuilder, FormGroup, Validators } from '@angular/forms'; import { select, Store } from '@ngrx/store'; -import { Observable } from 'rxjs'; +import {Observable, Subject} from 'rxjs'; import { AuthenticateAction, ResetAuthenticationMessagesAction @@ -17,11 +17,12 @@ import { } from '../../core/auth/selectors'; import { CoreState } from '../../core/core.reducers'; -import {isEmpty, isNotEmpty} from '../empty.util'; +import {isNotEmpty} from '../empty.util'; import { fadeOut } from '../animations/fade'; import {AuthService, LOGIN_ROUTE} from '../../core/auth/auth.service'; -import {NavigationEnd, Router, RoutesRecognized} from '@angular/router'; +import {Router} from '@angular/router'; import {HostWindowService} from '../host-window.service'; +import {RouteService} from '../services/route.service'; /** * /users/sign-in @@ -89,6 +90,7 @@ export class LogInComponent implements OnDestroy, OnInit { * @constructor * @param {AuthService} authService * @param {FormBuilder} formBuilder + * @param {RouteService} routeService * @param {Router} router * @param {HostWindowService} windowService * @param {Store} store @@ -96,6 +98,7 @@ export class LogInComponent implements OnDestroy, OnInit { constructor( private authService: AuthService, private formBuilder: FormBuilder, + private routeService: RouteService, private router: Router, private windowService: HostWindowService, private store: Store @@ -106,18 +109,19 @@ export class LogInComponent implements OnDestroy, OnInit { * Lifecycle hook that is called after data-bound properties of a directive are initialized. * @method ngOnInit */ - public ngOnInit() { // set isAuthenticated + public ngOnInit() { + // set isAuthenticated this.isAuthenticated = this.store.pipe(select(isAuthenticated)); // for mobile login, set the redirect url to the previous route this.windowService.isXs().pipe(take(1)) .subscribe((isMobile) => { if (isMobile) { - this.router.events.pipe( - filter((e: any) => e instanceof NavigationEnd), - pairwise() - ).subscribe((e: any) => { - this.setRedirectUrl(e[0].urlAfterRedirects); + this.routeService.getHistory().pipe( + take(1) + ).subscribe((history) => { + const previousIndex = history.length - 2; + this.setRedirectUrl(history[previousIndex]); }); } }); @@ -211,7 +215,7 @@ export class LogInComponent implements OnDestroy, OnInit { } /** - * Sets the redirect url if not LOGIN_ROUTE + * Sets the redirect url if not equal to LOGIN_ROUTE * @param url */ private setRedirectUrl(url: string) { diff --git a/src/app/shared/testing/route-service-stub.ts b/src/app/shared/testing/route-service-stub.ts index e3cdd9d8c6..22e039a815 100644 --- a/src/app/shared/testing/route-service-stub.ts +++ b/src/app/shared/testing/route-service-stub.ts @@ -27,6 +27,9 @@ export const routeServiceStub: any = { }, getRouteDataValue: (param) => { return observableOf({}) + }, + getHistory: () => { + return observableOf(['/home','collection/123','/login']) } /* tslint:enable:no-empty */ }; diff --git a/src/app/shared/testing/router-events-stub.ts b/src/app/shared/testing/router-events-stub.ts deleted file mode 100644 index 313f6a2b4e..0000000000 --- a/src/app/shared/testing/router-events-stub.ts +++ /dev/null @@ -1,24 +0,0 @@ -import {Observable, of as observableOf} from 'rxjs'; -export class RouterEventsStub { - url: string; - routeReuseStrategy = {shouldReuseRoute: {}}; - //noinspection TypeScriptUnresolvedFunction - navigate = jasmine.createSpy('navigate'); - parseUrl = jasmine.createSpy('parseUrl'); - events = new Observable((observer) => { - this.eventArr.forEach((e) => { - observer.next(e); - }); - observer.complete(); - }); - eventArr: any; - - // Stub constructor takes array of event objects. - constructor( events: any = observableOf({})) { - this.eventArr = events; - } - - navigateByUrl(url): void { - this.url = url; - } -} From f0813fcbc18901ed3c33d1892c4e56815a8e4c6f Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 4 Sep 2019 14:02:07 -0700 Subject: [PATCH 06/14] Added @Input to the log-in component that indicates whether the component is being used in the standalone login page or the drop-down. --- src/app/+login-page/login-page.component.html | 3 ++- .../auth-nav-menu.component.html | 3 ++- .../shared/log-in/log-in.component.spec.ts | 9 ++++---- src/app/shared/log-in/log-in.component.ts | 23 ++++++++----------- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/app/+login-page/login-page.component.html b/src/app/+login-page/login-page.component.html index 6dcb11fbb0..84059877f4 100644 --- a/src/app/+login-page/login-page.component.html +++ b/src/app/+login-page/login-page.component.html @@ -3,7 +3,8 @@

{{"login.form.header" | translate}}

- +
diff --git a/src/app/shared/auth-nav-menu/auth-nav-menu.component.html b/src/app/shared/auth-nav-menu/auth-nav-menu.component.html index b560283ad5..4df07880d8 100644 --- a/src/app/shared/auth-nav-menu/auth-nav-menu.component.html +++ b/src/app/shared/auth-nav-menu/auth-nav-menu.component.html @@ -3,7 +3,8 @@
{{ 'nav.login' | translate }}
- +
diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts index 43004b79c7..8ad423629d 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -58,8 +58,7 @@ describe('LogInComponent', () => { {provide: AuthService, useClass: AuthServiceStub}, {provide: APP_BASE_HREF, useValue: '/'}, {provide: Router, useClass: RouterStub}, - {provide: RouteService, useValue: routeServiceStub }, - {provide: HostWindowService, useValue: new HostWindowServiceStub(900) } + {provide: RouteService, useValue: routeServiceStub } ], schemas: [ CUSTOM_ELEMENTS_SCHEMA @@ -151,6 +150,8 @@ describe('LogInComponent', () => { const authService: AuthService = TestBed.get(AuthService); spyOn(authService, 'setRedirectUrl'); + component.isStandalonePage = false; + fixture.detectChanges(); expect(authService.setRedirectUrl).not.toHaveBeenCalledWith(); @@ -193,8 +194,7 @@ describe('LogInComponent on small screen', () => { {provide: AuthService, useClass: AuthServiceStub}, {provide: APP_BASE_HREF, useValue: '/'}, {provide: Router, useClass: RouterStub}, - {provide: RouteService, useValue: routeServiceStub }, - {provide: HostWindowService, useValue: new HostWindowServiceStub(300) } + {provide: RouteService, useValue: routeServiceStub } ], schemas: [ CUSTOM_ELEMENTS_SCHEMA @@ -230,6 +230,7 @@ describe('LogInComponent on small screen', () => { it('should set the redirect url on init', () => { const authService: AuthService = TestBed.get(AuthService); spyOn(authService, 'setRedirectUrl'); + component.isStandalonePage = true; fixture.detectChanges(); // set FormControl values component.form.controls.email.setValue('user'); diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index 5097b2a0c7..3a4ff151bf 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -1,5 +1,5 @@ import {filter, map, pairwise, take, takeUntil, takeWhile, tap} from 'rxjs/operators'; -import { Component, OnDestroy, OnInit } from '@angular/core'; +import {Component, Input, OnDestroy, OnInit} from '@angular/core'; import { FormBuilder, FormGroup, Validators } from '@angular/forms'; import { select, Store } from '@ngrx/store'; @@ -86,13 +86,14 @@ export class LogInComponent implements OnDestroy, OnInit { private redirectUrl = LOGIN_ROUTE; + @Input() isStandalonePage: boolean; + /** * @constructor * @param {AuthService} authService * @param {FormBuilder} formBuilder * @param {RouteService} routeService * @param {Router} router - * @param {HostWindowService} windowService * @param {Store} store */ constructor( @@ -100,7 +101,6 @@ export class LogInComponent implements OnDestroy, OnInit { private formBuilder: FormBuilder, private routeService: RouteService, private router: Router, - private windowService: HostWindowService, private store: Store ) { } @@ -114,17 +114,14 @@ export class LogInComponent implements OnDestroy, OnInit { this.isAuthenticated = this.store.pipe(select(isAuthenticated)); // for mobile login, set the redirect url to the previous route - this.windowService.isXs().pipe(take(1)) - .subscribe((isMobile) => { - if (isMobile) { - this.routeService.getHistory().pipe( - take(1) - ).subscribe((history) => { - const previousIndex = history.length - 2; - this.setRedirectUrl(history[previousIndex]); - }); - } + if (this.isStandalonePage) { + this.routeService.getHistory().pipe( + take(1) + ).subscribe((history) => { + const previousIndex = history.length - 2; + this.setRedirectUrl(history[previousIndex]); }); + } // set formGroup this.form = this.formBuilder.group({ From 0935bd4afd6902cf0f364ff0e200dbd7f3b9a882 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Thu, 5 Sep 2019 17:49:56 -0700 Subject: [PATCH 07/14] Proposed authentication service method that sets redirect url and dispatches auth request. --- src/app/core/auth/auth.service.spec.ts | 43 +++++- src/app/core/auth/auth.service.ts | 40 ++++-- .../shared/log-in/log-in.component.spec.ts | 131 ------------------ src/app/shared/log-in/log-in.component.ts | 35 +---- src/app/shared/testing/auth-service-stub.ts | 3 + src/app/shared/testing/route-service-stub.ts | 2 +- 6 files changed, 79 insertions(+), 175 deletions(-) diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index ab2e6fd86b..6044e9ecb6 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -4,7 +4,7 @@ import { ActivatedRoute, Router } from '@angular/router'; import { Store, StoreModule } from '@ngrx/store'; import { REQUEST } from '@nguniversal/express-engine/tokens'; -import { of as observableOf } from 'rxjs'; +import {of, of as observableOf} from 'rxjs'; import { authReducer, AuthState } from './auth.reducer'; import { NativeWindowRef, NativeWindowService } from '../services/window.service'; @@ -23,11 +23,14 @@ import { AppState } from '../../app.reducer'; import { ClientCookieService } from '../services/client-cookie.service'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; import { getMockRemoteDataBuildService } from '../../shared/mocks/mock-remote-data-build.service'; +import {routeServiceStub} from '../../shared/testing/route-service-stub'; +import {RouteService} from '../services/route.service'; describe('AuthService test', () => { let mockStore: Store; let authService: AuthService; + let routeServiceMock: RouteService; let authRequest; let window; let routerStub; @@ -74,6 +77,7 @@ describe('AuthService test', () => { { provide: NativeWindowService, useValue: window }, { provide: REQUEST, useValue: {} }, { provide: Router, useValue: routerStub }, + { provide: RouteService, useValue: routeServiceStub }, { provide: ActivatedRoute, useValue: routeStub }, { provide: Store, useValue: mockStore }, { provide: RemoteDataBuildService, useValue: rdbService }, @@ -82,6 +86,8 @@ describe('AuthService test', () => { ], }); authService = TestBed.get(AuthService); + routeServiceMock = TestBed.get(RouteService); + spyOn(authService, 'setRedirectUrl'); }); it('should return the authentication status object when user credentials are correct', () => { @@ -124,6 +130,31 @@ describe('AuthService test', () => { expect(authService.logout.bind(null)).toThrow(); }); + it ('should dispatch authentication', () => { + authService.doAuthentication(true, '', ''); + expect(mockStore.dispatch).toHaveBeenCalled(); + }); + + it ('should set redirect url to previous page', () => { + spyOn(routeServiceMock, 'getHistory').and.callThrough(); + authService.doAuthentication(true, '', ''); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(authService.setRedirectUrl).toHaveBeenCalledWith('/collection/123'); + }); + + it ('should set redirect url to current page', () => { + spyOn(routeServiceMock, 'getHistory').and.callThrough(); + authService.doAuthentication(false, '', ''); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(authService.setRedirectUrl).toHaveBeenCalledWith('/home'); + }); + + it ('should not set redirect url to /login', () => { + spyOn(routeServiceMock, 'getHistory').and.returnValue(of(['/login', '/login'])); + authService.doAuthentication(true, '', ''); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(authService.setRedirectUrl).not.toHaveBeenCalled(); + }); }); describe('', () => { @@ -138,6 +169,7 @@ describe('AuthService test', () => { { provide: AuthRequestService, useValue: authRequest }, { provide: REQUEST, useValue: {} }, { provide: Router, useValue: routerStub }, + { provide: RouteService, useValue: routeServiceStub }, { provide: RemoteDataBuildService, useValue: rdbService }, CookieService, AuthService @@ -145,13 +177,13 @@ describe('AuthService test', () => { }).compileComponents(); })); - beforeEach(inject([CookieService, AuthRequestService, Store, Router], (cookieService: CookieService, authReqService: AuthRequestService, store: Store, router: Router) => { + beforeEach(inject([CookieService, AuthRequestService, Store, Router, RouteService], (cookieService: CookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService) => { store .subscribe((state) => { (state as any).core = Object.create({}); (state as any).core.auth = authenticatedState; }); - authService = new AuthService({}, window, undefined, authReqService, router, cookieService, store, rdbService); + authService = new AuthService({}, window, undefined, authReqService, router, routeService, cookieService, store, rdbService); })); it('should return true when user is logged in', () => { @@ -189,6 +221,7 @@ describe('AuthService test', () => { { provide: AuthRequestService, useValue: authRequest }, { provide: REQUEST, useValue: {} }, { provide: Router, useValue: routerStub }, + { provide: RouteService, useValue: routeServiceStub }, { provide: RemoteDataBuildService, useValue: rdbService }, ClientCookieService, CookieService, @@ -197,7 +230,7 @@ describe('AuthService test', () => { }).compileComponents(); })); - beforeEach(inject([ClientCookieService, AuthRequestService, Store, Router], (cookieService: ClientCookieService, authReqService: AuthRequestService, store: Store, router: Router) => { + beforeEach(inject([ClientCookieService, AuthRequestService, Store, Router, RouteService], (cookieService: ClientCookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService) => { const expiredToken: AuthTokenInfo = new AuthTokenInfo('test_token'); expiredToken.expires = Date.now() - (1000 * 60 * 60); authenticatedState = { @@ -212,7 +245,7 @@ describe('AuthService test', () => { (state as any).core = Object.create({}); (state as any).core.auth = authenticatedState; }); - authService = new AuthService({}, window, undefined, authReqService, router, cookieService, store, rdbService); + authService = new AuthService({}, window, undefined, authReqService, router, routeService, cookieService, store, rdbService); storage = (authService as any).storage; spyOn(storage, 'get'); spyOn(storage, 'remove'); diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 08c94b02f2..615fee5d56 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -22,6 +22,7 @@ import { ResetAuthenticationMessagesAction, SetRedirectUrlAction } from './auth. import { NativeWindowRef, NativeWindowService } from '../services/window.service'; import { Base64EncodeUrl } from '../../shared/utils/encode-decode.util'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; +import {RouteService} from '../services/route.service'; export const LOGIN_ROUTE = '/login'; export const LOGOUT_ROUTE = '/logout'; @@ -45,6 +46,7 @@ export class AuthService { protected authRequestService: AuthRequestService, @Optional() @Inject(RESPONSE) private response: any, protected router: Router, + protected routeService: RouteService, protected storage: CookieService, protected store: Store, protected rdbService: RemoteDataBuildService @@ -337,7 +339,7 @@ export class AuthService { /** * Redirect to the route navigated before the login */ - public redirectToPreviousUrl() { + public redirectToPreviousUrl(isStandalonePage: boolean) { this.getRedirectUrl().pipe( take(1)) .subscribe((redirectUrl) => { @@ -346,18 +348,39 @@ export class AuthService { this.clearRedirectUrl(); this.router.onSameUrlNavigation = 'reload'; const url = decodeURIComponent(redirectUrl); - this.router.navigateByUrl(url); - /* TODO Reenable hard redirect when REST API can handle x-forwarded-for, see https://github.com/DSpace/DSpace/pull/2207 */ - // this._window.nativeWindow.location.href = url; + this.navigateToRedirectUrl(url); } else { - this.router.navigate(['/']); - /* TODO Reenable hard redirect when REST API can handle x-forwarded-for, see https://github.com/DSpace/DSpace/pull/2207 */ - // this._window.nativeWindow.location.href = '/'; + // If redirectUrl is empty use history. + this.routeService.getHistory().pipe( + take(1) + ).subscribe((history) => { + let redirUrl; + if (isStandalonePage) { + // For standalone login pages, use the previous route. + redirUrl = history[history.length - 2] || ''; + } else { + redirUrl = history[history.length - 1] || ''; + } + this.navigateToRedirectUrl(redirUrl); + }); } - }) + }); } + private navigateToRedirectUrl(url: string) { + // in case the user navigates directly to /login (via bookmark, etc), or the route history is not found. + if (isEmpty(url) || url.startsWith(LOGIN_ROUTE)) { + this.router.navigate(['/']); + /* TODO Reenable hard redirect when REST API can handle x-forwarded-for, see https://github.com/DSpace/DSpace/pull/2207 */ + // this._window.nativeWindow.location.href = '/'; + } else { + /* TODO Reenable hard redirect when REST API can handle x-forwarded-for, see https://github.com/DSpace/DSpace/pull/2207 */ + // this._window.nativeWindow.location.href = url; + this.router.navigate([url]); + } + } + /** * Refresh route navigated */ @@ -400,4 +423,5 @@ export class AuthService { this.store.dispatch(new SetRedirectUrlAction('')); this.storage.remove(REDIRECT_COOKIE); } + } diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts index 8ad423629d..6df992fff7 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -16,8 +16,6 @@ import { AppState } from '../../app.reducer'; import {AppRoutingModule} from '../../app-routing.module'; import {PageNotFoundComponent} from '../../pagenotfound/pagenotfound.component'; import {APP_BASE_HREF} from '@angular/common'; -import {HostWindowService} from '../host-window.service'; -import {HostWindowServiceStub} from '../testing/host-window-service-stub'; import {RouterStub} from '../testing/router-stub'; import {Router} from '@angular/router'; import {RouteService} from '../services/route.service'; @@ -110,135 +108,6 @@ describe('LogInComponent', () => { expect(page.navigateSpy.calls.any()).toBe(true, 'Store.dispatch not invoked'); }); - it('should set the redirect url', () => { - fixture.detectChanges(); - - // set FormControl values - component.form.controls.email.setValue('user'); - component.form.controls.password.setValue('password'); - - const authService: AuthService = TestBed.get(AuthService); - spyOn(authService, 'setRedirectUrl'); - - component.submit(); - - // the redirect url should be set upon submit - expect(authService.setRedirectUrl).toHaveBeenCalled(); - }); - - it('should not set the redirect url to /login', () => { - fixture.detectChanges(); - - const router: Router = TestBed.get(Router); - router.navigateByUrl('/login') - - const authService: AuthService = TestBed.get(AuthService); - - // set FormControl values - component.form.controls.email.setValue('user'); - component.form.controls.password.setValue('password'); - - spyOn(authService, 'setRedirectUrl'); - - component.submit(); - - expect(authService.setRedirectUrl).not.toHaveBeenCalled(); - }); - - it('should not set the redirect url on init', () => { - - const authService: AuthService = TestBed.get(AuthService); - spyOn(authService, 'setRedirectUrl'); - - component.isStandalonePage = false; - - fixture.detectChanges(); - expect(authService.setRedirectUrl).not.toHaveBeenCalledWith(); - - }); - -}); - -describe('LogInComponent on small screen', () => { - - let component: LogInComponent; - let fixture: ComponentFixture; - let page: Page; - let user: EPerson; - - const authState = { - authenticated: false, - loaded: false, - loading: false, - }; - - beforeEach(() => { - user = EPersonMock; - }); - - beforeEach(async(() => { - // refine the test module by declaring the test component - TestBed.configureTestingModule({ - imports: [ - FormsModule, - ReactiveFormsModule, - StoreModule.forRoot(authReducer), - AppRoutingModule, - TranslateModule.forRoot() - ], - declarations: [ - LogInComponent, - PageNotFoundComponent - ], - providers: [ - {provide: AuthService, useClass: AuthServiceStub}, - {provide: APP_BASE_HREF, useValue: '/'}, - {provide: Router, useClass: RouterStub}, - {provide: RouteService, useValue: routeServiceStub } - ], - schemas: [ - CUSTOM_ELEMENTS_SCHEMA - ] - }) - .compileComponents(); - - })); - - beforeEach(inject([Store], (store: Store) => { - store - .subscribe((state) => { - (state as any).core = Object.create({}); - (state as any).core.auth = authState; - }); - - // create component and test fixture - fixture = TestBed.createComponent(LogInComponent); - - // get test component from the fixture - component = fixture.componentInstance; - - // create page - page = new Page(component, fixture); - - // verify the fixture is stable (no pending tasks) - fixture.whenStable().then(() => { - page.addPageElements(); - }); - - })); - - it('should set the redirect url on init', () => { - const authService: AuthService = TestBed.get(AuthService); - spyOn(authService, 'setRedirectUrl'); - component.isStandalonePage = true; - fixture.detectChanges(); - // set FormControl values - component.form.controls.email.setValue('user'); - component.form.controls.password.setValue('password'); - expect(authService.setRedirectUrl).toHaveBeenCalledWith('collection/123'); - - }); - }); /** diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index 3a4ff151bf..aa798548f4 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -1,9 +1,9 @@ -import {filter, map, pairwise, take, takeUntil, takeWhile, tap} from 'rxjs/operators'; +import {filter, map, takeWhile } from 'rxjs/operators'; import {Component, Input, OnDestroy, OnInit} from '@angular/core'; import { FormBuilder, FormGroup, Validators } from '@angular/forms'; import { select, Store } from '@ngrx/store'; -import {Observable, Subject} from 'rxjs'; +import {Observable} from 'rxjs'; import { AuthenticateAction, ResetAuthenticationMessagesAction @@ -19,10 +19,8 @@ import { CoreState } from '../../core/core.reducers'; import {isNotEmpty} from '../empty.util'; import { fadeOut } from '../animations/fade'; -import {AuthService, LOGIN_ROUTE} from '../../core/auth/auth.service'; +import {AuthService} from '../../core/auth/auth.service'; import {Router} from '@angular/router'; -import {HostWindowService} from '../host-window.service'; -import {RouteService} from '../services/route.service'; /** * /users/sign-in @@ -84,8 +82,6 @@ export class LogInComponent implements OnDestroy, OnInit { */ private alive = true; - private redirectUrl = LOGIN_ROUTE; - @Input() isStandalonePage: boolean; /** @@ -99,7 +95,6 @@ export class LogInComponent implements OnDestroy, OnInit { constructor( private authService: AuthService, private formBuilder: FormBuilder, - private routeService: RouteService, private router: Router, private store: Store ) { @@ -113,16 +108,6 @@ export class LogInComponent implements OnDestroy, OnInit { // set isAuthenticated this.isAuthenticated = this.store.pipe(select(isAuthenticated)); - // for mobile login, set the redirect url to the previous route - if (this.isStandalonePage) { - this.routeService.getHistory().pipe( - take(1) - ).subscribe((history) => { - const previousIndex = history.length - 2; - this.setRedirectUrl(history[previousIndex]); - }); - } - // set formGroup this.form = this.formBuilder.group({ email: ['', Validators.required], @@ -156,7 +141,7 @@ export class LogInComponent implements OnDestroy, OnInit { takeWhile(() => this.alive), filter((authenticated) => authenticated)) .subscribe(() => { - this.authService.redirectToPreviousUrl(); + this.authService.redirectToPreviousUrl(this.isStandalonePage); } ); } @@ -203,21 +188,11 @@ export class LogInComponent implements OnDestroy, OnInit { email.trim(); password.trim(); - this.setRedirectUrl(this.router.url); // dispatch AuthenticationAction this.store.dispatch(new AuthenticateAction(email, password)); + // clear form this.form.reset(); - } - /** - * Sets the redirect url if not equal to LOGIN_ROUTE - * @param url - */ - private setRedirectUrl(url: string) { - if (url !== this.redirectUrl) { - this.authService.setRedirectUrl(url) - } - } } diff --git a/src/app/shared/testing/auth-service-stub.ts b/src/app/shared/testing/auth-service-stub.ts index a6d24d5c8b..c5f9d92a53 100644 --- a/src/app/shared/testing/auth-service-stub.ts +++ b/src/app/shared/testing/auth-service-stub.ts @@ -96,6 +96,9 @@ export class AuthServiceStub { return observableOf(this.redirectUrl); } + public doAuthentication(isStandalonePage, email, password) { + return; + } public storeToken(token: AuthTokenInfo) { return; } diff --git a/src/app/shared/testing/route-service-stub.ts b/src/app/shared/testing/route-service-stub.ts index 22e039a815..a493f10a13 100644 --- a/src/app/shared/testing/route-service-stub.ts +++ b/src/app/shared/testing/route-service-stub.ts @@ -29,7 +29,7 @@ export const routeServiceStub: any = { return observableOf({}) }, getHistory: () => { - return observableOf(['/home','collection/123','/login']) + return observableOf(['/home','/collection/123','/home']) } /* tslint:enable:no-empty */ }; From d9fb68dce97405c890c2bc15ba2164eff98b22bf Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Fri, 6 Sep 2019 12:18:28 -0700 Subject: [PATCH 08/14] Modified redirectToPreviousUrl to use the standalone page parameter if no redirect url is found in the store. Removed unused import that was causing merge conflict. Once again try to fix merge conflict. Added routeService to server module providers. Changed order of providers. Minor change to ServerAuthService to make method signature consistent with AuthService. Try adding RouteService to browser-app module to see if that fixes travis build. One more try at getting the CI build to work. Removed change to browser module. --- src/app/core/auth/auth.service.spec.ts | 57 ++++++++++--------- src/app/core/auth/server-auth.service.ts | 2 +- .../shared/log-in/log-in.component.spec.ts | 16 +----- src/app/shared/log-in/log-in.component.ts | 1 - src/modules/app/server-app.module.ts | 2 +- 5 files changed, 34 insertions(+), 44 deletions(-) diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index 6044e9ecb6..380c50d64b 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -86,8 +86,6 @@ describe('AuthService test', () => { ], }); authService = TestBed.get(AuthService); - routeServiceMock = TestBed.get(RouteService); - spyOn(authService, 'setRedirectUrl'); }); it('should return the authentication status object when user credentials are correct', () => { @@ -130,31 +128,6 @@ describe('AuthService test', () => { expect(authService.logout.bind(null)).toThrow(); }); - it ('should dispatch authentication', () => { - authService.doAuthentication(true, '', ''); - expect(mockStore.dispatch).toHaveBeenCalled(); - }); - - it ('should set redirect url to previous page', () => { - spyOn(routeServiceMock, 'getHistory').and.callThrough(); - authService.doAuthentication(true, '', ''); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); - expect(authService.setRedirectUrl).toHaveBeenCalledWith('/collection/123'); - }); - - it ('should set redirect url to current page', () => { - spyOn(routeServiceMock, 'getHistory').and.callThrough(); - authService.doAuthentication(false, '', ''); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); - expect(authService.setRedirectUrl).toHaveBeenCalledWith('/home'); - }); - - it ('should not set redirect url to /login', () => { - spyOn(routeServiceMock, 'getHistory').and.returnValue(of(['/login', '/login'])); - authService.doAuthentication(true, '', ''); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); - expect(authService.setRedirectUrl).not.toHaveBeenCalled(); - }); }); describe('', () => { @@ -247,9 +220,12 @@ describe('AuthService test', () => { }); authService = new AuthService({}, window, undefined, authReqService, router, routeService, cookieService, store, rdbService); storage = (authService as any).storage; + routeServiceMock = TestBed.get(RouteService); + routerStub = TestBed.get(Router); spyOn(storage, 'get'); spyOn(storage, 'remove'); spyOn(storage, 'set'); + })); it('should throw false when token is not valid', () => { @@ -271,5 +247,32 @@ describe('AuthService test', () => { expect(storage.remove).toHaveBeenCalled(); }); + it ('should set redirect url to previous page', () => { + spyOn(routeServiceMock, 'getHistory').and.callThrough(); + authService.redirectToPreviousUrl(true); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(routerStub.navigate).toHaveBeenCalledWith(['/collection/123']); + }); + + it ('should set redirect url to current page', () => { + spyOn(routeServiceMock, 'getHistory').and.callThrough(); + authService.redirectToPreviousUrl(false); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(routerStub.navigate).toHaveBeenCalledWith(['/home']); + }); + + it ('should redirect to / and not to /login', () => { + spyOn(routeServiceMock, 'getHistory').and.returnValue(of(['/login', '/login'])); + authService.redirectToPreviousUrl(true); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(routerStub.navigate).toHaveBeenCalledWith(['/']); + }); + + it ('should redirect to / when no redirect url is found', () => { + spyOn(routeServiceMock, 'getHistory').and.returnValue(of([''])); + authService.redirectToPreviousUrl(true); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(routerStub.navigate).toHaveBeenCalledWith(['/']); + }); }); }); diff --git a/src/app/core/auth/server-auth.service.ts b/src/app/core/auth/server-auth.service.ts index c344683e38..50e3bc53e1 100644 --- a/src/app/core/auth/server-auth.service.ts +++ b/src/app/core/auth/server-auth.service.ts @@ -54,7 +54,7 @@ export class ServerAuthService extends AuthService { /** * Redirect to the route navigated before the login */ - public redirectToPreviousUrl() { + public redirectToPreviousUrl(isStandalonePage: boolean) { this.getRedirectUrl().pipe( take(1)) .subscribe((redirectUrl) => { diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts index 6df992fff7..c3bee04d76 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -13,13 +13,6 @@ import { TranslateModule } from '@ngx-translate/core'; import { AuthService } from '../../core/auth/auth.service'; import { AuthServiceStub } from '../testing/auth-service-stub'; import { AppState } from '../../app.reducer'; -import {AppRoutingModule} from '../../app-routing.module'; -import {PageNotFoundComponent} from '../../pagenotfound/pagenotfound.component'; -import {APP_BASE_HREF} from '@angular/common'; -import {RouterStub} from '../testing/router-stub'; -import {Router} from '@angular/router'; -import {RouteService} from '../services/route.service'; -import {routeServiceStub} from '../testing/route-service-stub'; describe('LogInComponent', () => { @@ -45,18 +38,13 @@ describe('LogInComponent', () => { FormsModule, ReactiveFormsModule, StoreModule.forRoot(authReducer), - AppRoutingModule, TranslateModule.forRoot() ], declarations: [ - LogInComponent, - PageNotFoundComponent + LogInComponent ], providers: [ - {provide: AuthService, useClass: AuthServiceStub}, - {provide: APP_BASE_HREF, useValue: '/'}, - {provide: Router, useClass: RouterStub}, - {provide: RouteService, useValue: routeServiceStub } + {provide: AuthService, useClass: AuthServiceStub} ], schemas: [ CUSTOM_ELEMENTS_SCHEMA diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index aa798548f4..c7f67ea28b 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -88,7 +88,6 @@ export class LogInComponent implements OnDestroy, OnInit { * @constructor * @param {AuthService} authService * @param {FormBuilder} formBuilder - * @param {RouteService} routeService * @param {Router} router * @param {Store} store */ diff --git a/src/modules/app/server-app.module.ts b/src/modules/app/server-app.module.ts index bd3379c8de..dfe936df25 100644 --- a/src/modules/app/server-app.module.ts +++ b/src/modules/app/server-app.module.ts @@ -64,7 +64,7 @@ export function createTranslateLoader() { { provide: SubmissionService, useClass: ServerSubmissionService - }, + } ] }) export class ServerAppModule { From c3102b9d437f8d4aa2e0798d53fc21f2a2c85911 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Mon, 9 Sep 2019 12:15:09 -0700 Subject: [PATCH 09/14] A little cleanup in unit tests. --- src/app/shared/log-in/log-in.component.spec.ts | 2 +- src/app/shared/testing/auth-service-stub.ts | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts index c3bee04d76..13f9e5369a 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -3,7 +3,7 @@ import { async, ComponentFixture, inject, TestBed } from '@angular/core/testing' import { FormGroup, FormsModule, ReactiveFormsModule } from '@angular/forms'; import { By } from '@angular/platform-browser'; -import { Store, StoreModule, select } from '@ngrx/store'; +import { Store, StoreModule } from '@ngrx/store'; import { LogInComponent } from './log-in.component'; import { authReducer } from '../../core/auth/auth.reducer'; diff --git a/src/app/shared/testing/auth-service-stub.ts b/src/app/shared/testing/auth-service-stub.ts index c5f9d92a53..a6d24d5c8b 100644 --- a/src/app/shared/testing/auth-service-stub.ts +++ b/src/app/shared/testing/auth-service-stub.ts @@ -96,9 +96,6 @@ export class AuthServiceStub { return observableOf(this.redirectUrl); } - public doAuthentication(isStandalonePage, email, password) { - return; - } public storeToken(token: AuthTokenInfo) { return; } From 6425f6e7d1542c08c3ab0d9d187e1158d29eb0e4 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Mon, 9 Sep 2019 13:33:51 -0700 Subject: [PATCH 10/14] Removed the router injection that slipped into auth service during rebase, test now passes. --- src/app/shared/log-in/log-in.component.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index c7f67ea28b..a6c26381e4 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -94,7 +94,6 @@ export class LogInComponent implements OnDestroy, OnInit { constructor( private authService: AuthService, private formBuilder: FormBuilder, - private router: Router, private store: Store ) { } From 8fe617bd74dd1b6b7ccc357632c103c7401be954 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Mon, 16 Sep 2019 13:14:45 -0700 Subject: [PATCH 11/14] Changed method name. --- src/app/core/auth/auth.service.spec.ts | 8 ++++---- src/app/core/auth/auth.service.ts | 2 +- src/app/core/auth/server-auth.service.ts | 2 +- src/app/shared/log-in/log-in.component.ts | 14 +++++++------- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index 380c50d64b..a5c9f0e3a8 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -249,28 +249,28 @@ describe('AuthService test', () => { it ('should set redirect url to previous page', () => { spyOn(routeServiceMock, 'getHistory').and.callThrough(); - authService.redirectToPreviousUrl(true); + authService.redirectAfterLoginSuccess(true); expect(routeServiceMock.getHistory).toHaveBeenCalled(); expect(routerStub.navigate).toHaveBeenCalledWith(['/collection/123']); }); it ('should set redirect url to current page', () => { spyOn(routeServiceMock, 'getHistory').and.callThrough(); - authService.redirectToPreviousUrl(false); + authService.redirectAfterLoginSuccess(false); expect(routeServiceMock.getHistory).toHaveBeenCalled(); expect(routerStub.navigate).toHaveBeenCalledWith(['/home']); }); it ('should redirect to / and not to /login', () => { spyOn(routeServiceMock, 'getHistory').and.returnValue(of(['/login', '/login'])); - authService.redirectToPreviousUrl(true); + authService.redirectAfterLoginSuccess(true); expect(routeServiceMock.getHistory).toHaveBeenCalled(); expect(routerStub.navigate).toHaveBeenCalledWith(['/']); }); it ('should redirect to / when no redirect url is found', () => { spyOn(routeServiceMock, 'getHistory').and.returnValue(of([''])); - authService.redirectToPreviousUrl(true); + authService.redirectAfterLoginSuccess(true); expect(routeServiceMock.getHistory).toHaveBeenCalled(); expect(routerStub.navigate).toHaveBeenCalledWith(['/']); }); diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 615fee5d56..b3b95ce6ea 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -339,7 +339,7 @@ export class AuthService { /** * Redirect to the route navigated before the login */ - public redirectToPreviousUrl(isStandalonePage: boolean) { + public redirectAfterLoginSuccess(isStandalonePage: boolean) { this.getRedirectUrl().pipe( take(1)) .subscribe((redirectUrl) => { diff --git a/src/app/core/auth/server-auth.service.ts b/src/app/core/auth/server-auth.service.ts index 50e3bc53e1..7e6876e43f 100644 --- a/src/app/core/auth/server-auth.service.ts +++ b/src/app/core/auth/server-auth.service.ts @@ -54,7 +54,7 @@ export class ServerAuthService extends AuthService { /** * Redirect to the route navigated before the login */ - public redirectToPreviousUrl(isStandalonePage: boolean) { + public redirectAfterLoginSuccess(isStandalonePage: boolean) { this.getRedirectUrl().pipe( take(1)) .subscribe((redirectUrl) => { diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index a6c26381e4..b6b97230dd 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -1,9 +1,9 @@ -import {filter, map, takeWhile } from 'rxjs/operators'; -import {Component, Input, OnDestroy, OnInit} from '@angular/core'; +import { filter, map, takeWhile } from 'rxjs/operators'; +import { Component, Input, OnDestroy, OnInit } from '@angular/core'; import { FormBuilder, FormGroup, Validators } from '@angular/forms'; import { select, Store } from '@ngrx/store'; -import {Observable} from 'rxjs'; +import { Observable } from 'rxjs'; import { AuthenticateAction, ResetAuthenticationMessagesAction @@ -17,10 +17,10 @@ import { } from '../../core/auth/selectors'; import { CoreState } from '../../core/core.reducers'; -import {isNotEmpty} from '../empty.util'; +import { isNotEmpty } from '../empty.util'; import { fadeOut } from '../animations/fade'; -import {AuthService} from '../../core/auth/auth.service'; -import {Router} from '@angular/router'; +import { AuthService } from '../../core/auth/auth.service'; +import { Router } from '@angular/router'; /** * /users/sign-in @@ -139,7 +139,7 @@ export class LogInComponent implements OnDestroy, OnInit { takeWhile(() => this.alive), filter((authenticated) => authenticated)) .subscribe(() => { - this.authService.redirectToPreviousUrl(this.isStandalonePage); + this.authService.redirectAfterLoginSuccess(this.isStandalonePage); } ); } From e70b9c59947ae4e4a8c2fc5d9d07b5740505121c Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 17 Sep 2019 12:44:02 -0700 Subject: [PATCH 12/14] Added new auth redirect code for ssr. --- src/app/core/auth/auth.service.ts | 2 +- src/app/core/auth/server-auth.service.ts | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index b3b95ce6ea..5287e537ee 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -368,7 +368,7 @@ export class AuthService { } - private navigateToRedirectUrl(url: string) { + protected navigateToRedirectUrl(url: string) { // in case the user navigates directly to /login (via bookmark, etc), or the route history is not found. if (isEmpty(url) || url.startsWith(LOGIN_ROUTE)) { this.router.navigate(['/']); diff --git a/src/app/core/auth/server-auth.service.ts b/src/app/core/auth/server-auth.service.ts index 7e6876e43f..cf4d4a658e 100644 --- a/src/app/core/auth/server-auth.service.ts +++ b/src/app/core/auth/server-auth.service.ts @@ -1,12 +1,12 @@ -import { map, switchMap, take } from 'rxjs/operators'; +import { filter, map, switchMap, take } from 'rxjs/operators'; import { Injectable } from '@angular/core'; import { Observable } from 'rxjs'; import { HttpHeaders } from '@angular/common/http'; import { HttpOptions } from '../dspace-rest-v2/dspace-rest-v2.service'; import { AuthStatus } from './models/auth-status.model'; -import { isNotEmpty } from '../../shared/empty.util'; -import { AuthService } from './auth.service'; +import { isEmpty, isNotEmpty } from '../../shared/empty.util'; +import { AuthService, LOGIN_ROUTE } from './auth.service'; import { AuthTokenInfo } from './models/auth-token-info.model'; import { CheckAuthenticationTokenAction } from './auth.actions'; import { EPerson } from '../eperson/models/eperson.model'; @@ -67,10 +67,15 @@ export class ServerAuthService extends AuthService { const url = decodeURIComponent(redirectUrl); this.router.navigateByUrl(url); } else { - this.router.navigate(['/']); + // If redirectUrl is empty use history. For ssr the history array should contain the requested url. + this.routeService.getHistory().pipe( + filter((history) => history.length > 0), + take(1) + ).subscribe((history) => { + this.navigateToRedirectUrl(history[history.length - 1] || ''); + }); } }) - } } From 73dd0147bc32af8faf626131f03d293a788bfc3d Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 18 Sep 2019 10:37:12 -0700 Subject: [PATCH 13/14] Minor update of the imports in unit test. --- src/app/core/auth/auth.service.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index a5c9f0e3a8..4ece5eeadd 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -4,7 +4,7 @@ import { ActivatedRoute, Router } from '@angular/router'; import { Store, StoreModule } from '@ngrx/store'; import { REQUEST } from '@nguniversal/express-engine/tokens'; -import {of, of as observableOf} from 'rxjs'; +import { of, of as observableOf } from 'rxjs'; import { authReducer, AuthState } from './auth.reducer'; import { NativeWindowRef, NativeWindowService } from '../services/window.service'; @@ -23,8 +23,8 @@ import { AppState } from '../../app.reducer'; import { ClientCookieService } from '../services/client-cookie.service'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; import { getMockRemoteDataBuildService } from '../../shared/mocks/mock-remote-data-build.service'; -import {routeServiceStub} from '../../shared/testing/route-service-stub'; -import {RouteService} from '../services/route.service'; +import { routeServiceStub } from '../../shared/testing/route-service-stub'; +import { RouteService } from '../services/route.service'; describe('AuthService test', () => { From ca048736b51ead7f90b3fdebe2ecc150c0f1fc81 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 18 Sep 2019 12:13:41 -0700 Subject: [PATCH 14/14] Removed unnecessary import in unit test. --- src/app/core/auth/auth.service.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index 4ece5eeadd..5084dc8596 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -4,7 +4,7 @@ import { ActivatedRoute, Router } from '@angular/router'; import { Store, StoreModule } from '@ngrx/store'; import { REQUEST } from '@nguniversal/express-engine/tokens'; -import { of, of as observableOf } from 'rxjs'; +import { of as observableOf } from 'rxjs'; import { authReducer, AuthState } from './auth.reducer'; import { NativeWindowRef, NativeWindowService } from '../services/window.service'; @@ -262,14 +262,14 @@ describe('AuthService test', () => { }); it ('should redirect to / and not to /login', () => { - spyOn(routeServiceMock, 'getHistory').and.returnValue(of(['/login', '/login'])); + spyOn(routeServiceMock, 'getHistory').and.returnValue(observableOf(['/login', '/login'])); authService.redirectAfterLoginSuccess(true); expect(routeServiceMock.getHistory).toHaveBeenCalled(); expect(routerStub.navigate).toHaveBeenCalledWith(['/']); }); it ('should redirect to / when no redirect url is found', () => { - spyOn(routeServiceMock, 'getHistory').and.returnValue(of([''])); + spyOn(routeServiceMock, 'getHistory').and.returnValue(observableOf([''])); authService.redirectAfterLoginSuccess(true); expect(routeServiceMock.getHistory).toHaveBeenCalled(); expect(routerStub.navigate).toHaveBeenCalledWith(['/']);