Merge pull request #2065 from atmire/single-page-of-bitstreams-for-citation-pdf-url-tag

Only check 1 page of bitstreams when generating the citation_pdf_url tag

src/app/core/metadata/metadata.service.spec.ts

@@ -8,7 +8,12 @@ import { Observable, of as observableOf, of } from 'rxjs';
 import { RemoteData } from '../data/remote-data';
 import { Item } from '../shared/item.model';
 
-import { ItemMock, MockBitstream1, MockBitstream3 } from '../../shared/mocks/item.mock';
+import {
+  ItemMock,
+  MockBitstream1,
+  MockBitstream3,
+  MockBitstream2
+} from '../../shared/mocks/item.mock';
 import { createSuccessfulRemoteDataObject, createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils';
 import { PaginatedList } from '../data/paginated-list.model';
 import { Bitstream } from '../shared/bitstream.model';
@@ -24,6 +29,7 @@ import { HardRedirectService } from '../services/hard-redirect.service';
 import { getMockStore } from '@ngrx/store/testing';
 import { AddMetaTagAction, ClearMetaTagAction } from './meta-tag.actions';
 import { AuthorizationDataService } from '../data/feature-authorization/authorization-data.service';
+import { AppConfig } from '../../../config/app-config.interface';
 
 describe('MetadataService', () => {
   let metadataService: MetadataService;
@@ -44,6 +50,8 @@ describe('MetadataService', () => {
   let router: Router;
   let store;
 
+  let appConfig: AppConfig;
+
   const initialState = { 'core': { metaTag: { tagsInUse: ['title', 'description'] }}};
 
 
@@ -86,6 +94,14 @@ describe('MetadataService', () => {
     store = getMockStore({ initialState });
     spyOn(store, 'dispatch');
 
+    appConfig = {
+      item: {
+        bitstream: {
+          pageSize: 5
+        }
+      }
+    } as any;
+
     metadataService = new MetadataService(
       router,
       translateService,
@@ -98,6 +114,7 @@ describe('MetadataService', () => {
       rootService,
       store,
       hardRedirectService,
+      appConfig,
       authorizationService
     );
   });
@@ -358,13 +375,18 @@ describe('MetadataService', () => {
         });
       }));
 
-      it('should link to first Bitstream with allowed format', fakeAsync(() => {
+      describe(`when there's a bitstream with an allowed format on the first page`, () => {
-        const bitstreams = [MockBitstream3, MockBitstream3, MockBitstream1];
+        let bitstreams;
+
+        beforeEach(() => {
+          bitstreams = [MockBitstream2, MockBitstream3, MockBitstream1];
           (bundleDataService.findByItemAndName as jasmine.Spy).and.returnValue(mockBundleRD$(bitstreams));
           (bitstreamDataService.findListByHref as jasmine.Spy).and.returnValues(
             ...mockBitstreamPages$(bitstreams).map(bp => createSuccessfulRemoteDataObject$(bp)),
           );
+        });
 
+        it('should link to first Bitstream with allowed format', fakeAsync(() => {
           (metadataService as any).processRouteChange({
             data: {
               value: {
@@ -375,12 +397,44 @@ describe('MetadataService', () => {
           tick();
           expect(meta.addTag).toHaveBeenCalledWith({
             name: 'citation_pdf_url',
-          content: 'https://request.org/bitstreams/cf9b0c8e-a1eb-4b65-afd0-567366448713/download'
+            content: 'https://request.org/bitstreams/99b00f3c-1cc6-4689-8158-91965bee6b28/download'
           });
         }));
+
+      });
+
     });
   });
 
+  describe(`when there's no bitstream with an allowed format on the first page`, () => {
+    let bitstreams;
+
+    beforeEach(() => {
+      bitstreams = [MockBitstream1, MockBitstream3, MockBitstream2];
+      (bundleDataService.findByItemAndName as jasmine.Spy).and.returnValue(mockBundleRD$(bitstreams));
+      (bitstreamDataService.findListByHref as jasmine.Spy).and.returnValues(
+        ...mockBitstreamPages$(bitstreams).map(bp => createSuccessfulRemoteDataObject$(bp)),
+      );
+    });
+
+    it(`shouldn't add a citation_pdf_url meta tag`, fakeAsync(() => {
+      (metadataService as any).processRouteChange({
+        data: {
+          value: {
+            dso: createSuccessfulRemoteDataObject(ItemMock),
+          }
+        }
+      });
+      tick();
+      expect(meta.addTag).not.toHaveBeenCalledWith({
+        name: 'citation_pdf_url',
+        content: 'https://request.org/bitstreams/99b00f3c-1cc6-4689-8158-91965bee6b28/download'
+      });
+    }));
+
+  });
+
+
   describe('tagstore', () => {
     beforeEach(fakeAsync(() => {
       (metadataService as any).processRouteChange({

src/app/core/metadata/metadata.service.ts

@@ -1,14 +1,21 @@
-import { Injectable } from '@angular/core';
+import { Injectable, Inject } from '@angular/core';
 
 import { Meta, MetaDefinition, Title } from '@angular/platform-browser';
 import { ActivatedRoute, NavigationEnd, Router } from '@angular/router';
 
 import { TranslateService } from '@ngx-translate/core';
 
-import { BehaviorSubject, combineLatest, EMPTY, Observable, of as observableOf } from 'rxjs';
+import {
-import { expand, filter, map, switchMap, take } from 'rxjs/operators';
+  BehaviorSubject,
+  combineLatest,
+  Observable,
+  of as observableOf,
+  concat as observableConcat,
+  EMPTY
+} from 'rxjs';
+import { filter, map, switchMap, take, mergeMap } from 'rxjs/operators';
 
-import { hasNoValue, hasValue } from '../../shared/empty.util';
+import { hasNoValue, hasValue, isNotEmpty } from '../../shared/empty.util';
 import { DSONameService } from '../breadcrumbs/dso-name.service';
 import { BitstreamDataService } from '../data/bitstream-data.service';
 import { BitstreamFormatDataService } from '../data/bitstream-format-data.service';
@@ -37,6 +44,7 @@ import { coreSelector } from '../core.selectors';
 import { CoreState } from '../core-state.model';
 import { AuthorizationDataService } from '../data/feature-authorization/authorization-data.service';
 import { getDownloadableBitstream } from '../shared/bitstream.operators';
+import { APP_CONFIG, AppConfig } from '../../../config/app-config.interface';
 
 /**
  * The base selector function to select the metaTag section in the store
@@ -87,6 +95,7 @@ export class MetadataService {
     private rootService: RootDataService,
     private store: Store<CoreState>,
     private hardRedirectService: HardRedirectService,
+    @Inject(APP_CONFIG) private appConfig: AppConfig,
     private authorizationService: AuthorizationDataService
   ) {
   }
@@ -298,7 +307,13 @@ export class MetadataService {
         true,
         true,
         followLink('primaryBitstream'),
-        followLink('bitstreams', {}, followLink('format')),
+        followLink('bitstreams', {
+            findListOptions: {
+              // limit the number of bitstreams used to find the citation pdf url to the number
+              // shown by default on an item page
+              elementsPerPage: this.appConfig.item.bitstream.pageSize
+            }
+        }, followLink('format')),
       ).pipe(
         getFirstSucceededRemoteDataPayload(),
         switchMap((bundle: Bundle) =>
@@ -363,53 +378,30 @@ export class MetadataService {
   }
 
   /**
-   * For Items with more than one Bitstream (and no primary Bitstream), link to the first Bitstream with a MIME type
+   * For Items with more than one Bitstream (and no primary Bitstream), link to the first Bitstream
+   * with a MIME type.
+   *
+   * Note this will only check the current page (page size determined item.bitstream.pageSize in the
+   * config) of bitstreams for performance reasons.
+   * See https://github.com/DSpace/DSpace/issues/8648 for more info
+   *
    * included in {@linkcode CITATION_PDF_URL_MIMETYPES}
    * @param bitstreamRd
    * @private
    */
   private getFirstAllowedFormatBitstreamLink(bitstreamRd: RemoteData<PaginatedList<Bitstream>>): Observable<string> {
-    return observableOf(bitstreamRd.payload).pipe(
+    if (hasValue(bitstreamRd.payload) && isNotEmpty(bitstreamRd.payload.page)) {
-      // Because there can be more than one page of bitstreams, this expand operator
+      // Retrieve the formats of all bitstreams in the page sequentially
-      // will retrieve them in turn. Due to the take(1) at the bottom, it will only
+      return observableConcat(
-      // retrieve pages until a match is found
+        ...bitstreamRd.payload.page.map((bitstream: Bitstream) => bitstream.format.pipe(
-      expand((paginatedList: PaginatedList<Bitstream>) => {
-        if (hasNoValue(paginatedList.next)) {
-          // If there's no next page, stop.
-          return EMPTY;
-        } else {
-          // Otherwise retrieve the next page
-          return this.bitstreamDataService.findListByHref(
-            paginatedList.next,
-            undefined,
-            true,
-            true,
-            followLink('format')
-          ).pipe(
-            getFirstCompletedRemoteData(),
-            map((next: RemoteData<PaginatedList<Bitstream>>) => {
-              if (hasValue(next.payload)) {
-                return next.payload;
-              } else {
-                return EMPTY;
-              }
-            })
-          );
-        }
-      }),
-      // Return the array of bitstreams inside each paginated list
-      map((paginatedList: PaginatedList<Bitstream>) => paginatedList.page),
-      // Emit the bitstreams in the list one at a time
-      switchMap((bitstreams: Bitstream[]) => bitstreams),
-      // Retrieve the format for each bitstream
-      switchMap((bitstream: Bitstream) => bitstream.format.pipe(
           getFirstSucceededRemoteDataPayload(),
           // Keep the original bitstream, because it, not the format, is what we'll need
           // for the link at the end
           map((format: BitstreamFormat) => [bitstream, format])
-      )),
+        ))
-      // Check if bitstream downloadable
+      ).pipe(
-      switchMap(([bitstream, format]: [Bitstream, BitstreamFormat]) => observableOf(bitstream).pipe(
+        // Verify that the bitstream is downloadable
+        mergeMap(([bitstream, format]: [Bitstream, BitstreamFormat]) => observableOf(bitstream).pipe(
           getDownloadableBitstream(this.authorizationService),
           map((bit: Bitstream) => [bit, format])
         )),
@@ -419,8 +411,12 @@ export class MetadataService {
         // We only need 1
         take(1),
         // Emit the link of the match
+        // tap((v) => console.log('result', v)),
         map(([bitstream, ]: [Bitstream, BitstreamFormat]) => getBitstreamDownloadRoute(bitstream))
       );
+    } else {
+      return EMPTY;
+    }
   }
 
   /**