Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement basic 301 Redirect for DSpace 6.x URLs when SSR is used. #2331

Merged
merged 2 commits into from
Jun 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions src/app/core/data/dso-redirect.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -38,9 +39,6 @@ describe('DsoRedirectService', () => {
generateRequestId: requestUUID,
send: true
});
router = {
navigate: jasmine.createSpy('navigate')
};

remoteData = createSuccessfulRemoteDataObject(Object.assign(new Item(), {
type: 'item',
Expand All @@ -52,12 +50,17 @@ describe('DsoRedirectService', () => {
a: remoteData
})
});

redirectService = jasmine.createSpyObj('redirectService', {
redirect: {}
});

service = new DsoRedirectService(
requestService,
rdbService,
objectCache,
halService,
router,
redirectService
);
});

Expand Down Expand Up @@ -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';
Expand All @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -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);
});
});

Expand Down
12 changes: 8 additions & 4 deletions src/app/core/data/dso-redirect.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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)
*/
Expand All @@ -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);
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion src/app/core/services/hard-redirect.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 16 additions & 3 deletions src/app/core/services/server-hard-redirect.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down
12 changes: 8 additions & 4 deletions src/app/core/services/server-hard-redirect.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
Loading