Skip to content

SF-3434 Move training file upload into draft sources page #3284

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

Merged
merged 10 commits into from
Jul 9, 2025

Conversation

josephmyers
Copy link
Collaborator

@josephmyers josephmyers commented Jun 23, 2025

This follows the Balsamiq design on SF-3434.


This change is Reviewable

@josephmyers josephmyers added the will require testing PR should not be merged until testers confirm testing is complete label Jun 23, 2025
Copy link

codecov bot commented Jun 23, 2025

Codecov Report

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

Project coverage is 82.52%. Comparing base (e34ce95) to head (c7b0e73).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...eneration/draft-sources/draft-sources.component.ts 92.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3284      +/-   ##
==========================================
- Coverage   82.52%   82.52%   -0.01%     
==========================================
  Files         605      605              
  Lines       35217    35194      -23     
  Branches     5701     5720      +19     
==========================================
- Hits        29064    29044      -20     
+ Misses       5312     5299      -13     
- Partials      841      851      +10     

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@josephmyers josephmyers marked this pull request as ready for review June 24, 2025 01:58
@Nateowami
Copy link
Collaborator

Thanks for your work on this. A couple things I notice (which I think are not new):

  1. The color of the upload icon is wrong.
  2. The file should not be toggleable. (In this context it's really not obvious what toggling it does).

@josephmyers
Copy link
Collaborator Author

Though I do agree with this direction, a fallout of this is that uploading, and deleting, now circumvents the Save & Sync behavior that the rest of the page follows. In effect, if you upload a file and then Cancel, the file remains and would be used for builds. I can rewrite the upload dialog to postpone the upload until Save & Sync, if that works.

@josephmyers josephmyers force-pushed the feature/SF-3434 branch 2 times, most recently from 2995f34 to 200d27e Compare June 30, 2025 03:58
@josephmyers josephmyers requested a review from Nateowami June 30, 2025 06:56
@pmachapman pmachapman self-assigned this Jul 1, 2025
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 can rewrite the upload dialog to postpone the upload until Save & Sync, if that works.

This is a good idea. If someone uploads or deletes files erroneously, they may cancel out and want it back the way it was. (The offline support for uploading files may help you with this?) Also, can you postpone the deletions to the Save & sync action as well? Thanks!

Reviewed 25 of 25 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/Models/DraftConfig.cs line 9 at r1 (raw file):

{
    [Obsolete("For backwards compatibility with older frontend clients. Deprecated June 2025.")]
    public bool AdditionalTrainingData { get; set; }

You have removed this from the TypeScript model, so it should be removed here too. The C# model and TypeScript model must be in sync with what is in Mongo.

Code quote:

    [Obsolete("For backwards compatibility with older frontend clients. Deprecated June 2025.")]
    public bool AdditionalTrainingData { get; set; }

src/RealtimeServer/scriptureforge/models/translate-config.ts line 58 at r1 (raw file):

  additionalTrainingSourceEnabled: boolean;
  additionalTrainingSource?: TranslateSource;
  alternateSourceEnabled: boolean;

You will need a migrator to remove the additionalTrainingData property.

Code quote:

export interface DraftConfig {
  additionalTrainingSourceEnabled: boolean;
  additionalTrainingSource?: TranslateSource;
  alternateSourceEnabled: boolean;

@josephmyers josephmyers force-pushed the feature/SF-3434 branch 2 times, most recently from f197d98 to 5820caf Compare July 7, 2025 11:19
@josephmyers josephmyers requested a review from pmachapman July 7, 2025 11:19
@josephmyers josephmyers force-pushed the feature/SF-3434 branch 3 times, most recently from 999afc4 to 5db9aa2 Compare July 7, 2025 11:36
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.

Looks good - apologies - just one tiny change I noticed while doing a final review:

Can you please remove additionalTrainingData from the validationSchema in sf-project-service.ts? Thanks!

Reviewed 1 of 5 files at r2, 8 of 8 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

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: Thank you for your work on this!

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

@pmachapman pmachapman added ready to test 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 ready to test labels Jul 8, 2025
This follows the Balsamiq design on SF-3434. The only surprise here was that the training files needed to be added to the project settings, since they weren't actually saved until you start a build. I coordinated with Peter on this part.
Any files that are present will be made part of the build settings when the page is saved. The app initializes from any training files available and will set the files to use with whatever is present.

The only downside of this approach, as opposed to a more involved rewrite delaying the upload, is when the user uploads, leaves the page, then returns to this page to save. Since the files were uploaded, they are present and selected upon returning. If the user doesn't see that, they could send unexpected files to Serval. I'm not sure how common it will be to do this, however.

This also impacts the "changesMade" for the confirm prompt. I chose to show the prompt, even though leaving the page doesn't actually discard the uploads, because it seems valuable to remind them to save.

We may also be able to track the delta of what is uploaded and deleted, then undo those if they leave the page from the draft sources page.
To do this, we can compare what's last saved with what the current files are, and create for the difference. We don't have to worry about deleted files, since those are deleted immediately.

I am wondering, however, about the file upload, which does happen in the dialog. If they upload a file, then leave the page, is that file trapped on our filesystem forever?
And attempt to undo any added files on confirmLeave
This is because only Admin's can configure sources.
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 all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

@pmachapman pmachapman merged commit 62c4e10 into master Jul 9, 2025
18 checks passed
@pmachapman pmachapman deleted the feature/SF-3434 branch July 9, 2025 19:46
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.

None yet

3 participants