Merge pull request #4249 from atmire/fix-embargoed-thumbnails_contribute-9.0

Show restricted thumbnails to users with access rights

Author: tdonohue

cypress/e2e/homepage.cy.ts

@@ -26,6 +26,12 @@ describe('Homepage', () => {
     // Wait for homepage tag to appear
     cy.get('ds-home-page').should('be.visible');
 
+    // Wait for at least one loading component to show up
+    cy.get('ds-loading').should('exist');
+
+    // Wait until all loading components have disappeared
+    cy.get('ds-loading').should('not.exist');
+
     // Analyze <ds-home-page> for accessibility issues
     testA11y('ds-home-page');
   });

src/app/item-page/media-viewer/media-viewer.component.html

@@ -5,7 +5,7 @@
       [showMessage]="false"
     ></ds-loading>
   }
-  @if (!isLoading) {
+  @else {
     <div class="media-viewer">
       @if (mediaList.length > 0) {
         <ng-container *ngVar="mediaOptions.video && ['audio', 'video'].includes(mediaList[0]?.format) as showVideo">
@@ -33,17 +33,9 @@
           </ng-container>
         </ng-container>
       } @else {
-        @if (mediaOptions.image && mediaOptions.video) {
-          <ds-media-viewer-image
-            [image]="(thumbnailsRD$ | async)?.payload?.page[0]?._links.content.href || thumbnailPlaceholder"
-            [preview]="false"
-          ></ds-media-viewer-image>
-        }
-        @if (!(mediaOptions.image && mediaOptions.video)) {
-          <ds-thumbnail
-            [thumbnail]="(thumbnailsRD$ | async)?.payload?.page[0]">
-          </ds-thumbnail>
-        }
+        <ds-thumbnail
+          [thumbnail]="(thumbnailsRD$ | async)?.payload?.page[0]">
+        </ds-thumbnail>
       }
     </div>
   }

src/app/item-page/media-viewer/media-viewer.component.spec.ts

@@ -1,4 +1,7 @@
-import { NO_ERRORS_SCHEMA } from '@angular/core';
+import {
+  NO_ERRORS_SCHEMA,
+  PLATFORM_ID,
+} from '@angular/core';
 import {
   ComponentFixture,
   TestBed,
@@ -15,7 +18,9 @@ import { of as observableOf } from 'rxjs';
 
 import { AuthService } from '../../core/auth/auth.service';
 import { BitstreamDataService } from '../../core/data/bitstream-data.service';
+import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
 import { Bitstream } from '../../core/shared/bitstream.model';
+import { FileService } from '../../core/shared/file.service';
 import { MediaViewerItem } from '../../core/shared/media-viewer-item.model';
 import { MetadataFieldWrapperComponent } from '../../shared/metadata-field-wrapper/metadata-field-wrapper.component';
 import { AuthServiceMock } from '../../shared/mocks/auth.service.mock';
@@ -33,6 +38,9 @@ import { MediaViewerComponent } from './media-viewer.component';
 describe('MediaViewerComponent', () => {
   let comp: MediaViewerComponent;
   let fixture: ComponentFixture<MediaViewerComponent>;
+  let authService;
+  let authorizationService;
+  let fileService;
 
   const mockBitstream: Bitstream = Object.assign(new Bitstream(), {
     sizeBytes: 10201,
@@ -57,7 +65,7 @@ describe('MediaViewerComponent', () => {
       'dc.title': [
         {
           language: null,
-          value: 'test_word.docx',
+          value: 'test_image.jpg',
         },
       ],
     },
@@ -75,6 +83,15 @@ describe('MediaViewerComponent', () => {
   );
 
   beforeEach(waitForAsync(() => {
+    authService = jasmine.createSpyObj('AuthService', {
+      isAuthenticated: observableOf(true),
+    });
+    authorizationService = jasmine.createSpyObj('AuthorizationService', {
+      isAuthorized: observableOf(true),
+    });
+    fileService = jasmine.createSpyObj('FileService', {
+      retrieveFileDownloadLink: null,
+    });
     return TestBed.configureTestingModule({
       imports: [
         TranslateModule.forRoot({
@@ -90,6 +107,10 @@ describe('MediaViewerComponent', () => {
         MetadataFieldWrapperComponent,
       ],
       providers: [
+        { provide: AuthService, useValue: authService },
+        { provide: AuthorizationDataService, useValue: authorizationService },
+        { provide: FileService, useValue: fileService },
+        { provide: PLATFORM_ID, useValue: 'browser' },
         { provide: BitstreamDataService, useValue: bitstreamDataService },
         { provide: ThemeService, useValue: getMockThemeService() },
         { provide: AuthService, useValue: new AuthServiceMock() },
@@ -153,9 +174,9 @@ describe('MediaViewerComponent', () => {
       expect(mediaItem.thumbnail).toBe(null);
     });
 
-    it('should display a default, thumbnail', () => {
+    it('should display a default thumbnail', () => {
       const defaultThumbnail = fixture.debugElement.query(
-        By.css('ds-media-viewer-image'),
+        By.css('ds-thumbnail'),
       );
       expect(defaultThumbnail.nativeElement).toBeDefined();
     });

src/app/thumbnail/thumbnail.component.html

@@ -1,5 +1,5 @@
 <div class="thumbnail" [class.limit-width]="limitWidth">
-  @if (isLoading) {
+  @if (isLoading()) {
   <div class="thumbnail-content outer">
     <div class="inner">
       <div class="centered">
@@ -9,16 +9,16 @@
   </div>
   }
   <!-- don't use *ngIf="!isLoading" so the thumbnail can load in while the animation is playing -->
-  @if (src !== null) {
+  @if (src() !== null) {
   <img
     class="thumbnail-content img-fluid"
-    [ngClass]="{ 'd-none': isLoading }"
-    [src]="src | dsSafeUrl"
-    [alt]="alt | translate"
-    (error)="errorHandler()"
-    (load)="successHandler()"
+    [ngClass]="{ 'd-none': isLoading ()}"
+         [src]="src() | dsSafeUrl"
+         [alt]="alt | translate"
+         (error)="errorHandler()"
+         (load)="successHandler()"
   />
-  } @if (src === null && !isLoading) {
+  } @if (src() === null && isLoading() === false) {
   <div class="thumbnail-content outer">
     <div class="inner">
       <div class="thumbnail-placeholder centered">

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

@@ -99,32 +99,32 @@ describe('ThumbnailComponent', () => {
     });
 
     describe('loading', () => {
-      it('should start out with isLoading$ true', () => {
-        expect(comp.isLoading).toBeTrue();
+      it('should start out with isLoading true', () => {
+        expect(comp.isLoading()).toBeTrue();
       });
 
       it('should set isLoading$ to false once an image is successfully loaded', () => {
         comp.setSrc('http://bit.stream');
         fixture.debugElement.query(By.css('img.thumbnail-content')).triggerEventHandler('load', new Event('load'));
-        expect(comp.isLoading).toBeFalse();
+        expect(comp.isLoading()).toBeFalse();
       });
 
       it('should set isLoading$ to false once the src is set to null', () => {
         comp.setSrc(null);
-        expect(comp.isLoading).toBeFalse();
+        expect(comp.isLoading()).toBeFalse();
       });
 
       it('should show a loading animation while isLoading$ is true', () => {
         expect(de.query(By.css('ds-loading'))).toBeTruthy();
 
-        comp.isLoading = false;
+        comp.isLoading.set(false);
         fixture.detectChanges();
         expect(fixture.debugElement.query(By.css('ds-loading'))).toBeFalsy();
       });
 
       describe('with a thumbnail image', () => {
         beforeEach(() => {
-          comp.src = 'https://bit.stream';
+          comp.src.set('https://bit.stream');
           fixture.detectChanges();
         });
 
@@ -133,7 +133,7 @@ describe('ThumbnailComponent', () => {
           expect(img).toBeTruthy();
           expect(img.classes['d-none']).toBeTrue();
 
-          comp.isLoading = false;
+          comp.isLoading.set(false);
           fixture.detectChanges();
           img = fixture.debugElement.query(By.css('img.thumbnail-content'));
           expect(img).toBeTruthy();
@@ -144,14 +144,14 @@ describe('ThumbnailComponent', () => {
 
       describe('without a thumbnail image', () => {
         beforeEach(() => {
-          comp.src = null;
+          comp.src.set(null);
           fixture.detectChanges();
         });
 
         it('should only show the HTML placeholder once done loading', () => {
           expect(fixture.debugElement.query(By.css('div.thumbnail-placeholder'))).toBeFalsy();
 
-          comp.isLoading = false;
+          comp.isLoading.set(false);
           fixture.detectChanges();
           expect(fixture.debugElement.query(By.css('div.thumbnail-placeholder'))).toBeTruthy();
         });
@@ -247,14 +247,14 @@ describe('ThumbnailComponent', () => {
     describe('fallback', () => {
       describe('if there is a default image', () => {
         it('should display the default image', () => {
-          comp.src = 'http://bit.stream';
+          comp.src.set('http://bit.stream');
           comp.defaultImage = 'http://default.img';
           comp.errorHandler();
-          expect(comp.src).toBe(comp.defaultImage);
+          expect(comp.src()).toBe(comp.defaultImage);
         });
 
         it('should include the alt text', () => {
-          comp.src = 'http://bit.stream';
+          comp.src.set('http://bit.stream');
           comp.defaultImage = 'http://default.img';
           comp.errorHandler();
 
@@ -266,10 +266,10 @@ describe('ThumbnailComponent', () => {
 
       describe('if there is no default image', () => {
         it('should display the HTML placeholder', () => {
-          comp.src = 'http://default.img';
+          comp.src.set('http://default.img');
           comp.defaultImage = null;
           comp.errorHandler();
-          expect(comp.src).toBe(null);
+          expect(comp.src()).toBe(null);
 
           fixture.detectChanges();
           const placeholder = fixture.debugElement.query(By.css('div.thumbnail-placeholder')).nativeElement;
@@ -363,7 +363,7 @@ describe('ThumbnailComponent', () => {
         it('should show the default image', () => {
           comp.defaultImage = 'default/image.jpg';
           comp.ngOnChanges({});
-          expect(comp.src).toBe('default/image.jpg');
+          expect(comp.src()).toBe('default/image.jpg');
         });
       });
     });
@@ -419,7 +419,7 @@ describe('ThumbnailComponent', () => {
     });
 
     it('should start out with isLoading$ true', () => {
-      expect(comp.isLoading).toBeTrue();
+      expect(comp.isLoading()).toBeTrue();
       expect(de.query(By.css('ds-loading'))).toBeTruthy();
     });
 

src/app/thumbnail/thumbnail.component.ts

@@ -8,7 +8,9 @@ import {
   Input,
   OnChanges,
   PLATFORM_ID,
+  signal,
   SimpleChanges,
+  WritableSignal,
 } from '@angular/core';
 import { TranslateModule } from '@ngx-translate/core';
 import { of as observableOf } from 'rxjs';
@@ -26,7 +28,6 @@ import {
 } from '../shared/empty.util';
 import { ThemedLoadingComponent } from '../shared/loading/themed-loading.component';
 import { SafeUrlPipe } from '../shared/utils/safe-url-pipe';
-import { VarDirective } from '../shared/utils/var.directive';
 
 /**
  * This component renders a given Bitstream as a thumbnail.
@@ -38,7 +39,7 @@ import { VarDirective } from '../shared/utils/var.directive';
   styleUrls: ['./thumbnail.component.scss'],
   templateUrl: './thumbnail.component.html',
   standalone: true,
-  imports: [VarDirective, CommonModule, ThemedLoadingComponent, TranslateModule, SafeUrlPipe],
+  imports: [CommonModule, ThemedLoadingComponent, TranslateModule, SafeUrlPipe],
 })
 export class ThumbnailComponent implements OnChanges {
   /**
@@ -55,7 +56,7 @@ export class ThumbnailComponent implements OnChanges {
   /**
    * The src attribute used in the template to render the image.
    */
-  src: string = undefined;
+  src: WritableSignal<string> = signal(undefined);
 
   retriedWithToken = false;
 
@@ -78,7 +79,7 @@ export class ThumbnailComponent implements OnChanges {
    * Whether the thumbnail is currently loading
    * Start out as true to avoid flashing the alt text while a thumbnail is being loaded.
    */
-  isLoading = true;
+  isLoading: WritableSignal<boolean> = signal(true);
 
   constructor(
     @Inject(PLATFORM_ID) private platformID: any,
@@ -134,7 +135,7 @@ export class ThumbnailComponent implements OnChanges {
    * Otherwise, fall back to the default image or a HTML placeholder
    */
   errorHandler() {
-    const src = this.src;
+    const src = this.src();
     const thumbnail = this.bitstream;
     const thumbnailSrc = thumbnail?._links?.content?.href;
 
@@ -186,16 +187,29 @@ export class ThumbnailComponent implements OnChanges {
    * @param src
    */
   setSrc(src: string): void {
-    this.src = src;
-    if (src === null) {
-      this.isLoading = false;
+    // only update the src if it has changed (the parent component may fire the same one multiple times
+    if (this.src() !== src) {
+      // every time the src changes we need to start the loading animation again, as it's possible
+      // that it is first set to null when the parent component initializes and then set to
+      // the actual value
+      //
+      // isLoading$ will be set to false by the error or success handler afterwards, except in the
+      // case where src is null, then we have to set it manually here (because those handlers won't
+      // trigger)
+      if (src !== null && this.isLoading() === false) {
+        this.isLoading.set(true);
+      }
+      this.src.set(src);
+      if (src === null && this.isLoading() === true) {
+        this.isLoading.set(false);
+      }
     }
   }
 
   /**
    * Stop the loading animation once the thumbnail is successfully loaded
    */
   successHandler() {
-    this.isLoading = false;
+    this.isLoading.set(false);
   }
 }