Merge pull request #3753 from atmire/w2p-121787_Investigate-internal-server-error-on-browse-page-experiment

Get rid of unnecessary and failing REST requests when navigating between different browse indexes

Author: alanorth

src/app/browse-by/browse-by-date/browse-by-date.component.ts

@@ -18,7 +18,10 @@ import {
   combineLatest as observableCombineLatest,
   Observable,
 } from 'rxjs';
-import { map } from 'rxjs/operators';
+import {
+  map,
+  take,
+} from 'rxjs/operators';
 import { ThemedBrowseByComponent } from 'src/app/shared/browse-by/themed-browse-by.component';
 
 import {
@@ -53,7 +56,6 @@ import { VarDirective } from '../../shared/utils/var.directive';
 import {
   BrowseByMetadataComponent,
   browseParamsToOptions,
-  getBrowseSearchOptions,
 } from '../browse-by-metadata/browse-by-metadata.component';
 
 @Component({
@@ -104,15 +106,18 @@ export class BrowseByDateComponent extends BrowseByMetadataComponent implements
   ngOnInit(): void {
     const sortConfig = new SortOptions('default', SortDirection.ASC);
     this.startsWithType = StartsWithType.date;
-    // include the thumbnail configuration in browse search options
-    this.updatePage(getBrowseSearchOptions(this.defaultBrowseId, this.paginationConfig, sortConfig, this.fetchThumbnails));
     this.currentPagination$ = this.paginationService.getCurrentPagination(this.paginationConfig.id, this.paginationConfig);
     this.currentSort$ = this.paginationService.getCurrentSort(this.paginationConfig.id, sortConfig);
     this.subs.push(
-      observableCombineLatest([this.route.params, this.route.queryParams, this.scope$, this.route.data,
-        this.currentPagination$, this.currentSort$]).pipe(
-        map(([routeParams, queryParams, scope, data, currentPage, currentSort]) => {
-          return [Object.assign({}, routeParams, queryParams, data), scope, currentPage, currentSort];
+      observableCombineLatest(
+        [ this.route.params.pipe(take(1)),
+          this.route.queryParams,
+          this.scope$,
+          this.currentPagination$,
+          this.currentSort$,
+        ]).pipe(
+        map(([routeParams, queryParams, scope, currentPage, currentSort]) => {
+          return [Object.assign({}, routeParams, queryParams), scope, currentPage, currentSort];
         }),
       ).subscribe(([params, scope, currentPage, currentSort]: [Params, string, PaginationComponentOptions, SortOptions]) => {
         const metadataKeys = params.browseDefinition ? params.browseDefinition.metadataKeys : this.defaultMetadataKeys;

src/app/browse-by/browse-by-metadata/browse-by-metadata.component.ts

@@ -23,7 +23,10 @@ import {
   of as observableOf,
   Subscription,
 } from 'rxjs';
-import { map } from 'rxjs/operators';
+import {
+  map,
+  take,
+} from 'rxjs/operators';
 import { ThemedBrowseByComponent } from 'src/app/shared/browse-by/themed-browse-by.component';
 
 import {
@@ -216,7 +219,13 @@ export class BrowseByMetadataComponent implements OnInit, OnChanges, OnDestroy {
     this.currentPagination$ = this.paginationService.getCurrentPagination(this.paginationConfig.id, this.paginationConfig);
     this.currentSort$ = this.paginationService.getCurrentSort(this.paginationConfig.id, sortConfig);
     this.subs.push(
-      observableCombineLatest([this.route.params, this.route.queryParams, this.scope$, this.currentPagination$, this.currentSort$]).pipe(
+      observableCombineLatest(
+        [ this.route.params.pipe(take(1)),
+          this.route.queryParams,
+          this.scope$,
+          this.currentPagination$,
+          this.currentSort$,
+        ]).pipe(
         map(([routeParams, queryParams, scope, currentPage, currentSort]) => {
           return [Object.assign({}, routeParams, queryParams), scope, currentPage, currentSort];
         }),

src/app/browse-by/browse-by-page/browse-by-page.component.ts

@@ -23,7 +23,7 @@ import { BrowseBySwitcherComponent } from '../browse-by-switcher/browse-by-switc
 })
 export class BrowseByPageComponent implements OnInit {
 
-  browseByType$: Observable<BrowseByDataType>;
+  browseByType$: Observable<{type: BrowseByDataType }>;
 
   constructor(
     protected route: ActivatedRoute,
@@ -35,7 +35,7 @@ export class BrowseByPageComponent implements OnInit {
    */
   ngOnInit(): void {
     this.browseByType$ = this.route.data.pipe(
-      map((data: { browseDefinition: BrowseDefinition }) => data.browseDefinition.getRenderType()),
+      map((data: { browseDefinition: BrowseDefinition }) => ({ type: data.browseDefinition.getRenderType() })),
     );
   }
 

src/app/browse-by/browse-by-switcher/browse-by-switcher.component.spec.ts

@@ -85,7 +85,7 @@ describe('BrowseBySwitcherComponent', () => {
   types.forEach((type: NonHierarchicalBrowseDefinition) => {
     describe(`when switching to a browse-by page for "${type.id}"`, () => {
       beforeEach(async () => {
-        comp.browseByType = type.dataType;
+        comp.browseByType = type as any;
         comp.ngOnChanges({
           browseByType: new SimpleChange(undefined, type.dataType, true),
         });

src/app/browse-by/browse-by-switcher/browse-by-switcher.component.ts

@@ -24,7 +24,7 @@ export class BrowseBySwitcherComponent extends AbstractComponentLoaderComponent<
 
   @Input() context: Context;
 
-  @Input() browseByType: BrowseByDataType;
+  @Input() browseByType: { type: BrowseByDataType };
 
   @Input() displayTitle: boolean;
 
@@ -43,7 +43,7 @@ export class BrowseBySwitcherComponent extends AbstractComponentLoaderComponent<
   ];
 
   public getComponent(): GenericConstructor<Component> {
-    return getComponentByBrowseByType(this.browseByType, this.context, this.themeService.getThemeName());
+    return getComponentByBrowseByType(this.browseByType.type, this.context, this.themeService.getThemeName());
   }
 
 }

src/app/browse-by/browse-by-title/browse-by-title.component.ts

@@ -9,7 +9,10 @@ import {
 import { Params } from '@angular/router';
 import { TranslateModule } from '@ngx-translate/core';
 import { combineLatest as observableCombineLatest } from 'rxjs';
-import { map } from 'rxjs/operators';
+import {
+  map,
+  take,
+} from 'rxjs/operators';
 
 import {
   SortDirection,
@@ -28,7 +31,6 @@ import { VarDirective } from '../../shared/utils/var.directive';
 import {
   BrowseByMetadataComponent,
   browseParamsToOptions,
-  getBrowseSearchOptions,
 } from '../browse-by-metadata/browse-by-metadata.component';
 
 @Component({
@@ -58,12 +60,16 @@ export class BrowseByTitleComponent extends BrowseByMetadataComponent implements
 
   ngOnInit(): void {
     const sortConfig = new SortOptions('dc.title', SortDirection.ASC);
-    // include the thumbnail configuration in browse search options
-    this.updatePage(getBrowseSearchOptions(this.defaultBrowseId, this.paginationConfig, sortConfig, this.fetchThumbnails));
     this.currentPagination$ = this.paginationService.getCurrentPagination(this.paginationConfig.id, this.paginationConfig);
     this.currentSort$ = this.paginationService.getCurrentSort(this.paginationConfig.id, sortConfig);
     this.subs.push(
-      observableCombineLatest([this.route.params, this.route.queryParams, this.scope$, this.currentPagination$, this.currentSort$]).pipe(
+      observableCombineLatest(
+        [ this.route.params.pipe(take(1)),
+          this.route.queryParams,
+          this.scope$,
+          this.currentPagination$,
+          this.currentSort$,
+        ]).pipe(
         map(([routeParams, queryParams, scope, currentPage, currentSort]) => {
           return [Object.assign({}, routeParams, queryParams), scope, currentPage, currentSort];
         }),

src/app/menu-resolver.service.ts

@@ -7,7 +7,9 @@ import { NgbModal } from '@ng-bootstrap/ng-bootstrap';
 import {
   combineLatest,
   combineLatest as observableCombineLatest,
+  mergeMap,
   Observable,
+  of as observableOf,
 } from 'rxjs';
 import {
   filter,
@@ -17,6 +19,7 @@ import {
 } from 'rxjs/operators';
 
 import { PUBLICATION_CLAIMS_PATH } from './admin/admin-notifications/admin-notifications-routing-paths';
+import { AuthService } from './core/auth/auth.service';
 import { BrowseService } from './core/browse/browse.service';
 import { ConfigurationDataService } from './core/data/configuration-data.service';
 import { AuthorizationDataService } from './core/data/feature-authorization/authorization-data.service';
@@ -62,6 +65,7 @@ export class MenuResolverService  {
     protected modalService: NgbModal,
     protected scriptDataService: ScriptDataService,
     protected configurationDataService: ConfigurationDataService,
+    protected authService: AuthService,
   ) {
   }
 
@@ -71,7 +75,7 @@ export class MenuResolverService  {
   resolve(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean> {
     return combineLatest([
       this.createPublicMenu$(),
-      this.createAdminMenu$(),
+      this.createAdminMenuIfLoggedIn$(),
     ]).pipe(
       map((menusDone: boolean[]) => menusDone.every(Boolean)),
     );
@@ -147,6 +151,15 @@ export class MenuResolverService  {
     return this.waitForMenu$(MenuID.PUBLIC);
   }
 
+  /**
+   * Initialize all menu sections and items for {@link MenuID.ADMIN}, only if the user is logged in.
+   */
+  createAdminMenuIfLoggedIn$() {
+    return this.authService.isAuthenticated().pipe(
+      mergeMap((isAuthenticated) => isAuthenticated ? this.createAdminMenu$() : observableOf(true)),
+    );
+  }
+
   /**
    * Initialize all menu sections and items for {@link MenuID.ADMIN}
    */
@@ -156,8 +169,6 @@ export class MenuResolverService  {
     this.createExportMenuSections();
     this.createImportMenuSections();
     this.createAccessControlMenuSections();
-    this.createReportMenuSections();
-
     return this.waitForMenu$(MenuID.ADMIN);
   }
 

src/app/menu.resolver.spec.ts

@@ -26,7 +26,9 @@ import { ConfigurationDataServiceStub } from './shared/testing/configuration-dat
 import { MenuServiceStub } from './shared/testing/menu-service.stub';
 import { createPaginatedList } from './shared/testing/utils.test';
 import createSpy = jasmine.createSpy;
+import { AuthService } from './core/auth/auth.service';
 import { MenuResolverService } from './menu-resolver.service';
+import { AuthServiceStub } from './shared/testing/auth-service.stub';
 
 const BOOLEAN = { t: true, f: false };
 const MENU_STATE = {
@@ -80,6 +82,7 @@ describe('menuResolver', () => {
         { provide: ScriptDataService, useValue: scriptService },
         { provide: ConfigurationDataService, useValue: configurationDataService },
         { provide: NgbModal, useValue: mockNgbModal },
+        { provide: AuthService, useValue: AuthServiceStub },
         MenuResolverService,
       ],
       schemas: [NO_ERRORS_SCHEMA],
@@ -94,19 +97,19 @@ describe('menuResolver', () => {
   describe('resolve', () => {
     it('should create all menus', (done) => {
       spyOn(resolver, 'createPublicMenu$').and.returnValue(observableOf(true));
-      spyOn(resolver, 'createAdminMenu$').and.returnValue(observableOf(true));
+      spyOn(resolver, 'createAdminMenuIfLoggedIn$').and.returnValue(observableOf(true));
 
       resolver.resolve(null, null).subscribe(resolved => {
         expect(resolved).toBeTrue();
         expect(resolver.createPublicMenu$).toHaveBeenCalled();
-        expect(resolver.createAdminMenu$).toHaveBeenCalled();
+        expect(resolver.createAdminMenuIfLoggedIn$).toHaveBeenCalled();
         done();
       });
     });
 
     it('should return an Observable that emits true as soon as all menus are created', () => {
       spyOn(resolver, 'createPublicMenu$').and.returnValue(cold('--(t|)', BOOLEAN));
-      spyOn(resolver, 'createAdminMenu$').and.returnValue(cold('----(t|)', BOOLEAN));
+      spyOn(resolver, 'createAdminMenuIfLoggedIn$').and.returnValue(cold('----(t|)', BOOLEAN));
 
       expect(resolver.resolve(null, null)).toBeObservable(cold('----(t|)', BOOLEAN));
     });