From d6e9d71289cdab8cc72aa9c3ff7075bb6a0ed083 Mon Sep 17 00:00:00 2001 From: Raymond Luong Date: Fri, 15 Nov 2024 15:02:45 -0700 Subject: [PATCH 1/2] SF-3081 Delay closing draft generation stepper until build queued --- .../draft-generation-steps.component.html | 33 +++++++++++++-- .../draft-generation-steps.component.scss | 1 + .../draft-generation-steps.component.spec.ts | 11 ++++- .../draft-generation-steps.component.ts | 2 + .../draft-generation.component.spec.ts | 42 ++++++++----------- .../draft-generation.component.ts | 2 +- 6 files changed, 62 insertions(+), 29 deletions(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html index 95d03684a6..6383a1f342 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html @@ -115,9 +115,18 @@

}
- + @if (isStepsCompleted) { + + }
@if (trainingDataFilesAvailable) { @@ -134,9 +143,18 @@

{{ t("choose_additional_training_data_files") }}

>
- + @if (isStepsCompleted) { + + }
} @@ -154,9 +172,18 @@

{{ t("configure_advanced_settings") }}

}
- + @if (isStepsCompleted) { + + }
} diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.scss b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.scss index a0e2684674..bedeb73bf4 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.scss +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.scss @@ -37,6 +37,7 @@ h1 { margin-top: 20px; display: flex; justify-content: flex-start; + align-items: center; gap: 8px; } diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.spec.ts index 70ae56514e..5f342936b5 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.spec.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.spec.ts @@ -227,12 +227,15 @@ describe('DraftGenerationStepsComponent', () => { component.selectedTrainingDataIds = trainingDataFiles; spyOn(component.done, 'emit'); - + expect(component.isStepsCompleted).toBe(false); // Advance to the next step when at last step should emit books result fixture.detectChanges(); component.tryAdvanceStep(); fixture.detectChanges(); + const generateDraftButton: HTMLElement = fixture.nativeElement.querySelector('.advance-button'); + expect(generateDraftButton['disabled']).toBe(false); component.tryAdvanceStep(); + fixture.detectChanges(); expect(component.done.emit).toHaveBeenCalledWith({ translationBooks, @@ -240,6 +243,8 @@ describe('DraftGenerationStepsComponent', () => { trainingBooks: trainingBooks.filter(book => !translationBooks.includes(book)), fastTraining: false } as DraftGenerationStepsResult); + expect(component.isStepsCompleted).toBe(true); + expect(generateDraftButton['disabled']).toBe(true); }); it('should emit the correct selected books when bookSelect is called', fakeAsync(() => { @@ -325,7 +330,10 @@ describe('DraftGenerationStepsComponent', () => { // Click next on the final step to generate the draft fixture.detectChanges(); + const generateDraftButton: HTMLElement = fixture.nativeElement.querySelector('.advance-button'); + expect(generateDraftButton['disabled']).toBe(false); component.tryAdvanceStep(); + fixture.detectChanges(); expect(component.done.emit).toHaveBeenCalledWith({ trainingBooks, @@ -333,6 +341,7 @@ describe('DraftGenerationStepsComponent', () => { translationBooks, fastTraining: true } as DraftGenerationStepsResult); + expect(generateDraftButton['disabled']).toBe(true); }); }); diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.ts index 032e3f8d51..6739b63123 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.ts @@ -83,6 +83,7 @@ export class DraftGenerationStepsComponent extends SubscriptionDisposable implem expandUnusableTranslateBooks = false; expandUnusableTrainingBooks = false; + isStepsCompleted = false; private trainingDataQuery?: RealtimeQuery; private trainingDataSub?: Subscription; @@ -246,6 +247,7 @@ export class DraftGenerationStepsComponent extends SubscriptionDisposable implem if (this.stepper.selected !== this.stepper.steps.last) { this.stepper.next(); } else { + this.isStepsCompleted = true; this.done.emit({ trainingBooks: this.userSelectedTrainingBooks, trainingDataFiles: this.selectedTrainingDataIds, diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts index 4307b894d8..1de075a78c 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts @@ -10,7 +10,7 @@ import { SFProjectRole } from 'realtime-server/lib/esm/scriptureforge/models/sf- import { createTestProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-test-data'; import { TextInfoPermission } from 'realtime-server/lib/esm/scriptureforge/models/text-info-permission'; import { ProjectType } from 'realtime-server/lib/esm/scriptureforge/models/translate-config'; -import { BehaviorSubject, EMPTY, of, throwError } from 'rxjs'; +import { BehaviorSubject, EMPTY, Subject, of, throwError } from 'rxjs'; import { instance, mock, verify, when } from 'ts-mockito'; import { ActivatedProjectService } from 'xforge-common/activated-project.service'; import { AuthService } from 'xforge-common/auth.service'; @@ -79,6 +79,7 @@ describe('DraftGenerationComponent', () => { class TestEnvironment { readonly testOnlineStatusService: TestOnlineStatusService; + readonly startedOrActiveBuild$: Subject = new Subject(); component!: DraftGenerationComponent; fixture!: ComponentFixture; @@ -155,6 +156,7 @@ describe('DraftGenerationComponent', () => { mockI18nService.getLanguageDisplayName.and.returnValue('English'); mockPreTranslationSignupUrlService.generateSignupUrl.and.returnValue(Promise.resolve('')); + mockDraftGenerationService.startBuildOrGetActiveBuild.and.returnValue(this.startedOrActiveBuild$); mockDraftGenerationService.getBuildProgress.and.returnValue(of(buildDto)); mockDraftGenerationService.pollBuildProgress.and.returnValue(of(buildDto)); mockDraftGenerationService.getLastCompletedBuild.and.returnValue(of(buildDto)); @@ -1742,10 +1744,9 @@ describe('DraftGenerationComponent', () => { describe('startBuild', () => { it('should start the draft build', () => { - let env = new TestEnvironment(() => { - mockDraftGenerationService.startBuildOrGetActiveBuild.and.returnValue(of(buildDto)); - }); + let env = new TestEnvironment(); + env.component.currentPage = 'steps'; env.component.startBuild({ trainingBooks: [], trainingDataFiles: [], @@ -1753,6 +1754,8 @@ describe('DraftGenerationComponent', () => { fastTraining: false, projectId: projectId }); + env.fixture.detectChanges(); + expect(env.component.currentPage).toBe('steps'); expect(mockDraftGenerationService.startBuildOrGetActiveBuild).toHaveBeenCalledWith({ projectId: projectId, trainingBooks: [], @@ -1760,14 +1763,13 @@ describe('DraftGenerationComponent', () => { translationBooks: [], fastTraining: false }); + env.startedOrActiveBuild$.next(buildDto); + env.fixture.detectChanges(); + expect(env.component.currentPage).toBe('initial'); }); it('should not attempt "cancel dialog" close for queued build', () => { - let env = new TestEnvironment(() => { - mockDraftGenerationService.startBuildOrGetActiveBuild.and.returnValue( - of({ ...buildDto, state: BuildStates.Queued }) - ); - }); + let env = new TestEnvironment(); const mockDialogRef: MatDialogRef = mock(MatDialogRef); env.component.cancelDialogRef = instance(mockDialogRef); @@ -1779,6 +1781,7 @@ describe('DraftGenerationComponent', () => { fastTraining: false, projectId: projectId }); + env.startedOrActiveBuild$.next({ ...buildDto, state: BuildStates.Queued }); expect(mockDraftGenerationService.startBuildOrGetActiveBuild).toHaveBeenCalledWith({ projectId: projectId, trainingBooks: [], @@ -1791,11 +1794,7 @@ describe('DraftGenerationComponent', () => { }); it('should not attempt "cancel dialog" close for pending build', () => { - let env = new TestEnvironment(() => { - mockDraftGenerationService.startBuildOrGetActiveBuild.and.returnValue( - of({ ...buildDto, state: BuildStates.Pending }) - ); - }); + let env = new TestEnvironment(); const mockDialogRef: MatDialogRef = mock(MatDialogRef); env.component.cancelDialogRef = instance(mockDialogRef); @@ -1807,6 +1806,7 @@ describe('DraftGenerationComponent', () => { fastTraining: false, projectId: projectId }); + env.startedOrActiveBuild$.next({ ...buildDto, state: BuildStates.Pending }); expect(mockDraftGenerationService.startBuildOrGetActiveBuild).toHaveBeenCalledWith({ projectId: projectId, trainingBooks: [], @@ -1819,11 +1819,7 @@ describe('DraftGenerationComponent', () => { }); it('should not attempt "cancel dialog" close for active build', () => { - let env = new TestEnvironment(() => { - mockDraftGenerationService.startBuildOrGetActiveBuild.and.returnValue( - of({ ...buildDto, state: BuildStates.Active }) - ); - }); + let env = new TestEnvironment(); const mockDialogRef: MatDialogRef = mock(MatDialogRef); env.component.cancelDialogRef = instance(mockDialogRef); @@ -1835,6 +1831,7 @@ describe('DraftGenerationComponent', () => { fastTraining: false, projectId: projectId }); + env.startedOrActiveBuild$.next({ ...buildDto, state: BuildStates.Active }); expect(mockDraftGenerationService.startBuildOrGetActiveBuild).toHaveBeenCalledWith({ projectId: projectId, trainingBooks: [], @@ -1847,11 +1844,7 @@ describe('DraftGenerationComponent', () => { }); it('should attempt "cancel dialog" close for cancelled build', () => { - let env = new TestEnvironment(() => { - mockDraftGenerationService.startBuildOrGetActiveBuild.and.returnValue( - of({ ...buildDto, state: BuildStates.Canceled }) - ); - }); + let env = new TestEnvironment(); const mockDialogRef: MatDialogRef = mock(MatDialogRef); when(mockDialogRef.getState()).thenReturn(MatDialogState.OPEN); @@ -1864,6 +1857,7 @@ describe('DraftGenerationComponent', () => { fastTraining: false, projectId: projectId }); + env.startedOrActiveBuild$.next({ ...buildDto, state: BuildStates.Canceled }); expect(mockDraftGenerationService.startBuildOrGetActiveBuild).toHaveBeenCalledWith({ projectId: projectId, trainingBooks: [], diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.ts index c833a994fa..88875fa721 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.ts @@ -439,7 +439,6 @@ export class DraftGenerationComponent extends DataLoadingComponent implements On } onPreGenerationStepsComplete(result: DraftGenerationStepsResult): void { - this.currentPage = 'initial'; this.startBuild({ projectId: this.activatedProject.projectId!, trainingBooks: result.trainingBooks, @@ -501,6 +500,7 @@ export class DraftGenerationComponent extends DataLoadingComponent implements On this.jobSubscription = this.subscribe( this.draftGenerationService.startBuildOrGetActiveBuild(buildConfig).pipe( tap((job?: BuildDto) => { + this.currentPage = 'initial'; // Handle automatic closing of dialog if job finishes while cancel dialog is open if (!this.canCancel(job)) { if (this.cancelDialogRef?.getState() === MatDialogState.OPEN) { From 3ce7aa4433bbd9647421ffca66083c116a88f394 Mon Sep 17 00:00:00 2001 From: Raymond Luong Date: Mon, 18 Nov 2024 14:00:06 -0700 Subject: [PATCH 2/2] Code review fix --- .../draft-generation/draft-generation.component.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts index 1de075a78c..ddaf3f5875 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts @@ -79,7 +79,7 @@ describe('DraftGenerationComponent', () => { class TestEnvironment { readonly testOnlineStatusService: TestOnlineStatusService; - readonly startedOrActiveBuild$: Subject = new Subject(); + readonly startedOrActiveBuild$ = new Subject(); component!: DraftGenerationComponent; fixture!: ComponentFixture;