diff --git a/src/app/core/metadata/metadata.service.spec.ts b/src/app/core/metadata/metadata.service.spec.ts index 66d9b9921c6..553b437d712 100644 --- a/src/app/core/metadata/metadata.service.spec.ts +++ b/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,29 +375,66 @@ describe('MetadataService', () => { }); })); - it('should link to first Bitstream with allowed format', fakeAsync(() => { - const bitstreams = [MockBitstream3, MockBitstream3, MockBitstream1]; - (bundleDataService.findByItemAndName as jasmine.Spy).and.returnValue(mockBundleRD$(bitstreams)); - (bitstreamDataService.findListByHref as jasmine.Spy).and.returnValues( - ...mockBitstreamPages$(bitstreams).map(bp => createSuccessfulRemoteDataObject$(bp)), - ); + describe(`when there's a bitstream with an allowed format on the first page`, () => { + let bitstreams; - (metadataService as any).processRouteChange({ - data: { - value: { - dso: createSuccessfulRemoteDataObject(ItemMock), - } - } + 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)), + ); }); - tick(); - expect(meta.addTag).toHaveBeenCalledWith({ - name: 'citation_pdf_url', - content: 'https://request.org/bitstreams/cf9b0c8e-a1eb-4b65-afd0-567366448713/download' - }); - })); + + it('should link to first Bitstream with allowed format', fakeAsync(() => { + (metadataService as any).processRouteChange({ + data: { + value: { + dso: createSuccessfulRemoteDataObject(ItemMock), + } + } + }); + tick(); + expect(meta.addTag).toHaveBeenCalledWith({ + name: 'citation_pdf_url', + 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({ diff --git a/src/app/core/metadata/metadata.service.ts b/src/app/core/metadata/metadata.service.ts index 9d5ee80bba1..204c925e6bb 100644 --- a/src/app/core/metadata/metadata.service.ts +++ b/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 { expand, filter, map, switchMap, take } from 'rxjs/operators'; - -import { hasNoValue, hasValue } from '../../shared/empty.util'; +import { + BehaviorSubject, + combineLatest, + Observable, + of as observableOf, + concat as observableConcat, + EMPTY +} from 'rxjs'; +import { filter, map, switchMap, take, mergeMap } from 'rxjs/operators'; + +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, 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,64 +378,45 @@ 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>): Observable { - return observableOf(bitstreamRd.payload).pipe( - // Because there can be more than one page of bitstreams, this expand operator - // will retrieve them in turn. Due to the take(1) at the bottom, it will only - // retrieve pages until a match is found - expand((paginatedList: PaginatedList) => { - 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>) => { - if (hasValue(next.payload)) { - return next.payload; - } else { - return EMPTY; - } - }) - ); - } - }), - // Return the array of bitstreams inside each paginated list - map((paginatedList: PaginatedList) => 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 - switchMap(([bitstream, format]: [Bitstream, BitstreamFormat]) => observableOf(bitstream).pipe( - getDownloadableBitstream(this.authorizationService), - map((bit: Bitstream) => [bit, format]) - )), - // Filter out only pairs with whitelisted formats and non-null bitstreams, null from download check - filter(([bitstream, format]: [Bitstream, BitstreamFormat]) => - hasValue(format) && hasValue(bitstream) && this.CITATION_PDF_URL_MIMETYPES.includes(format.mimetype)), - // We only need 1 - take(1), - // Emit the link of the match - map(([bitstream, ]: [Bitstream, BitstreamFormat]) => getBitstreamDownloadRoute(bitstream)) - ); + if (hasValue(bitstreamRd.payload) && isNotEmpty(bitstreamRd.payload.page)) { + // Retrieve the formats of all bitstreams in the page sequentially + return observableConcat( + ...bitstreamRd.payload.page.map((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]) + )) + ).pipe( + // Verify that the bitstream is downloadable + mergeMap(([bitstream, format]: [Bitstream, BitstreamFormat]) => observableOf(bitstream).pipe( + getDownloadableBitstream(this.authorizationService), + map((bit: Bitstream) => [bit, format]) + )), + // Filter out only pairs with whitelisted formats and non-null bitstreams, null from download check + filter(([bitstream, format]: [Bitstream, BitstreamFormat]) => + hasValue(format) && hasValue(bitstream) && this.CITATION_PDF_URL_MIMETYPES.includes(format.mimetype)), + // 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; + } } /**