SF-3817 Partial book drafting#3964
Conversation
Nateowami
added
will require testing
Nateowami
temporarily deployed
to
screenshot_diff
GitHub Actions
Inactive
|
📸 Screenshot diff deployed! (25 changes) View the visual diff at: https://pr-3964--sf-screenshot-diffs.netlify.app |
Nateowami
left a comment
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 1 comment.
Reviewable status: 0 of 75 files reviewed, 1 unresolved discussion.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft-logic-handler.ts line 151 at r1 (raw file):
* Overridable on the class (NewDraftLogicHandler.ALLOW_DRAFTING_BOOKS_NOT_IN_TARGET) so tests can exercise both branches. */ static ALLOW_DRAFTING_BOOKS_NOT_IN_TARGET = false;
Once #3965 is merged (likely before this PR is merged) this can be changed ripped out and all code paths that depend on it can assume it's always true.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “New Draft” wizard/stepper to support partial-book (chapter-level) drafting & training behind an experimental feature flag, while keeping the legacy draft-generation flow intact. It also extends the backend build/config plumbing to accept an explicit target training scripture range and persists additional client selection state (available training data files), plus enhances progress reporting to include chapter-level detail.
Changes:
- Added a new draft-generation wizard flow (UI + supporting utilities), plus an E2E workflow covering partial-book drafting.
- Updated build configuration handling end-to-end (JSON converter, API persistence, training-range derivation) to support explicit target training ranges and “available training files”.
- Extended project progress reporting to return chapter-level progress, and updated client code to consume/format chapter-level draft ranges.
Reviewed changes
Copilot reviewed 75 out of 75 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/SIL.XForge.Scripture.Tests/Services/MachineProjectServiceTests.cs | Adds test for explicit target training range behavior |
| test/SIL.XForge.Scripture.Tests/Services/MachineApiServiceTests.cs | Adds test for persisting available training data files |
| test/SIL.XForge.Scripture.Tests/Services/BuildConfigJsonConverterTests.cs | Adds serialization/deserialization coverage for available training files |
| src/SIL.XForge.Scripture/Services/SFProjectService.cs | Adds chapter-level progress aggregation + mapping |
| src/SIL.XForge.Scripture/Services/MachineProjectService.cs | Supports explicit target training scripture range (fallback for legacy) |
| src/SIL.XForge.Scripture/Services/MachineApiService.cs | Persists last-available training files when reported |
| src/SIL.XForge.Scripture/Services/BuildConfigJsonConverter.cs | Serializes AvailableTrainingDataFiles when present |
| src/SIL.XForge.Scripture/Models/DraftConfig.cs | Adds LastAvailableTrainingDataFiles persisted field |
| src/SIL.XForge.Scripture/Models/BuildConfig.cs | Adds AvailableTrainingDataFiles to request model |
| src/SIL.XForge.Scripture/Models/BookProgress.cs | Adds chapter-level progress DTOs |
| src/SIL.XForge.Scripture/ClientApp/src/xforge-common/util/set-util.ts | Adds set composition helpers (polyfill-style) |
| src/SIL.XForge.Scripture/ClientApp/src/xforge-common/util/set-util.spec.ts | Unit tests for set helpers |
| src/SIL.XForge.Scripture/ClientApp/src/xforge-common/i18n.service.ts | Caches Intl.ListFormat; adds formatToParts helper |
| src/SIL.XForge.Scripture/ClientApp/src/xforge-common/feature-flags/feature-flag.service.ts | Adds PartialBookDrafting feature flag |
| src/SIL.XForge.Scripture/ClientApp/src/xforge-common/experimental-features/experimental-features.service.ts | Adds experimental-features wrapper/service |
| src/SIL.XForge.Scripture/ClientApp/src/xforge-common/experimental-features/experimental-features-dialog.component.ts | Adds experimental features dialog component |
| src/SIL.XForge.Scripture/ClientApp/src/xforge-common/experimental-features/experimental-features-dialog.component.spec.ts | Unit tests for experimental-features dialog |
| src/SIL.XForge.Scripture/ClientApp/src/xforge-common/experimental-features/experimental-features-dialog.component.scss | Styles for experimental-features dialog |
| src/SIL.XForge.Scripture/ClientApp/src/xforge-common/experimental-features/experimental-features-dialog.component.html | Dialog template for toggling experimental features |
| src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json | Adds NewDraft translation keys (non-checking bundle) |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/training-data-summary.ts | Chapter-aware training-book summary formatting |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/training-data-summary.spec.ts | Tests for training-book summary formatting |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/training-data-file-selection.ts | Default selection logic for training data files |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/training-data-file-selection.spec.ts | Tests for default training-file selection logic |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.stories.ts | Storybook coverage for NewDraft wizard states |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.scss | Styles for NewDraft wizard |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.html | NewDraft wizard template (steps, chapter inputs, summary) |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/draft-pending-updates/draft-pending-updates.component.ts | Adds pre-step interstitial to sync pending updates |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/draft-pending-updates/draft-pending-updates.component.stories.ts | Storybook scenarios for pending-updates interstitial |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/draft-pending-updates/draft-pending-updates.component.spec.ts | Unit tests for pending-updates interstitial logic |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/draft-pending-updates/draft-pending-updates.component.scss | Styles for pending-updates interstitial |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/draft-pending-updates/draft-pending-updates.component.html | Template for pending-updates interstitial |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources.service.ts | Filters out unloaded targetDoc emissions to avoid empty latch |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources.service.spec.ts | Updates tests for new filter behavior |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-source.ts | Dedupes copyright banners; adds helper |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html | Adjusts BookMultiSelect usage (projectName binding removed) |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.ts | Displays drafted chapter ranges; navigates to first drafted chapter |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.spec.ts | Tests for partial-book preview behavior |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.html | Shows drafted chapter range in book button |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts | Imports only drafted chapters; scopes overwrite warnings |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.spec.ts | Tests for partial-draft import scoping |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.ts | Extends frontend BuildConfig with availableTrainingDataFiles |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts | Prepends \id when missing for partial chapter downloads |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.spec.ts | Tests for \id prepending behavior |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.ts | Routes to NewDraft when feature flag enabled |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts | Fixes/updates assertion around existing draft run behavior |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.ts | Reuses shared “book appears complete” logic + copyright helper |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.scss | Makes h1 margin configurable via CSS var |
| src/SIL.XForge.Scripture/ClientApp/src/app/shared/utils.ts | Changes expandNumbers to use ChapterSet + return null on invalid |
| src/SIL.XForge.Scripture/ClientApp/src/app/shared/utils.spec.ts | Updates expandNumbers expectations for invalid input |
| src/SIL.XForge.Scripture/ClientApp/src/app/shared/scripture-range.ts | Adds ChapterSet + VerboseScriptureRange chapter-level model |
| src/SIL.XForge.Scripture/ClientApp/src/app/shared/scripture-range.spec.ts | Tests for ChapterSet + VerboseScriptureRange |
| src/SIL.XForge.Scripture/ClientApp/src/app/shared/progress-service/progress.service.ts | Adds chapter progress types; improves request coalescing semantics |
| src/SIL.XForge.Scripture/ClientApp/src/app/shared/progress-service/progress.service.spec.ts | Updates tests for new progress types and refresh behavior |
| src/SIL.XForge.Scripture/ClientApp/src/app/shared/book-multi-select/book-multi-select.component.ts | Avoids repeated progress fetches; prevents change-detection loops |
| src/SIL.XForge.Scripture/ClientApp/src/app/shared/book-multi-select/book-multi-select.component.spec.ts | Adds tests for progress-fetch caching behavior |
| src/SIL.XForge.Scripture/ClientApp/src/app/shared/book-multi-select/book-multi-select.component.html | Tweaks header rendering conditions and scope selection block |
| src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.ts | Reuses ChapterSet for compact range formatting |
| src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-builds.component.spec.ts | Minor test adjustment (explicit expect().nothing()) |
| src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-build-report.ts | Treats invalid chapter notation as whole-book fallback |
| src/SIL.XForge.Scripture/ClientApp/src/app/core/sf-project.service.ts | Updates progress API typing + expandNumbers null-handling |
| src/SIL.XForge.Scripture/ClientApp/src/app/app.routes.ts | Adds route for NewDraft wizard |
| src/SIL.XForge.Scripture/ClientApp/src/app/app.component.ts | Adds experimental-features dialog wiring |
| src/SIL.XForge.Scripture/ClientApp/src/app/app.component.html | Adds menu item to open experimental features when available |
| src/SIL.XForge.Scripture/ClientApp/e2e/workflows/partial-book-drafting.ts | Adds E2E workflow for partial-book drafting |
| src/SIL.XForge.Scripture/ClientApp/e2e/test-definitions.ts | Registers partial-book drafting E2E test |
| src/SIL.XForge.Scripture/ClientApp/e2e/test_data/partial_draft_training_1.tsv | Adds E2E training-data fixture |
| src/SIL.XForge.Scripture/ClientApp/e2e/test_data/partial_draft_training_2.tsv | Adds E2E training-data fixture |
| src/SIL.XForge.Scripture/ClientApp/e2e/test_characterization.json | Adds characterization stats for new E2E test |
| src/SIL.XForge.Scripture/ClientApp/e2e/characterize-tests.mts | Adjusts characterization retry threshold |
| src/RealtimeServer/scriptureforge/models/translate-config.ts | Adds lastAvailableTrainingDataFiles to DraftConfig interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } @empty { | ||
| <p>No experimental features are currently available.</p> | ||
| } |
There was a problem hiding this comment.
This was deliberate because the dialog should never be shown unless there are actually experimental features available. However, since the dialog can't enforce that itself, I defined the empty state for completeness. I don't want to waste translators' time by having them translate strings users won't actually see.
| if (this.featureFlags.partialBookDrafting.enabled) { | ||
| const projectId = this.activatedProject.projectId; | ||
| if (projectId != null) { | ||
| await this.router.navigate(['/projects', projectId, 'draft-generation', 'new-draft']); | ||
| } | ||
| } else { | ||
| this.currentPage = 'steps'; | ||
| } |
There was a problem hiding this comment.
This is merely theoretical. It shouldn't be possible to be on this page and have this.activatedProject.projectId be nullish except in a very weird edge case/race condition. And if we reach that, this function won't crash, and the button is the least of our worries.
| async initBookOptions(): Promise<void> { | ||
| // Only load progress if not in basic mode | ||
| let progress: ProjectProgress | undefined; | ||
| if (this.basicMode === false) { | ||
| // projectId may arrive asynchronously; show loading state until it arrives. | ||
| if (this.projectId == null) { | ||
| throw new Error('app-book-multi-select requires a projectId input to initialize when not in basic mode'); | ||
| this.loaded = false; | ||
| return; | ||
| } | ||
| if (this.projectId !== this.loadedProgressProjectId) { | ||
| this.cachedProgress = await this.progressService.getProgress(this.projectId, { maxStalenessMs: 30_000 }); | ||
| this.loadedProgressProjectId = this.projectId; | ||
| } | ||
| progress = await this.progressService.getProgress(this.projectId, { maxStalenessMs: 30_000 }); | ||
| progress = this.cachedProgress; |
There was a problem hiding this comment.
This is really quite an edge case (project ID really shouldn't change), but the fix is simple so I've added a followup commit.
Nateowami
force-pushed
the
feature/SF-3817-partial-book-drafting
branch
from
50de8dd to
5a323bc
Compare
Nateowami
temporarily deployed
to
screenshot_diff
GitHub Actions
Inactive
Nateowami
temporarily deployed
to
screenshot_diff
GitHub Actions
Inactive
Nateowami
force-pushed
the
feature/SF-3817-partial-book-drafting
branch
from
d33d165 to
0baa9a6
Compare
Nateowami
temporarily deployed
to
screenshot_diff
GitHub Actions
Inactive
RaymondLuong3
left a comment
RaymondLuong3
left a comment
There was a problem hiding this comment.
I am about halfway through this review. Nothing major to point out.
@RaymondLuong3 partially reviewed 41 files and made 7 comments.
Reviewable status: 40 of 75 files reviewed, 10 unresolved discussions (waiting on Nateowami).
src/SIL.XForge.Scripture/ClientApp/e2e/workflows/partial-book-drafting.ts line 156 at r1 (raw file):
await chapterInput.fill('23-21'); await chapterInput.blur(); await expect(page.locator('.partial-book-drafting-table .chapter-error')).toBeVisible();
Nit: to directly contrast between this and 3 lines below, this could be written as .toHaveCount(1).
Code quote:
.toBeVisible();src/SIL.XForge.Scripture/ClientApp/src/app/shared/scripture-range.ts line 6 at r1 (raw file):
export class ChapterSet { static chapterRangeSeparator = ','; static chapterRangeStartEndSeparator = '-';
These need to be readonly too.
Code quote:
static chapterRangeSeparator = ',';
static chapterRangeStartEndSeparator = '-';src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 320 at r1 (raw file):
this.draftedScriptureRange = VerboseScriptureRange.fromCombinedRanges( (this.data.additionalInfo?.translationScriptureRanges ?? []).map(range => range.scriptureRange) );
I think this initializes a draftedScriptureRange with no chapters instead of all chapters assuming that translationScriptureRanges returns the previous ScriptureRange that does not require specific chapters. I see below it accounts for this an the empty ChapterSet is treated as book level drafting.
Code quote:
this.draftedScriptureRange = VerboseScriptureRange.fromCombinedRanges(
(this.data.additionalInfo?.translationScriptureRanges ?? []).map(range => range.scriptureRange)
);src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft-logic-handler.ts line 232 at r1 (raw file):
/** * Sets up the state by loading the project, checking for changes in Paratext that haven't synced to SF yet, loading * progress data, and setting up subscripts that watch for changes that should result in bailing out (forcing the
Nit: Should say subscriptions
Code quote:
setting up subscripts src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft-logic-handler.ts line 308 at r1 (raw file):
const staleness = (id: string): { maxStalenessMs?: number } => freshProjectIds.has(id) ? { maxStalenessMs: 0 } : {};
This syntax is really hard for me to understand. It looks like the type for staleness is a function that accepts an projectId and returns an object with maxStalenessMs. Can this be made into a function of the class?
Code quote:
const staleness = (id: string): { maxStalenessMs?: number } =>
freshProjectIds.has(id) ? { maxStalenessMs: 0 } : {};src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft-logic-handler.ts line 513 at r1 (raw file):
sourceChapterCount >= MIN_SOURCE_CHAPTERS_FOR_PARTIAL_DRAFTING && targetChaptersWithContent != null && targetChaptersWithContent >= 1
This looks like we only allow books for target drafting if the book has content in at least one chapter. I understand it here, but it might be tricky communicating with users why some books allows partial drafting and others do not. Just noting that here.
Code quote:
targetChaptersWithContent >= 1
Nateowami
temporarily deployed
to
screenshot_diff
GitHub Actions
Inactive
Nateowami
temporarily deployed
to
screenshot_diff
GitHub Actions
Inactive
Nateowami
temporarily deployed
to
screenshot_diff
GitHub Actions
Inactive
Nateowami
temporarily deployed
to
screenshot_diff
GitHub Actions
Inactive
Nateowami
left a comment
Nateowami
left a comment
There was a problem hiding this comment.
Thank you so much for reviewing Raymond. This PR is basically a marathon to review.
@Nateowami made 7 comments and resolved 1 discussion.
Reviewable status: 34 of 80 files reviewed, 9 unresolved discussions (waiting on RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/e2e/workflows/partial-book-drafting.ts line 156 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Nit: to directly contrast between this and 3 lines below, this could be written as .toHaveCount(1).
Done
src/SIL.XForge.Scripture/ClientApp/src/app/shared/scripture-range.ts line 6 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
These need to be readonly too.
Good point. Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 320 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I think this initializes a draftedScriptureRange with no chapters instead of all chapters assuming that translationScriptureRanges returns the previous ScriptureRange that does not require specific chapters. I see below it accounts for this an the empty ChapterSet is treated as book level drafting.
Yes, that's correct. We store translationScriptureRanges as an array (even though there's really only one range; theoretically in the future we could draft from multiple projects), then join them together.
It's really quite ugly that an empty set is treated as meaning "everything", even though as it's used here the logic all holds together.
It might be less ugly if we represented "chapters not specified" as a null or undefined ChapterSet (or defined the ChapterSet type as something like type ChapterSet = Set<number> | 'all_chapters', But then the difference set operation will be undefined (You can't do (all chapters in Genesis) - (Genesis 14-28) and get an answer without knowing the number of chapters in the project the chapters are in. In practice though, there are only two calls to .difference, both in the same method, so probably I should have just handled the difficult edge case there. I'm not sure. Alternatively, we could have ScriptureRange and VerboseScriptureRange, where VerboseScriptureRange can be converted to ScriptureRange, but you can't convert the other way, and difference is only defined on VerboseScriptureRange.
I'm open to making a change if we can figure out what the actual best approach would be.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft-logic-handler.ts line 232 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Nit: Should say subscriptions
Good catch. Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft-logic-handler.ts line 308 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
This syntax is really hard for me to understand. It looks like the type for staleness is a function that accepts an projectId and returns an object with maxStalenessMs. Can this be made into a function of the class?
I realize it's a bit ugly, but I think it's the most elegant solution. It's a local helper function that takes an id arg, and also can access freshProjectIds, which is a local variable. If it were a method on the class it would have to take two arguments. And nothing outside this method would care about it.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft-logic-handler.ts line 513 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
This looks like we only allow books for target drafting if the book has content in at least one chapter. I understand it here, but it might be tricky communicating with users why some books allows partial drafting and others do not. Just noting that here.
Yes, that's a design decision that has tradeoffs. The reason is that we don't want to offer partial book drafting to users unless there might be a reason for them to use it, in the interest of keeping the UI simple when possible.
I do think we might need to change the logic somewhat; there's a threshold for "does this chapter have enough content to be drafted or used for training", which is quite minimal. If you have a few verses and you want to draft them, or use them for training, we're not going to stop you.
But maybe it shouldn't be the same threshold for deciding what should be selected by default. If a chapter has only a couple verses, it probably makes more sense to draft it than to keep it as training data. I haven't fully worked out whether that's the right option or not. The spec we came up with before I implemented was pretty clear about what to do with chapters with content vs not content, but less clear on drawing the exact lines for measuring content.
Nateowami
force-pushed
the
feature/SF-3817-partial-book-drafting
branch
from
43da4d3 to
1c6ad39
Compare
Nateowami
temporarily deployed
to
screenshot_diff
GitHub Actions
Inactive
RaymondLuong3
left a comment
RaymondLuong3
left a comment
There was a problem hiding this comment.
Thanks for taking out the gate for limiting drafting books to the books available in the target. All this code review has made me hungry :O
@RaymondLuong3 reviewed 48 files and all commit messages, made 10 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on Nateowami).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.html line 43 at r3 (raw file):
} <button mat-button class="configure-sources-link" (click)="goToConfigureSources()"> <mat-icon>edit</mat-icon>{{ t("change_source_configuration") }}
I think I prefer the old gear icon. I think it communicates settings that can be changed. Did you have a reason for updating it to use the pencil icon? We should make this consistent.
Code quote:
<mat-icon>edit</mat-icon>{{ t("change_source_configuration") }}src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.html line 175 at r3 (raw file):
@if (targetTrainingChapterErrors.get(bookId); as error) { <span class="chapter-error">{{ t(error.key, error.params) }}</span> }
I like that it is really easy on a big screen for this layout to work, but on smaller screen showing errors next the the input elements is more tricky. I think putting errors beneath the inputs makes sense and will most likely improve this behaviour.
Code quote:
@if (targetTrainingChapterErrors.get(bookId); as error) {
<span class="chapter-error">{{ t(error.key, error.params) }}</span>
}src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.html line 228 at r3 (raw file):
</div> <button mat-button color="primary" class="card-header-action" (click)="goToPage('draft_books')"> <mat-icon>edit</mat-icon>{{ t("summary.choose_different_books") }}
Now I see the advantage of now showing the linear progression stepper. I like that we can just directly to the step we want to fix while still showing the current step and progress.
Code quote:
<mat-icon>edit</mat-icon>{{ t("summary.choose_different_books") }}src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.ts line 64 at r3 (raw file):
{ page: 'draft_books', inputState: 'draft_books' }, { page: 'training_books', inputState: 'training_books' }, { page: 'suffix' }
I think 'summary' is a better term. Suffix normally refers to additions to the end, which could apply here, but it's not the typical meaning of the word.
Code quote:
{ page: 'suffix' }src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.ts line 106 at r3 (raw file):
fastTraining: boolean = false; useEcho: boolean = false; isTrainingOptional: boolean = false;
This can be private
Code quote:
isTrainingOptional: boolean = false;src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.ts line 114 at r3 (raw file):
private hasInitializedTrainingDataSelection = false; // Data that is guarnateed to be loaded post init
typo
Code quote:
// Data that is guarnateed to be loaded post initsrc/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.ts line 266 at r3 (raw file):
get selectedTrainingDataFileTitles(): string[] { return this.trainingDataFiles.filter(f => this.selectedTrainingDataFileIds.has(f.dataId)).map(f => f.title); }
I like having getters grouped together so I can quickly scan a class and see what pieces of information I can get from the component.
Code quote:
/** Titles of the training data files the user has selected, for the summary recap. */
get selectedTrainingDataFileTitles(): string[] {
return this.trainingDataFiles.filter(f => this.selectedTrainingDataFileIds.has(f.dataId)).map(f => f.title);
}src/SIL.XForge.Scripture/ClientApp/src/xforge-common/feature-flags/feature-flag.service.ts line 368 at r3 (raw file):
); readonly partialBookDrafting: FeatureFlag = new FeatureFlagFromStorage(
We should add this feature to parse-version.ts
Code quote:
readonly partialBookDrafting: FeatureFlag = new FeatureFlagFromStorage(src/SIL.XForge.Scripture/Models/DraftConfig.cs line 17 at r3 (raw file):
/// builds made before this was recorded (an empty list means a build recorded that zero files were available). /// </summary> public IList<string>? LastAvailableTrainingDataFiles { get; set; }
Adding a new property to the DraftConfig model means that the schema needs to be updated in sf-project-service.ts
Code quote:
public IList<string>? LastAvailableTrainingDataFiles { get; set; }
|
@pmachapman Would you mind reviewing the c# files. I am not be able to pick out nuances in the serval integration layer was well as you can. |
pmachapman
left a comment
pmachapman
left a comment
There was a problem hiding this comment.
Would you mind reviewing the c# files. I am not be able to pick out nuances in the serval integration layer was well as you can.
@RaymondLuong3 No problem - just one comment.
@pmachapman reviewed 10 files and made 2 comments.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on Nateowami and RaymondLuong3).
src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2748 at r3 (raw file):
p => p.TranslateConfig.DraftConfig.LastAvailableTrainingDataFiles, [.. buildConfig.AvailableTrainingDataFiles] );
An equality comparer is needed, otherwise an op will be created and written to ShareDB every time this code is run, even if the collection has the same values. One option is to implement a custom version of SIL.ObjectModel.SequenceEqualityComparer that supports nullable lists, or you could make the collection not nullable, and have it created initially via a migrator (like LastSelectedTrainingDataFiles).
Code quote:
// No equality comparer here: the field starts null, and _listStringComparer cannot compare against null.
if (buildConfig.AvailableTrainingDataFiles is not null)
{
op.Set<IList<string>?>(
p => p.TranslateConfig.DraftConfig.LastAvailableTrainingDataFiles,
[.. buildConfig.AvailableTrainingDataFiles]
);
Test team noted that when going to the next step while offline an error was shown due to book-multi-select fetching progress. Since progress is not critical to the functionality, I've changed it to wait until the user is online to fetch it.
Nateowami
force-pushed
the
feature/SF-3817-partial-book-drafting
branch
from
1c6ad39 to
b31e7d4
Compare
Nateowami
left a comment
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 9 comments and resolved 2 discussions.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on pmachapman and RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.html line 43 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I think I prefer the old gear icon. I think it communicates settings that can be changed. Did you have a reason for updating it to use the pencil icon? We should make this consistent.
Good point. Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.html line 175 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I like that it is really easy on a big screen for this layout to work, but on smaller screen showing errors next the the input elements is more tricky. I think putting errors beneath the inputs makes sense and will most likely improve this behaviour.
Moved below. Good catch.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.ts line 64 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I think 'summary' is a better term. Suffix normally refers to additions to the end, which could apply here, but it's not the typical meaning of the word.
I was afraid "summary" and the initial confirmation page ("Review draft setup") would be too easy to mix up, but it's probably not really a problem.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.ts line 106 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
This can be private
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.ts line 114 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
typo
Fixed
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-draft/new-draft.component.ts line 266 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I like having getters grouped together so I can quickly scan a class and see what pieces of information I can get from the component.
The class is mostly organized by feature section rather than by member kind (which I think is way more important for readability). This one was misplaced; moved it into the Summary section.
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/feature-flags/feature-flag.service.ts line 368 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
We should add this feature to
parse-version.ts
Done.
src/SIL.XForge.Scripture/Models/DraftConfig.cs line 17 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Adding a new property to the DraftConfig model means that the schema needs to be updated in
sf-project-service.ts
Done. Thanks.
src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2748 at r3 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
An equality comparer is needed, otherwise an op will be created and written to ShareDB every time this code is run, even if the collection has the same values. One option is to implement a custom version of
SIL.ObjectModel.SequenceEqualityComparerthat supports nullable lists, or you could make the collection not nullable, and have it created initially via a migrator (likeLastSelectedTrainingDataFiles).
Fixed. Making it not nullable would mean initializing with an empty array which breaks the semantics.
The right solution is probably to have a key/value indicating which were selected or not selected, but it's not trivial to migrate to that at this point. (It should get easier once the old stepper is gone and we only have one consumer)
This PR creates a new implementation of the stepper for generating drafts, while leaving the old one alone. I chose an architecture where as much of the business logic as possible is put into NewDraftLogicHandler, where it can be unit tested without UI concerns. It is used by NewDraftComponent to determine which books and chapters are available for training or drafting.
New components
Back end changes
Front end changes
Data model changes
Other
This change is