From db329ec5ff1a94b3ce810ef96c3cff8914304fa7 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Tue, 28 Apr 2026 15:46:08 +0200 Subject: [PATCH] fix(sync): warn before destructive SYNC_IMPORT actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously the 'Server Already Has Data' dialog described a destructive SYNC_IMPORT as a 'merge' with a primary-colored 'Upload Local Data' button — leading users to clobber syncing devices' data. The decrypt- error 'Overwrite Remote' button had similarly understated copy and no final confirmation gate. - Rewrite D_SERVER_MIGRATION_CONFIRM body to call out 'overwrite' / 'replace' / 'other devices'; affirmative button is now 'Replace Server Data' with color=warn. - Rewrite D_DECRYPT_ERROR P3 + button label to make cross-device blast radius explicit. - Gate updatePWAndForceUpload behind a confirmDialog with a stronger warning string. - Add component spec for the migration dialog as a regression guard. --- ...log-handle-decrypt-error.component.spec.ts | 26 +++- .../dialog-handle-decrypt-error.component.ts | 6 + ...og-server-migration-confirm.component.html | 6 +- ...server-migration-confirm.component.spec.ts | 137 ++++++++++++++++++ src/app/t.const.ts | 1 + src/assets/i18n/en.json | 15 +- 6 files changed, 180 insertions(+), 11 deletions(-) create mode 100644 src/app/op-log/sync/dialog-server-migration-confirm/dialog-server-migration-confirm.component.spec.ts diff --git a/src/app/imex/sync/dialog-handle-decrypt-error/dialog-handle-decrypt-error.component.spec.ts b/src/app/imex/sync/dialog-handle-decrypt-error/dialog-handle-decrypt-error.component.spec.ts index 8c385dad68..096f00b74b 100644 --- a/src/app/imex/sync/dialog-handle-decrypt-error/dialog-handle-decrypt-error.component.spec.ts +++ b/src/app/imex/sync/dialog-handle-decrypt-error/dialog-handle-decrypt-error.component.spec.ts @@ -68,7 +68,20 @@ describe('DialogHandleDecryptErrorComponent', () => { }); describe('updatePWAndForceUpload()', () => { - it('should update password, clear field, and close with isForceUpload', async () => { + let originalConfirm: typeof window.confirm; + let confirmReturn: boolean; + + beforeEach(() => { + originalConfirm = window.confirm; + confirmReturn = true; + window.confirm = (() => confirmReturn) as typeof window.confirm; + }); + + afterEach(() => { + window.confirm = originalConfirm; + }); + + it('should update password, clear field, and close with isForceUpload when confirmed', async () => { component.passwordVal = 'new-password'; mockSyncConfigService.updateEncryptionPassword.and.resolveTo(); @@ -81,6 +94,17 @@ describe('DialogHandleDecryptErrorComponent', () => { expect(mockDialogRef.close).toHaveBeenCalledWith({ isForceUpload: true }); }); + it('should abort without changes when user cancels confirmation', async () => { + confirmReturn = false; + component.passwordVal = 'new-password'; + + await component.updatePWAndForceUpload(); + + expect(mockSyncConfigService.updateEncryptionPassword).not.toHaveBeenCalled(); + expect(mockDialogRef.close).not.toHaveBeenCalled(); + expect(component.passwordVal).toBe('new-password'); + }); + it('should show error snack and not close on failure', async () => { component.passwordVal = 'new-password'; mockSyncConfigService.updateEncryptionPassword.and.rejectWith( diff --git a/src/app/imex/sync/dialog-handle-decrypt-error/dialog-handle-decrypt-error.component.ts b/src/app/imex/sync/dialog-handle-decrypt-error/dialog-handle-decrypt-error.component.ts index 5e13b39e1a..d944d16370 100644 --- a/src/app/imex/sync/dialog-handle-decrypt-error/dialog-handle-decrypt-error.component.ts +++ b/src/app/imex/sync/dialog-handle-decrypt-error/dialog-handle-decrypt-error.component.ts @@ -5,6 +5,7 @@ import { MatDialogRef, MatDialogTitle, } from '@angular/material/dialog'; +import { TranslateService } from '@ngx-translate/core'; import { T } from '../../../t.const'; import { MatFormField, MatLabel } from '@angular/material/form-field'; import { MatInput } from '@angular/material/input'; @@ -15,6 +16,7 @@ import { TranslatePipe } from '@ngx-translate/core'; import { SyncConfigService } from '../sync-config.service'; import { SnackService } from '../../../core/snack/snack.service'; import { SyncLog } from '../../../core/log'; +import { confirmDialog } from '../../../util/native-dialogs'; @Component({ selector: 'dialog-handle-decrypt-error', @@ -37,6 +39,7 @@ import { SyncLog } from '../../../core/log'; export class DialogHandleDecryptErrorComponent { private _syncConfigService = inject(SyncConfigService); private _snackService = inject(SnackService); + private _translateService = inject(TranslateService); private _matDialogRef = inject>(MatDialogRef); @@ -45,6 +48,9 @@ export class DialogHandleDecryptErrorComponent { passwordVal: string = ''; async updatePWAndForceUpload(): Promise { + if (!confirmDialog(this._translateService.instant(T.F.SYNC.C.DECRYPT_OVERWRITE))) { + return; + } try { await this._syncConfigService.updateEncryptionPassword(this.passwordVal); this.passwordVal = ''; diff --git a/src/app/op-log/sync/dialog-server-migration-confirm/dialog-server-migration-confirm.component.html b/src/app/op-log/sync/dialog-server-migration-confirm/dialog-server-migration-confirm.component.html index 9cb5e6e0b3..7f6ba6dbe0 100644 --- a/src/app/op-log/sync/dialog-server-migration-confirm/dialog-server-migration-confirm.component.html +++ b/src/app/op-log/sync/dialog-server-migration-confirm/dialog-server-migration-confirm.component.html @@ -4,7 +4,7 @@ -

{{ T.F.SYNC.D_SERVER_MIGRATION_CONFIRM.MESSAGE | translate }}

+

@@ -16,10 +16,10 @@ diff --git a/src/app/op-log/sync/dialog-server-migration-confirm/dialog-server-migration-confirm.component.spec.ts b/src/app/op-log/sync/dialog-server-migration-confirm/dialog-server-migration-confirm.component.spec.ts new file mode 100644 index 0000000000..aa0519426c --- /dev/null +++ b/src/app/op-log/sync/dialog-server-migration-confirm/dialog-server-migration-confirm.component.spec.ts @@ -0,0 +1,137 @@ +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { NoopAnimationsModule } from '@angular/platform-browser/animations'; +import { MatDialogRef } from '@angular/material/dialog'; +import { + TranslateModule, + TranslateLoader, + TranslateService, + TranslationObject, +} from '@ngx-translate/core'; +import { Observable, of } from 'rxjs'; +import { DialogServerMigrationConfirmComponent } from './dialog-server-migration-confirm.component'; +import enTranslations from '../../../../assets/i18n/en.json'; + +/** + * Regression guard for the destructive-replace dialog wording. + * + * The original copy advertised this destructive SYNC_IMPORT action as a + * "merge" with a primary-colored "Upload Local Data" button — which led at + * least one user (johannes, 2026-04-27) to clobber a syncing device's data + * thinking they were merging it with another device's. The dialog must: + * - Never use "merge" in its body. + * - Mention "overwrite" or "replace" so the destructive intent is plain. + * - Mention "other device" so the cross-device blast radius is visible. + * - Render the affirmative button with color="warn" (not "primary"). + * - Label the affirmative button "Replace…" (not "Upload Local Data"). + */ + +class JsonTranslateLoader implements TranslateLoader { + getTranslation(): Observable { + return of(enTranslations as TranslationObject); + } +} + +describe('DialogServerMigrationConfirmComponent', () => { + let fixture: ComponentFixture; + let mockDialogRef: jasmine.SpyObj>; + + beforeEach(async () => { + mockDialogRef = jasmine.createSpyObj('MatDialogRef', ['close'], { + disableClose: false, + }); + + await TestBed.configureTestingModule({ + imports: [ + DialogServerMigrationConfirmComponent, + NoopAnimationsModule, + TranslateModule.forRoot({ + loader: { provide: TranslateLoader, useClass: JsonTranslateLoader }, + }), + ], + providers: [{ provide: MatDialogRef, useValue: mockDialogRef }], + }).compileComponents(); + + const translate = TestBed.inject(TranslateService); + translate.use('en'); + + fixture = TestBed.createComponent(DialogServerMigrationConfirmComponent); + fixture.detectChanges(); + await fixture.whenStable(); + fixture.detectChanges(); + }); + + describe('body copy', () => { + let bodyText: string; + + beforeEach(() => { + bodyText = + fixture.nativeElement + .querySelector('mat-dialog-content') + ?.textContent?.toLowerCase() ?? ''; + }); + + it('does not call the action a "merge" (the original misleading phrasing)', () => { + // Original copy: "Would you like to upload your local data to merge with it?" + // The fixed copy is allowed to *deny* a merge (e.g. "This is not a merge"), + // so just check that no positive merge-marketing phrase remains. + expect(bodyText).not.toMatch(/merge with|to merge|will merge/); + }); + + it('uses destructive language ("overwrite" or "replace")', () => { + expect(bodyText).toMatch(/overwrite|replace/); + }); + + it('warns about cross-device impact ("other device")', () => { + expect(bodyText).toMatch(/other device/); + }); + }); + + describe('action buttons', () => { + let buttons: HTMLButtonElement[]; + + beforeEach(() => { + buttons = Array.from( + fixture.nativeElement.querySelectorAll('mat-dialog-actions button'), + ); + }); + + it('renders exactly two action buttons', () => { + expect(buttons.length).toBe(2); + }); + + it('affirmative button is labelled "Replace…", not "Upload Local Data"', () => { + const affirmative = buttons[1]; + const label = affirmative.textContent?.toLowerCase() ?? ''; + expect(label).toContain('replace'); + expect(label).not.toContain('upload local data'); + }); + + it('affirmative button uses warn color (not primary)', () => { + const affirmative = buttons[1]; + expect(affirmative.getAttribute('color')).toBe('warn'); + }); + + it('cancel button has no color (per cancel-button-color rule)', () => { + const cancel = buttons[0]; + expect(cancel.getAttribute('color')).toBeNull(); + }); + }); + + describe('close behavior', () => { + it('cancel button closes with false', () => { + const cancel = fixture.nativeElement.querySelectorAll( + 'mat-dialog-actions button', + )[0] as HTMLButtonElement; + cancel.click(); + expect(mockDialogRef.close).toHaveBeenCalledWith(false); + }); + + it('affirmative button closes with true', () => { + const affirmative = fixture.nativeElement.querySelectorAll( + 'mat-dialog-actions button', + )[1] as HTMLButtonElement; + affirmative.click(); + expect(mockDialogRef.close).toHaveBeenCalledWith(true); + }); + }); +}); diff --git a/src/app/t.const.ts b/src/app/t.const.ts index 5edd74f105..14b5267a72 100644 --- a/src/app/t.const.ts +++ b/src/app/t.const.ts @@ -1062,6 +1062,7 @@ const T = { BTN_SYNC_NOW: 'F.SYNC.BTN_SYNC_NOW', C: { FORCE_UPLOAD: 'F.SYNC.C.FORCE_UPLOAD', + DECRYPT_OVERWRITE: 'F.SYNC.C.DECRYPT_OVERWRITE', }, D_AUTH_CODE: { FOLLOW_LINK: 'F.SYNC.D_AUTH_CODE.FOLLOW_LINK', diff --git a/src/assets/i18n/en.json b/src/assets/i18n/en.json index 508c880625..5b036f95c1 100644 --- a/src/assets/i18n/en.json +++ b/src/assets/i18n/en.json @@ -1044,7 +1044,8 @@ "BTN_RESTORE_FROM_HISTORY": "Restore from History", "BTN_SYNC_NOW": "Sync Now", "C": { - "FORCE_UPLOAD": "Forcing an upload of your data could lead to data loss. Are you sure you want to continue?" + "FORCE_UPLOAD": "This will REPLACE all data on the server and on every other device with this device's local copy. Any unsynced changes on those devices will be permanently lost. Continue?", + "DECRYPT_OVERWRITE": "This will REPLACE the encrypted data on the server with this device's local copy and re-encrypt it with the new password. All other devices will need to re-enter the password and download the replaced data. Any unsynced changes on those devices will be permanently lost. Continue?" }, "D_AUTH_CODE": { "FOLLOW_LINK": "Please open the following link and copy the auth code provided there into the input field below.", @@ -1093,11 +1094,11 @@ "UNKNOWN_ERROR": "Unknown Sync Error: {{err}}" }, "D_DECRYPT_ERROR": { - "BTN_OVER_WRITE_REMOTE": "Use This Password & Overwrite Remote", + "BTN_OVER_WRITE_REMOTE": "Overwrite Server & Other Devices", "CHANGE_PW_AND_DECRYPT": "Retry Decrypt", "P1": "Your remote data is encrypted and decryption failed.", "P2_FORGOT_PW": "If you forgot your password, enter the correct password and click \"Retry Decrypt\".", - "P3_PW_CHANGED": "If you changed your password on another device, enter your new password and click \"Overwrite Remote\". This will upload your local data to the server.", + "P3_PW_CHANGED": "If you changed your password on another device, enter your new password and click Overwrite Server & Other Devices. ⚠️ This is destructive: it replaces ALL data on the server and on your other devices with this device's local copy. Any unsynced changes on those devices will be lost.", "PASSWORD": "Password", "TITLE": "Decryption Failed" }, @@ -1160,10 +1161,10 @@ "WARNING": "This will replace ALL your current data with the selected backup. This action cannot be undone!" }, "D_SERVER_MIGRATION_CONFIRM": { - "CONFIRM": "Upload Local Data", - "MESSAGE": "This sync server already contains data. Would you like to upload your local data to merge with it? If you skip this, some of your local changes may not sync.", - "SKIP": "Skip", - "TITLE": "Server Already Has Data" + "CONFIRM": "Replace Server Data", + "MESSAGE": "This sync server already contains data — likely from your other devices. Choosing Replace Server Data will overwrite all data on the server and on every other device with this device's local copy. This is not a merge. Choose Cancel to download the server's data instead (recommended unless you're sure this device has the newest version).", + "SKIP": "Cancel", + "TITLE": "Server Already Contains Data" }, "D_SYNC_IMPORT_CONFLICT": { "FILTERED_CHANGES": "Affected changes",