Merge pull request #1242 from atmire/w2p-80141_Support-for-legacy-bitstream-download-urls

Add support for legacy bitstream download URLs

src/app/+bitstream-page/bitstream-page-routing.module.ts

@@ -9,6 +9,7 @@ import { ResourcePolicyCreateComponent } from '../shared/resource-policies/creat
 import { ResourcePolicyResolver } from '../shared/resource-policies/resolvers/resource-policy.resolver';
 import { ResourcePolicyEditComponent } from '../shared/resource-policies/edit/resource-policy-edit.component';
 import { BitstreamAuthorizationsComponent } from './bitstream-authorizations/bitstream-authorizations.component';
+import { LegacyBitstreamUrlResolver } from './legacy-bitstream-url.resolver';
 
 const EDIT_BITSTREAM_PATH = ':id/edit';
 const EDIT_BITSTREAM_AUTHORIZATIONS_PATH = ':id/authorizations';
@@ -20,7 +21,24 @@ const EDIT_BITSTREAM_AUTHORIZATIONS_PATH = ':id/authorizations';
   imports: [
     RouterModule.forChild([
       {
-        path:':id/download',
+        // Resolve XMLUI bitstream download URLs
+        path: 'handle/:prefix/:suffix/:filename',
+        component: BitstreamDownloadPageComponent,
+        resolve: {
+          bitstream: LegacyBitstreamUrlResolver
+        },
+      },
+      {
+        // Resolve JSPUI bitstream download URLs
+        path: ':prefix/:suffix/:sequence_id/:filename',
+        component: BitstreamDownloadPageComponent,
+        resolve: {
+          bitstream: LegacyBitstreamUrlResolver
+        },
+      },
+      {
+        // Resolve angular bitstream download URLs
+        path: ':id/download',
         component: BitstreamDownloadPageComponent,
         resolve: {
           bitstream: BitstreamPageResolver

src/app/+bitstream-page/legacy-bitstream-url.resolver.spec.ts

@@ -0,0 +1,145 @@
+import { LegacyBitstreamUrlResolver } from './legacy-bitstream-url.resolver';
+import { of as observableOf, EMPTY } from 'rxjs';
+import { BitstreamDataService } from '../core/data/bitstream-data.service';
+import { RemoteData } from '../core/data/remote-data';
+import { RequestEntryState } from '../core/data/request.reducer';
+import { TestScheduler } from 'rxjs/testing';
+
+describe(`LegacyBitstreamUrlResolver`, () => {
+  let resolver: LegacyBitstreamUrlResolver;
+  let bitstreamDataService: BitstreamDataService;
+  let testScheduler;
+  let remoteDataMocks;
+  let route;
+  let state;
+
+  beforeEach(() => {
+    testScheduler = new TestScheduler((actual, expected) => {
+      expect(actual).toEqual(expected);
+    });
+
+    route = {
+      params: {},
+      queryParams: {}
+    };
+    state = {};
+    remoteDataMocks = {
+      RequestPending: new RemoteData(undefined, 0, 0, RequestEntryState.RequestPending, undefined, undefined, undefined),
+      ResponsePending: new RemoteData(undefined, 0, 0, RequestEntryState.ResponsePending, undefined, undefined, undefined),
+      Success: new RemoteData(0, 0, 0, RequestEntryState.Success, undefined, {}, 200),
+      Error: new RemoteData(0, 0, 0, RequestEntryState.Error, 'Internal server error', undefined, 500),
+    };
+    bitstreamDataService = {
+      findByItemHandle: () => undefined
+    } as any;
+    resolver = new LegacyBitstreamUrlResolver(bitstreamDataService);
+  });
+
+  describe(`resolve`, () => {
+    describe(`For JSPUI-style URLs`, () => {
+      beforeEach(() => {
+        spyOn(bitstreamDataService, 'findByItemHandle').and.returnValue(EMPTY);
+        route = Object.assign({}, route, {
+          params: {
+            prefix: '123456789',
+            suffix: '1234',
+            filename: 'some-file.pdf',
+            sequence_id: '5'
+          }
+        });
+      });
+      it(`should call findByItemHandle with the handle, sequence id, and filename from the route`, () => {
+        testScheduler.run(() => {
+          resolver.resolve(route, state);
+          expect(bitstreamDataService.findByItemHandle).toHaveBeenCalledWith(
+            `${route.params.prefix}/${route.params.suffix}`,
+            route.params.sequence_id,
+            route.params.filename
+          );
+        });
+      });
+    });
+
+    describe(`For XMLUI-style URLs`, () => {
+      describe(`when there is a sequenceId query parameter`, () => {
+        beforeEach(() => {
+          spyOn(bitstreamDataService, 'findByItemHandle').and.returnValue(EMPTY);
+          route = Object.assign({}, route, {
+            params: {
+              prefix: '123456789',
+              suffix: '1234',
+              filename: 'some-file.pdf',
+            },
+            queryParams: {
+              sequenceId: '5'
+            }
+          });
+        });
+        it(`should call findByItemHandle with the handle and filename from the route, and the sequence ID from the queryParams`, () => {
+          testScheduler.run(() => {
+            resolver.resolve(route, state);
+            expect(bitstreamDataService.findByItemHandle).toHaveBeenCalledWith(
+              `${route.params.prefix}/${route.params.suffix}`,
+              route.queryParams.sequenceId,
+              route.params.filename
+            );
+          });
+        });
+      });
+      describe(`when there's no sequenceId query parameter`, () => {
+        beforeEach(() => {
+          spyOn(bitstreamDataService, 'findByItemHandle').and.returnValue(EMPTY);
+          route = Object.assign({}, route, {
+            params: {
+              prefix: '123456789',
+              suffix: '1234',
+              filename: 'some-file.pdf',
+            },
+          });
+        });
+        it(`should call findByItemHandle with the handle, and filename from the route`, () => {
+          testScheduler.run(() => {
+            resolver.resolve(route, state);
+            expect(bitstreamDataService.findByItemHandle).toHaveBeenCalledWith(
+              `${route.params.prefix}/${route.params.suffix}`,
+              undefined,
+              route.params.filename
+            );
+          });
+        });
+      });
+    });
+    describe(`should return and complete after the remotedata has...`, () => {
+      it(`...failed`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(bitstreamDataService, 'findByItemHandle').and.returnValue(cold('a-b-c', {
+            a: remoteDataMocks.RequestPending,
+            b: remoteDataMocks.ResponsePending,
+            c: remoteDataMocks.Error,
+          }));
+          const expected = '----(c|)';
+          const values = {
+            c: remoteDataMocks.Error,
+          };
+
+          expectObservable(resolver.resolve(route, state)).toBe(expected, values);
+        });
+      });
+      it(`...succeeded`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(bitstreamDataService, 'findByItemHandle').and.returnValue(cold('a-b-c', {
+            a: remoteDataMocks.RequestPending,
+            b: remoteDataMocks.ResponsePending,
+            c: remoteDataMocks.Success,
+          }));
+          const expected = '----(c|)';
+          const values = {
+            c: remoteDataMocks.Success,
+          };
+
+          expectObservable(resolver.resolve(route, state)).toBe(expected, values);
+        });
+      });
+    });
+  });
+});

src/app/+bitstream-page/legacy-bitstream-url.resolver.ts

@@ -0,0 +1,48 @@
+import { Injectable } from '@angular/core';
+import { ActivatedRouteSnapshot, Resolve, RouterStateSnapshot } from '@angular/router';
+import { Observable } from 'rxjs';
+import { RemoteData } from '../core/data/remote-data';
+import { Bitstream } from '../core/shared/bitstream.model';
+import { getFirstCompletedRemoteData } from '../core/shared/operators';
+import { hasNoValue } from '../shared/empty.util';
+import { BitstreamDataService } from '../core/data/bitstream-data.service';
+
+/**
+ * This class resolves a bitstream based on the DSpace 6 XMLUI or JSPUI bitstream download URLs
+ */
+@Injectable({
+  providedIn: 'root'
+})
+export class LegacyBitstreamUrlResolver implements Resolve<RemoteData<Bitstream>> {
+  constructor(protected bitstreamDataService: BitstreamDataService) {
+  }
+
+  /**
+   * Resolve a bitstream based on the handle of the item, and the sequence id or the filename of the
+   * bitstream
+   *
+   * @param {ActivatedRouteSnapshot} route The current ActivatedRouteSnapshot
+   * @param {RouterStateSnapshot} state The current RouterStateSnapshot
+   * @returns Observable<<RemoteData<Item>> Emits the found bitstream based on the parameters in
+   * current route, or an error if something went wrong
+   */
+  resolve(route: ActivatedRouteSnapshot, state: RouterStateSnapshot):
+    Observable<RemoteData<Bitstream>> {
+      const prefix = route.params.prefix;
+      const suffix = route.params.suffix;
+      const filename = route.params.filename;
+
+      let sequenceId = route.params.sequence_id;
+      if (hasNoValue(sequenceId)) {
+        sequenceId = route.queryParams.sequenceId;
+      }
+
+      return this.bitstreamDataService.findByItemHandle(
+        `${prefix}/${suffix}`,
+        sequenceId,
+        filename,
+      ).pipe(
+        getFirstCompletedRemoteData()
+      );
+  }
+}

src/app/app-routing-paths.ts

@@ -10,6 +10,11 @@ import { URLCombiner } from './core/url-combiner/url-combiner';
 
 export const BITSTREAM_MODULE_PATH = 'bitstreams';
 
+/**
+ * The bitstream module path to resolve XMLUI and JSPUI bitstream download URLs
+ */
+export const LEGACY_BITSTREAM_MODULE_PATH = 'bitstream';
+
 export function getBitstreamModuleRoute() {
   return `/${BITSTREAM_MODULE_PATH}`;
 }

src/app/app-routing.module.ts

@@ -14,6 +14,7 @@ import {
   PROFILE_MODULE_PATH,
   REGISTER_PATH,
   WORKFLOW_ITEM_MODULE_PATH,
+  LEGACY_BITSTREAM_MODULE_PATH,
 } from './app-routing-paths';
 import { COLLECTION_MODULE_PATH } from './+collection-page/collection-page-routing-paths';
 import { COMMUNITY_MODULE_PATH } from './+community-page/community-page-routing-paths';
@@ -93,6 +94,12 @@ import { GroupAdministratorGuard } from './core/data/feature-authorization/featu
               .then((m) => m.ItemPageModule),
             canActivate: [EndUserAgreementCurrentUserGuard]
           },
+          {
+            path: LEGACY_BITSTREAM_MODULE_PATH,
+            loadChildren: () => import('./+bitstream-page/bitstream-page.module')
+              .then((m) => m.BitstreamPageModule),
+            canActivate: [EndUserAgreementCurrentUserGuard]
+          },
           {
             path: BITSTREAM_MODULE_PATH,
             loadChildren: () => import('./+bitstream-page/bitstream-page.module')

src/app/core/data/bitstream-data.service.ts

@@ -28,6 +28,7 @@ import { HttpOptions } from '../dspace-rest/dspace-rest.service';
 import { sendRequest } from '../shared/operators';
 import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils';
 import { PageInfo } from '../shared/page-info.model';
+import { RequestParam } from '../cache/models/request-param.model';
 
 /**
  * A service to retrieve {@link Bitstream}s from the REST API
@@ -136,4 +137,50 @@ export class BitstreamDataService extends DataService<Bitstream> {
     return this.rdbService.buildFromRequestUUID(requestId);
   }
 
+  /**
+   * Returns an observable of {@link RemoteData} of a {@link Bitstream}, based on a handle and an
+   * optional sequenceId or filename, with a list of {@link FollowLinkConfig}, to automatically
+   * resolve {@link HALLink}s of the object
+   *
+   * @param handle                      The handle of the bitstream we want to retrieve
+   * @param sequenceId                  The sequence id of the bitstream we want to retrieve
+   * @param filename                    The filename of the bitstream we want to retrieve
+   * @param useCachedVersionIfAvailable If this is true, the request will only be sent if there's
+   *                                    no valid cached version. Defaults to true
+   * @param reRequestOnStale            Whether or not the request should automatically be re-
+   *                                    requested after the response becomes stale
+   * @param linksToFollow               List of {@link FollowLinkConfig} that indicate which
+   *                                    {@link HALLink}s should be automatically resolved
+   */
+  findByItemHandle(
+    handle: string,
+    sequenceId?: string,
+    filename?: string,
+    useCachedVersionIfAvailable = true,
+    reRequestOnStale = true,
+    ...linksToFollow: FollowLinkConfig<Bitstream>[]
+  ): Observable<RemoteData<Bitstream>> {
+    const searchParams = [];
+    searchParams.push(new RequestParam('handle', handle));
+    if (hasValue(sequenceId)) {
+      searchParams.push(new RequestParam('sequenceId', sequenceId));
+    }
+    if (hasValue(filename)) {
+      searchParams.push(new RequestParam('filename', filename));
+    }
+
+    const hrefObs = this.getSearchByHref(
+      'byItemHandle',
+      { searchParams },
+      ...linksToFollow
+    );
+
+    return this.findByHref(
+      hrefObs,
+      useCachedVersionIfAvailable,
+      reRequestOnStale,
+      ...linksToFollow
+    );
+  }
+
 }

src/app/core/shared/operators.spec.ts

@@ -15,7 +15,12 @@ import {
   redirectOn4xx
 } from './operators';
 import { of as observableOf } from 'rxjs';
-import { createFailedRemoteDataObject, createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils';
+import {
+  createFailedRemoteDataObject,
+  createSuccessfulRemoteDataObject
+} from '../../shared/remote-data.utils';
+
+// tslint:disable:no-shadowed-variable
 
 describe('Core Module - RxJS Operators', () => {
   let scheduler: TestScheduler;
@@ -172,8 +177,12 @@ describe('Core Module - RxJS Operators', () => {
   describe('redirectOn4xx', () => {
     let router;
     let authService;
+    let testScheduler;
 
     beforeEach(() => {
+      testScheduler = new TestScheduler((actual, expected) => {
+        expect(actual).toEqual(expected);
+      });
       router = jasmine.createSpyObj('router', ['navigateByUrl']);
       authService = jasmine.createSpyObj('authService', {
         isAuthenticated: observableOf(true),
@@ -181,32 +190,69 @@ describe('Core Module - RxJS Operators', () => {
       });
     });
 
-    it('should call navigateByUrl to a 404 page, when the remote data contains a 404 error', () => {
-      const testRD = createFailedRemoteDataObject('Object was not found', 404);
+    it('should call navigateByUrl to a 404 page, when the remote data contains a 404 error, and not emit anything', () => {
+      testScheduler.run(({ cold, expectObservable, flush }) => {
+        const testRD = createFailedRemoteDataObject('Object was not found', 404);
+        const source = cold('a', { a: testRD });
+        const expected = '-';
+        const values = {};
 
-      observableOf(testRD).pipe(redirectOn4xx(router, authService)).subscribe();
-      expect(router.navigateByUrl).toHaveBeenCalledWith('/404', { skipLocationChange: true });
+        expectObservable(source.pipe(redirectOn4xx(router, authService))).toBe(expected, values);
+        flush();
+        expect(router.navigateByUrl).toHaveBeenCalledWith('/404', { skipLocationChange: true });
+      });
     });
 
-    it('should call navigateByUrl to a 401 page, when the remote data contains a 403 error', () => {
-      const testRD = createFailedRemoteDataObject('Forbidden', 403);
+    it('should call navigateByUrl to a 404 page, when the remote data contains a 422 error, and not emit anything', () => {
+      testScheduler.run(({ cold, expectObservable, flush }) => {
+        const testRD = createFailedRemoteDataObject('Unprocessable Entity', 422);
+        const source = cold('a', { a: testRD });
+        const expected = '-';
+        const values = {};
 
-      observableOf(testRD).pipe(redirectOn4xx(router, authService)).subscribe();
-      expect(router.navigateByUrl).toHaveBeenCalledWith('/403', { skipLocationChange: true });
+        expectObservable(source.pipe(redirectOn4xx(router, authService))).toBe(expected, values);
+        flush();
+        expect(router.navigateByUrl).toHaveBeenCalledWith('/404', { skipLocationChange: true });
+      });
     });
 
-    it('should not call navigateByUrl to a 404, 403 or 401 page, when the remote data contains another error than a 404, 403 or 401', () => {
-      const testRD = createFailedRemoteDataObject('Something went wrong', 500);
+    it('should call navigateByUrl to a 401 page, when the remote data contains a 403 error, and not emit anything', () => {
+      testScheduler.run(({ cold, expectObservable, flush }) => {
+        const testRD = createFailedRemoteDataObject('Forbidden', 403);
+        const source = cold('a', { a: testRD });
+        const expected = '-';
+        const values = {};
 
-      observableOf(testRD).pipe(redirectOn4xx(router, authService)).subscribe();
-      expect(router.navigateByUrl).not.toHaveBeenCalled();
+        expectObservable(source.pipe(redirectOn4xx(router, authService))).toBe(expected, values);
+        flush();
+        expect(router.navigateByUrl).toHaveBeenCalledWith('/403', { skipLocationChange: true });
+      });
     });
 
-    it('should not call navigateByUrl to a 404, 403 or 401 page, when the remote data contains no error', () => {
-      const testRD = createSuccessfulRemoteDataObject(undefined);
+    it('should not call navigateByUrl to a 404, 403 or 401 page, when the remote data contains another error than a 404, 422, 403 or 401, and emit the source rd', () => {
+      testScheduler.run(({ cold, expectObservable, flush }) => {
+        const testRD = createFailedRemoteDataObject('Something went wrong', 500);
+        const source = cold('a', { a: testRD });
+        const expected = 'a';
+        const values = { a: testRD };
 
-      observableOf(testRD).pipe(redirectOn4xx(router, authService)).subscribe();
-      expect(router.navigateByUrl).not.toHaveBeenCalled();
+        expectObservable(source.pipe(redirectOn4xx(router, authService))).toBe(expected, values);
+        flush();
+        expect(router.navigateByUrl).not.toHaveBeenCalled();
+      });
+    });
+
+    it('should not call navigateByUrl to a 404, 403 or 401 page, when the remote data contains no error, and emit the source rd', () => {
+      testScheduler.run(({ cold, expectObservable, flush }) => {
+        const testRD = createSuccessfulRemoteDataObject(undefined);
+        const source = cold('a', { a: testRD });
+        const expected = 'a';
+        const values = { a: testRD };
+
+        expectObservable(source.pipe(redirectOn4xx(router, authService))).toBe(expected, values);
+        flush();
+        expect(router.navigateByUrl).not.toHaveBeenCalled();
+      });
     });
 
     describe('when the user is not authenticated', () => {
@@ -214,20 +260,32 @@ describe('Core Module - RxJS Operators', () => {
         (authService.isAuthenticated as jasmine.Spy).and.returnValue(observableOf(false));
       });
 
-      it('should set the redirect url and navigate to login when the remote data contains a 401 error', () => {
-        const testRD = createFailedRemoteDataObject('The current user is unauthorized', 401);
+      it('should set the redirect url and navigate to login when the remote data contains a 401 error, and not emit anything', () => {
+        testScheduler.run(({ cold, expectObservable, flush }) => {
+          const testRD = createFailedRemoteDataObject('The current user is unauthorized', 401);
+          const source = cold('a', { a: testRD });
+          const expected = '-';
+          const values = {};
 
-        observableOf(testRD).pipe(redirectOn4xx(router, authService)).subscribe();
-        expect(authService.setRedirectUrl).toHaveBeenCalled();
-        expect(router.navigateByUrl).toHaveBeenCalledWith('login');
+          expectObservable(source.pipe(redirectOn4xx(router, authService))).toBe(expected, values);
+          flush();
+          expect(authService.setRedirectUrl).toHaveBeenCalled();
+          expect(router.navigateByUrl).toHaveBeenCalledWith('login');
+        });
       });
 
-      it('should set the redirect url and navigate to login when the remote data contains a 403 error', () => {
-        const testRD = createFailedRemoteDataObject('Forbidden', 403);
+      it('should set the redirect url and navigate to login when the remote data contains a 403 error, and not emit anything', () => {
+        testScheduler.run(({ cold, expectObservable, flush }) => {
+          const testRD = createFailedRemoteDataObject('Forbidden', 403);
+          const source = cold('a', { a: testRD });
+          const expected = '-';
+          const values = {};
 
-        observableOf(testRD).pipe(redirectOn4xx(router, authService)).subscribe();
-        expect(authService.setRedirectUrl).toHaveBeenCalled();
-        expect(router.navigateByUrl).toHaveBeenCalledWith('login');
+          expectObservable(source.pipe(redirectOn4xx(router, authService))).toBe(expected, values);
+          flush();
+          expect(authService.setRedirectUrl).toHaveBeenCalled();
+          expect(router.navigateByUrl).toHaveBeenCalledWith('login');
+        });
       });
     });
   });

src/app/core/shared/operators.ts

@@ -1,6 +1,17 @@
 import { Router, UrlTree } from '@angular/router';
 import { combineLatest as observableCombineLatest, Observable } from 'rxjs';
-import { debounceTime, filter, find, map, mergeMap, switchMap, take, takeWhile, tap } from 'rxjs/operators';
+import {
+  debounceTime,
+  filter,
+  find,
+  map,
+  mergeMap,
+  switchMap,
+  take,
+  takeWhile,
+  tap,
+  withLatestFrom
+} from 'rxjs/operators';
 import { hasNoValue, hasValue, hasValueOperator, isNotEmpty } from '../../shared/empty.util';
 import { SearchResult } from '../../shared/search/search-result.model';
 import { PaginatedList } from '../data/paginated-list.model';
@@ -22,6 +33,11 @@ export const DEBOUNCE_TIME_OPERATOR = new InjectionToken<<T>(dueTime: number) =>
   factory: () => debounceTime
 });
 
+export const REDIRECT_ON_4XX = new InjectionToken<<T>(router: Router, authService: AuthService) => (source: Observable<RemoteData<T>>) => Observable<RemoteData<T>>>('redirectOn4xx', {
+  providedIn: 'root',
+  factory: () => redirectOn4xx
+});
+
 /**
  * This file contains custom RxJS operators that can be used in multiple places
  */
@@ -175,29 +191,37 @@ export const getAllSucceededRemoteListPayload = <T>() =>
     );
 
 /**
- * Operator that checks if a remote data object returned a 401 or 404 error
- * When it does contain such an error, it will redirect the user to the related error page, without altering the current URL
+ * Operator that checks if a remote data object returned a 4xx error
+ * When it does contain such an error, it will redirect the user to the related error page, without
+ * altering the current URL
+ *
  * @param router The router used to navigate to a new page
  * @param authService Service to check if the user is authenticated
  */
 export const redirectOn4xx = <T>(router: Router, authService: AuthService) =>
   (source: Observable<RemoteData<T>>): Observable<RemoteData<T>> =>
-    observableCombineLatest(source, authService.isAuthenticated()).pipe(
-      map(([rd, isAuthenticated]: [RemoteData<T>, boolean]) => {
+    source.pipe(
+      withLatestFrom(authService.isAuthenticated()),
+      filter(([rd, isAuthenticated]: [RemoteData<T>, boolean]) => {
         if (rd.hasFailed) {
-          if (rd.statusCode === 404) {
-            router.navigateByUrl(getPageNotFoundRoute(), {skipLocationChange: true});
+          if (rd.statusCode === 404 || rd.statusCode === 422) {
+            router.navigateByUrl(getPageNotFoundRoute(), { skipLocationChange: true });
+            return false;
           } else if (rd.statusCode === 403 || rd.statusCode === 401) {
             if (isAuthenticated) {
-              router.navigateByUrl(getForbiddenRoute(), {skipLocationChange: true});
+              router.navigateByUrl(getForbiddenRoute(), { skipLocationChange: true });
+              return false;
             } else {
               authService.setRedirectUrl(router.url);
               router.navigateByUrl('login');
+              return false;
             }
           }
         }
-        return rd;
-      }));
+        return true;
+      }),
+      map(([rd,]: [RemoteData<T>, boolean]) => rd)
+    );
 
 /**
  * Operator that returns a UrlTree to a forbidden page or the login page when the boolean received is false