From d9c5c956cb2dacf7f51ff4744b43d237d5353df0 Mon Sep 17 00:00:00 2001 From: aasandei-vsp Date: Mon, 10 Aug 2026 14:47:15 +0300 Subject: [PATCH 1/2] Replace navigateMin with getWithChildren NavigateMin was used mainly by the folder picker modal, so this is a more encapsulated change. But because we needed to map some new properties in the folder object, there are side effects that need extensive testing and care to make sure we do not do any regression. This would happen because properties that up until now would be ignored suddenly do have a value and we need to make sure it is the correct one. It is worth mentioning that we also had a positive side effect. With this change, we fixed a bug related to the back button on the folder picker, where instead of creating a folder from its ID, we actually load the correct folder with all its information. Issues: PER-10476, PER-10735 --- .../folder-picker.component.spec.ts | 301 +++++++++++++++++- .../folder-picker/folder-picker.component.ts | 81 +++-- .../shared/services/api/folder.repo.spec.ts | 66 ++++ src/app/shared/services/api/folder.repo.ts | 36 +-- 4 files changed, 434 insertions(+), 50 deletions(-) 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 976b35c17..9665d8302 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 @@ -12,8 +12,11 @@ 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'; -import { of } from 'rxjs'; -import { FolderPickerComponent } from './folder-picker.component'; +import { MessageService } from '@shared/services/message/message.service'; +import { + FolderPickerComponent, + FolderPickerOperations, +} from './folder-picker.component'; describe('FolderPickerComponent', () => { let component: FolderPickerComponent; @@ -48,16 +51,17 @@ describe('FolderPickerComponent', () => { it('should initialize a folder, strip out records, and load lean child folders', async () => { const api = TestBed.inject(ApiService) as ApiService; - const navigateMinExpected = require('@root/test/responses/folder.navigateMin.myFiles.success.json'); - const myFiles = new FolderResponse(navigateMinExpected).getFolderVO(); + const folderExpected = require('@root/test/responses/folder.navigateMin.myFiles.success.json'); + const myFiles = new FolderResponse(folderExpected).getFolderVO(); - spyOn(api.folder, 'navigate').and.returnValue( - of(new FolderResponse(navigateMinExpected)), - ); + const getWithChildrenSpy = spyOn( + api.folder, + 'getWithChildren', + ).and.resolveTo(new FolderResponse(folderExpected)); await component.setFolder(myFiles); - expect(api.folder.navigate).toHaveBeenCalledTimes(1); + expect(getWithChildrenSpy).toHaveBeenCalledTimes(1); expect(component.currentFolder).toBeTruthy(); expect(component.currentFolder.folder_linkId).toEqual( myFiles.folder_linkId, @@ -66,9 +70,7 @@ describe('FolderPickerComponent', () => { expect(some(component.currentFolder.ChildItemVOs, 'isRecord')).toBeFalsy(); const getLeanItemsExpected = require('@root/test/responses/folder.getLeanItems.folderPicker.myFiles.success.json'); - spyOn(api.folder, 'getWithChildren').and.returnValue( - Promise.resolve(new FolderResponse(getLeanItemsExpected)), - ); + getWithChildrenSpy.and.resolveTo(new FolderResponse(getLeanItemsExpected)); await component.loadCurrentFolderChildData(); @@ -128,4 +130,281 @@ describe('FolderPickerComponent', () => { expect(background.bgSrc).toBe('https://example.com/thumb.jpg'); }); + + it('should load the virtual root folder through getRoot, not getWithChildren', async () => { + const api = TestBed.inject(ApiService) as ApiService; + const rootExpected = require('@root/test/responses/folder.navigateMin.myFiles.success.json'); + + const getRootSpy = spyOn(api.folder, 'getRoot').and.resolveTo( + new FolderResponse(rootExpected), + ); + const getWithChildrenSpy = spyOn(api.folder, 'getWithChildren'); + + await component.setFolder( + new FolderVO({ type: 'type.folder.root.root', folderId: 1 }), + ); + + expect(getRootSpy).toHaveBeenCalledTimes(1); + expect(getWithChildrenSpy).not.toHaveBeenCalled(); + }); + + it('should strip app and vault folders out of the root listing', async () => { + const api = TestBed.inject(ApiService) as ApiService; + + spyOn(api.folder, 'getRoot').and.resolveTo( + new FolderResponse({ + isSuccessful: true, + Results: [ + { + data: [ + { + FolderVO: { + type: 'type.folder.root.root', + folderId: 1, + ChildItemVOs: [ + { folderId: 2, type: 'type.folder.root.private' }, + { folderId: 3, type: 'type.folder.root.app' }, + { folderId: 4, type: 'type.folder.root.vault' }, + ], + }, + }, + ], + }, + ], + }), + ); + + await component.setFolder( + new FolderVO({ type: 'type.folder.root.root', folderId: 1 }), + ); + + expect(component.currentFolder.ChildItemVOs.length).toBe(1); + expect(component.isRootFolder).toBeTrue(); + }); + + it('should keep the requested folder_linkId when the response omits it', async () => { + const api = TestBed.inject(ApiService) as ApiService; + + // A Stela-shaped response with no folder_linkId or archiveNbr, which is + // what Copy and Move need for the destination. + spyOn(api.folder, 'getWithChildren').and.resolveTo( + new FolderResponse({ + isSuccessful: true, + Results: [ + { + data: [ + { + FolderVO: { + type: 'type.folder.private', + folderId: '200', + ChildItemVOs: [], + }, + }, + ], + }, + ], + }), + ); + + await component.setFolder( + new FolderVO({ + type: 'type.folder.private', + folderId: '200', + folder_linkId: 158329, + archiveNbr: '0001-0002', + }), + ); + + expect(component.currentFolder.folder_linkId).toBe(158329); + expect(component.currentFolder.archiveNbr).toBe('0001-0002'); + }); + + it('should not override a folder_linkId the response does provide', async () => { + const api = TestBed.inject(ApiService) as ApiService; + + spyOn(api.folder, 'getWithChildren').and.resolveTo( + new FolderResponse({ + isSuccessful: true, + Results: [ + { + data: [ + { + FolderVO: { + type: 'type.folder.private', + folderId: '200', + folder_linkId: 999, + ChildItemVOs: [], + }, + }, + ], + }, + ], + }), + ); + + await component.setFolder( + new FolderVO({ + type: 'type.folder.private', + folderId: '200', + folder_linkId: 158329, + }), + ); + + expect(component.currentFolder.folder_linkId).toBe(999); + }); + + it('should replay the folder it came from when going back', async () => { + const api = TestBed.inject(ApiService) as ApiService; + const rootExpected = require('@root/test/responses/folder.getRoot.success.json'); + const folderExpected = require('@root/test/responses/folder.navigateMin.myFiles.success.json'); + + const getRootSpy = spyOn(api.folder, 'getRoot').and.resolveTo( + new FolderResponse(rootExpected), + ); + spyOn(api.folder, 'getWithChildren').and.resolveTo( + new FolderResponse(folderExpected), + ); + + // Start at the root, then navigate into My Files. + await component.setFolder( + new FolderVO({ type: 'type.folder.root.root', folderId: 140682 }), + ); + await component.navigate( + new FolderVO({ type: 'type.folder.root.private', folderId: '140683' }), + ); + getRootSpy.calls.reset(); + + await component.goToParentFolder(); + + // One Back returns to the root, without the intermediate "Archive Root" + // folder that Stela would have served from the parent ids. + expect(getRootSpy).toHaveBeenCalledTimes(1); + expect(component.isRootFolder).toBeTrue(); + }); + + it('should load child data when going back, so thumbnails come back too', async () => { + const api = TestBed.inject(ApiService) as ApiService; + const dataService = TestBed.inject(DataService) as DataService; + const folderExpected = require('@root/test/responses/folder.navigateMin.myFiles.success.json'); + + spyOn(api.folder, 'getWithChildren').and.resolveTo( + new FolderResponse(folderExpected), + ); + const fetchLeanItems = spyOn(dataService, 'fetchLeanItems').and.resolveTo( + 0, + ); + + component.allowRecords = true; + await component.setFolder( + new FolderVO({ type: 'type.folder.private', folderId: '100' }), + ); + await component.navigate( + new FolderVO({ type: 'type.folder.private', folderId: '200' }), + ); + fetchLeanItems.calls.reset(); + + await component.goToParentFolder(); + + expect(fetchLeanItems).toHaveBeenCalledTimes(1); + }); + + it('should go to the root when there is nothing left to go back to', async () => { + const api = TestBed.inject(ApiService) as ApiService; + const rootExpected = require('@root/test/responses/folder.getRoot.success.json'); + const folderExpected = require('@root/test/responses/folder.navigateMin.myFiles.success.json'); + + const getRootSpy = spyOn(api.folder, 'getRoot').and.resolveTo( + new FolderResponse(rootExpected), + ); + spyOn(api.folder, 'getWithChildren').and.resolveTo( + new FolderResponse(folderExpected), + ); + + // The record choosers open directly on My Files, so Back is available + // with no visited folder to return to. + await component.setFolder( + new FolderVO({ type: 'type.folder.root.private', folderId: '140683' }), + ); + + await component.goToParentFolder(); + + expect(getRootSpy).toHaveBeenCalledTimes(1); + }); + + it('should forget its history when reopened', async () => { + const api = TestBed.inject(ApiService) as ApiService; + const rootExpected = require('@root/test/responses/folder.getRoot.success.json'); + const folderExpected = require('@root/test/responses/folder.navigateMin.myFiles.success.json'); + + const getRootSpy = spyOn(api.folder, 'getRoot').and.resolveTo( + new FolderResponse(rootExpected), + ); + spyOn(api.folder, 'getWithChildren').and.resolveTo( + new FolderResponse(folderExpected), + ); + + await component.setFolder( + new FolderVO({ type: 'type.folder.private', folderId: '100' }), + ); + await component.navigate( + new FolderVO({ type: 'type.folder.private', folderId: '200' }), + ); + + // Reopening must not inherit the previous session's trail. + void component.show( + new FolderVO({ type: 'type.folder.root.private', folderId: '140683' }), + FolderPickerOperations.ChooseRecord, + ); + await component.goToParentFolder(); + + expect(getRootSpy).toHaveBeenCalled(); + }); + + it('should filter out folders listed in filterFolderLinkIds', async () => { + const api = TestBed.inject(ApiService) as ApiService; + const folderExpected = cloneDeep( + require('@root/test/responses/folder.navigateMin.myFiles.success.json'), + ); + const myFiles = new FolderResponse(folderExpected).getFolderVO(true); + const excludedFolder = (myFiles.ChildItemVOs as FolderVO[]).find( + (item) => item.isFolder, + ); + + spyOn(api.folder, 'getWithChildren').and.resolveTo( + new FolderResponse(folderExpected), + ); + + component.filterFolderLinkIds = [excludedFolder.folder_linkId]; + await component.setFolder(myFiles); + + expect( + some( + component.currentFolder.ChildItemVOs, + (item) => item.folder_linkId === excludedFolder.folder_linkId, + ), + ).toBeFalse(); + }); + + it('should show an error and stop waiting when the folder fails to load', async () => { + const api = TestBed.inject(ApiService) as ApiService; + const message = TestBed.inject(MessageService) as MessageService; + const showError = spyOn(message, 'showError'); + + spyOn(api.folder, 'getWithChildren').and.rejectWith( + new Error('network down'), + ); + + await expectAsync( + component.setFolder( + new FolderVO({ type: 'type.folder.private', folderId: 1 }), + ), + ).toBeResolved(); + + expect(showError).toHaveBeenCalledOnceWith({ + message: 'error.generic.internal', + translate: true, + }); + + expect(component.waiting).toBeFalse(); + }); }); 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 a2469c16e..738cb4655 100644 --- a/src/app/core/components/folder-picker/folder-picker.component.ts +++ b/src/app/core/components/folder-picker/folder-picker.component.ts @@ -39,6 +39,11 @@ export class FolderPickerComponent implements OnDestroy { public filterFolderLinkIds: number[]; + // The folders navigated through to reach the current one, most recent last. + // Back replays these rather than rebuilding a parent from ids, so setFolder + // always receives a complete FolderVO. + private visitedFolders: FolderVO[] = []; + private cancelResetTimeout: ReturnType; constructor( @@ -82,9 +87,8 @@ export class FolderPickerComponent implements OnDestroy { break; } - this.setFolder(startingFolder).then(() => { - this.loadCurrentFolderChildData(); - }); + this.visitedFolders = []; + void this.setFolderAndLoadChildData(startingFolder); const { promise, resolve } = Promise.withResolvers(); this.chooseFolderPromise = promise; @@ -106,8 +110,10 @@ export class FolderPickerComponent implements OnDestroy { } async navigate(folder: FolderVO) { - await this.setFolder(folder); - this.loadCurrentFolderChildData(); + if (this.currentFolder) { + this.visitedFolders.push(this.currentFolder); + } + await this.setFolderAndLoadChildData(folder); } showRecord(record: RecordVO) { @@ -121,16 +127,25 @@ export class FolderPickerComponent implements OnDestroy { async setFolder(folder: FolderVO) { this.waiting = true; try { - const folderResponse = await this.api.folder - .navigate( - new FolderVO({ - folder_linkId: folder.folder_linkId, - folderId: folder.folderId, - archiveNbr: folder.archiveNbr, - }), - ) - .toPromise(); + // The root keeps loading through getRoot -- see isRootRootFolder. + const folderResponse = this.isRootRootFolder(folder) + ? await this.api.folder.getRoot() + : await this.api.folder.getWithChildren([ + new FolderVO({ + folder_linkId: folder.folder_linkId, + folderId: folder.folderId, + archiveNbr: folder.archiveNbr, + }), + ]); this.currentFolder = folderResponse.getFolderVO(true); + + // Copy and Move send only the destination's folder_linkId to the + // legacy endpoints, and Stela's folder response does not reliably + // carry it. We asked for this folder by id, so keep the ids we had + // rather than trusting the response to echo them back. + this.currentFolder.folder_linkId ??= folder.folder_linkId; + this.currentFolder.archiveNbr ??= folder.archiveNbr; + this.isRootFolder = this.currentFolder.type.includes( 'type.folder.root.root', ); @@ -152,7 +167,14 @@ export class FolderPickerComponent implements OnDestroy { if (err instanceof FolderResponse) { this.message.showError({ message: err.getMessage(), translate: true }); } else { - throw err; + // getWithChildren rejects with the raw HTTP error rather than a + // FolderResponse, so there is no server message to surface. Fall + // back to the generic one instead of rethrowing into an unhandled + // rejection. + this.message.showError({ + message: 'error.generic.internal', + translate: true, + }); } } finally { this.waiting = false; @@ -168,11 +190,17 @@ export class FolderPickerComponent implements OnDestroy { } async goToParentFolder() { - const parentFolder = new FolderVO({ - folder_linkId: this.currentFolder.parentFolder_linkId, - folderId: this.currentFolder.parentFolderId, - }); - return await this.setFolder(parentFolder); + // Replay the folder we came from, so this behaves exactly like navigating + // forward: a complete FolderVO goes to setFolder, and the child data is + // loaded afterwards so thumbnails come back too. + const previousFolder = this.visitedFolders.pop(); + + // Nothing to pop when the picker was opened directly on a workspace + // folder, as the record choosers do with My Files. Going up from there + // means the archive root. + return await this.setFolderAndLoadChildData( + previousFolder ?? new FolderVO({ type: 'type.folder.root.root' }), + ); } async loadCurrentFolderChildData() { @@ -182,6 +210,11 @@ export class FolderPickerComponent implements OnDestroy { ); } + private async setFolderAndLoadChildData(folder: FolderVO) { + await this.setFolder(folder); + this.loadCurrentFolderChildData(); + } + chooseFolder() { if (this.shouldConfirmFolderSelection()) { this.prompt @@ -209,6 +242,7 @@ export class FolderPickerComponent implements OnDestroy { this.chooseFolderPromise = null; this.chooseFolderResolve = null; this.isRootFolder = true; + this.visitedFolders = []; this.cancelResetTimeout = null; }, 500); } @@ -246,4 +280,11 @@ export class FolderPickerComponent implements OnDestroy { protected shouldConfirmFolderSelection(): boolean { return this.currentFolder.type.endsWith('public'); } + + // Stela serves the archive root as an ordinary folder -- it lists Apps and + // reports a type that does not read as root -- so the root keeps loading + // through the legacy getRoot endpoint. + private isRootRootFolder(folder: FolderVO): boolean { + return !!folder.type?.includes('type.folder.root.root'); + } } diff --git a/src/app/shared/services/api/folder.repo.spec.ts b/src/app/shared/services/api/folder.repo.spec.ts index 436163dc4..eebe6f41a 100644 --- a/src/app/shared/services/api/folder.repo.spec.ts +++ b/src/app/shared/services/api/folder.repo.spec.ts @@ -395,4 +395,70 @@ describe('Folder repo', () => { }); }); }); + + describe('Stela folder conversion', () => { + const convertFolder = async (overrides: Record) => { + httpV2Spy.get.and.returnValue( + of([{ items: [{ ...mockStelaFolder, ...overrides }] }]), + ); + const result = await folderRepo.getStelaFolderVOs([ + new FolderVO({ folderId: 123 }), + ]); + return result.getFolderVOs()[0]; + }; + + it('should map archiveNumber to archiveNbr', async () => { + const folder = await convertFolder({ archiveNumber: '0001-0002' }); + + expect(folder.archiveNbr).toBe('0001-0002'); + }); + + it('should map folderLinkId to a numeric folder_linkId', async () => { + const folder = await convertFolder({ folderLinkId: '158329' }); + + expect(folder.folder_linkId).toBe(158329); + }); + + it('should accept link ids that already arrive as numbers', async () => { + const folder = await convertFolder({ folderLinkId: 158329 }); + + expect(folder.folder_linkId).toBe(158329); + }); + + // The folder picker renders child thumbnails straight from this response, + // with no follow-up lean fetch, so the mapping has to survive the + // folder -> children conversion. + it('should carry child record thumbnails through getWithChildren', async () => { + httpV2Spy.get.and.returnValues( + of([{ items: [mockStelaFolder] }]), + of([ + { + items: [ + { + recordId: '77', + displayName: 'A photo', + folderLinkId: '900', + archiveNumber: '0001-0003', + thumbnailUrls: { '200': 'thumb200', '256': 'thumb256' }, + }, + ], + }, + ]), + ); + + const result = await folderRepo.getWithChildren([ + new FolderVO({ folderId: 123 }), + ]); + const child = result.getFolderVO(true).ChildItemVOs[0]; + + expect(child.thumbURL200).toBe('thumb200'); + expect(child.thumbnail256).toBe('thumb256'); + }); + + it('should leave link ids undefined rather than NaN when absent', async () => { + const folder = await convertFolder({ folderLinkId: undefined }); + + expect(folder.folder_linkId).toBeUndefined(); + }); + }); }); diff --git a/src/app/shared/services/api/folder.repo.ts b/src/app/shared/services/api/folder.repo.ts index 188d18012..54979a3fb 100644 --- a/src/app/shared/services/api/folder.repo.ts +++ b/src/app/shared/services/api/folder.repo.ts @@ -55,6 +55,8 @@ interface StelaFolder { id: string; name: string; }; + archiveNumber: string; + folderLinkId: string; createdAt: string; updatedAt: string; description: string; @@ -91,6 +93,16 @@ type StelaFolderChild = StelaFolder | StelaRecord; const isStelaRecord = (child: StelaFolderChild): child is StelaRecord => child && 'recordId' in child; +// Returns undefined rather than NaN for a missing id, so callers can tell +// "not provided" apart from a real link id. +const toFolderLinkId = (folderLinkId: string): number | undefined => { + if (folderLinkId === null || folderLinkId === undefined) { + return undefined; + } + const parsed = Number(folderLinkId); + return Number.isNaN(parsed) ? undefined : parsed; +}; + const convertStelaFolderToFolderVO = (stelaFolder: StelaFolder): FolderVO => { stelaFolder.children ??= []; const childFolderVOs = stelaFolder.children @@ -103,6 +115,11 @@ const convertStelaFolderToFolderVO = (stelaFolder: StelaFolder): FolderVO => { ...stelaFolder, folderId: stelaFolder.folderId, archiveId: stelaFolder.archive?.id, + archiveNbr: stelaFolder.archiveNumber, + // Stela returns link ids as strings, the same way it does for records. + // The FolderVO field is a number and is compared with strict equality + // (e.g. the folder picker's filterFolderLinkIds), so coerce it here. + folder_linkId: toFolderLinkId(stelaFolder.folderLinkId), displayName: stelaFolder.displayName, displayDT: stelaFolder.displayTimestamp, displayEndDT: stelaFolder.displayEndTimestamp, @@ -356,25 +373,6 @@ export class FolderRepo extends BaseRepo { return folderResponse; } - public navigate(folderVO: FolderVO): Observable { - const response = { - ...folderVO, - }; - if (folderVO.type === 'type.folder.root.private') { - response.displayName = 'Private'; - } - - const data = [ - { - FolderVO: new FolderVO(response), - }, - ]; - - return this.http.sendRequest('/folder/navigateMin', data, { - ResponseClass: FolderResponse, - }); - } - public navigateLean(folderVO: FolderVO): Observable { const data = [ { From 25f05efdf7820d11cf844942df5dd86953550465 Mon Sep 17 00:00:00 2001 From: aasandei-vsp Date: Fri, 14 Aug 2026 15:55:29 +0300 Subject: [PATCH 2/2] Keep folder picker history and link ids consistent on failure The picker now navigates only when the correct folder loads. toFolderLinkId now accepts and rejects values more accurate, without failing uncontrolled. Issue: PER-10476 --- .../folder-picker.component.spec.ts | 118 ++++++++++++++++++ .../folder-picker/folder-picker.component.ts | 43 +++++-- .../shared/services/api/folder.repo.spec.ts | 12 ++ src/app/shared/services/api/folder.repo.ts | 26 ++-- 4 files changed, 181 insertions(+), 18 deletions(-) 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 9665d8302..9b939701f 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 @@ -385,6 +385,124 @@ describe('FolderPickerComponent', () => { ).toBeFalse(); }); + // The trail is built from loaded folders, so the tests below need each + // response to echo back the folder that was asked for. + const folderResponseFor = (folderId: string) => + new FolderResponse({ + isSuccessful: true, + Results: [ + { + data: [ + { + FolderVO: { + type: 'type.folder.private', + folderId, + ChildItemVOs: [], + }, + }, + ], + }, + ], + }); + + it('should not record history for a navigation that failed', async () => { + const api = TestBed.inject(ApiService) as ApiService; + const rootExpected = require('@root/test/responses/folder.getRoot.success.json'); + + const getRootSpy = spyOn(api.folder, 'getRoot').and.resolveTo( + new FolderResponse(rootExpected), + ); + const getWithChildrenSpy = spyOn( + api.folder, + 'getWithChildren', + ).and.callFake(async (folderVOs: FolderVO[]) => + folderResponseFor(String(folderVOs[0].folderId)), + ); + spyOn(TestBed.inject(MessageService), 'showError'); + spyOn(TestBed.inject(DataService), 'fetchLeanItems').and.resolveTo(0); + + await component.setFolder( + new FolderVO({ type: 'type.folder.private', folderId: '100' }), + ); + + getWithChildrenSpy.and.rejectWith(new Error('network down')); + await component.navigate( + new FolderVO({ type: 'type.folder.private', folderId: '200' }), + ); + + getWithChildrenSpy.calls.reset(); + + await component.goToParentFolder(); + + // Back goes up from where we still are rather than replaying the folder + // we never left. + expect(getWithChildrenSpy).not.toHaveBeenCalled(); + expect(getRootSpy).toHaveBeenCalledTimes(1); + }); + + it('should keep its history when going back fails', async () => { + const api = TestBed.inject(ApiService) as ApiService; + + const getWithChildrenSpy = spyOn( + api.folder, + 'getWithChildren', + ).and.callFake(async (folderVOs: FolderVO[]) => + folderResponseFor(String(folderVOs[0].folderId)), + ); + spyOn(TestBed.inject(MessageService), 'showError'); + spyOn(TestBed.inject(DataService), 'fetchLeanItems').and.resolveTo(0); + + await component.setFolder( + new FolderVO({ type: 'type.folder.private', folderId: '100' }), + ); + await component.navigate( + new FolderVO({ type: 'type.folder.private', folderId: '200' }), + ); + await component.navigate( + new FolderVO({ type: 'type.folder.private', folderId: '300' }), + ); + + getWithChildrenSpy.and.rejectWith(new Error('network down')); + await component.goToParentFolder(); + + getWithChildrenSpy.and.callFake(async (folderVOs: FolderVO[]) => + folderResponseFor(String(folderVOs[0].folderId)), + ); + getWithChildrenSpy.calls.reset(); + + // A retry still has the same folder to go back to, and the one below it + // is still there after that. + await component.goToParentFolder(); + + expect(component.currentFolder.folderId).toBe('200'); + + await component.goToParentFolder(); + + expect(component.currentFolder.folderId).toBe('100'); + }); + + it('should not load child data when the folder fails to load', async () => { + const api = TestBed.inject(ApiService) as ApiService; + const dataService = TestBed.inject(DataService) as DataService; + + spyOn(api.folder, 'getWithChildren').and.rejectWith( + new Error('network down'), + ); + const fetchLeanItems = spyOn(dataService, 'fetchLeanItems').and.resolveTo( + 0, + ); + spyOn(TestBed.inject(MessageService), 'showError'); + + await expectAsync( + component.navigate( + new FolderVO({ type: 'type.folder.private', folderId: '200' }), + ), + ).toBeResolved(); + + expect(fetchLeanItems).not.toHaveBeenCalled(); + expect(component.currentFolder).toBeFalsy(); + }); + it('should show an error and stop waiting when the folder fails to load', async () => { const api = TestBed.inject(ApiService) as ApiService; const message = TestBed.inject(MessageService) as MessageService; 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 738cb4655..22166d302 100644 --- a/src/app/core/components/folder-picker/folder-picker.component.ts +++ b/src/app/core/components/folder-picker/folder-picker.component.ts @@ -110,10 +110,14 @@ export class FolderPickerComponent implements OnDestroy { } async navigate(folder: FolderVO) { - if (this.currentFolder) { - this.visitedFolders.push(this.currentFolder); + const folderNavigatedFrom = this.currentFolder; + const didLoadFolder = await this.setFolderAndLoadChildData(folder); + + // Only record where we came from once the new folder actually loaded, + // otherwise a failed navigation costs an extra Back press to undo. + if (didLoadFolder && folderNavigatedFrom) { + this.visitedFolders.push(folderNavigatedFrom); } - await this.setFolderAndLoadChildData(folder); } showRecord(record: RecordVO) { @@ -124,7 +128,9 @@ export class FolderPickerComponent implements OnDestroy { return GetThumbnail(item); } - async setFolder(folder: FolderVO) { + // Resolves to whether the folder loaded, so callers can leave the navigation + // history alone when it did not. + async setFolder(folder: FolderVO): Promise { this.waiting = true; try { // The root keeps loading through getRoot -- see isRootRootFolder. @@ -163,6 +169,7 @@ export class FolderPickerComponent implements OnDestroy { remove(this.currentFolder.ChildItemVOs, (item) => item.type.includes('type.folder.root.vault'), ); + return true; } catch (err) { if (err instanceof FolderResponse) { this.message.showError({ message: err.getMessage(), translate: true }); @@ -176,6 +183,7 @@ export class FolderPickerComponent implements OnDestroy { translate: true, }); } + return false; } finally { this.waiting = false; } @@ -192,15 +200,20 @@ export class FolderPickerComponent implements OnDestroy { async goToParentFolder() { // Replay the folder we came from, so this behaves exactly like navigating // forward: a complete FolderVO goes to setFolder, and the child data is - // loaded afterwards so thumbnails come back too. - const previousFolder = this.visitedFolders.pop(); + // loaded afterwards so thumbnails come back too. Peek rather than pop, so + // a failed load leaves the history where it was and Back still works. + const previousFolder = this.visitedFolders[this.visitedFolders.length - 1]; - // Nothing to pop when the picker was opened directly on a workspace + // Nothing to go back to when the picker was opened directly on a workspace // folder, as the record choosers do with My Files. Going up from there // means the archive root. - return await this.setFolderAndLoadChildData( + const didLoadFolder = await this.setFolderAndLoadChildData( previousFolder ?? new FolderVO({ type: 'type.folder.root.root' }), ); + + if (didLoadFolder && previousFolder) { + this.visitedFolders.pop(); + } } async loadCurrentFolderChildData() { @@ -210,9 +223,17 @@ export class FolderPickerComponent implements OnDestroy { ); } - private async setFolderAndLoadChildData(folder: FolderVO) { - await this.setFolder(folder); - this.loadCurrentFolderChildData(); + private async setFolderAndLoadChildData(folder: FolderVO): Promise { + const didLoadFolder = await this.setFolder(folder); + + // setFolder no longer rethrows, so without this guard a failed load would + // read child items off a currentFolder that is still unset (or, worse, + // still the folder we were leaving). + if (didLoadFolder) { + void this.loadCurrentFolderChildData(); + } + + return didLoadFolder; } chooseFolder() { diff --git a/src/app/shared/services/api/folder.repo.spec.ts b/src/app/shared/services/api/folder.repo.spec.ts index eebe6f41a..1e8033331 100644 --- a/src/app/shared/services/api/folder.repo.spec.ts +++ b/src/app/shared/services/api/folder.repo.spec.ts @@ -460,5 +460,17 @@ describe('Folder repo', () => { expect(folder.folder_linkId).toBeUndefined(); }); + + it('should leave link ids undefined rather than 0 when blank', async () => { + const folder = await convertFolder({ folderLinkId: ' ' }); + + expect(folder.folder_linkId).toBeUndefined(); + }); + + it('should leave link ids undefined when they are not numeric', async () => { + const folder = await convertFolder({ folderLinkId: 'not-a-number' }); + + expect(folder.folder_linkId).toBeUndefined(); + }); }); }); diff --git a/src/app/shared/services/api/folder.repo.ts b/src/app/shared/services/api/folder.repo.ts index 54979a3fb..ef687f720 100644 --- a/src/app/shared/services/api/folder.repo.ts +++ b/src/app/shared/services/api/folder.repo.ts @@ -55,8 +55,10 @@ interface StelaFolder { id: string; name: string; }; - archiveNumber: string; - folderLinkId: string; + archiveNumber?: string; + // Stela sends link ids as strings, but not every folder payload carries one, + // and some endpoints already send them as numbers. + folderLinkId?: string | number; createdAt: string; updatedAt: string; description: string; @@ -94,13 +96,23 @@ const isStelaRecord = (child: StelaFolderChild): child is StelaRecord => child && 'recordId' in child; // Returns undefined rather than NaN for a missing id, so callers can tell -// "not provided" apart from a real link id. -const toFolderLinkId = (folderLinkId: string): number | undefined => { - if (folderLinkId === null || folderLinkId === undefined) { +// "not provided" apart from a real link id. Accepts numbers as well as strings +// because different Stela endpoints disagree on which one they send. +const toFolderLinkId = ( + folderLinkId: string | number | null | undefined, +): number | undefined => { + if (typeof folderLinkId === 'number') { + return Number.isFinite(folderLinkId) ? folderLinkId : undefined; + } + if ( + folderLinkId === null || + folderLinkId === undefined || + folderLinkId.trim() === '' + ) { return undefined; } - const parsed = Number(folderLinkId); - return Number.isNaN(parsed) ? undefined : parsed; + const parsedFolderLinkId = Number(folderLinkId); + return Number.isFinite(parsedFolderLinkId) ? parsedFolderLinkId : undefined; }; const convertStelaFolderToFolderVO = (stelaFolder: StelaFolder): FolderVO => {