Skip to content

Commit 3f79932

Browse files
SF-3556 Wait for finishing build to complete before showing draft (#3479)
Co-authored-by: Peter Chapman <peter@conglomo.co.nz>
1 parent 3360c9c commit 3f79932

File tree

6 files changed

+445
-75
lines changed

6 files changed

+445
-75
lines changed

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -273,10 +273,6 @@ export class DraftHistoryEntryComponent {
273273
return this.activatedProjectService.projectDoc?.data?.translateConfig.draftConfig.usfmConfig != null;
274274
}
275275

276-
get buildStateIsCompleted(): boolean {
277-
return this._entry?.state === BuildStates.Completed;
278-
}
279-
280276
@Input() isLatestBuild: boolean = false;
281277
trainingConfigurationOpen = false;
282278

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-draft/editor-draft.component.spec.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { Delta } from 'quill';
1010
import { SFProjectRole } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-role';
1111
import { createTestProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-test-data';
1212
import { ParagraphBreakFormat, QuoteFormat } from 'realtime-server/lib/esm/scriptureforge/models/translate-config';
13-
import { of } from 'rxjs';
13+
import { BehaviorSubject, of } from 'rxjs';
1414
import { anything, mock, verify, when } from 'ts-mockito';
1515
import { ActivatedProjectService } from 'xforge-common/activated-project.service';
1616
import { CommandError, CommandErrorCode } from 'xforge-common/command.service';
@@ -27,6 +27,8 @@ import { configureTestingModule, TestTranslocoModule } from 'xforge-common/test-
2727
import { SFProjectProfileDoc } from '../../../core/models/sf-project-profile-doc';
2828
import { SF_TYPE_REGISTRY } from '../../../core/models/sf-type-registry';
2929
import { Revision } from '../../../core/paratext.service';
30+
import { BuildDto } from '../../../machine-api/build-dto';
31+
import { BuildStates } from '../../../machine-api/build-states';
3032
import { SharedModule } from '../../../shared/shared.module';
3133
import { EDITOR_READY_TIMEOUT } from '../../../shared/text/text.component';
3234
import { DraftSegmentMap } from '../../draft-generation/draft-generation';
@@ -48,6 +50,7 @@ describe('EditorDraftComponent', () => {
4850
let fixture: ComponentFixture<EditorDraftComponent>;
4951
let component: EditorDraftComponent;
5052
let testOnlineStatus: TestOnlineStatusService;
53+
const buildProgress$ = new BehaviorSubject<BuildDto | undefined>(undefined);
5154

5255
configureTestingModule(() => ({
5356
declarations: [EditorDraftComponent, HistoryRevisionFormatPipe],
@@ -78,6 +81,8 @@ describe('EditorDraftComponent', () => {
7881

7982
beforeEach(() => {
8083
when(mockFeatureFlagService.usfmFormat).thenReturn(createTestFeatureFlag(true));
84+
when(mockDraftGenerationService.pollBuildProgress(anything())).thenReturn(buildProgress$.asObservable());
85+
buildProgress$.next({ state: BuildStates.Completed } as BuildDto);
8186

8287
fixture = TestBed.createComponent(EditorDraftComponent);
8388
component = fixture.componentInstance;
@@ -99,6 +104,41 @@ describe('EditorDraftComponent', () => {
99104
flush();
100105
}));
101106

107+
it('should handle build is finishing', fakeAsync(() => {
108+
buildProgress$.next({ state: BuildStates.Finishing } as BuildDto);
109+
const testProjectDoc: SFProjectProfileDoc = {
110+
data: createTestProjectProfile()
111+
} as SFProjectProfileDoc;
112+
when(mockFeatureFlagService.newDraftHistory).thenReturn(createTestFeatureFlag(true));
113+
when(mockDraftGenerationService.draftExists(anything(), anything(), anything())).thenReturn(of(true));
114+
when(mockDraftGenerationService.getGeneratedDraftHistory(anything(), anything(), anything())).thenReturn(
115+
of(draftHistory)
116+
);
117+
when(mockActivatedProjectService.changes$).thenReturn(of(testProjectDoc));
118+
spyOn<any>(component, 'getTargetOps').and.returnValue(of(targetDelta.ops!));
119+
when(mockDraftHandlingService.getDraft(anything(), anything())).thenReturn(of(cloneDeep(draftDelta.ops!)));
120+
when(mockDraftHandlingService.draftDataToOps(anything(), anything())).thenReturn(draftDelta.ops!);
121+
when(mockDraftHandlingService.isDraftSegmentMap(anything())).thenReturn(false);
122+
123+
fixture.detectChanges();
124+
tick(EDITOR_READY_TIMEOUT);
125+
126+
verify(mockDraftHandlingService.getDraft(anything(), anything())).never();
127+
verify(mockDraftHandlingService.draftDataToOps(anything(), anything())).never();
128+
expect(component.draftCheckState).toEqual('draft-unknown');
129+
expect(component.draftText).not.toBeUndefined();
130+
131+
buildProgress$.next({ state: BuildStates.Completed } as BuildDto);
132+
fixture.detectChanges();
133+
tick(EDITOR_READY_TIMEOUT);
134+
135+
verify(mockDraftHandlingService.getDraft(anything(), anything())).once();
136+
verify(mockDraftHandlingService.draftDataToOps(anything(), anything())).once();
137+
expect(component.draftCheckState).toEqual('draft-present');
138+
expect(component.draftText).not.toBeUndefined();
139+
flush();
140+
}));
141+
102142
it('should populate draft text correctly and then handle going offline/online', fakeAsync(() => {
103143
const testProjectDoc: SFProjectProfileDoc = {
104144
data: createTestProjectProfile()

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-draft/editor-draft.component.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import { isString } from '../../../../type-utils';
3838
import { TextDocId } from '../../../core/models/text-doc';
3939
import { Revision } from '../../../core/paratext.service';
4040
import { SFProjectService } from '../../../core/sf-project.service';
41+
import { BuildStates } from '../../../machine-api/build-states';
4142
import { TextComponent } from '../../../shared/text/text.component';
4243
import { DraftGenerationService } from '../../draft-generation/draft-generation.service';
4344
import { DraftHandlingService } from '../../draft-generation/draft-handling.service';
@@ -144,12 +145,13 @@ export class EditorDraftComponent implements AfterViewInit, OnChanges {
144145
populateDraftTextInit(): void {
145146
combineLatest([
146147
this.onlineStatusService.onlineStatus$,
148+
this.draftGenerationService.pollBuildProgress(this.textDocId!.projectId),
147149
this.draftText.editorCreated as EventEmitter<any>,
148150
this.inputChanged$.pipe(startWith(undefined))
149151
])
150152
.pipe(
151153
quietTakeUntilDestroyed(this.destroyRef),
152-
filter(([isOnline]) => isOnline),
154+
filter(([isOnline, build]) => isOnline && build != null && build.state !== BuildStates.Finishing),
153155
tap(() => this.setInitialState()),
154156
switchMap(() =>
155157
combineLatest([

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.spec.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,8 @@ import { PermissionsService } from '../../core/permissions.service';
9595
import { SFProjectService } from '../../core/sf-project.service';
9696
import { TextDocService } from '../../core/text-doc.service';
9797
import { TranslationEngineService } from '../../core/translation-engine.service';
98+
import { BuildDto } from '../../machine-api/build-dto';
99+
import { BuildStates } from '../../machine-api/build-states';
98100
import { HttpClient } from '../../machine-api/http-client';
99101
import { RemoteTranslationEngine } from '../../machine-api/remote-translation-engine';
100102
import { CopyrightBannerComponent } from '../../shared/copyright-banner/copyright-banner.component';
@@ -4864,6 +4866,9 @@ class TestEnvironment {
48644866
});
48654867
when(mockedDraftGenerationService.getLastCompletedBuild(anything())).thenReturn(of({} as any));
48664868
when(mockedDraftGenerationService.getGeneratedDraft(anything(), anything(), anything())).thenReturn(of({}));
4869+
when(mockedDraftGenerationService.pollBuildProgress(anything())).thenReturn(
4870+
of({ state: BuildStates.Completed } as BuildDto)
4871+
);
48674872
when(
48684873
mockedDraftGenerationService.getGeneratedDraftDeltaOperations(
48694874
anything(),

src/SIL.XForge.Scripture/Services/MachineApiService.cs

Lines changed: 118 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,14 @@ IRepository<UserSecret> userSecrets
8484
/// </remarks>
8585
internal const string BuildStateFinishing = "FINISHING";
8686

87+
/// <summary>
88+
/// The Completed build state.
89+
/// </summary>
90+
/// <remarks>
91+
/// Serval returns this state when the build is completed.
92+
/// </remarks>
93+
internal const string BuildStateCompleted = "COMPLETED";
94+
8795
private static readonly IEqualityComparer<IList<string>> _listStringComparer = SequenceEqualityComparer.Create(
8896
EqualityComparer<string>.Default
8997
);
@@ -836,7 +844,17 @@ CancellationToken cancellationToken
836844
return buildDto;
837845
}
838846

839-
public async Task<IReadOnlyList<ServalBuildDto>> GetBuildsAsync(
847+
/// <summary>
848+
/// Gets the builds for the specified project.
849+
/// </summary>
850+
/// <param name="curUserId">The current user identifier.</param>
851+
/// <param name="sfProjectId">The Scripture Forge project identifier.</param>
852+
/// <param name="preTranslate">If <c>true</c>, return NMT builds only; otherwise, return SMT builds.</param>
853+
/// <param name="isServalAdmin">If <c>true</c>, the current user is a Serval Administrator.</param>
854+
/// <param name="cancellationToken">The cancellation token.</param>
855+
/// <returns>The builds.</returns>
856+
/// <remarks>This function is virtual to allow mocking in unit tests.</remarks>
857+
public virtual async Task<IReadOnlyList<ServalBuildDto>> GetBuildsAsync(
840858
string curUserId,
841859
string sfProjectId,
842860
bool preTranslate,
@@ -1415,58 +1433,29 @@ public async Task<IReadOnlyList<DocumentRevision>> GetPreTranslationRevisionsAsy
14151433
CancellationToken cancellationToken
14161434
)
14171435
{
1418-
// Set up the list of revisions to be returned
1419-
List<DocumentRevision> revisions = [];
1420-
14211436
// Ensure that the user has permission
14221437
await EnsureProjectPermissionAsync(curUserId, sfProjectId, isServalAdmin, cancellationToken);
14231438

1424-
await using IConnection connection = await realtimeService.ConnectAsync(curUserId);
1425-
string id = TextDocument.GetDocId(sfProjectId, bookNum, chapterNum, TextDocument.Draft);
1426-
Op[] ops = await connection.GetOpsAsync<TextDocument>(id);
1439+
IReadOnlyList<ServalBuildDto> builds = await GetBuildsAsync(
1440+
curUserId,
1441+
sfProjectId,
1442+
preTranslate: true,
1443+
isServalAdmin,
1444+
cancellationToken
1445+
);
1446+
builds = FilterBuildsByBook(builds, bookNum);
14271447

1428-
// If there are no ops, just get the most recent revision from Serval
1429-
if (ops.Length == 0)
1430-
{
1431-
ServalBuildDto? build = await GetLastCompletedPreTranslationBuildAsync(
1432-
curUserId,
1433-
sfProjectId,
1434-
isServalAdmin,
1435-
cancellationToken
1436-
);
1437-
if (build is not null)
1438-
{
1439-
revisions.Add(
1440-
new DocumentRevision
1441-
{
1442-
Source = OpSource.Draft,
1443-
Timestamp = build.AdditionalInfo?.DateFinished?.UtcDateTime ?? DateTime.UtcNow,
1444-
}
1445-
);
1446-
}
1447-
}
1448-
else
1449-
{
1450-
// Draft Ops are not user created, so we do not need to milestone them,
1451-
// like we do in ParatextService.GetRevisionHistoryAsync()
1452-
foreach (Op op in ops)
1453-
{
1454-
// Allow cancellation
1455-
if (cancellationToken.IsCancellationRequested)
1448+
// Set up the list of revisions to be returned
1449+
List<DocumentRevision> revisions =
1450+
[
1451+
.. builds
1452+
.Where(b => b.AdditionalInfo?.DateFinished is not null)
1453+
.Select(build => new DocumentRevision
14561454
{
1457-
break;
1458-
}
1459-
1460-
revisions.Add(
1461-
new DocumentRevision
1462-
{
1463-
Source = op.Metadata.Source ?? OpSource.Draft,
1464-
Timestamp = op.Metadata.Timestamp,
1465-
UserId = op.Metadata.UserId,
1466-
}
1467-
);
1468-
}
1469-
}
1455+
Source = OpSource.Draft,
1456+
Timestamp = build.AdditionalInfo?.DateFinished?.UtcDateTime ?? DateTime.UtcNow,
1457+
}),
1458+
];
14701459

14711460
// Display the revisions in descending order to match the history API endpoint
14721461
revisions.Reverse();
@@ -1547,6 +1536,15 @@ CancellationToken cancellationToken
15471536
await using IConnection connection = await realtimeService.ConnectAsync(userId);
15481537
string id = TextDocument.GetDocId(sfProjectId, bookNum, chapterNum, TextDocument.Draft);
15491538

1539+
DateTime latestTimestampForRevision = await LatestTimestampForRevisionAsync(
1540+
curUserId,
1541+
sfProjectId,
1542+
bookNum,
1543+
isServalAdmin,
1544+
timestamp,
1545+
cancellationToken
1546+
);
1547+
15501548
// First, see if the document exists in the realtime service, if the chapter is not 0
15511549
IDocument<TextDocument>? textDocument = null;
15521550
if (chapterNum != 0 && draftUsfmConfig is null)
@@ -1555,7 +1553,10 @@ CancellationToken cancellationToken
15551553
if (textDocument.IsLoaded)
15561554
{
15571555
// Retrieve the snapshot if it exists
1558-
Snapshot<TextDocument> snapshot = await connection.FetchSnapshotAsync<TextDocument>(id, timestamp);
1556+
Snapshot<TextDocument> snapshot = await connection.FetchSnapshotAsync<TextDocument>(
1557+
id,
1558+
latestTimestampForRevision
1559+
);
15591560
if (snapshot.Data is not null)
15601561
{
15611562
return snapshot.Data;
@@ -2263,6 +2264,72 @@ private static string GetTranslationEngineId(
22632264
return translationEngineId;
22642265
}
22652266

2267+
/// <summary>
2268+
/// Retrieves the latest timestamp for the revision corresponding to the specified timestamp.
2269+
/// </summary>
2270+
/// <param name="curUserId">The current user identifier.</param>
2271+
/// <param name="sfProjectId">The Scripture Forge project identifier.</param>
2272+
/// <param name="bookNum">The book number.</param>
2273+
/// <param name="isServalAdmin">If <c>true</c>, the current user is a Serval Administrator.</param>
2274+
/// <param name="timestamp">The timestamp to retrieve the timestamp of the closest revision for.</param>
2275+
/// <param name="cancellationToken">The cancellation token.</param>
2276+
/// <returns>The timestamp of the draft that immediate follows the intended draft revision.</returns>
2277+
/// <remarks>This function is internal so it can be unit tests.</remarks>
2278+
internal async Task<DateTime> LatestTimestampForRevisionAsync(
2279+
string curUserId,
2280+
string sfProjectId,
2281+
int bookNum,
2282+
bool isServalAdmin,
2283+
DateTime timestamp,
2284+
CancellationToken cancellationToken
2285+
)
2286+
{
2287+
IReadOnlyList<ServalBuildDto> builds = await GetBuildsAsync(
2288+
curUserId,
2289+
sfProjectId,
2290+
preTranslate: true,
2291+
isServalAdmin,
2292+
cancellationToken
2293+
);
2294+
builds = FilterBuildsByBook(builds, bookNum);
2295+
2296+
// See if there is a build that was requested after the timestamp
2297+
DateTimeOffset? time = builds
2298+
.FirstOrDefault(b => b.AdditionalInfo?.DateRequested?.UtcDateTime > timestamp)
2299+
?.AdditionalInfo?.DateRequested;
2300+
2301+
// If not, search for a build that comes before the timestamp and use the current time if the build exists
2302+
time ??= builds.LastOrDefault(b => b.AdditionalInfo?.DateRequested?.UtcDateTime < timestamp) is not null
2303+
? DateTime.UtcNow
2304+
: null;
2305+
2306+
// Return the latest time to access a draft, or the original timestamp is none is found
2307+
return time?.UtcDateTime ?? timestamp;
2308+
}
2309+
2310+
/// <summary>
2311+
/// Filters a list of builds to only those that contain the specified book number in their translation scripture ranges.
2312+
/// </summary>
2313+
/// <param name="builds">The builds.</param>
2314+
/// <param name="bookNum">The book number.</param>
2315+
/// <returns>The builds containing the specified book.</returns>
2316+
private static IReadOnlyList<ServalBuildDto> FilterBuildsByBook(IReadOnlyList<ServalBuildDto> builds, int bookNum)
2317+
{
2318+
// As we are only parsing books, we do not need to set the versification
2319+
ScriptureRangeParser scriptureRangeParser = new ScriptureRangeParser();
2320+
return
2321+
[
2322+
.. builds.Where(b =>
2323+
b.State == BuildStateCompleted
2324+
&& (
2325+
b.AdditionalInfo?.TranslationScriptureRanges.Any(r =>
2326+
scriptureRangeParser.GetChapters(r.ScriptureRange).ContainsKey(Canon.BookNumberToId(bookNum))
2327+
) ?? false
2328+
)
2329+
),
2330+
];
2331+
}
2332+
22662333
/// <summary>
22672334
/// This method maps Serval API exceptions to the exceptions that Machine.js understands.
22682335
/// </summary>
@@ -2412,9 +2479,9 @@ private static ServalEngineDto UpdateDto(ServalEngineDto engineDto, string sfPro
24122479
/// Ensures that the user has permission to access Serval and the project.
24132480
/// </summary>
24142481
/// <param name="curUserId">The current user identifier.</param>
2415-
/// <param name="sfProjectId"></param>
2482+
/// <param name="sfProjectId">The Scripture Forge project identifier.</param>
24162483
/// <param name="isServalAdmin">If <c>true</c>, the current user is a Serval Administrator.</param>
2417-
/// <param name="cancellationToken">The cancellatioon token.</param>
2484+
/// <param name="cancellationToken">The cancellation token.</param>
24182485
/// <returns>The project.</returns>
24192486
/// <exception cref="DataNotFoundException">The project does not exist.</exception>
24202487
/// <exception cref="ForbiddenException">

0 commit comments

Comments
 (0)