Request-a-copy: Error handling (review feedback)

* New accessExpired flag can be checked along with
  acceptRequest, so expired or not-granted links
  will show an error and draw 'normal' download links
  instead of doing a hard 4xx redirect
* Old ItemWithSupplementaryData component removed
* Notifications moved to sub-component of item page
  and display a warning on valid access token, or a
  danger alert if expired or not-granted, with an
  appropriate error message
* Date formatted in messages as yyyy-MM-dd

Author: kshepherd

src/app/core/auth/access-token.resolver.ts

@@ -4,11 +4,15 @@ import {
   Router,
 } from '@angular/router';
 import { Observable } from 'rxjs';
-import { tap } from 'rxjs/operators';
+import {
+  map,
+  tap,
+} from 'rxjs/operators';
 
 import { getForbiddenRoute } from '../../app-routing-paths';
 import { hasValue } from '../../shared/empty.util';
 import { ItemRequestDataService } from '../data/item-request-data.service';
+import { RemoteData } from '../data/remote-data';
 import { redirectOn4xx } from '../shared/authorized.operators';
 import { ItemRequest } from '../shared/item-request.model';
 import {
@@ -43,6 +47,7 @@ export const accessTokenResolver: ResolveFn<ItemRequest> = (
     getFirstCompletedRemoteData(),
     // Handle authorization errors, not found errors and forbidden errors as normal
     redirectOn4xx(router, authService),
+    map((rd: RemoteData<ItemRequest>) => rd),
     // Get payload of the item request
     getFirstSucceededRemoteDataPayload(),
     tap(request => {

src/app/core/shared/item-request.model.ts

@@ -91,6 +91,8 @@ export class ItemRequest implements CacheableObject {
   @autoserialize
   accessExpiry: string;
 
+  @autoserialize
+  accessExpired: boolean;
   /**
    * The {@link HALLink}s for this ItemRequest
    */

src/app/core/shared/item-with-supplementary-data.model.ts

@@ -6,6 +6,7 @@ import {
 } from '@angular/core/testing';
 import { By } from '@angular/platform-browser';
 import { BrowserAnimationsModule } from '@angular/platform-browser/animations';
+import { ActivatedRoute } from '@angular/router';
 import {
   TranslateLoader,
   TranslateModule,
@@ -22,6 +23,7 @@ import { MockBitstreamFormat1 } from '../../shared/mocks/item.mock';
 import { getMockThemeService } from '../../shared/mocks/theme-service.mock';
 import { TranslateLoaderMock } from '../../shared/mocks/translate-loader.mock';
 import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils';
+import { ActivatedRouteStub } from '../../shared/testing/active-router.stub';
 import { createPaginatedList } from '../../shared/testing/utils.test';
 import { ThemeService } from '../../shared/theme-support/theme.service';
 import { FileSizePipe } from '../../shared/utils/file-size-pipe';
@@ -91,6 +93,7 @@ describe('MediaViewerComponent', () => {
         { provide: BitstreamDataService, useValue: bitstreamDataService },
         { provide: ThemeService, useValue: getMockThemeService() },
         { provide: AuthService, useValue: new AuthServiceMock() },
+        { provide: ActivatedRoute, useValue: new ActivatedRouteStub() },
       ],
       schemas: [NO_ERRORS_SCHEMA],
     }).compileComponents();

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

@@ -6,6 +6,7 @@ import {
   OnDestroy,
   OnInit,
 } from '@angular/core';
+import { ActivatedRoute } from '@angular/router';
 import { TranslateModule } from '@ngx-translate/core';
 import {
   BehaviorSubject,
@@ -25,7 +26,7 @@ import { RemoteData } from '../../core/data/remote-data';
 import { Bitstream } from '../../core/shared/bitstream.model';
 import { BitstreamFormat } from '../../core/shared/bitstream-format.model';
 import { Item } from '../../core/shared/item.model';
-import { ItemWithSupplementaryData } from '../../core/shared/item-with-supplementary-data.model';
+import { ItemRequest } from '../../core/shared/item-request.model';
 import { MediaViewerItem } from '../../core/shared/media-viewer-item.model';
 import { getFirstSucceededRemoteDataPayload } from '../../core/shared/operators';
 import { hasValue } from '../../shared/empty.util';
@@ -71,9 +72,12 @@ export class MediaViewerComponent implements OnDestroy, OnInit {
 
   subs: Subscription[] = [];
 
+  itemRequest: ItemRequest;
+
   constructor(
     protected bitstreamDataService: BitstreamDataService,
     protected changeDetectorRef: ChangeDetectorRef,
+    protected route: ActivatedRoute,
   ) {
   }
 
@@ -85,6 +89,7 @@ export class MediaViewerComponent implements OnDestroy, OnInit {
    * This method loads all the Bitstreams and Thumbnails and converts it to {@link MediaViewerItem}s
    */
   ngOnInit(): void {
+    this.itemRequest = this.route.snapshot.data.itemRequest;
     const types: string[] = [
       ...(this.mediaOptions.image ? ['image'] : []),
       ...(this.mediaOptions.video ? ['audio', 'video'] : []),
@@ -170,9 +175,10 @@ export class MediaViewerComponent implements OnDestroy, OnInit {
    * Get access token, if this is accessed via a Request-a-Copy link
    */
   get accessToken() {
-    if (this.item instanceof ItemWithSupplementaryData && hasValue(this.item.itemRequest)) {
-      return this.item.itemRequest.accessToken;
+    if (hasValue(this.itemRequest) && this.itemRequest.accessToken && !this.itemRequest.accessExpired) {
+      return this.itemRequest.accessToken;
     }
     return null;
   }
+
 }

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

@@ -0,0 +1,25 @@
+<ng-container *ngVar="(itemRequest$ | async) as itemRequest">
+  @if (hasValue(itemRequest)) {
+    @if (!itemRequest.acceptRequest) {
+      <!-- The request has NOT been accepted, display an error -->
+      <div class="alert alert-danger wb-100 mb-2 request-a-copy-access-success">
+        <p><span role="img" class="request-a-copy-access-error-icon" [attr.aria-label]="'bitstream-request-a-copy.access-by-token.alt-text' | translate"><i class="fa-solid fa-lock"></i></span>{{'bitstream-request-a-copy.access-by-token.not-granted' | translate}}</p>
+        <p>{{'bitstream-request-a-copy.access-by-token.re-request' | translate}}</p>
+      </div>
+    } @else if (itemRequest.accessExpired) {
+      <!-- The request is accepted, but the access period has expired, display an error -->
+      <div class="alert alert-danger wb-100 mb-2 request-a-copy-access-expired">
+        <p><span role="img" class="request-a-copy-access-error-icon" [attr.aria-label]="'bitstream-request-a-copy.access-by-token.alt-text' | translate"><i class="fa-solid fa-lock"></i></span>{{'bitstream-request-a-copy.access-by-token.expired' | translate}} {{ formatDate(itemRequest.accessExpiry) }}</p>
+        <p>{{'bitstream-request-a-copy.access-by-token.re-request' | translate}}</p>
+      </div>
+    } @else {
+      <div class="alert alert-warning wb-100 mb-2 request-a-copy-access-denied">
+        <p><span role="img" class="request-a-copy-access-icon" [attr.aria-label]="'bitstream-request-a-copy.access-by-token.alt-text' | translate"><i class="fa-solid fa-lock-open"></i></span>{{'bitstream-request-a-copy.access-by-token.warning' | translate}}</p>
+        <!-- Only show the expiry date if it's not null, and doesn't start with the "FOREVER" year -->
+        @if (hasValue(itemRequest.accessExpiry) && !itemRequest.accessExpiry.startsWith('+294276')) {
+          <p>{{ 'bitstream-request-a-copy.access-by-token.expiry-label' | translate }} {{ formatDate(itemRequest.accessExpiry) }}</p>
+        }
+      </div>
+    }
+  }
+</ng-container>

src/app/item-page/simple/access-by-token-notification/access-by-token-notification.component.html

@@ -0,0 +1,7 @@
+.request-a-copy-access-icon {
+  margin-right: 4px;
+  color: var(--bs-success);
+}
+.request-a-copy-access-error-icon {
+  margin-right: 4px;
+}

src/app/item-page/simple/access-by-token-notification/access-by-token-notification.component.scss

@@ -0,0 +1,114 @@
+import { CommonModule } from '@angular/common';
+import {
+  ComponentFixture,
+  TestBed,
+} from '@angular/core/testing';
+import { By } from '@angular/platform-browser';
+import { ActivatedRoute } from '@angular/router';
+import { provideMockStore } from '@ngrx/store/testing';
+import { TranslateModule } from '@ngx-translate/core';
+import { BehaviorSubject } from 'rxjs';
+import { map } from 'rxjs/operators';
+import { SplitPipe } from 'src/app/shared/utils/split.pipe';
+
+import { APP_DATA_SERVICES_MAP } from '../../../../config/app-config.interface';
+import { RemoteDataBuildService } from '../../../core/cache/builders/remote-data-build.service';
+import { ObjectCacheService } from '../../../core/cache/object-cache.service';
+import { RequestService } from '../../../core/data/request.service';
+import { HALEndpointService } from '../../../core/shared/hal-endpoint.service';
+import { ItemRequest } from '../../../core/shared/item-request.model';
+import { NotificationsService } from '../../../shared/notifications/notifications.service';
+import { ActivatedRouteStub } from '../../../shared/testing/active-router.stub';
+import { HALEndpointServiceStub } from '../../../shared/testing/hal-endpoint-service.stub';
+import { VarDirective } from '../../../shared/utils/var.directive';
+import { AccessByTokenNotificationComponent } from './access-by-token-notification.component';
+
+describe('AccessByTokenNotificationComponent', () => {
+  let component: AccessByTokenNotificationComponent;
+  let fixture: ComponentFixture<AccessByTokenNotificationComponent>;
+  let activatedRouteStub: ActivatedRouteStub;
+  let itemRequestSubject: BehaviorSubject<ItemRequest>;
+
+  const createItemRequest = (acceptRequest: boolean, accessExpired: boolean, accessExpiry?: string): ItemRequest => {
+    const itemRequest = new ItemRequest();
+    itemRequest.acceptRequest = acceptRequest;
+    itemRequest.accessExpired = accessExpired;
+    itemRequest.accessExpiry = accessExpiry;
+    return itemRequest;
+  };
+
+  beforeEach(async () => {
+    itemRequestSubject = new BehaviorSubject<ItemRequest>(null);
+    activatedRouteStub = new ActivatedRouteStub({}, { itemRequest: null });
+    (activatedRouteStub as any).data = itemRequestSubject.asObservable().pipe(
+      map(itemRequest => ({ itemRequest })),
+    );
+
+    await TestBed.configureTestingModule({
+      imports: [
+        CommonModule,
+        TranslateModule.forRoot(),
+        AccessByTokenNotificationComponent,
+        SplitPipe,
+        VarDirective,
+      ],
+      providers: [
+        { provide: APP_DATA_SERVICES_MAP, useValue: {} },
+        { provide: ActivatedRoute, useValue: activatedRouteStub },
+        { provide: RequestService, useValue: {} },
+        { provide: NotificationsService, useValue: {} },
+        { provide: HALEndpointService, useValue: new HALEndpointServiceStub('test') },
+        ObjectCacheService,
+        RemoteDataBuildService,
+        provideMockStore({}),
+      ],
+    })
+      .compileComponents();
+
+    fixture = TestBed.createComponent(AccessByTokenNotificationComponent);
+    component = fixture.componentInstance;
+  });
+
+  it('should create', () => {
+    fixture.detectChanges();
+    expect(component).toBeTruthy();
+  });
+
+  it('should not display any alert when no itemRequest is present', () => {
+    itemRequestSubject.next(null);
+    fixture.detectChanges();
+
+    const alertElements = fixture.debugElement.queryAll(By.css('.alert'));
+    expect(alertElements.length).toBe(0);
+  });
+
+  it('should display an error alert when request has not been accepted', () => {
+    // Set up a request that has not been accepted
+    const itemRequest = createItemRequest(false, false);
+    itemRequestSubject.next(itemRequest);
+    fixture.detectChanges();
+
+    // Check for the error alert with the correct class
+    const alertElement = fixture.debugElement.query(By.css('.alert.alert-danger.request-a-copy-access-success'));
+    expect(alertElement).toBeTruthy();
+
+    // Verify the content includes the lock icon
+    const lockIcon = alertElement.query(By.css('.fa-lock'));
+    expect(lockIcon).toBeTruthy();
+
+    // Verify the text content mentions re-requesting
+    const paragraphs = alertElement.queryAll(By.css('p'));
+    expect(paragraphs.length).toBe(2);
+  });
+
+  it('should display an expired access alert when access period has expired', () => {
+    // Set up a request that has been accepted but expired
+    const itemRequest = createItemRequest(true, true, '2023-01-01');
+    itemRequestSubject.next(itemRequest);
+    fixture.detectChanges();
+
+    // Check for the expired alert with the correct class
+    const alertElement = fixture.debugElement.query(By.css('.alert.alert-danger.request-a-copy-access-expired'));
+    expect(alertElement).toBeTruthy();
+  });
+});

src/app/item-page/simple/access-by-token-notification/access-by-token-notification.component.spec.ts

@@ -0,0 +1,56 @@
+import { AsyncPipe } from '@angular/common';
+import {
+  Component,
+  OnInit,
+} from '@angular/core';
+import { ActivatedRoute } from '@angular/router';
+import { TranslateModule } from '@ngx-translate/core';
+import { Observable } from 'rxjs';
+import { map } from 'rxjs/operators';
+
+import { ItemRequest } from '../../../core/shared/item-request.model';
+import {
+  dateToString,
+  stringToNgbDateStruct,
+} from '../../../shared/date.util';
+import {
+  hasValue,
+  isNotEmpty,
+} from '../../../shared/empty.util';
+import { VarDirective } from '../../../shared/utils/var.directive';
+
+@Component({
+  selector: 'ds-access-by-token-notification',
+  templateUrl: './access-by-token-notification.component.html',
+  styleUrls: ['./access-by-token-notification.component.scss'],
+  imports: [
+    AsyncPipe,
+    TranslateModule,
+    VarDirective,
+  ],
+  standalone: true,
+})
+export class AccessByTokenNotificationComponent implements OnInit {
+
+  itemRequest$: Observable<ItemRequest>;
+  protected readonly hasValue = hasValue;
+
+  constructor(protected route: ActivatedRoute) {
+  }
+
+  ngOnInit() {
+    this.itemRequest$ = this.route.data.pipe(
+      map((data) => data.itemRequest as ItemRequest),
+    );
+  }
+
+  /**
+   * Returns a date in simplified format (YYYY-MM-DD).
+   *
+   * @param date
+   * @return a string with formatted date
+   */
+  formatDate(date: string): string {
+    return isNotEmpty(date) ? dateToString(stringToNgbDateStruct(date)) : '';
+  }
+}

src/app/item-page/simple/access-by-token-notification/access-by-token-notification.component.ts

@@ -3,18 +3,9 @@
     <div class="item-page" @fadeInOut>
       @if (itemRD?.payload; as item) {
         <div>
-          <ng-container *ngVar="(itemRequest$ | async) as itemRequest">
-            @if (hasValue(itemRequest)) {
-              <div class="alert alert-warning wb-100 mb-2">
-                <p><span role="img" class="request-a-copy-access-icon" [attr.aria-label]="'bitstream-request-a-copy.access-by-token.alt-text' | translate"><i class="fa-solid fa-lock-open"></i></span>{{'bitstream-request-a-copy.access-by-token.warning' | translate}}</p>
-                <!-- Only show the expiry date if it's not null, and doesn't start with the "FOREVER" year -->
-                @if (hasValue(itemRequest.accessExpiry) && !itemRequest.accessExpiry.startsWith('+294276')) {
-                  <p>{{ 'bitstream-request-a-copy.access-by-token.expiry-label' | translate }} {{ itemRequest.accessExpiry }}</p>
-                }
-              </div>
-            }
-          </ng-container>
+
           <ds-item-alerts [item]="item"></ds-item-alerts>
+          <ds-access-by-token-notification></ds-access-by-token-notification>
           <ds-qa-event-notification [item]="item"></ds-qa-event-notification>
           <ds-notify-requests-status [itemUuid]="item.uuid"></ds-notify-requests-status>
           <ds-item-versions-notice [item]="item"></ds-item-versions-notice>