From 9d18ac0497d677a37f5d845a3df7ca3a95ddb45f Mon Sep 17 00:00:00 2001 From: Joseph Myers Date: Thu, 25 Sep 2025 14:29:43 -0500 Subject: [PATCH 1/6] Fix Stay on Page option in Draft Formatting Apparently, the Location object is not aware of navigation guards, where Router is. The Location object was popping off the top item on history immediately after clicking Cancel, even when the confirm prompt returned "stay on page." Changing to use Router fixes the issue. --- .../draft-usfm-format.component.spec.ts | 11 +++++------ .../draft-usfm-format.component.ts | 14 +++++++------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts index d0cbddd0995..b7389369c2c 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts @@ -1,9 +1,8 @@ import { TestbedHarnessEnvironment } from '@angular/cdk/testing/testbed'; -import { Location } from '@angular/common'; import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing'; import { MatRadioButtonHarness } from '@angular/material/radio/testing'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; -import { ActivatedRoute } from '@angular/router'; +import { ActivatedRoute, Router } from '@angular/router'; import { createTestProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-test-data'; import { TextInfo } from 'realtime-server/lib/esm/scriptureforge/models/text-info'; import { @@ -44,7 +43,7 @@ const mockedActivatedProjectService = mock(ActivatedProjectService); const mockedProjectService = mock(SFProjectService); const mockedUserService = mock(UserService); const mockedServalAdministration = mock(ServalAdministrationService); -const mockedLocation = mock(Location); +const mockedRouter = mock(Router); const mockI18nService = mock(I18nService); const mockedNoticeService = mock(NoticeService); const mockedDialogService = mock(DialogService); @@ -67,7 +66,7 @@ describe('DraftUsfmFormatComponent', () => { { provide: SFProjectService, useMock: mockedProjectService }, { provide: UserService, useMock: mockedUserService }, { provide: ServalAdministrationService, useMock: mockedServalAdministration }, - { provide: Location, useMock: mockedLocation }, + { provide: Router, useMock: mockedRouter }, { provide: OnlineStatusService, useClass: TestOnlineStatusService }, { provide: I18nService, useMock: mockI18nService }, { provide: NoticeService, useMock: mockedNoticeService }, @@ -160,7 +159,7 @@ describe('DraftUsfmFormatComponent', () => { // user will be prompted that there are unsaved changes expect(await env.component.confirmLeave()).toBe(true); verify(mockedProjectService.onlineSetUsfmConfig(env.projectId, anything())).never(); - verify(mockedLocation.back()).once(); + verify(mockedRouter.navigate(deepEqual(['..']), anything())).once(); })); it('should save changes to the draft format', fakeAsync(async () => { @@ -185,7 +184,7 @@ describe('DraftUsfmFormatComponent', () => { env.fixture.detectChanges(); verify(mockedProjectService.onlineSetUsfmConfig(env.projectId, deepEqual(config))).once(); verify(mockedServalAdministration.onlineRetrievePreTranslationStatus(env.projectId)).once(); - verify(mockedLocation.back()).once(); + verify(mockedRouter.navigate(deepEqual(['..']), anything())).once(); })); it('should not save if format is empty', fakeAsync(() => { diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts index 077a3cc776b..b7b170d7d7e 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts @@ -1,4 +1,4 @@ -import { CommonModule, Location } from '@angular/common'; +import { CommonModule } from '@angular/common'; import { AfterViewInit, Component, DestroyRef, EventEmitter, ViewChild } from '@angular/core'; import { FormControl, FormGroup, FormsModule, ReactiveFormsModule } from '@angular/forms'; import { MatButtonModule } from '@angular/material/button'; @@ -9,7 +9,7 @@ import { MatIconModule } from '@angular/material/icon'; import { MatProgressSpinnerModule } from '@angular/material/progress-spinner'; import { MatRadioModule } from '@angular/material/radio'; import { MatSelectModule } from '@angular/material/select'; -import { ActivatedRoute } from '@angular/router'; +import { ActivatedRoute, Router } from '@angular/router'; import { TranslocoModule } from '@ngneat/transloco'; import { Canon } from '@sillsdev/scripture'; import { Delta } from 'quill'; @@ -91,7 +91,7 @@ export class DraftUsfmFormatComponent extends DataLoadingComponent implements Af private readonly dialogService: DialogService, readonly noticeService: NoticeService, readonly i18n: I18nService, - private readonly location: Location, + private readonly router: Router, private destroyRef: DestroyRef ) { super(noticeService); @@ -197,8 +197,8 @@ export class DraftUsfmFormatComponent extends DataLoadingComponent implements Af this.reloadText(); } - close(): void { - this.location.back(); + async close(): Promise { + await this.router.navigate(['..'], { relativeTo: this.activatedRoute }); } reloadText(): void { @@ -215,10 +215,10 @@ export class DraftUsfmFormatComponent extends DataLoadingComponent implements Af this.lastSavedState = this.currentFormat; // The user is redirected to the draft generation page if the format is saved. await this.servalAdministration.onlineRetrievePreTranslationStatus(this.projectId); - this.close(); + await this.close(); } catch (err) { console.error('Error occurred while saving draft format', err); - this.noticeService.showError(this.i18n.translateStatic('draft_usfm_format.failed_to_save')); + await this.noticeService.showError(this.i18n.translateStatic('draft_usfm_format.failed_to_save')); } finally { this.saving = false; } From 9f2029596dead4a1fedae53109735d4f9b61fa66 Mon Sep 17 00:00:00 2001 From: Joseph Myers Date: Fri, 26 Sep 2025 14:57:10 -0500 Subject: [PATCH 2/6] Revert "Fix Stay on Page option in Draft Formatting" This reverts commit 4088eed18215bd5120eb724007ddb843df4d757c. --- .../draft-usfm-format.component.spec.ts | 11 ++++++----- .../draft-usfm-format.component.ts | 14 +++++++------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts index b7389369c2c..d0cbddd0995 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts @@ -1,8 +1,9 @@ import { TestbedHarnessEnvironment } from '@angular/cdk/testing/testbed'; +import { Location } from '@angular/common'; import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing'; import { MatRadioButtonHarness } from '@angular/material/radio/testing'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; -import { ActivatedRoute, Router } from '@angular/router'; +import { ActivatedRoute } from '@angular/router'; import { createTestProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-test-data'; import { TextInfo } from 'realtime-server/lib/esm/scriptureforge/models/text-info'; import { @@ -43,7 +44,7 @@ const mockedActivatedProjectService = mock(ActivatedProjectService); const mockedProjectService = mock(SFProjectService); const mockedUserService = mock(UserService); const mockedServalAdministration = mock(ServalAdministrationService); -const mockedRouter = mock(Router); +const mockedLocation = mock(Location); const mockI18nService = mock(I18nService); const mockedNoticeService = mock(NoticeService); const mockedDialogService = mock(DialogService); @@ -66,7 +67,7 @@ describe('DraftUsfmFormatComponent', () => { { provide: SFProjectService, useMock: mockedProjectService }, { provide: UserService, useMock: mockedUserService }, { provide: ServalAdministrationService, useMock: mockedServalAdministration }, - { provide: Router, useMock: mockedRouter }, + { provide: Location, useMock: mockedLocation }, { provide: OnlineStatusService, useClass: TestOnlineStatusService }, { provide: I18nService, useMock: mockI18nService }, { provide: NoticeService, useMock: mockedNoticeService }, @@ -159,7 +160,7 @@ describe('DraftUsfmFormatComponent', () => { // user will be prompted that there are unsaved changes expect(await env.component.confirmLeave()).toBe(true); verify(mockedProjectService.onlineSetUsfmConfig(env.projectId, anything())).never(); - verify(mockedRouter.navigate(deepEqual(['..']), anything())).once(); + verify(mockedLocation.back()).once(); })); it('should save changes to the draft format', fakeAsync(async () => { @@ -184,7 +185,7 @@ describe('DraftUsfmFormatComponent', () => { env.fixture.detectChanges(); verify(mockedProjectService.onlineSetUsfmConfig(env.projectId, deepEqual(config))).once(); verify(mockedServalAdministration.onlineRetrievePreTranslationStatus(env.projectId)).once(); - verify(mockedRouter.navigate(deepEqual(['..']), anything())).once(); + verify(mockedLocation.back()).once(); })); it('should not save if format is empty', fakeAsync(() => { diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts index b7b170d7d7e..077a3cc776b 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts @@ -1,4 +1,4 @@ -import { CommonModule } from '@angular/common'; +import { CommonModule, Location } from '@angular/common'; import { AfterViewInit, Component, DestroyRef, EventEmitter, ViewChild } from '@angular/core'; import { FormControl, FormGroup, FormsModule, ReactiveFormsModule } from '@angular/forms'; import { MatButtonModule } from '@angular/material/button'; @@ -9,7 +9,7 @@ import { MatIconModule } from '@angular/material/icon'; import { MatProgressSpinnerModule } from '@angular/material/progress-spinner'; import { MatRadioModule } from '@angular/material/radio'; import { MatSelectModule } from '@angular/material/select'; -import { ActivatedRoute, Router } from '@angular/router'; +import { ActivatedRoute } from '@angular/router'; import { TranslocoModule } from '@ngneat/transloco'; import { Canon } from '@sillsdev/scripture'; import { Delta } from 'quill'; @@ -91,7 +91,7 @@ export class DraftUsfmFormatComponent extends DataLoadingComponent implements Af private readonly dialogService: DialogService, readonly noticeService: NoticeService, readonly i18n: I18nService, - private readonly router: Router, + private readonly location: Location, private destroyRef: DestroyRef ) { super(noticeService); @@ -197,8 +197,8 @@ export class DraftUsfmFormatComponent extends DataLoadingComponent implements Af this.reloadText(); } - async close(): Promise { - await this.router.navigate(['..'], { relativeTo: this.activatedRoute }); + close(): void { + this.location.back(); } reloadText(): void { @@ -215,10 +215,10 @@ export class DraftUsfmFormatComponent extends DataLoadingComponent implements Af this.lastSavedState = this.currentFormat; // The user is redirected to the draft generation page if the format is saved. await this.servalAdministration.onlineRetrievePreTranslationStatus(this.projectId); - await this.close(); + this.close(); } catch (err) { console.error('Error occurred while saving draft format', err); - await this.noticeService.showError(this.i18n.translateStatic('draft_usfm_format.failed_to_save')); + this.noticeService.showError(this.i18n.translateStatic('draft_usfm_format.failed_to_save')); } finally { this.saving = false; } From 7dc9ea8eb083dc972ddd3e23374119f6e0daf0d2 Mon Sep 17 00:00:00 2001 From: Joseph Myers Date: Fri, 26 Sep 2025 15:02:38 -0500 Subject: [PATCH 3/6] Using computed navigationResolution This change makes navigation for canceled navigation attempts, like CanDeactivate, navigate the opposite direction, instead of replacing the item in the history. Replacing the item in the history was corrupting the history when using location.back() and then canceling. I'm actually not sure why you would want to be messing with the history in the first place. 'computed' seems like the sensible option. If the user hits back, then cancels, the router cancels by going forward. --- .../ClientApp/src/app/app-routing.module.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/app-routing.module.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/app-routing.module.ts index 0db19784b17..e685977e0cc 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/app-routing.module.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/app-routing.module.ts @@ -37,7 +37,11 @@ const routes: Routes = [ ]; @NgModule({ - imports: [RouterModule.forRoot(routes)], + imports: [ + RouterModule.forRoot(routes, { + canceledNavigationResolution: 'computed' + }) + ], exports: [RouterModule] }) export class AppRoutingModule {} From 3a9eb475b55ad7cf8653d83c1bb8e2a8f43a1ee2 Mon Sep 17 00:00:00 2001 From: Joseph Myers Date: Mon, 29 Sep 2025 12:18:56 -0500 Subject: [PATCH 4/6] Revert "Using computed navigationResolution" This reverts commit 098802f29049149181fef058e07013a2d57d497c. --- .../ClientApp/src/app/app-routing.module.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/app-routing.module.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/app-routing.module.ts index e685977e0cc..0db19784b17 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/app-routing.module.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/app-routing.module.ts @@ -37,11 +37,7 @@ const routes: Routes = [ ]; @NgModule({ - imports: [ - RouterModule.forRoot(routes, { - canceledNavigationResolution: 'computed' - }) - ], + imports: [RouterModule.forRoot(routes)], exports: [RouterModule] }) export class AppRoutingModule {} From a1a02ce9af9fe0e86187edf72dd2123b0dcfe20d Mon Sep 17 00:00:00 2001 From: Joseph Myers Date: Tue, 30 Sep 2025 13:16:38 -0500 Subject: [PATCH 5/6] Implemented a simpler fix for just Cancel This solution does not fix the problem when using the Back button. Maybe we don't care? --- .../draft-usfm-format/draft-usfm-format.component.html | 2 +- .../draft-usfm-format/draft-usfm-format.component.ts | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html index ea3228dd56a..f4c58a2d82c 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html @@ -58,7 +58,7 @@

{{ t("formatting_options") }}

- +
diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts index 077a3cc776b..27f75997db3 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts @@ -197,6 +197,11 @@ export class DraftUsfmFormatComponent extends DataLoadingComponent implements Af this.reloadText(); } + async cancelClicked(): Promise { + if (!(await this.confirmLeave())) return; + this.close(); + } + close(): void { this.location.back(); } From 3582107392778658daff1860221dbc77e9d95d81 Mon Sep 17 00:00:00 2001 From: Joseph Myers Date: Mon, 6 Oct 2025 10:41:01 -0500 Subject: [PATCH 6/6] Removed deactivate guard from formatting page This was in response to review comments. It prevents the confirmation from being shown twice (I think), but it also removes the guard from Back altogether. So now we're just trying to remind them about unsaved changes on Cancel. --- .../draft-usfm-format/draft-usfm-format.component.spec.ts | 4 ++-- .../draft-usfm-format/draft-usfm-format.component.ts | 5 ++--- .../ClientApp/src/app/translate/translate-routing.module.ts | 3 +-- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts index d0cbddd0995..8c532572f54 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts @@ -130,7 +130,7 @@ describe('DraftUsfmFormatComponent', () => { const env = new TestEnvironment({ quotationAnalysis: QuotationAnalysis.Successful }); expect(env.component.paragraphFormat.value).toBe(ParagraphBreakFormat.BestGuess); expect(env.component.quoteFormat.value).toBe(QuoteFormat.Denormalized); - expect(await env.component.confirmLeave()).toBe(true); + expect(await env.component['confirmLeave']()).toBe(true); expect(env.quoteFormatWarning).toBeNull(); })); @@ -158,7 +158,7 @@ describe('DraftUsfmFormatComponent', () => { tick(); env.fixture.detectChanges(); // user will be prompted that there are unsaved changes - expect(await env.component.confirmLeave()).toBe(true); + expect(await env.component['confirmLeave']()).toBe(true); verify(mockedProjectService.onlineSetUsfmConfig(env.projectId, anything())).never(); verify(mockedLocation.back()).once(); })); diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts index 27f75997db3..59f6b93bf65 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts @@ -31,7 +31,6 @@ import { TextDocId } from '../../../core/models/text-doc'; import { SFProjectService } from '../../../core/sf-project.service'; import { QuotationAnalysis } from '../../../machine-api/quotation-denormalization'; import { ServalAdministrationService } from '../../../serval-administration/serval-administration.service'; -import { ConfirmOnLeave } from '../../../shared/project-router.guard'; import { SharedModule } from '../../../shared/shared.module'; import { TextComponent } from '../../../shared/text/text.component'; import { DraftGenerationService } from '../draft-generation.service'; @@ -57,7 +56,7 @@ import { DraftHandlingService } from '../draft-handling.service'; templateUrl: './draft-usfm-format.component.html', styleUrl: './draft-usfm-format.component.scss' }) -export class DraftUsfmFormatComponent extends DataLoadingComponent implements AfterViewInit, ConfirmOnLeave { +export class DraftUsfmFormatComponent extends DataLoadingComponent implements AfterViewInit { @ViewChild(TextComponent) draftText!: TextComponent; bookNum: number = 1; booksWithDrafts: number[] = []; @@ -229,7 +228,7 @@ export class DraftUsfmFormatComponent extends DataLoadingComponent implements Af } } - async confirmLeave(): Promise { + private async confirmLeave(): Promise { if ( this.lastSavedState?.paragraphFormat === this.currentFormat?.paragraphFormat && this.lastSavedState?.quoteFormat === this.currentFormat?.quoteFormat diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/translate-routing.module.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/translate-routing.module.ts index e7d1abaa6d6..dab0d1cb395 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/translate-routing.module.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/translate-routing.module.ts @@ -29,8 +29,7 @@ const routes: Routes = [ { path: 'projects/:projectId/draft-generation/format', component: DraftUsfmFormatComponent, - canActivate: [NmtDraftAuthGuard], - canDeactivate: [DraftNavigationAuthGuard] + canActivate: [NmtDraftAuthGuard] }, { path: 'projects/:projectId/draft-generation/format/:bookId',