SF-3757 Simplify draft sources configuration page#3763
SF-3757 Simplify draft sources configuration page#3763
Conversation
151ad27 to
e0e3d80
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3763 +/- ##
==========================================
+ Coverage 81.27% 81.28% +0.01%
==========================================
Files 622 622
Lines 39322 39317 -5
Branches 6391 6416 +25
==========================================
+ Hits 31958 31959 +1
+ Misses 6379 6363 -16
- Partials 985 995 +10 ☔ View full report in Codecov by Sentry. |
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 12 comments.
Reviewable status: 0 of 25 files reviewed, 6 unresolved discussions.
scripts/db_tools/parse-version.ts line 36 at r1 (raw file):
'Use In-Process Machine for Suggestions', 'Use Serval for Suggestions', 'Allow Echo for Pre-Translation Drafting',
The changes here are to bring the strings in line with how they're written in the feature flag service.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.html line 100 at r1 (raw file):
></app-project-select> } <p class="no-bottom-margin">
The bottom margin was only hidden because the training-data-multi-select component created padding it shouldn't have created. Once that was fixed, this needed to be removed.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 5 at r1 (raw file):
<app-notice class="experimental-notice" type="primary" mode="fill-dark" icon="science"> This is an experimental simplified page for configuring sources. Please
This text is intentionally not localized as it will only exist while this is available as an experimental feature.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 10 at r1 (raw file):
<mat-expansion-panel class="introduction"> <mat-expansion-panel-header>How to configure draft sources</mat-expansion-panel-header>
This intro area is intentionally not localized as we are nowhere close to settled on wording.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/supported-back-translation-languages-dialog/supported-back-translation-languages-dialog.component.ts line 11 at r1 (raw file):
@Component({ selector: 'app-supported-back-translation-languages-dialog', imports: [MatIcon, MatIconButton, MatDialogTitle, MatDialogContent, MatDialogClose, TranslocoModule],
The close button for the dialog did not have the correct appearance because this module wasn't included.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/training-data/training-data-multi-select.component.html line 2 at r1 (raw file):
<ng-container *transloco="let t; read: 'training_data_multi_select'"> @if (availableTrainingData.length > 0) {
The mat-list had padding or margin on top and bottom, which created spacing even when the list is empty. This creaetd challenges with proper spacing in the parent component.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 420 at r1 (raw file):
"unknown_language_code": "unknown" }, "new_configure_sources": {
I wasn't completely certain how to handle this. We're mostly keeping the same wording as the existing component, but two strings have been edited, so I put them in their own section.
...rc/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html
Outdated
Show resolved
Hide resolved
.../src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.ts
Show resolved
Hide resolved
...lientApp/src/xforge-common/experimental-features/experimental-features-dialog.component.html
Outdated
Show resolved
Hide resolved
e0e3d80 to
acd14da
Compare
acd14da to
f344e55
Compare
RaymondLuong3
left a comment
There was a problem hiding this comment.
You will need to updated scripts/parse-version.ts with you new feature flag. Otherwise, the experimental feature service might be handy as we iterate on other designs :)
@RaymondLuong3 reviewed 25 files and all commit messages, made 7 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Nateowami).
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 420 at r1 (raw file):
Previously, Nateowami wrote…
I wasn't completely certain how to handle this. We're mostly keeping the same wording as the existing component, but two strings have been edited, so I put them in their own section.
Yea, it is possible putting it into the original draft_sources section would have been fine. This section should be merged with the original once the new draft sources component is merged.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 38 at r3 (raw file):
The model will create drafts by translating books from the 'Drafting Source Project'. It is rare that the model will give useful results if the Drafting Source Project isn't included in the Source projects for training the model.
This is strange. Instead of telling me why it is bad to not use the source project, it would be better to tell me the advantages of using the source project for drafting.
Code quote:
The model will create drafts by translating books from the 'Drafting Source Project'. It is rare that the model
will give useful results if the Drafting Source Project isn't included in the Source projects for training the
model.src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 43 at r3 (raw file):
<p> You will be able to look at the drafts before you need to decide how to use them. Scripture Forge will not automatically add the generated drafts to any project. They will only be available in Scripture Forge until you
Automatically is not needed.
Code quote:
automatically add the generated drafts to any projectsrc/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.scss line 35 at r3 (raw file):
margin-bottom: 0em; } h3 + p {
Nice, I haven't seen this syntax before
Code quote:
h3 + p {src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.ts line 14 at r3 (raw file):
import { TrainingData } from 'realtime-server/lib/esm/scriptureforge/models/training-data'; import { filter, merge, Subscription } from 'rxjs'; import { NoticeComponent } from 'src/app/shared/notice/notice.component';
Nit: this could be a relative path
Code quote:
import { NoticeComponent } from 'src/app/shared/notice/notice.component';src/SIL.XForge.Scripture/ClientApp/src/xforge-common/experimental-features/experimental-features-dialog.component.scss line 28 at r3 (raw file):
::ng-deep .mdc-label { display: block;
I don't see this is doing anything.
Code quote:
display: block;f344e55 to
fac7eca
Compare
fac7eca to
5d563b0
Compare
Nateowami
left a comment
There was a problem hiding this comment.
parse-version has already been updated.
@Nateowami made 6 comments and resolved 2 discussions.
Reviewable status: 23 of 25 files reviewed, 3 unresolved discussions (waiting on RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 38 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
This is strange. Instead of telling me why it is bad to not use the source project, it would be better to tell me the advantages of using the source project for drafting.
This wording was an initial draft. I've just removed it
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.html line 43 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Automaticallyis not needed.
Ditto.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.scss line 35 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Nice, I haven't seen this syntax before
It means a paragraph that comes right after an h3
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.ts line 14 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Nit: this could be a relative path
...why?
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/experimental-features/experimental-features-dialog.component.scss line 28 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I don't see this is doing anything.
Done.
...lientApp/src/app/translate/draft-generation/configure-sources/configure-sources.component.ts
Show resolved
Hide resolved
| removedFiles.forEach(f => this.trainingDataService.deleteTrainingDataAsync(f)); | ||
| addedFiles.forEach(f => this.trainingDataService.createTrainingDataAsync(f)); |
There was a problem hiding this comment.
🟡 Unawaited async promises in save() cause silent failures for training data operations
In save(), deleteTrainingDataAsync and createTrainingDataAsync return Promises that are called inside forEach without being awaited. If any of these operations fail, the error becomes an unhandled promise rejection while the settings update on line 390-393 proceeds regardless, potentially leaving training data in an inconsistent state with the saved project settings. This is the same pattern as the existing draft-sources.component.ts:399-400, but it's newly introduced code in this PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not fixing any bugs in the original
| } | ||
|
|
||
| @Injectable({ providedIn: 'root' }) | ||
| export class ExperimentalFeaturesService { |
There was a problem hiding this comment.
🟡 Missing class comment on ExperimentalFeaturesService (AGENTS.md violation)
AGENTS.md states: "All classes and interfaces should have a comment to briefly explain why it is there and what its purpose is in the overall system, even if it seems obvious. Please do not fail to add a comment to any classes or interfaces that are created." The new ExperimentalFeaturesService class has no JSDoc comment describing its purpose.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I'm not an agent; I don't have to follow the agent rules.
| FormsModule | ||
| ] | ||
| }) | ||
| export class ExperimentalFeaturesDialogComponent { |
There was a problem hiding this comment.
🟡 Missing class comment on ExperimentalFeaturesDialogComponent (AGENTS.md violation)
AGENTS.md states: "All classes and interfaces should have a comment to briefly explain why it is there and what its purpose is in the overall system, even if it seems obvious." The new ExperimentalFeaturesDialogComponent class has no JSDoc comment describing its purpose.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I'm not an agent; I don't have to follow the agent rules.
| } | ||
| } | ||
|
|
||
| export interface DraftSourcesSettingsChange { |
There was a problem hiding this comment.
🟡 Missing interface comment on DraftSourcesSettingsChange in new component
AGENTS.md mandates: "Please do not fail to add a comment to any classes or interfaces that are created. All classes and interfaces should have a comment." The DraftSourcesSettingsChange interface at line 461 of the new component has no doc comment.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I'm not an agent; I don't have to follow the agent rules.
RaymondLuong3
left a comment
There was a problem hiding this comment.
It looks good. This PR will not make the new form active, it will be available through the experimental features. Is that the desire to make this active in a following PR?
@RaymondLuong3 reviewed 2 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on Nateowami).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.ts line 14 at r3 (raw file):
Previously, Nateowami wrote…
...why?
Just for consistency with the other imports.
5d563b0 to
7934c74
Compare
Nateowami
left a comment
There was a problem hiding this comment.
Yes
@Nateowami made 8 comments.
Reviewable status: 22 of 26 files reviewed, 6 unresolved discussions (waiting on RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/new-configure-sources/new-configure-sources.component.ts line 14 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Just for consistency with the other imports.
Somehow I missed that we were that consistent with doing this. I also updated the one other place where we use 'src/
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 420 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Yea, it is possible putting it into the original
draft_sourcessection would have been fine. This section should be merged with the original once the new draft sources component is merged.
I wanted to keep the strings separate so it's easy to see what we're adding. If we merge the components, we'll merge the strings.
...lientApp/src/app/translate/draft-generation/configure-sources/configure-sources.component.ts
Show resolved
Hide resolved
| removedFiles.forEach(f => this.trainingDataService.deleteTrainingDataAsync(f)); | ||
| addedFiles.forEach(f => this.trainingDataService.createTrainingDataAsync(f)); |
There was a problem hiding this comment.
Not fixing any bugs in the original
| } | ||
| } | ||
|
|
||
| export interface DraftSourcesSettingsChange { |
There was a problem hiding this comment.
I'm not an agent; I don't have to follow the agent rules.
| FormsModule | ||
| ] | ||
| }) | ||
| export class ExperimentalFeaturesDialogComponent { |
There was a problem hiding this comment.
I'm not an agent; I don't have to follow the agent rules.
| } | ||
|
|
||
| @Injectable({ providedIn: 'root' }) | ||
| export class ExperimentalFeaturesService { |
There was a problem hiding this comment.
I'm not an agent; I don't have to follow the agent rules.
RaymondLuong3
left a comment
There was a problem hiding this comment.
@RaymondLuong3 reviewed 4 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Nateowami).
|
This looks good. Can you add acceptance tests to the issue and move it into the ready to test column? |
7934c74 to
21e5c92
Compare
...ge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts
Outdated
Show resolved
Hide resolved
| public experimentalFeatures: ExperimentalFeature[] = [ | ||
| // { | ||
| // name: 'New configure sources page', | ||
| // description: `A new configure sources page is available for testing. It has the same functionality as the current page, but with an updated design and some additional information to help users understand the options.`, | ||
| // available: () => this.doesUserHaveRoleOnAnyProject(SFProjectRole.ParatextAdministrator), | ||
| // featureFlag: this.featureFlagService.newConfigureSourcesPage | ||
| // } | ||
| ]; |
There was a problem hiding this comment.
🚩 ExperimentalFeaturesService is infrastructure-only with no active features
The experimentalFeatures array is empty (with commented-out code referencing a future newConfigureSourcesPage feature flag). This means showExperimentalFeaturesInMenu always returns false, and the 'Experimental features' menu item in app.component.html:152-156 will never be shown. The constructor parameters are also commented out. This is clearly deliberate scaffolding for a future feature, but it means the new ExperimentalFeaturesDialogComponent, its test, localization strings, and the menu button are all dead code until a feature is added to the array. Reviewers should confirm this is intentional scaffolding versus an incomplete implementation.
Was this helpful? React with 👍 or 👎 to provide feedback.
21e5c92 to
721489c
Compare
721489c to
0d17bbc
Compare
|
Sorry to change so many things on you @RaymondLuong3. The latest plan is this will not be an experimental feature, but rather a normal change. I've also made some significant adjustments to the layout of the component so the inputs are aligned both vertically and horizontally. |
0d17bbc to
62d6b92
Compare
| <button | ||
| mat-button | ||
| (click)="trainingSources.push(undefined); $event.preventDefault()" | ||
| class="add-another-project" | ||
| > | ||
| <mat-icon>add</mat-icon> {{ t("add_another_reference_project") }} | ||
| </button> | ||
| } |
There was a problem hiding this comment.
🚩 Stepper removal means empty project-select slots can no longer be cleaned up
The old goToStep() method cleaned up undefined entries from trainingSources/draftingSources arrays when navigating between steps. With the stepper removed, there's no cleanup mechanism. If a user clicks "Add another reference project" but doesn't select anything, the empty slot persists with no way to remove it (the "Add another reference project" button disappears because allowAddingATrainingSource checks trainingSources.length < 2). This doesn't cause a functional bug — save() filters out undefined values with filter(notNull) — but it's a minor UX regression where users can't undo adding an empty slot.
Was this helpful? React with 👍 or 👎 to provide feedback.
This PR creates a new, simplified page for configuring draft sources.
I initially made changes to the draft sources component, moving the content from the left, while keeping the layout of the overview on the right. See diagram below:
During a recent meeting we discussed the best way to get user feedback before rolling this out. It was decided to create a new "Experimental features" system that allows users to turn it on, so that it can be tested with real users and real projects before we roll it out.
I created an experimental features service and dialog that merely exposes certain feature flags through a different UI to allow users to turn them on or off. When no experimental features are available, it's not available in the menu. Some features (such as the new configure sources page) are only available to users that are an admin on at least one project.
Mostly I deleted a lot of code, especially in the template and styles, plus a few things in the component TS file that were no longer used by the template. Unfortunately, copying the component makes it a lot harder for a reviewer to tell what was changed.
This change is