diff --git a/src/app/core/data/dso-redirect.service.spec.ts b/src/app/core/data/dso-redirect.service.spec.ts index ca064b5608..2122dc663a 100644 --- a/src/app/core/data/dso-redirect.service.spec.ts +++ b/src/app/core/data/dso-redirect.service.spec.ts @@ -10,6 +10,7 @@ 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; @@ -17,7 +18,7 @@ describe('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'; @@ -38,9 +39,6 @@ describe('DsoRedirectService', () => { generateRequestId: requestUUID, send: true }); - router = { - navigate: jasmine.createSpy('navigate') - }; remoteData = createSuccessfulRemoteDataObject(Object.assign(new Item(), { type: 'item', @@ -52,12 +50,17 @@ describe('DsoRedirectService', () => { a: remoteData }) }); + + redirectService = jasmine.createSpyObj('redirectService', { + redirect: {} + }); + service = new DsoRedirectService( requestService, rdbService, objectCache, halService, - router, + redirectService ); }); @@ -104,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'; @@ -121,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', () => { @@ -130,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', () => { @@ -139,7 +142,7 @@ describe('DsoRedirectService', () => { redir.subscribe(); scheduler.schedule(() => redir); scheduler.flush(); - expect(router.navigate).toHaveBeenCalledWith(['/communities/' + remoteData.payload.uuid]); + expect(redirectService.redirect).toHaveBeenCalledWith('/communities/' + remoteData.payload.uuid, 301); }); }); diff --git a/src/app/core/data/dso-redirect.service.ts b/src/app/core/data/dso-redirect.service.ts index 81ce678e43..a27d1fb11f 100644 --- a/src/app/core/data/dso-redirect.service.ts +++ b/src/app/core/data/dso-redirect.service.ts @@ -7,7 +7,6 @@ */ /* eslint-disable max-classes-per-file */ import { Injectable } from '@angular/core'; -import { Router } from '@angular/router'; import { Observable } from 'rxjs'; import { tap } from 'rxjs/operators'; import { hasValue } from '../../shared/empty.util'; @@ -21,6 +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 { HardRedirectService } from '../services/hard-redirect.service'; const ID_ENDPOINT = 'pid'; const UUID_ENDPOINT = 'dso'; @@ -74,13 +74,16 @@ export class DsoRedirectService { protected rdbService: RemoteDataBuildService, protected objectCache: ObjectCacheService, protected halService: HALEndpointService, - private router: Router, + private hardRedirectService: HardRedirectService ) { this.dataService = new DsoByIdOrUUIDDataService(requestService, rdbService, objectCache, halService); } /** - * Retrieve a DSpaceObject by + * Redirect to a DSpaceObject's path using the given identifier type and ID. + * This is used to redirect paths like "/handle/[prefix]/[suffix]" to the object's path (e.g. /items/[uuid]). + * See LookupGuard for more examples. + * * @param id the identifier of the object to retrieve * @param identifierType the type of the given identifier (defaults to UUID) */ @@ -94,7 +97,8 @@ export class DsoRedirectService { if (hasValue(dso.uuid)) { let newRoute = getDSORoute(dso); if (hasValue(newRoute)) { - this.router.navigate([newRoute]); + // Use a "301 Moved Permanently" redirect for SEO purposes + this.hardRedirectService.redirect(newRoute, 301); } } } diff --git a/src/app/core/services/hard-redirect.service.ts b/src/app/core/services/hard-redirect.service.ts index 3733059283..826c7e4fa9 100644 --- a/src/app/core/services/hard-redirect.service.ts +++ b/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 diff --git a/src/app/core/services/server-hard-redirect.service.spec.ts b/src/app/core/services/server-hard-redirect.service.spec.ts index 4501547b92..6bd5828921 100644 --- a/src/app/core/services/server-hard-redirect.service.spec.ts +++ b/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(() => { diff --git a/src/app/core/services/server-hard-redirect.service.ts b/src/app/core/services/server-hard-redirect.service.ts index de8b45b0e5..8c45cc864b 100644 --- a/src/app/core/services/server-hard-redirect.service.ts +++ b/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;