Refactor to use HardRedirectService. Update its redirect method to support optional statusCode

Author: tdonohue

src/app/core/data/dso-redirect.service.spec.ts

@@ -10,14 +10,15 @@ import { RequestService } from './request.service';
 import { createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils';
 import { Item } from '../shared/item.model';
 import { EMBED_SEPARATOR } from './base/base-data.service';
+import { HardRedirectService } from '../services/hard-redirect.service';
 
 describe('DsoRedirectService', () => {
   let scheduler: TestScheduler;
   let service: DsoRedirectService;
   let halService: HALEndpointService;
   let requestService: RequestService;
   let rdbService: RemoteDataBuildService;
-  let router;
+  let redirectService: HardRedirectService;
   let remoteData;
   const dsoUUID = '9b4f22f4-164a-49db-8817-3316b6ee5746';
   const dsoHandle = '1234567789/22';
@@ -27,9 +28,6 @@ describe('DsoRedirectService', () => {
   const requestUUIDURL = `https://rest.api/rest/api/pid/find?id=${dsoUUID}`;
   const requestUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2';
   const objectCache = {} as ObjectCacheService;
-  const mockResponse = jasmine.createSpyObj(['redirect']);
-  const mockPlatformBrowser = 'browser';
-  const mockPlatformServer = 'server';
 
   beforeEach(() => {
     scheduler = getTestScheduler();
@@ -41,9 +39,6 @@ describe('DsoRedirectService', () => {
       generateRequestId: requestUUID,
       send: true
     });
-    router = {
-      navigate: jasmine.createSpy('navigate')
-    };
 
     remoteData = createSuccessfulRemoteDataObject(Object.assign(new Item(), {
       type: 'item',
@@ -55,14 +50,17 @@ describe('DsoRedirectService', () => {
         a: remoteData
       })
     });
+
+    redirectService = jasmine.createSpyObj('redirectService', {
+      redirect: {}
+    });
+
     service = new DsoRedirectService(
       requestService,
       rdbService,
       objectCache,
       halService,
-      router,
-      mockResponse,
-      mockPlatformBrowser  // default to CSR except where explicitly SSR below
+      redirectService
     );
   });
 
@@ -109,7 +107,7 @@ describe('DsoRedirectService', () => {
       redir.subscribe();
       scheduler.schedule(() => redir);
       scheduler.flush();
-      expect(router.navigate).toHaveBeenCalledWith(['/items/' + remoteData.payload.uuid]);
+      expect(redirectService.redirect).toHaveBeenCalledWith('/items/' + remoteData.payload.uuid, 301);
     });
     it('should navigate to entities route with the corresponding entity type', () => {
       remoteData.payload.type = 'item';
@@ -126,7 +124,7 @@ describe('DsoRedirectService', () => {
       redir.subscribe();
       scheduler.schedule(() => redir);
       scheduler.flush();
-      expect(router.navigate).toHaveBeenCalledWith(['/entities/publication/' + remoteData.payload.uuid]);
+      expect(redirectService.redirect).toHaveBeenCalledWith('/entities/publication/' + remoteData.payload.uuid, 301);
     });
 
     it('should navigate to collections route', () => {
@@ -135,7 +133,7 @@ describe('DsoRedirectService', () => {
       redir.subscribe();
       scheduler.schedule(() => redir);
       scheduler.flush();
-      expect(router.navigate).toHaveBeenCalledWith(['/collections/' + remoteData.payload.uuid]);
+      expect(redirectService.redirect).toHaveBeenCalledWith('/collections/' + remoteData.payload.uuid, 301);
     });
 
     it('should navigate to communities route', () => {
@@ -144,26 +142,7 @@ describe('DsoRedirectService', () => {
       redir.subscribe();
       scheduler.schedule(() => redir);
       scheduler.flush();
-      expect(router.navigate).toHaveBeenCalledWith(['/communities/' + remoteData.payload.uuid]);
-    });
-
-    it('should return 301 redirect when SSR is used', () => {
-      service = new DsoRedirectService(
-        requestService,
-        rdbService,
-        objectCache,
-        halService,
-        router,
-        mockResponse,
-        mockPlatformServer  // explicitly SSR mode
-      );
-      remoteData.payload.type = 'item';
-      const redir = service.findByIdAndIDType(dsoHandle, IdentifierType.HANDLE);
-      // The framework would normally subscribe but do it here so we can test navigation.
-      redir.subscribe();
-      scheduler.schedule(() => redir);
-      scheduler.flush();
-      expect(mockResponse.redirect).toHaveBeenCalledWith(301, '/items/' + remoteData.payload.uuid);
+      expect(redirectService.redirect).toHaveBeenCalledWith('/communities/' + remoteData.payload.uuid, 301);
     });
   });
 

src/app/core/data/dso-redirect.service.ts

@@ -6,8 +6,7 @@
  * http://www.dspace.org/license/
  */
 /* eslint-disable max-classes-per-file */
-import { Inject, Injectable, Optional, PLATFORM_ID } from '@angular/core';
-import { Router } from '@angular/router';
+import { Injectable } from '@angular/core';
 import { Observable } from 'rxjs';
 import { tap } from 'rxjs/operators';
 import { hasValue } from '../../shared/empty.util';
@@ -21,8 +20,7 @@ import { getFirstCompletedRemoteData } from '../shared/operators';
 import { DSpaceObject } from '../shared/dspace-object.model';
 import { IdentifiableDataService } from './base/identifiable-data.service';
 import { getDSORoute } from '../../app-routing-paths';
-import { RESPONSE } from '@nguniversal/express-engine/tokens';
-import { isPlatformServer } from '@angular/common';
+import { HardRedirectService } from '../services/hard-redirect.service';
 
 const ID_ENDPOINT = 'pid';
 const UUID_ENDPOINT = 'dso';
@@ -76,9 +74,7 @@ export class DsoRedirectService {
     protected rdbService: RemoteDataBuildService,
     protected objectCache: ObjectCacheService,
     protected halService: HALEndpointService,
-    private router: Router,
-    @Optional() @Inject(RESPONSE) private response: any,
-    @Inject(PLATFORM_ID) private platformId: any
+    private hardRedirectService: HardRedirectService
   ) {
     this.dataService = new DsoByIdOrUUIDDataService(requestService, rdbService, objectCache, halService);
   }
@@ -88,9 +84,6 @@ export class DsoRedirectService {
    * This is used to redirect paths like "/handle/[prefix]/[suffix]" to the object's path (e.g. /items/[uuid]).
    * See LookupGuard for more examples.
    *
-   * If this is called server side (via SSR), it performs a 301 Redirect.
-   * If this is called client side (via CSR), it simply uses the Angular router to do the redirect.
-   *
    * @param id              the identifier of the object to retrieve
    * @param identifierType  the type of the given identifier (defaults to UUID)
    */
@@ -104,12 +97,8 @@ export class DsoRedirectService {
           if (hasValue(dso.uuid)) {
             let newRoute = getDSORoute(dso);
             if (hasValue(newRoute)) {
-              // If running via SSR, perform a "301 Moved Permanently" redirect for SEO purposes.
-              if (isPlatformServer(this.platformId)) {
-                this.response.redirect(301, newRoute);
-              } else {
-                this.router.navigate([newRoute]);
-              }
+              // Use a "301 Moved Permanently" redirect for SEO purposes
+              this.hardRedirectService.redirect(newRoute, 301);
             }
           }
         }

src/app/core/services/hard-redirect.service.ts

@@ -11,8 +11,10 @@ export abstract class HardRedirectService {
    *
    * @param url
    *    the page to redirect to
+   * @param statusCode
+   *    optional HTTP status code to use for redirect (default = 302, which is a temporary redirect)
    */
-  abstract redirect(url: string);
+  abstract redirect(url: string, statusCode?: number);
 
   /**
    * Get the current route, with query params included

src/app/core/services/server-hard-redirect.service.spec.ts

@@ -22,20 +22,33 @@ describe('ServerHardRedirectService', () => {
     expect(service).toBeTruthy();
   });
 
-  describe('when performing a redirect', () => {
-
+  describe('when performing a default redirect', () => {
     const redirect = 'test redirect';
 
     beforeEach(() => {
       service.redirect(redirect);
     });
 
-    it('should update the response object', () => {
+    it('should perform a 302 redirect', () => {
       expect(mockResponse.redirect).toHaveBeenCalledWith(302, redirect);
       expect(mockResponse.end).toHaveBeenCalled();
     });
   });
 
+  describe('when performing a 301 redirect', () => {
+    const redirect = 'test 301 redirect';
+    const redirectStatusCode = 301;
+
+    beforeEach(() => {
+      service.redirect(redirect, redirectStatusCode);
+    });
+
+    it('should redirect with passed in status code', () => {
+      expect(mockResponse.redirect).toHaveBeenCalledWith(redirectStatusCode, redirect);
+      expect(mockResponse.end).toHaveBeenCalled();
+    });
+  });
+
   describe('when requesting the current route', () => {
 
     beforeEach(() => {

src/app/core/services/server-hard-redirect.service.ts

@@ -17,10 +17,14 @@ export class ServerHardRedirectService extends HardRedirectService {
   }
 
   /**
-   * Perform a hard redirect to URL
+   * Perform a hard redirect to a given location.
+   *
    * @param url
+   *    the page to redirect to
+   * @param statusCode
+   *    optional HTTP status code to use for redirect (default = 302, which is a temporary redirect)
    */
-  redirect(url: string) {
+  redirect(url: string, statusCode?: number) {
 
     if (url === this.req.url) {
       return;
@@ -38,8 +42,8 @@ export class ServerHardRedirectService extends HardRedirectService {
         process.exit(1);
       }
     } else {
-      // attempt to use the already set status
-      let status = this.res.statusCode || 0;
+      // attempt to use passed in statusCode or the already set status (in request)
+      let status = statusCode || this.res.statusCode || 0;
       if (status < 300 || status >= 400) {
         // temporary redirect
         status = 302;