Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SF-3077 Add UI for mixed training sources #2875

Merged
merged 7 commits into from
Dec 19, 2024

Conversation

RaymondLuong3
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 commented Nov 28, 2024

This PR adds the ability to mix in training data from multiple sources. See the balsamiq for the original design.

No training books selected
Mixed Training No Books Selected

Training books selected
Mixed training current books selected


This change is Reviewable

@pmachapman pmachapman force-pushed the feature/SF-2900 branch 2 times, most recently from d49a457 to 4bd2738 Compare December 2, 2024 19:06
@Nateowami
Copy link
Collaborator

Observations:

  • Bulk selecting NT or OT in the top panel doesn't cause the relevant books to update in the bottom panel

@RaymondLuong3 RaymondLuong3 force-pushed the feature/SF-3077-multiple-source-ui branch from 1ac6820 to d58cf38 Compare December 4, 2024 23:35
@RaymondLuong3 RaymondLuong3 marked this pull request as ready for review December 4, 2024 23:35
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 97.91667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.90%. Comparing base (1897259) to head (7274455).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...neration-steps/draft-generation-steps.component.ts 97.22% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2875      +/-   ##
==========================================
+ Coverage   80.84%   80.90%   +0.05%     
==========================================
  Files         533      533              
  Lines       31122    31193      +71     
  Branches     5061     5060       -1     
==========================================
+ Hits        25161    25236      +75     
- Misses       5198     5208      +10     
+ Partials      763      749      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RaymondLuong3 RaymondLuong3 force-pushed the feature/SF-3077-multiple-source-ui branch from d58cf38 to b9268e3 Compare December 5, 2024 00:36
@Nateowami
Copy link
Collaborator

Here are some issues I'm seeing:

Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also want to add an optional title parameter to the book selection component, which contains the bulk select checkboxes. Then place the project name on the same line as the checkboxes.

Reviewable status: 0 of 14 files reviewed, all discussions resolved

@Nateowami
Copy link
Collaborator

@RaymondLuong3 I should note that when selecting books in bulk, it seems that the first time yous select them it selects correctly, but then upon unselecting and re-selecting it doesn't.

@RaymondLuong3 RaymondLuong3 force-pushed the feature/SF-3077-multiple-source-ui branch from b9268e3 to 231c790 Compare December 5, 2024 22:29
Copy link
Collaborator Author

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding that. I figured out the main issue was that arrays were being passed around by reference rather than by values. I've cleaned it up and now it works. I've also addressed the extra training books notice and the project label for the multi-book-select component.

Reviewable status: 0 of 16 files reviewed, all discussions resolved

@RaymondLuong3 RaymondLuong3 force-pushed the feature/SF-3077-multiple-source-ui branch from b6d0947 to deef1aa Compare December 6, 2024 22:14
@RaymondLuong3 RaymondLuong3 force-pushed the feature/SF-3077-multiple-source-ui branch from deef1aa to 8a007bb Compare December 9, 2024 20:07
@RaymondLuong3 RaymondLuong3 added the will require testing PR should not be merged until testers confirm testing is complete label Dec 9, 2024
@RaymondLuong3 RaymondLuong3 force-pushed the feature/SF-3077-multiple-source-ui branch from 8a007bb to f2f1d2d Compare December 11, 2024 19:05
@pmachapman pmachapman force-pushed the feature/SF-2900 branch 2 times, most recently from 341fa66 to f66ab50 Compare December 15, 2024 23:36
Base automatically changed from feature/SF-2900 to master December 16, 2024 16:33
@RaymondLuong3 RaymondLuong3 force-pushed the feature/SF-3077-multiple-source-ui branch from f2f1d2d to c630eda Compare December 16, 2024 19:32
@pmachapman pmachapman self-requested a review December 17, 2024 01:25
@pmachapman pmachapman self-assigned this Dec 17, 2024
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the new interface! Just a couple of minor nits.

...and thank you for fixing my frontend/realtimeserver mistakes on SF-2900 :-S

Reviewed 6 of 14 files at r3, 7 of 16 files at r4, 10 of 10 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @RaymondLuong3)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.spec.ts line 162 at r5 (raw file):

        mockAlternateTrainingSourceProjectDoc
      );
      when(mockFeatureFlagService.allowFastTraining).thenReturn(createTestFeatureFlag(false));

NIT: Should this be removed as it occurs 4 lines below?

Code quote:

when(mockFeatureFlagService.allowFastTraining).thenReturn(createTestFeatureFlag(false));

src/RealtimeServer/scriptureforge/services/sf-project-migrations.ts line 419 at r5 (raw file):

    }

    await submitMigrationOp(SFProjectMigration21.VERSION, doc, ops);

NIT: This should be:

await submitMigrationOp(SFProjectMigration22.VERSION, doc, ops);

Code quote:

await submitMigrationOp(SFProjectMigration21.VERSION, doc, ops);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.ts line 414 at r5 (raw file):

    if (previousTrainingRange === '' && trainingScriptureRange != null) {
      previousTrainingRange =
        this.activatedProject.projectDoc?.data?.translateConfig.draftConfig.lastSelectedTrainingScriptureRange ?? '';

NIT: Should this just be previousTrainingRange = trainingScriptureRange, or am I misunderstanding the code?

Code quote:

      previousTrainingRange =
        this.activatedProject.projectDoc?.data?.translateConfig.draftConfig.lastSelectedTrainingScriptureRange ?? '';

Copy link
Collaborator Author

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)


src/RealtimeServer/scriptureforge/services/sf-project-migrations.ts line 419 at r5 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: This should be:

await submitMigrationOp(SFProjectMigration22.VERSION, doc, ops);

Good catch!


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.ts line 414 at r5 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should this just be previousTrainingRange = trainingScriptureRange, or am I misunderstanding the code?

Nice. Thanks


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.spec.ts line 162 at r5 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: Should this be removed as it occurs 4 lines below?

Done.

Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)

@RaymondLuong3 RaymondLuong3 force-pushed the feature/SF-3077-multiple-source-ui branch from 6ebab5e to 7274455 Compare December 18, 2024 21:45
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)

@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed will require testing PR should not be merged until testers confirm testing is complete labels Dec 19, 2024
@Nateowami Nateowami merged commit 1a603bd into master Dec 19, 2024
17 checks passed
@Nateowami Nateowami deleted the feature/SF-3077-multiple-source-ui branch December 19, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing complete Testing of PR is complete and should no longer hold up merging of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants