diff --git a/src/app/core/components/folder-picker/folder-picker.component.html b/src/app/core/components/folder-picker/folder-picker.component.html index 13afb9ce8..8cf3e1335 100644 --- a/src/app/core/components/folder-picker/folder-picker.component.html +++ b/src/app/core/components/folder-picker/folder-picker.component.html @@ -33,7 +33,7 @@ } @default { -
+
} }
{{ item.displayName }}
@@ -50,7 +50,7 @@ } @if (selectedRecord) {
-
+
}
diff --git a/src/app/core/components/folder-picker/folder-picker.component.spec.ts b/src/app/core/components/folder-picker/folder-picker.component.spec.ts index 93316a970..976b35c17 100644 --- a/src/app/core/components/folder-picker/folder-picker.component.spec.ts +++ b/src/app/core/components/folder-picker/folder-picker.component.spec.ts @@ -6,7 +6,9 @@ import { DataService } from '@shared/services/data/data.service'; import { ApiService } from '@shared/services/api/api.service'; import { FolderResponse } from '@shared/services/api/index.repo'; import { SharedModule } from '@shared/shared.module'; -import { FolderVO } from '@root/app/models'; +import { By } from '@angular/platform-browser'; +import { BgImageSrcDirective } from '@shared/directives/bg-image-src.directive'; +import { FolderVO, RecordVO } from '@root/app/models'; import { HttpTestingController } from '@angular/common/http/testing'; import { FolderPickerService } from '@core/services/folder-picker/folder-picker.service'; import { DataStatus } from '@models/data-status.enum'; @@ -80,4 +82,50 @@ describe('FolderPickerComponent', () => { ), ).toBeFalsy(); }); + + it('should read the thumbnail currently on the item', () => { + const record = new RecordVO({ folder_linkId: 2, archiveNbr: 'a-2' }); + + expect(component.getThumbnailUrl(record)).toBeUndefined(); + + record.thumbnail256 = 'https://example.com/256'; + + expect(component.getThumbnailUrl(record)).toBe('https://example.com/256'); + }); + + it('should show a thumbnail that arrives after the row is rendered', () => { + const record = new RecordVO({ + folder_linkId: 1, + archiveNbr: 'a-1', + displayName: 'photo.jpg', + }); + const folder = new FolderVO({ + folder_linkId: 9, + folderId: 9, + displayName: 'Photos', + type: 'type.folder.private.folder', + }); + folder.ChildItemVOs = [record]; + + component.allowRecords = true; + component.currentFolder = folder; + fixture.detectChanges(); + + const backgrounds = fixture.debugElement.queryAll( + By.directive(BgImageSrcDirective), + ); + + expect(backgrounds.length).toBe(1); + + const background = backgrounds[0].injector.get(BgImageSrcDirective); + + expect(background.bgSrc).toBeFalsy(); + + // loadCurrentFolderChildData() writes the URL onto this same instance, so + // the row has to notice a mutation that leaves the reference unchanged. + record.thumbURL200 = 'https://example.com/thumb.jpg'; + fixture.detectChanges(); + + expect(background.bgSrc).toBe('https://example.com/thumb.jpg'); + }); }); diff --git a/src/app/core/components/folder-picker/folder-picker.component.ts b/src/app/core/components/folder-picker/folder-picker.component.ts index 3451f3c45..067921a16 100644 --- a/src/app/core/components/folder-picker/folder-picker.component.ts +++ b/src/app/core/components/folder-picker/folder-picker.component.ts @@ -2,6 +2,7 @@ import { Component, OnDestroy } from '@angular/core'; import { remove } from 'lodash'; import { DataService } from '@shared/services/data/data.service'; import { FolderVO, ItemVO, RecordVO } from '@root/app/models/index'; +import { GetThumbnail } from '@models/get-thumbnail'; import { ApiService } from '@shared/services/api/api.service'; import { FolderResponse } from '@shared/services/api/index.repo'; import { FolderPickerService } from '@core/services/folder-picker/folder-picker.service'; @@ -113,6 +114,15 @@ export class FolderPickerComponent implements OnDestroy { this.selectedRecord = record; } + // Read from the item on every check rather than through the getThumbnail pipe. + // The lean fetch that follows setFolder() writes thumbnail URLs onto these same + // item instances, and a pure pipe never re-runs for a mutation that leaves the + // reference unchanged, so the picker showed no thumbnails until its rows were + // rebuilt by navigating away and back. + getThumbnailUrl(item: ItemVO): string | undefined { + return GetThumbnail(item); + } + async setFolder(folder: FolderVO) { this.waiting = true; try { diff --git a/src/app/core/components/profile-edit/profile-edit.component.html b/src/app/core/components/profile-edit/profile-edit.component.html index 0b4a645c1..9bb3be5ef 100644 --- a/src/app/core/components/profile-edit/profile-edit.component.html +++ b/src/app/core/components/profile-edit/profile-edit.component.html @@ -136,7 +136,7 @@
diff --git a/src/app/core/components/profile-edit/profile-edit.component.spec.ts b/src/app/core/components/profile-edit/profile-edit.component.spec.ts index 692e2e20d..81118c31f 100644 --- a/src/app/core/components/profile-edit/profile-edit.component.spec.ts +++ b/src/app/core/components/profile-edit/profile-edit.component.spec.ts @@ -9,7 +9,7 @@ import { PromptService } from '@shared/services/prompt/prompt.service'; import { EventService } from '@shared/services/event/event.service'; import { CookieService } from 'ngx-cookie-service'; import { DIALOG_DATA, DialogRef } from '@angular/cdk/dialog'; -import { FolderVO } from '@models/index'; +import { ArchiveVO, FolderVO } from '@models/index'; import { RecordVO } from '@models/record-vo'; import { FolderResponse } from '@shared/services/api/folder.repo'; import { GetThumbnailPipe } from '@shared/pipes/get-thumbnail.pipe'; @@ -143,6 +143,18 @@ describe('ProfileEditComponent', () => { expect(component.publicRoot.thumbURL2000).toBe('new2000'); }); + it('should show a profile photo written onto the archive after the picker closes', () => { + component.archive = new ArchiveVO({ archiveNbr: 'a-1' }); + + expect(component.profileThumbnail).toBeNull(); + + // promptForProfilePicture() updates this same instance rather than + // replacing it, so the binding has to re-read an unchanged reference. + component.archive.thumbURL200 = 'new200'; + + expect(component.profileThumbnail).toBe('new200'); + }); + it('should restore original thumbArchiveNbr when chooseBannerPicture throws FolderResponse', async () => { const originalValue = component.publicRoot.thumbArchiveNbr; diff --git a/src/app/core/components/profile-edit/profile-edit.component.ts b/src/app/core/components/profile-edit/profile-edit.component.ts index 4c83b082a..ac50e4e1a 100644 --- a/src/app/core/components/profile-edit/profile-edit.component.ts +++ b/src/app/core/components/profile-edit/profile-edit.component.ts @@ -23,7 +23,7 @@ import { collapseAnimation, ngIfScaleAnimationDynamic, } from '@shared/animations'; -import { GetBanner } from '@models/get-thumbnail'; +import { GetBanner, GetThumbnail } from '@models/get-thumbnail'; import debug from 'debug'; import { PromptService, @@ -77,6 +77,18 @@ export class ProfileEditComponent implements OnInit, AfterViewInit { return this.publicRoot?.thumbArchiveNbr ? GetBanner(this.publicRoot) : null; } + // Read through to the archive rather than piping it, the way bannerThumbnail + // reads through to publicRoot. Choosing a photo writes the new URLs onto this + // same ArchiveVO instance, and a pure pipe never re-runs for a mutation that + // leaves the reference unchanged. + get profileThumbnail(): string | null { + if (!this.archive) { + return null; + } + + return GetThumbnail(this.archive) ?? null; + } + private debug = debug('component:profileEdit'); constructor( diff --git a/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts b/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts index 4140b020b..87c3e6ec4 100644 --- a/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts +++ b/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts @@ -1,6 +1,6 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; import { ElementRef, Pipe, PipeTransform } from '@angular/core'; -import { of } from 'rxjs'; +import { Subject, of } from 'rxjs'; import { ActivatedRoute, Router, provideRouter } from '@angular/router'; import { DataService } from '@shared/services/data/data.service'; @@ -41,6 +41,7 @@ describe('FileListItemComponent', () => { let component: FileListItemComponent; let fixture: ComponentFixture; let editService: EditService; + let thumbnailUpdatedSubject: Subject; const activatedRouteMock = { snapshot: { @@ -67,6 +68,8 @@ describe('FileListItemComponent', () => { }; beforeEach(async () => { + thumbnailUpdatedSubject = new Subject(); + await TestBed.configureTestingModule({ imports: [MockItemTypeIconPipe, MockPrDatePipe, MockPrConstantsPipe], declarations: [FileListItemComponent, GetThumbnailPipe], @@ -84,6 +87,7 @@ describe('FileListItemComponent', () => { beginPreparingForNavigate: jasmine.createSpy(), fetchLeanItems: jasmine.createSpy(), setItemMultiSelectStatus: jasmine.createSpy(), + thumbnailUpdated$: () => thumbnailUpdatedSubject.asObservable(), currentFolder: { type: '' }, }, }, @@ -333,6 +337,101 @@ describe('FileListItemComponent', () => { (router.routerState.snapshot as any).url = '/'; }); + it('should not replace the stock preview when a thumbnail arrives later', async () => { + // detectChanges() in beforeEach left an ngOnInit awaiting isUnlistedShare(). + // Drain it and tear the component down so this test starts from a single + // subscription, the way a real component instance does. + await new Promise((resolve) => { + setTimeout(resolve); + }); + component.ngOnDestroy(); + + const router = TestBed.inject(Router); + (router.routerState.snapshot as any).url = '/share/test'; + + const shareLinksService = TestBed.inject(ShareLinksService); + spyOn(shareLinksService, 'isUnlistedShare').and.returnValue( + Promise.resolve(false), + ); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + + await component.ngOnInit(); + + const stockPreview = component.recordThumbnailUrl; + + expect(stockPreview).toMatch(/^assets\/img\/preview\/preview-\d+\.jpg$/); + + component.item.thumbURL200 = 'https://example.com/thumb.jpg'; + thumbnailUpdatedSubject.next(component.item); + + expect(component.recordThumbnailUrl).toBe(stockPreview); + + (router.routerState.snapshot as any).url = '/'; + }); + + it('should not expose the real thumbnail until the share type is known', async () => { + // detectChanges() in beforeEach left an ngOnInit awaiting isUnlistedShare(). + // Drain it before changing the route, or its tail resumes against the route + // this test sets and overwrites the preview state with its own stale answer. + await new Promise((resolve) => { + setTimeout(resolve); + }); + + const router = TestBed.inject(Router); + (router.routerState.snapshot as any).url = '/share/test'; + + const shareLinksService = TestBed.inject(ShareLinksService); + let resolveIsUnlistedShare: (isUnlisted: boolean) => void = () => {}; + spyOn(shareLinksService, 'isUnlistedShare').and.returnValue( + new Promise((resolve) => { + resolveIsUnlistedShare = resolve; + }), + ); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + component.item.thumbURL200 = 'https://example.com/thumb.jpg'; + + // Deliberately not awaited: a listed share must not show the real + // thumbnail in the window before isUnlistedShare() settles. + const init = component.ngOnInit(); + + expect(component.recordThumbnailUrl).toBeUndefined(); + + resolveIsUnlistedShare(false); + await init; + + expect(component.recordThumbnailUrl).toMatch( + /^assets\/img\/preview\/preview-\d+\.jpg$/, + ); + + (router.routerState.snapshot as any).url = '/'; + }); + + it('should show the real thumbnail on an unlisted share', async () => { + // See above: let the ngOnInit started by beforeEach settle first. + await new Promise((resolve) => { + setTimeout(resolve); + }); + + const router = TestBed.inject(Router); + (router.routerState.snapshot as any).url = '/share/test'; + + const shareLinksService = TestBed.inject(ShareLinksService); + spyOn(shareLinksService, 'isUnlistedShare').and.returnValue( + Promise.resolve(true), + ); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + component.item.thumbURL200 = 'https://example.com/thumb.jpg'; + + await component.ngOnInit(); + + expect(component.recordThumbnailUrl).toBe('https://example.com/thumb.jpg'); + + (router.routerState.snapshot as any).url = '/'; + }); + it('should always set real thumbnail URL on init', async () => { component.item.isRecord = true; component.item.type = 'type.record.image'; @@ -343,6 +442,55 @@ describe('FileListItemComponent', () => { expect(component.recordThumbnailUrl).toBe('https://example.com/thumb.jpg'); }); + it('should pick up a thumbnail added to the item after init', async () => { + component.ngOnDestroy(); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + + await component.ngOnInit(); + + expect(component.recordThumbnailUrl).toBeUndefined(); + + // The thumbnail refresh poll in DataService mutates the existing item + // rather than replacing it, then announces the item it wrote to. + component.item.thumbnail256 = 'https://example.com/256'; + thumbnailUpdatedSubject.next(component.item); + + expect(component.recordThumbnailUrl).toBe('https://example.com/256'); + }); + + it('should ignore a thumbnail update for a different item', async () => { + component.ngOnDestroy(); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + + await component.ngOnInit(); + + component.item.thumbnail256 = 'https://example.com/256'; + // Same folder_linkId, different instance: only the item this row renders + // counts, so the update belongs to some other row. + thumbnailUpdatedSubject.next({ + folder_linkId: component.item.folder_linkId, + thumbnail256: 'https://example.com/other', + }); + + expect(component.recordThumbnailUrl).toBeUndefined(); + }); + + it('should stop applying thumbnail updates once destroyed', async () => { + component.ngOnDestroy(); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + + await component.ngOnInit(); + component.ngOnDestroy(); + + component.item.thumbnail256 = 'https://example.com/256'; + thumbnailUpdatedSubject.next(component.item); + + expect(component.recordThumbnailUrl).toBeUndefined(); + }); + it('should display displayTime instead of displayDT when displayTime is set', () => { component.item.displayTime = '2020-06-10'; component.item.displayDT = '2023-01-01T00:00:00.000Z'; diff --git a/src/app/file-browser/components/file-list-item/file-list-item.component.ts b/src/app/file-browser/components/file-list-item/file-list-item.component.ts index d67939338..0b36a9c01 100644 --- a/src/app/file-browser/components/file-list-item/file-list-item.component.ts +++ b/src/app/file-browser/components/file-list-item/file-list-item.component.ts @@ -62,6 +62,7 @@ import { unsubscribeAll, } from '@shared/utilities/hasSubscriptions'; import { Subscription } from 'rxjs'; +import { filter } from 'rxjs/operators'; import { ngIfFadeInAnimation } from '@shared/animations'; import { RouteData } from '@root/app/app.routes'; @@ -259,11 +260,15 @@ export class FileListItemComponent } async ngOnInit() { - this.recordThumbnailUrl = GetThumbnail(this.item); + this.isInSharePreview = + this.router.routerState.snapshot.url.includes('/share/'); const date = new Date(this.startDisplayTime); this.date = getFormattedDate(date); - this.isUnlistedShare = await this.shareLinksService.isUnlistedShare(); + const isUnlistedShare = await this.shareLinksService.isUnlistedShare(); + this.isUnlistedShare = isUnlistedShare; + + this.initializeThumbnail(isUnlistedShare); this.dataService.registerItem(this.item); if (this.item.type.includes('app')) { @@ -275,12 +280,8 @@ export class FileListItemComponent this.isPublicArchive = true; } - if (this.router.routerState.snapshot.url.includes('/share/')) { - if (!this.isUnlistedShare) { - this.recordThumbnailUrl = this.getRandomPreviewImage(); - } + if (this.isInSharePreview) { this.allowActions = false; - this.isInSharePreview = true; } if (this.router.routerState.snapshot.url.includes('/apps')) { @@ -1052,6 +1053,29 @@ export class FileListItemComponent }); } + // A listed share preview must never show the real content, only a stock image. + // The answer arrives asynchronously, so it is taken as an argument: the + // thumbnail cannot be resolved before the share type is known. + private initializeThumbnail(isUnlistedShare: boolean): void { + if (this.isInSharePreview && !isUnlistedShare) { + this.recordThumbnailUrl = this.getRandomPreviewImage(); + return; + } + + this.recordThumbnailUrl = GetThumbnail(this.item); + + // The thumbnail refresh poll in DataService writes new URLs onto this same + // item instance, which no binding can observe on its own. + this.subscriptions.push( + this.dataService + .thumbnailUpdated$() + .pipe(filter((updatedItem) => updatedItem === this.item)) + .subscribe(() => { + this.recordThumbnailUrl = GetThumbnail(this.item); + }), + ); + } + private getFolderThumbnail(): void { if (!this.showFolderThumbnails) { this.folderContentsType = FolderContentsType.BROKEN_THUMBNAILS; diff --git a/src/app/shared/services/data/data.service.spec.ts b/src/app/shared/services/data/data.service.spec.ts index f62bc9afa..20991ddaa 100644 --- a/src/app/shared/services/data/data.service.spec.ts +++ b/src/app/shared/services/data/data.service.spec.ts @@ -321,6 +321,247 @@ describe('DataService', () => { .catch(done.fail); }); + it('should announce the refreshed item when a thumbnail arrives', (done) => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: [ + { + folder_linkId: record.folder_linkId, + archiveNbr: record.archiveNbr, + parentFolderId: currentFolder.folderId, + thumbURL500: 'https://example.com/500', + }, + ], + }), + } as unknown as FolderResponse), + ); + + // The emitted reference is what lets a consumer tell its own item apart + // from every other row's. + service.thumbnailUpdated$().subscribe((updatedItem) => { + expect(updatedItem).toBe(record); + expect((updatedItem as RecordVO).thumbURL500).toBe( + 'https://example.com/500', + ); + done(); + }); + + service.fetchLeanItems([record]).catch(done.fail); + }); + + it('should not announce a refreshed item that still has no thumbnail', (done) => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: [ + { + folder_linkId: record.folder_linkId, + archiveNbr: record.archiveNbr, + parentFolderId: currentFolder.folderId, + }, + ], + }), + } as unknown as FolderResponse), + ); + + const thumbnailUpdated = jasmine.createSpy(); + service.thumbnailUpdated$().subscribe(thumbnailUpdated); + + service + .fetchLeanItems([record]) + .then(() => { + expect(thumbnailUpdated).not.toHaveBeenCalled(); + expect(service.getThumbRefreshQueue()).toContain(record); + done(); + }) + .catch(done.fail); + }); + + it('should update every child when the response is larger than the request', async () => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const allChildren = currentFolder.ChildItemVOs as (RecordVO | FolderVO)[]; + allChildren.forEach((item) => { + service.registerItem(item); + }); + + // Ask about two items but answer with every child of the folder. This is + // what stela's children endpoint does regardless of what was requested, so + // the response cannot be matched to the request by position. + const requested = allChildren.slice(0, 2); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: allChildren.map((item) => ({ + folder_linkId: item.folder_linkId, + archiveNbr: item.archiveNbr, + parentFolderId: currentFolder.folderId, + thumbURL500: `https://example.com/${item.folder_linkId}`, + })), + }), + } as unknown as FolderResponse), + ); + + const count = await service.fetchLeanItems(requested); + + expect(count).toBe(allChildren.length); + allChildren.forEach((item) => { + expect(item.dataStatus).toEqual(DataStatus.Lean); + expect(item.isFetching).toBeFalse(); + expect(item.thumbURL500).toBe( + `https://example.com/${item.folder_linkId}`, + ); + }); + }); + + it('should settle a requested item that is absent from the response', async () => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + // The record is no longer a child of the folder: it was moved or deleted + // between the fetch being issued and the response arriving. + const getWithChildren = spyOn( + api.folder, + 'getWithChildren', + ).and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ ChildItemVOs: [] }), + } as unknown as FolderResponse), + ); + + const inFlight = service.fetchLeanItems([record]); + const fetched = record.fetched; + await inFlight; + + await expectAsync(fetched).toBeRejected(); + + expect(record.isFetching).toBeFalse(); + expect(record.fetched).toBeNull(); + + // A stuck isFetching flag would filter the record out of every later + // fetch, leaving it permanently stale. + getWithChildren.calls.reset(); + await service.fetchLeanItems([record]); + + expect(getWithChildren).toHaveBeenCalled(); + }); + + it('should add a lean item to thumbRefreshQueue when the ids differ in type', (done) => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + // The folder came from the PHP API, so its folderId is a number, while + // stela reports the record's parentFolderId as a string. + expect(typeof currentFolder.folderId).toBe('number'); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: [ + { + folder_linkId: record.folder_linkId, + archiveNbr: record.archiveNbr, + parentFolderId: String(currentFolder.folderId), + }, + ], + }), + } as unknown as FolderResponse), + ); + + service + .fetchLeanItems([record]) + .then(() => { + expect(service.getThumbRefreshQueue()).toContain(record); + done(); + }) + .catch(done.fail); + }); + + it('should not add a lean item to thumbRefreshQueue when it belongs to another folder', (done) => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: [ + { + folder_linkId: record.folder_linkId, + archiveNbr: record.archiveNbr, + parentFolderId: `${currentFolder.folderId}0`, + }, + ], + }), + } as unknown as FolderResponse), + ); + + service + .fetchLeanItems([record]) + .then(() => { + expect(service.getThumbRefreshQueue()).not.toContain(record); + done(); + }) + .catch(done.fail); + }); + it('should add a lean item to thumbRefreshQueue when no thumbnail size is present', (done) => { const service = TestBed.inject(DataService); const api = TestBed.inject(ApiService); diff --git a/src/app/shared/services/data/data.service.ts b/src/app/shared/services/data/data.service.ts index 4a7ba8171..15c855512 100644 --- a/src/app/shared/services/data/data.service.ts +++ b/src/app/shared/services/data/data.service.ts @@ -1,6 +1,6 @@ import { Injectable, EventEmitter } from '@angular/core'; import { map } from 'rxjs/operators'; -import { remove, find, findIndex } from 'lodash'; +import { remove, find, findIndex, noop } from 'lodash'; import { ApiService } from '@shared/services/api/api.service'; import { @@ -23,6 +23,21 @@ import { TagsService } from '@core/services/tags/tags.service'; const THUMBNAIL_REFRESH_INTERVAL = 3000; +// Identifiers reach us as numbers from the PHP API and as strings from stela, and +// a single item can carry both over its lifetime: a record loaded via navigateLean +// has a numeric parentFolderId until update() overwrites it with stela's string. +// Compare them as strings so the source of the id does not change the answer. +// Null and undefined never match, including each other. +type ItemId = string | number | null | undefined; + +const isSameId = (a: ItemId, b: ItemId): boolean => { + if (a === null || a === undefined || b === null || b === undefined) { + return false; + } + + return String(a) === String(b); +}; + export type SelectedItemsSet = Set; export interface SelectKeyEvent { @@ -78,6 +93,14 @@ export class DataService { private unsharedItemSubject = new Subject(); + // Emits the item whose thumbnail URLs were just written by a lean fetch. + // Subscribers match on object identity: this service mutates the very same + // ItemVO instances its consumers render, so the emitted reference is the only + // unambiguous way to tell which consumer the update belongs to. Matching on + // folder_linkId would not be, since that id arrives as a number from stela + // and as a string from some PHP responses. + private thumbnailUpdatedSubject = new Subject(); + private eventSubject: Subject = new Subject(); public events: Observable = this.eventSubject.asObservable(); @@ -190,8 +213,22 @@ export class DataService { ): Promise { this.debug('fetchLeanItems %d items requested', items.length); - const itemResolves = []; - const itemRejects = []; + // Keyed by folder_linkId rather than by position: getWithChildren returns + // every child of the folder, not just the ones we asked about, so the + // response order tells us nothing about which resolver belongs to which + // item. Keys are stringified because folder_linkId reaches us as a number + // from stela and as a string from some PHP responses. The item reference is + // held here rather than looked up in byFolderLinkId on settle, so that an + // item whose row was destroyed (and therefore unregistered) mid-request + // still gets its isFetching flag cleared. + const itemResolvers = new Map< + string, + { + item: ItemVO; + resolve: (value: boolean) => void; + reject: () => void; + } + >(); let handleItemRegistration = false; if (currentFolder) { @@ -213,10 +250,18 @@ export class DataService { } item.isFetching = true; - item.fetched = new Promise((resolve, reject) => { - itemResolves.push(resolve); - itemRejects.push(reject); + item.fetched = new Promise((resolve, reject) => { + itemResolvers.set(String(item.folder_linkId), { + item, + resolve, + reject, + }); }); + // Not every caller awaits `fetched`, and the thumbnail refresh poll + // never does. Swallow the unawaited case so settling a missing item + // does not surface as an unhandled promise rejection. Callers that + // do chain onto `fetched` still see the rejection. + item.fetched.catch(noop); return true; }) .map((item) => ({ @@ -240,7 +285,7 @@ export class DataService { return fetchedFolder.ChildItemVOs; }) .then(async (leanItems) => { - leanItems.forEach((leanItem, index) => { + leanItems.forEach((leanItem) => { const item = this.byFolderLinkId[leanItem.folder_linkId]; if (item) { this.byArchiveNbr[leanItem.archiveNbr] = item; @@ -248,13 +293,24 @@ export class DataService { item.dataStatus = DataStatus.Lean; item.isFetching = false; - itemResolves[index](); + + const resolver = itemResolvers.get(String(leanItem.folder_linkId)); + if (resolver) { + itemResolvers.delete(String(leanItem.folder_linkId)); + resolver.resolve(true); + } item.fetched = null; + const thumbnailUrl = GetThumbnail(item); + + if (thumbnailUrl) { + this.thumbnailUpdatedSubject.next(item); + } + if ( !item.isFolder && - !GetThumbnail(item) && - item.parentFolderId === this.currentFolder.folderId + !thumbnailUrl && + isSameId(item.parentFolderId, this.currentFolder.folderId) ) { this.debug('thumbRefreshQueue push %s', item.archiveNbr); this.thumbRefreshQueue.push(item); @@ -262,6 +318,11 @@ export class DataService { } }); + // Anything we asked for but did not get back still has to be settled. + // A stuck isFetching flag excludes an item from every future + // fetchLeanItems call, so it would never refresh again. + this.settleUnresolvedItems(itemResolvers); + if (handleItemRegistration) { items.forEach((item) => { this.unregisterItem(item); @@ -272,16 +333,31 @@ export class DataService { return await Promise.resolve(leanItems.length); }) - .catch((response) => { - itemRejects.forEach((reject, index) => { - items[index].isFetching = false; - items[index].fetched = null; - reject(); - }); + .catch(() => { + this.settleUnresolvedItems(itemResolvers); return 0; }); } + private settleUnresolvedItems( + itemResolvers: Map< + string, + { + item: ItemVO; + resolve: (value: boolean) => void; + reject: () => void; + } + >, + ) { + for (const { item, reject } of itemResolvers.values()) { + item.isFetching = false; + item.fetched = null; + reject(); + } + + itemResolvers.clear(); + } + public async fetchFullItems(items: Array, withChildren?: boolean) { this.debug('fetchFullItems %d items requested', items.length); @@ -695,6 +771,10 @@ export class DataService { return this.unsharedItemSubject.asObservable(); } + public thumbnailUpdated$(): Observable { + return this.thumbnailUpdatedSubject.asObservable(); + } + public itemUnshared(item: ItemVO) { this.clearSelectedItems(); this.unsharedItemSubject.next(item);