Skip to content

SF-3931 Show draft running or failed in the menu - #4127

Merged
Nateowami merged 3 commits into
masterfrom
fix/SF-3931
Oct 7, 2026
Merged

Nateowami merged 3 commits into
masterfrom
fix/SF-3931

Conversation

@pmachapman

@pmachapman pmachapman commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

This PR makes the draft generation menu item act like the sync menu item by showing that a draft is in progress, or showing that a draft has failed.


This change is Reviewable

@pmachapman pmachapman added the will require testing PR should not be merged until testers confirm testing is complete label Sep 21, 2026
@codecov

codecov Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.16%. Comparing base (b777db1) to head (657514b).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4127      +/-   ##
==========================================
+ Coverage   81.15%   81.16%   +0.01%     
==========================================
  Files         666      666              
  Lines       42620    42660      +40     
  Branches     7070     7074       +4     
==========================================
+ Hits        34587    34627      +40     
  Misses       6888     6888              
  Partials     1145     1145              

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

@pmachapman
pmachapman deployed to screenshot_diff September 21, 2026 03:08 — with GitHub Actions Active
@github-actions

github-actions Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

📸 Screenshot diff deployed! (5 changes)

View the visual diff at: https://pr-4127--sf-screenshot-diffs.netlify.app

@RaymondLuong3 RaymondLuong3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RaymondLuong3 reviewed 12 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).


src/SIL.XForge.Scripture/Services/MachineApiService.cs line 901 at r1 (raw file):

            // Notify the UI that the draft failed
            await projectDoc.SubmitJson0OpAsync(op =>
                op.Set(pd => pd.TranslateConfig.DraftConfig.DraftInProgress, false)

The comment does not reflect what the code is doing. Is the code supposed to be marking the draft as failed? I don't think cancelled and failed should be equivalent states.

Code quote:

            // Notify the UI that the draft failed
            await projectDoc.SubmitJson0OpAsync(op =>
                op.Set(pd => pd.TranslateConfig.DraftConfig.DraftInProgress, false)

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 129 at r1 (raw file):

                // Notify the UI that the draft is no longer in process
                await projectDoc.SubmitJson0OpAsync(op =>
                    op.Set(pd => pd.TranslateConfig.DraftConfig.DraftInProgress, false)

I wonder if we should reset the isLastDraftSuccessful to be undefined. If the previous one failed, then cancelling a draft would make it appear that it had failed when in fact it was just cancelled.

Code quote:

                await projectDoc.SubmitJson0OpAsync(op =>
                    op.Set(pd => pd.TranslateConfig.DraftConfig.DraftInProgress, false)

@pmachapman pmachapman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@pmachapman made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on RaymondLuong3).


src/SIL.XForge.Scripture/Services/MachineApiService.cs line 901 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

The comment does not reflect what the code is doing. Is the code supposed to be marking the draft as failed? I don't think cancelled and failed should be equivalent states.

Done. Sorry that was a typo!


src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 129 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I wonder if we should reset the isLastDraftSuccessful to be undefined. If the previous one failed, then cancelling a draft would make it appear that it had failed when in fact it was just cancelled.

I think it might be confusing treating it as a ternary value by setting undefined. In this PR I treat undefined as this has not yet been set.

I did um-and-ah over cancellation for a bit. Our sync treats cancellation as failure, and I experimented doing that for drafts, but it didn't feel right given how the UI shows cancelled drafts vs failed drafts.

@pmachapman
pmachapman deployed to screenshot_diff September 22, 2026 23:39 — with GitHub Actions Active

@RaymondLuong3 RaymondLuong3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RaymondLuong3 reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).


src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 129 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I think it might be confusing treating it as a ternary value by setting undefined. In this PR I treat undefined as this has not yet been set.

I did um-and-ah over cancellation for a bit. Our sync treats cancellation as failure, and I experimented doing that for drafts, but it didn't feel right given how the UI shows cancelled drafts vs failed drafts.

That is a good point that we currently treat cancelled drafts more like a failure. I think sticking to that convention makes sense.

@RaymondLuong3 RaymondLuong3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:lgtm:

@RaymondLuong3 made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@RaymondLuong3 RaymondLuong3 added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Sep 23, 2026
@pmachapman
pmachapman deployed to screenshot_diff September 24, 2026 03:16 — with GitHub Actions Active
@pmachapman
pmachapman deployed to screenshot_diff September 24, 2026 03:34 — with GitHub Actions Active
@pmachapman
pmachapman deployed to screenshot_diff September 29, 2026 19:54 — with GitHub Actions Active
@pmachapman
pmachapman deployed to screenshot_diff October 4, 2026 23:33 — with GitHub Actions Active
@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Oct 7, 2026
@Nateowami
Nateowami enabled auto-merge (squash) October 7, 2026 13:44
@Nateowami
Nateowami deployed to screenshot_diff October 7, 2026 13:49 — with GitHub Actions Active
@Nateowami
Nateowami merged commit 3ebe77a into master Oct 7, 2026
28 of 29 checks passed
@Nateowami
Nateowami deleted the fix/SF-3931 branch October 7, 2026 13:52

This branch was successfully deployed

1 active deployment
screenshot_diff — 657514be Deployed Oct 7, 2026 by Nateowami via Compare Screenshots #1043
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