From 197c80ea6865865f06f3a816fc7cd689e72160cd Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Thu, 10 Sep 2026 23:37:49 -0700 Subject: [PATCH] Fix: ui version content switching inconsistencies (#14066) --- .../document-detail.component.spec.ts | 216 +++++++++++++++++- .../document-detail.component.ts | 95 ++++++-- src-ui/src/app/data/document.ts | 1 + 3 files changed, 289 insertions(+), 23 deletions(-) diff --git a/src-ui/src/app/components/document-detail/document-detail.component.spec.ts b/src-ui/src/app/components/document-detail/document-detail.component.spec.ts index 58322cfd5..9b285b1a0 100644 --- a/src-ui/src/app/components/document-detail/document-detail.component.spec.ts +++ b/src-ui/src/app/components/document-detail/document-detail.component.spec.ts @@ -28,8 +28,9 @@ import { Subject, of, throwError } from 'rxjs' import { routes } from 'src/app/app-routing.module' import { Correspondent } from 'src/app/data/correspondent' import { CustomFieldDataType } from 'src/app/data/custom-field' +import { CustomFieldInstance } from 'src/app/data/custom-field-instance' import { DataType } from 'src/app/data/datatype' -import { Document } from 'src/app/data/document' +import { Document, DocumentVersionInfo } from 'src/app/data/document' import { DocumentType } from 'src/app/data/document-type' import { FILTER_CORRESPONDENT, @@ -100,13 +101,18 @@ const doc: Document = { custom_fields: [ { field: 0, - document: 3, - created: new Date(), value: 'custom foo bar', }, - ], + ] as CustomFieldInstance[], } +// Newest first, as the API returns them: 12 is the latest, 3 is the root +const docVersions: DocumentVersionInfo[] = [ + { id: 12, is_root: false }, + { id: 10, is_root: false }, + { id: doc.id, is_root: true }, +] + const customFields = [ { id: 0, @@ -2045,6 +2051,208 @@ describe('DocumentDetailComponent', () => { expect(saveSpy).toHaveBeenCalled() }) + it('selectVersion should use the version content as the baseline and ignore stale responses', () => { + initNormally() + const version10Content = new Subject() + jest + .spyOn(documentService, 'get') + .mockReturnValueOnce(version10Content) + .mockReturnValueOnce(of({ content: 'version 12 content' } as Document)) + const version10Metadata = new Subject() + jest + .spyOn(documentService, 'getMetadata') + .mockReturnValueOnce(version10Metadata) + .mockReturnValueOnce(of({ lang: 'de' })) + + component.selectVersion(10) + component.selectVersion(12) + version10Content.next({ content: 'version 10 content' } as Document) + version10Metadata.next({ lang: 'en' }) + + expect(component.documentForm.get('content').value).toEqual( + 'version 12 content' + ) + expect(component.store.value.content).toEqual('version 12 content') + expect(component.metadata().lang).toEqual('de') + expect( + httpTestingController.expectOne(component.previewUrl()).cancelled + ).toBeFalsy() + expect( + httpTestingController.match((req) => req.url.includes('version=10'))[0] + ?.cancelled + ).toBeTruthy() + }) + + it('should confirm before discarding unsaved content edits when switching versions', () => { + initNormally() + component.document().versions = docVersions + jest + .spyOn(documentService, 'get') + .mockImplementation((id, versionID) => + of({ content: `version ${versionID} content` } as Document) + ) + let openModal: NgbModalRef + modalService.activeInstances.subscribe((modals) => (openModal = modals[0])) + const modalSpy = jest.spyOn(modalService, 'open') + + // shared fields carry over between versions, so no confirmation + component.documentForm.get('title').setValue('Edited title') + component.documentForm.get('title').markAsDirty() + component.documentForm.get('content').markAsDirty() + component.onVersionSelected(12) + expect(modalSpy).not.toHaveBeenCalled() + expect(component.selectedVersionId()).toEqual(12) + + component.documentForm.get('content').setValue('edited content') + component.documentForm.get('content').markAsDirty() + component.onVersionSelected(12) // already selected, nothing to do + expect(modalSpy).not.toHaveBeenCalled() + component.onVersionSelected(10) + expect(modalSpy).toHaveBeenCalledWith( + ConfirmDialogComponent, + expect.anything() + ) + openModal.componentInstance.cancel() + expect(component.selectedVersionId()).toEqual(12) + expect(component.documentForm.get('content').value).toEqual( + 'edited content' + ) + + component.onVersionSelected(10) + openModal.componentInstance.confirmClicked.emit() + expect(component.selectedVersionId()).toEqual(10) + expect(component.documentForm.get('content').value).toEqual( + 'version 10 content' + ) + expect(component.documentForm.get('content').dirty).toBeFalsy() + expect(component.documentForm.get('title').value).toEqual('Edited title') + }) + + it('should save unsaved content edits to the current version before switching, and stay if that fails', () => { + initNormally() + component.document().versions = docVersions + component.selectedVersionId.set(12) + jest + .spyOn(documentService, 'get') + .mockReturnValue(of({ content: 'version 10 content' } as Document)) + const savedDoc = new Subject() + const patchSpy = jest + .spyOn(documentService, 'patch') + .mockReturnValueOnce(throwError(() => new Error('failed to save'))) + .mockReturnValueOnce(savedDoc) + const modalSpy = jest.spyOn(modalService, 'open') + component.documentForm.get('content').setValue('edited content') + component.documentForm.get('content').markAsDirty() + + component.onVersionSelected(10) + let modal: NgbModalRef = modalSpy.mock.results[0].value + const closeSpy = jest.spyOn(modal, 'close') + modal.componentInstance.alternativeClicked.emit() + expect(closeSpy).toHaveBeenCalled() + expect(component.selectedVersionId()).toEqual(12) + expect(component.documentForm.get('content').value).toEqual( + 'edited content' + ) + + component.onVersionSelected(10) + modal = modalSpy.mock.results[1].value + modal.componentInstance.alternativeClicked.emit() + expect(patchSpy).toHaveBeenLastCalledWith( + expect.objectContaining({ content: 'edited content' }), + 12 + ) + component.onVersionSelected(doc.id) // ignored while saving + expect(modalSpy).toHaveBeenCalledTimes(2) + savedDoc.next(doc) + expect(component.selectedVersionId()).toEqual(10) + expect(component.documentForm.get('content').value).toEqual( + 'version 10 content' + ) + }) + + it('should switch without confirmation when the selected version was deleted, even while saving', () => { + initNormally() + component.document().versions = docVersions + component.selectedVersionId.set(10) + jest + .spyOn(documentService, 'get') + .mockReturnValue(of({ content: 'version 12 content' } as Document)) + const modalSpy = jest.spyOn(modalService, 'open') + component.documentForm.get('content').setValue('edited content') + component.documentForm.get('content').markAsDirty() + component.networkActive.set(true) + + // the version dropdown emits this after deleting the selected version + component.onVersionsUpdated(docVersions.filter((v) => v.id !== 10)) + component.onVersionSelected(12) + + expect(modalSpy).not.toHaveBeenCalled() + expect(component.selectedVersionId()).toEqual(12) + expect(component.documentForm.get('content').value).toEqual( + 'version 12 content' + ) + }) + + it('should restore the selected version and its unsaved content when returning to a document', () => { + initNormally() + const openDoc = component.document() + openDoc.versions = docVersions + jest.spyOn(openDocumentsService, 'getOpenDocument').mockReturnValue(openDoc) + jest + .spyOn(documentService, 'get') + .mockImplementation((id, versionID) => + of( + (versionID + ? { content: `version ${versionID} content` } + : { ...doc, versions: docVersions }) as Document + ) + ) + component.selectVersion(10) + // an edit that happens to match the latest version's content + component.documentForm.get('content').setValue(doc.content) + openDoc.__changedFields = ['content'] + + component['loadDocument'](doc.id) + + expect(component.selectedVersionId()).toEqual(10) + expect(component.documentForm.get('content').value).toEqual(doc.content) + expect(openDocumentsService.isDirty(openDoc)).toBeTruthy() + const patchSpy = jest + .spyOn(documentService, 'patch') + .mockReturnValue(of(doc)) + component.save() + expect(patchSpy).toHaveBeenCalledWith( + expect.objectContaining({ content: doc.content }), + 10 + ) + }) + + it('should fall back to the latest version when the remembered version no longer exists', () => { + initNormally() + const openDoc = component.document() + openDoc.versions = docVersions + jest.spyOn(openDocumentsService, 'getOpenDocument').mockReturnValue(openDoc) + jest.spyOn(documentService, 'get').mockImplementation((id, versionID) => + of( + (versionID + ? { content: `version ${versionID} content` } + : { + ...doc, + content: 'version 12 content', + versions: docVersions.filter((v) => v.id !== 10), + }) as Document + ) + ) + component.selectVersion(10) + + component['loadDocument'](doc.id) + + expect(component.selectedVersionId()).toEqual(12) + expect(component.documentForm.get('content').value).toEqual( + 'version 12 content' + ) + }) + it('createDisabled should return true if the user does not have permission to add the specified data type', () => { currentUserCan = false expect(component.createDisabled(DataType.Correspondent)).toBeTruthy() diff --git a/src-ui/src/app/components/document-detail/document-detail.component.ts b/src-ui/src/app/components/document-detail/document-detail.component.ts index f1f3188d0..b9ec0bb76 100644 --- a/src-ui/src/app/components/document-detail/document-detail.component.ts +++ b/src-ui/src/app/components/document-detail/document-detail.component.ts @@ -98,8 +98,8 @@ import { ISODateAdapter } from 'src/app/utils/ngb-iso-date-adapter' import * as UTIF from 'utif' import { DocumentDetailFieldID } from '../admin/settings/settings.component' import { ConfirmDialogComponent } from '../common/confirm-dialog/confirm-dialog.component' -import { ReprocessConfirmDialogComponent } from '../common/confirm-dialog/reprocess-confirm-dialog/reprocess-confirm-dialog.component' import { PasswordRemovalConfirmDialogComponent } from '../common/confirm-dialog/password-removal-confirm-dialog/password-removal-confirm-dialog.component' +import { ReprocessConfirmDialogComponent } from '../common/confirm-dialog/reprocess-confirm-dialog/reprocess-confirm-dialog.component' import { CustomFieldsDropdownComponent } from '../common/custom-fields-dropdown/custom-fields-dropdown.component' import { CorrespondentEditDialogComponent } from '../common/edit-dialog/correspondent-edit-dialog/correspondent-edit-dialog.component' import { DocumentTypeEditDialogComponent } from '../common/edit-dialog/document-type-edit-dialog/document-type-edit-dialog.component' @@ -304,6 +304,7 @@ export class DocumentDetailComponent isDirty$: Observable unsubscribeNotifier: Subject = new Subject() docChangeNotifier: Subject = new Subject() + versionChangeNotifier: Subject = new Subject() private incomingUpdateModal: NgbModalRef private pendingIncomingUpdate: IncomingDocumentUpdate private lastLocalSaveModified: string | null = null @@ -417,7 +418,8 @@ export class DocumentDetailComponent .pipe( first(), takeUntil(this.unsubscribeNotifier), - takeUntil(this.docChangeNotifier) + takeUntil(this.docChangeNotifier), + takeUntil(this.versionChangeNotifier) ) .subscribe({ next: (result) => { @@ -533,7 +535,8 @@ export class DocumentDetailComponent .pipe( first(), takeUntil(this.unsubscribeNotifier), - takeUntil(this.docChangeNotifier) + takeUntil(this.docChangeNotifier), + takeUntil(this.versionChangeNotifier) ) .subscribe({ next: (res) => this.previewText.set(res.toString()), @@ -595,6 +598,13 @@ export class DocumentDetailComponent openDocument.duplicate_documents = doc.duplicate_documents this.openDocumentService.save() } + // use server versions + if (openDocument) { + openDocument.versions = doc.versions + if (!openDocument.__changedFields?.includes('content')) { + openDocument.content = doc.content + } + } let useDoc = openDocument || doc if (openDocument && forceRemote) { Object.assign(openDocument, doc) @@ -642,7 +652,14 @@ export class DocumentDetailComponent this.documentForm.patchValue({ title: titleValue }) this.documentForm.get('title').markAsDirty() }) + const keepContentEdits = + useDoc.__selectedVersionId === this.selectedVersionId() && + !!useDoc.__changedFields?.includes('content') this.setupDirtyTracking(useDoc, doc) + // Maybe load the stored version + if (useDoc.__selectedVersionId) { + this.selectVersion(this.selectedVersionId(), keepContentEdits) + } }, }) } @@ -903,9 +920,11 @@ export class DocumentDetailComponent updateComponent(doc: Document) { this.document.set(doc) - // Default selected version is the newest version, which the API returns first + // Load the selected version, or default to API first (newest) const versions = doc.versions ?? [] - this.selectedVersionId.set(versions.length ? versions[0].id : doc.id) + const selectedVersion = + versions.find((v) => v.id === doc.__selectedVersionId) ?? versions[0] + this.selectedVersionId.set(selectedVersion?.id ?? doc.id) this.previewLoaded.set(false) this.requiresPassword = false this.updateFormForCustomFields() @@ -940,8 +959,12 @@ export class DocumentDetailComponent } // Update file preview and download target to a specific version (by document id) - selectVersion(versionId: number) { + selectVersion(versionId: number, keepContentEdits: boolean = false) { + this.versionChangeNotifier.next() this.selectedVersionId.set(versionId) + // remember so the version can be restored when returning to the document + this.document().__selectedVersionId = versionId + this.openDocumentService.save() this.previewLoaded.set(false) this.previewUrl.set( this.documentsService.getPreviewUrl( @@ -963,20 +986,20 @@ export class DocumentDetailComponent .pipe( first(), takeUntil(this.unsubscribeNotifier), - takeUntil(this.docChangeNotifier) + takeUntil(this.docChangeNotifier), + takeUntil(this.versionChangeNotifier) ) .subscribe({ next: (doc) => { const content = doc?.content ?? '' - this.document().content = content - this.documentForm.patchValue( - { - content, - }, - { - emitEvent: false, - } - ) + if (keepContentEdits) { + this.store.next({ ...this.store.value, content }) + } else { + // Update in-place and avoid the debounce wait + this.store.value.content = content + this.documentForm.patchValue({ content }) + this.documentForm.get('content').markAsPristine() + } }, error: (error) => { this.toastService.showError( @@ -991,7 +1014,8 @@ export class DocumentDetailComponent .pipe( first(), takeUntil(this.unsubscribeNotifier), - takeUntil(this.docChangeNotifier) + takeUntil(this.docChangeNotifier), + takeUntil(this.versionChangeNotifier) ) .subscribe({ next: (res) => this.previewText.set(res.toString()), @@ -1005,7 +1029,39 @@ export class DocumentDetailComponent } onVersionSelected(versionId: number) { - this.selectVersion(versionId) + if (versionId === this.selectedVersionId()) return + // Bail if the selected version was just deleted. + const selectedVersionExists = this.document()?.versions?.some( + (v) => v.id === this.selectedVersionId() + ) + if (this.networkActive() && selectedVersionExists) return + if ( + !selectedVersionExists || + this.documentForm.get('content').value === this.store.value.content + ) { + this.selectVersion(versionId) + return + } + + // Confirm any unsaved content changes + const modal = this.modalService.open(ConfirmDialogComponent, { + backdrop: 'static', + }) + modal.componentInstance.title = $localize`Unsaved Changes` + modal.componentInstance.messageBold = $localize`You have unsaved changes to the content of this version.` + modal.componentInstance.message = $localize`Switching versions will discard them.` + modal.componentInstance.btnClass = 'btn-secondary' + modal.componentInstance.btnCaption = $localize`Discard and switch` + modal.componentInstance.alternativeBtnClass = 'btn-primary' + modal.componentInstance.alternativeBtnCaption = $localize`Save and switch` + modal.componentInstance.confirmClicked.pipe(first()).subscribe(() => { + modal.close() + this.selectVersion(versionId) + }) + modal.componentInstance.alternativeClicked.pipe(first()).subscribe(() => { + modal.close() + this.save(false, () => this.selectVersion(versionId)) + }) } onVersionsUpdated(versions: DocumentVersionInfo[]) { @@ -1233,7 +1289,7 @@ export class DocumentDetailComponent return changes } - save(close: boolean = false) { + save(close: boolean = false, savedCallback: () => void = null) { this.networkActive.set(true) ;(document.activeElement as HTMLElement)?.dispatchEvent(new Event('change')) this.documentsService @@ -1266,6 +1322,7 @@ export class DocumentDetailComponent this.flushPendingIncomingUpdate() } this.savedViewService.maybeRefreshDocumentCounts() + savedCallback?.() }, error: (error) => { this.networkActive.set(false) diff --git a/src-ui/src/app/data/document.ts b/src-ui/src/app/data/document.ts index d33b64248..9a8230d70 100644 --- a/src-ui/src/app/data/document.ts +++ b/src-ui/src/app/data/document.ts @@ -167,6 +167,7 @@ export interface Document extends ObjectWithPermissions { // Frontend only __changedFields?: string[] + __selectedVersionId?: number } export interface DocumentVersionInfo {