Merge pull request #1860 from atmire/Fix-recent-submissions-list-thumbnails

Fix recent submissions list thumbnails

src/app/home-page/home-page.module.ts

@@ -10,6 +10,9 @@ import { StatisticsModule } from '../statistics/statistics.module';
 import { ThemedHomeNewsComponent } from './home-news/themed-home-news.component';
 import { ThemedHomePageComponent } from './themed-home-page.component';
 import { RecentItemListComponent } from './recent-item-list/recent-item-list.component';
+import { JournalEntitiesModule } from '../entity-groups/journal-entities/journal-entities.module';
+import { ResearchEntitiesModule } from '../entity-groups/research-entities/research-entities.module';
+
 const DECLARATIONS = [
   HomePageComponent,
   ThemedHomePageComponent,
@@ -22,7 +25,9 @@ const DECLARATIONS = [
 @NgModule({
   imports: [
     CommonModule,
-    SharedModule,
+    SharedModule.withEntryComponents(),
+    JournalEntitiesModule.withEntryComponents(),
+    ResearchEntitiesModule.withEntryComponents(),
     HomePageRoutingModule,
     StatisticsModule.forRoot()
   ],

src/app/home-page/recent-item-list/recent-item-list.component.html

@@ -1,5 +1,5 @@
 <ng-container *ngVar="(itemRD$ | async) as itemRD">
-        <div class="mt-4" *ngIf="itemRD?.hasSucceeded && itemRD?.payload?.page.length > 0" @fadeIn>
+        <div class="mt-4" [ngClass]="placeholderFontClass" *ngIf="itemRD?.hasSucceeded && itemRD?.payload?.page.length > 0" @fadeIn>
                 <div class="d-flex flex-row border-bottom mb-4 pb-4 ng-tns-c416-2"></div>
                 <h2> {{'home.recent-submissions.head' | translate}}</h2>
                 <div class="my-4" *ngFor="let item of itemRD?.payload?.page">
@@ -12,4 +12,4 @@
         <ds-error *ngIf="itemRD?.hasFailed" message="{{'error.recent-submissions' | translate}}"></ds-error>
         <ds-loading *ngIf="!itemRD || itemRD.isLoading" message="{{'loading.recent-submissions' | translate}}">
         </ds-loading>
-</ng-container>
+</ng-container>

src/app/home-page/recent-item-list/recent-item-list.component.spec.ts

@@ -10,8 +10,11 @@ import { SearchConfigurationService } from '../../core/shared/search/search-conf
 import { PaginatedSearchOptions } from '../../shared/search/models/paginated-search-options.model';
 import { PaginationComponentOptions } from '../../shared/pagination/pagination-component-options.model';
 import { SortDirection, SortOptions } from '../../core/cache/models/sort-options.model';
-import { ViewMode } from 'src/app/core/shared/view-mode.model';
 import { of as observableOf } from 'rxjs';
+import { APP_CONFIG } from '../../../config/app-config.interface';
+import { environment } from '../../../environments/environment';
+import { PLATFORM_ID } from '@angular/core';
+
 describe('RecentItemListComponent', () => {
   let component: RecentItemListComponent;
   let fixture: ComponentFixture<RecentItemListComponent>;
@@ -42,6 +45,8 @@ describe('RecentItemListComponent', () => {
         { provide: SearchService, useValue: searchServiceStub },
         { provide: PaginationService, useValue: paginationService },
         { provide: SearchConfigurationService, useValue: searchConfigServiceStub },
+        { provide: APP_CONFIG, useValue: environment },
+        { provide: PLATFORM_ID, useValue: 'browser' },
       ],
     })
     .compileComponents();

src/app/home-page/recent-item-list/recent-item-list.component.ts

@@ -1,4 +1,4 @@
-import { ChangeDetectionStrategy, Component, OnInit } from '@angular/core';
+import { ChangeDetectionStrategy, Component, ElementRef, Inject, OnInit, PLATFORM_ID } from '@angular/core';
 import { PaginatedSearchOptions } from '../../shared/search/models/paginated-search-options.model';
 import { fadeIn, fadeInOut } from '../../shared/animations/fade';
 import { RemoteData } from '../../core/data/remote-data';
@@ -11,12 +11,13 @@ import { SortDirection, SortOptions } from '../../core/cache/models/sort-options
 import { environment } from '../../../environments/environment';
 import { ViewMode } from '../../core/shared/view-mode.model';
 import { SearchConfigurationService } from '../../core/shared/search/search-configuration.service';
-import {
-  toDSpaceObjectListRD
-} from '../../core/shared/operators';
-import {
-  Observable,
-} from 'rxjs';
+import { toDSpaceObjectListRD } from '../../core/shared/operators';
+import { Observable } from 'rxjs';
+import { followLink, FollowLinkConfig } from '../../shared/utils/follow-link-config.model';
+import { APP_CONFIG, AppConfig } from '../../../config/app-config.interface';
+import { isPlatformBrowser } from '@angular/common';
+import { setPlaceHolderAttributes } from '../../shared/utils/object-list-utils';
+
 @Component({
   selector: 'ds-recent-item-list',
   templateUrl: './recent-item-list.component.html',
@@ -31,14 +32,22 @@ export class RecentItemListComponent implements OnInit {
   itemRD$: Observable<RemoteData<PaginatedList<Item>>>;
   paginationConfig: PaginationComponentOptions;
   sortConfig: SortOptions;
+
   /**
  * The view-mode we're currently on
  * @type {ViewMode}
  */
   viewMode = ViewMode.ListElement;
-  constructor(private searchService: SearchService,
+
+  private _placeholderFontClass: string;
+
+  constructor(
+    private searchService: SearchService,
     private paginationService: PaginationService,
-    public searchConfigurationService: SearchConfigurationService
+    public searchConfigurationService: SearchConfigurationService,
+    protected elementRef: ElementRef,
+    @Inject(APP_CONFIG) private appConfig: AppConfig,
+    @Inject(PLATFORM_ID) private platformId: Object,
   ) {
 
     this.paginationConfig = Object.assign(new PaginationComponentOptions(), {
@@ -50,16 +59,29 @@ export class RecentItemListComponent implements OnInit {
     this.sortConfig = new SortOptions(environment.homePage.recentSubmissions.sortField, SortDirection.DESC);
   }
   ngOnInit(): void {
+    const linksToFollow: FollowLinkConfig<Item>[] = [];
+    if (this.appConfig.browseBy.showThumbnails) {
+      linksToFollow.push(followLink('thumbnail'));
+    }
+
     this.itemRD$ = this.searchService.search(
       new PaginatedSearchOptions({
         pagination: this.paginationConfig,
         sort: this.sortConfig,
       }),
-    ).pipe(toDSpaceObjectListRD()) as Observable<RemoteData<PaginatedList<Item>>>;
+      undefined,
+      undefined,
+      undefined,
+      ...linksToFollow,
+    ).pipe(
+      toDSpaceObjectListRD()
+    ) as Observable<RemoteData<PaginatedList<Item>>>;
   }
+
   ngOnDestroy(): void {
     this.paginationService.clearPagination(this.paginationConfig.id);
   }
+
   onLoadMore(): void {
     this.paginationService.updateRouteWithUrl(this.searchConfigurationService.paginationID, ['search'], {
       sortField: environment.homePage.recentSubmissions.sortField,
@@ -68,5 +90,17 @@ export class RecentItemListComponent implements OnInit {
     });
   }
 
+  get placeholderFontClass(): string {
+    if (this._placeholderFontClass === undefined) {
+      if (isPlatformBrowser(this.platformId)) {
+        const width = this.elementRef.nativeElement.offsetWidth;
+        this._placeholderFontClass = setPlaceHolderAttributes(width);
+      } else {
+        this._placeholderFontClass = 'hide-placeholder-text';
+      }
+    }
+    return this._placeholderFontClass;
+  }
+
 }
 

src/app/thumbnail/thumbnail.component.spec.ts

@@ -127,19 +127,18 @@ describe('ThumbnailComponent', () => {
   });
 
   const errorHandler = () => {
-    let fallbackSpy;
+    let setSrcSpy;
 
     beforeEach(() => {
-      fallbackSpy = spyOn(comp, 'showFallback').and.callThrough();
+      // disconnect error handler to be sure it's only called once
+      const img = fixture.debugElement.query(By.css('img.thumbnail-content'));
+      img.nativeNode.onerror = null;
+
+      comp.ngOnChanges();
+      setSrcSpy = spyOn(comp, 'setSrc').and.callThrough();
     });
 
     describe('retry with authentication token', () => {
-      beforeEach(() => {
-        // disconnect error handler to be sure it's only called once
-        const img = fixture.debugElement.query(By.css('img.thumbnail-content'));
-        img.nativeNode.onerror = null;
-      });
-
       it('should remember that it already retried once', () => {
         expect(comp.retriedWithToken).toBeFalse();
         comp.errorHandler();
@@ -153,7 +152,7 @@ describe('ThumbnailComponent', () => {
 
         it('should fall back to default', () => {
           comp.errorHandler();
-          expect(fallbackSpy).toHaveBeenCalled();
+          expect(setSrcSpy).toHaveBeenCalledWith(comp.defaultImage);
         });
       });
 
@@ -172,11 +171,9 @@ describe('ThumbnailComponent', () => {
 
             if ((comp.thumbnail as RemoteData<Bitstream>)?.hasFailed) {
               // If we failed to retrieve the Bitstream in the first place, fall back to the default
-              expect(comp.src$.getValue()).toBe(null);
-              expect(fallbackSpy).toHaveBeenCalled();
+              expect(setSrcSpy).toHaveBeenCalledWith(comp.defaultImage);
             } else {
-              expect(comp.src$.getValue()).toBe(CONTENT + '?authentication-token=fake');
-              expect(fallbackSpy).not.toHaveBeenCalled();
+              expect(setSrcSpy).toHaveBeenCalledWith(CONTENT + '?authentication-token=fake');
             }
           });
         });
@@ -189,8 +186,7 @@ describe('ThumbnailComponent', () => {
           it('should fall back to default', () => {
             comp.errorHandler();
 
-            expect(comp.src$.getValue()).toBe(null);
-            expect(fallbackSpy).toHaveBeenCalled();
+            expect(setSrcSpy).toHaveBeenCalledWith(comp.defaultImage);
 
             // We don't need to check authorization if we failed to retrieve the Bitstreamin the first place
             if (!(comp.thumbnail as RemoteData<Bitstream>)?.hasFailed) {
@@ -210,7 +206,7 @@ describe('ThumbnailComponent', () => {
         comp.errorHandler();
         expect(authService.isAuthenticated).not.toHaveBeenCalled();
         expect(fileService.retrieveFileDownloadLink).not.toHaveBeenCalled();
-        expect(fallbackSpy).toHaveBeenCalled();
+        expect(setSrcSpy).toHaveBeenCalledWith(comp.defaultImage);
       });
     });
   };
@@ -263,21 +259,23 @@ describe('ThumbnailComponent', () => {
       comp.thumbnail = thumbnail;
     });
 
-    it('should display an image', () => {
-      comp.ngOnChanges();
-      fixture.detectChanges();
-      const image: HTMLElement = fixture.debugElement.query(By.css('img')).nativeElement;
-      expect(image.getAttribute('src')).toBe(thumbnail._links.content.href);
+    describe('if content can be loaded', () => {
+      it('should display an image', () => {
+        comp.ngOnChanges();
+        fixture.detectChanges();
+        const image: HTMLElement = fixture.debugElement.query(By.css('img')).nativeElement;
+        expect(image.getAttribute('src')).toBe(thumbnail._links.content.href);
+      });
+
+      it('should include the alt text', () => {
+        comp.ngOnChanges();
+        fixture.detectChanges();
+        const image: HTMLElement = fixture.debugElement.query(By.css('img')).nativeElement;
+        expect(image.getAttribute('alt')).toBe('TRANSLATED ' + comp.alt);
+      });
     });
 
-    it('should include the alt text', () => {
-      comp.ngOnChanges();
-      fixture.detectChanges();
-      const image: HTMLElement = fixture.debugElement.query(By.css('img')).nativeElement;
-      expect(image.getAttribute('alt')).toBe('TRANSLATED ' + comp.alt);
-    });
-
-    describe('when there is no thumbnail', () => {
+    describe('if content can\'t be loaded', () => {
       errorHandler();
     });
   });
@@ -296,36 +294,42 @@ describe('ThumbnailComponent', () => {
       };
     });
 
-    describe('when there is a thumbnail', () => {
+    describe('if RemoteData succeeded', () => {
       beforeEach(() => {
         comp.thumbnail = createSuccessfulRemoteDataObject(thumbnail);
       });
 
-      it('should display an image', () => {
-        comp.ngOnChanges();
-        fixture.detectChanges();
-        const image: HTMLElement = de.query(By.css('img')).nativeElement;
-        expect(image.getAttribute('src')).toBe(thumbnail._links.content.href);
+      describe('if content can be loaded', () => {
+        it('should display an image', () => {
+          comp.ngOnChanges();
+          fixture.detectChanges();
+          const image: HTMLElement = de.query(By.css('img')).nativeElement;
+          expect(image.getAttribute('src')).toBe(thumbnail._links.content.href);
+        });
+
+        it('should display the alt text', () => {
+          comp.ngOnChanges();
+          fixture.detectChanges();
+          const image: HTMLElement = de.query(By.css('img')).nativeElement;
+          expect(image.getAttribute('alt')).toBe('TRANSLATED ' + comp.alt);
+        });
       });
 
-      it('should display the alt text', () => {
-        comp.ngOnChanges();
-        fixture.detectChanges();
-        const image: HTMLElement = de.query(By.css('img')).nativeElement;
-        expect(image.getAttribute('alt')).toBe('TRANSLATED ' + comp.alt);
-      });
-
-      describe('but it can\'t be loaded', () => {
+      describe('if content can\'t be loaded', () => {
         errorHandler();
       });
     });
 
-    describe('when there is no thumbnail', () => {
+    describe('if RemoteData failed', () => {
       beforeEach(() => {
         comp.thumbnail = createFailedRemoteDataObject();
       });
 
-      errorHandler();
+      it('should show the default image', () => {
+        comp.defaultImage = 'default/image.jpg';
+        comp.ngOnChanges();
+        expect(comp.src$.getValue()).toBe('default/image.jpg');
+      });
     });
   });
 });

src/app/thumbnail/thumbnail.component.ts

@@ -12,7 +12,7 @@ import { FileService } from '../core/shared/file.service';
 /**
  * This component renders a given Bitstream as a thumbnail.
  * One input parameter of type Bitstream is expected.
- * If no Bitstream is provided, a HTML placeholder will be rendered instead.
+ * If no Bitstream is provided, an HTML placeholder will be rendered instead.
  */
 @Component({
   selector: 'ds-thumbnail',
@@ -75,11 +75,11 @@ export class ThumbnailComponent implements OnChanges {
       return;
     }
 
-    const thumbnail = this.bitstream;
-    if (hasValue(thumbnail?._links?.content?.href)) {
-      this.setSrc(thumbnail?._links?.content?.href);
+    const src = this.contentHref;
+    if (hasValue(src)) {
+      this.setSrc(src);
     } else {
-      this.showFallback();
+      this.setSrc(this.defaultImage);
     }
   }
 
@@ -95,22 +95,33 @@ export class ThumbnailComponent implements OnChanges {
     }
   }
 
+  private get contentHref(): string | undefined {
+    if (this.thumbnail instanceof Bitstream) {
+      return this.thumbnail?._links?.content?.href;
+    } else if (this.thumbnail instanceof RemoteData) {
+      return this.thumbnail?.payload?._links?.content?.href;
+    }
+  }
+
   /**
    * Handle image download errors.
    * If the image can't be loaded, try re-requesting it with an authorization token in case it's a restricted Bitstream
    * Otherwise, fall back to the default image or a HTML placeholder
    */
   errorHandler() {
-    if (!this.retriedWithToken && hasValue(this.thumbnail)) {
+    const src = this.src$.getValue();
+    const thumbnail = this.bitstream;
+    const thumbnailSrc = thumbnail?._links?.content?.href;
+
+    if (!this.retriedWithToken && hasValue(thumbnailSrc) && src === thumbnailSrc) {
       // the thumbnail may have failed to load because it's restricted
       //   → retry with an authorization token
       //     only do this once; fall back to the default if it still fails
       this.retriedWithToken = true;
 
-      const thumbnail = this.bitstream;
       this.auth.isAuthenticated().pipe(
         switchMap((isLoggedIn) => {
-          if (isLoggedIn && hasValue(thumbnail)) {
+          if (isLoggedIn) {
             return this.authorizationService.isAuthorized(FeatureID.CanDownload, thumbnail.self);
           } else {
             return observableOf(false);
@@ -118,7 +129,7 @@ export class ThumbnailComponent implements OnChanges {
         }),
         switchMap((isAuthorized) => {
           if (isAuthorized) {
-            return this.fileService.retrieveFileDownloadLink(thumbnail._links.content.href);
+            return this.fileService.retrieveFileDownloadLink(thumbnailSrc);
           } else {
             return observableOf(null);
           }
@@ -130,27 +141,17 @@ export class ThumbnailComponent implements OnChanges {
           // Otherwise, fall back to the default image right now
           this.setSrc(url);
         } else {
-          this.showFallback();
+          this.setSrc(this.defaultImage);
         }
       });
     } else {
-      this.showFallback();
-    }
-  }
-
-  /**
-   * To be called when the requested thumbnail could not be found
-   * - If the current src is not the default image, try that first
-   * - If this was already the case and the default image could not be found either,
-   *   show an HTML placecholder by setting src to null
-   *
-   * Also stops the loading animation.
-   */
-  showFallback() {
-    if (this.src$.getValue() !== this.defaultImage) {
-      this.setSrc(this.defaultImage);
-    } else {
-      this.setSrc(null);
+      if (src !== this.defaultImage) {
+        // we failed to get thumbnail (possibly retried with a token but failed again)
+        this.setSrc(this.defaultImage);
+      } else {
+        // we have failed to retrieve the default image, fall back to the placeholder
+        this.setSrc(null);
+      }
     }
   }