Fix default image logic in ds-thumbnail

Author: ybnd

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,30 +95,41 @@ 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);
           }
         }),
         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);
+      }
     }
   }