Skip to content

SF-3934 Translator offered option to import a chapter to a book that does not exist - #4175

Open
pmachapman wants to merge 4 commits into
masterfrom
fix/SF-3934
Open

pmachapman wants to merge 4 commits into
masterfrom
fix/SF-3934

Conversation

@pmachapman

@pmachapman pmachapman commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

This PR displays an error to a Translator if they attempt to apply a draft to a book or chapter that does not exist if they do not have permission to edit that books or chapter. Translators can have the "edit all books" permission granted in Paratext that gives the ability to create books, and they may or may not have permission to add a missing chapter depending on their chapter permissions in Paratext.

I also removed some older permissions for creating texts from frontend, as drafts are no longer applied to missing chapters in that way.


This change is Reviewable

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

codecov Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.54545% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.17%. Comparing base (b19dd3e) to head (08ddcc9).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...aft-import-wizard/draft-import-wizard.component.ts 0.00% 2 Missing ⚠️
...c/SIL.XForge.Scripture/Services/ParatextService.cs 96.29% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4175   +/-   ##
=======================================
  Coverage   81.17%   81.17%           
=======================================
  Files         666      666           
  Lines       42666    42699   +33     
  Branches     7069     7055   -14     
=======================================
+ Hits        34632    34661   +29     
- Misses       6870     6891   +21     
+ Partials     1164     1147   -17     

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

@pmachapman
pmachapman deployed to screenshot_diff October 7, 2026 02:28 — with GitHub Actions Active
@pmachapman
pmachapman force-pushed the fix/SF-3934 branch 2 times, most recently from 816e311 to 63456a3 Compare October 7, 2026 02:32
@pmachapman
pmachapman deployed to screenshot_diff October 7, 2026 02:39 — with GitHub Actions Active
@pmachapman
pmachapman deployed to screenshot_diff October 7, 2026 18:54 — with GitHub Actions Active
@RaymondLuong3 RaymondLuong3 self-assigned this Oct 8, 2026

@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.

The code looks good. A couple non-blocking comments.

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


src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 48 at r1 (raw file):

        try
        {
            // Ensure the user can apply to the draft to at least one of the chapters selected

typo:

Code quote:

// Ensure the user can apply to the draft to at 

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

    /// If one of those checks fails, an exception is thrown.
    /// </remarks>
    public async Task EnsureUserCanApplyDraftToProjectAsync(string userId, string projectId, string scriptureRange)

There are many nuances to this method. It really checks that a user has edit permission on at least 1 chapter in the range. The method comments make that explicit. I think the method name matches more with its intended use rather than its logic, so I think that is Ok.

Code quote:

EnsureUserCanApplyDraftToProjectAsync(string userId, string projectId, string scriptureRange)

@RaymondLuong3 RaymondLuong3 added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Oct 8, 2026

@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 and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).


src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 48 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

typo:

Done. Thank you!


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

Previously, RaymondLuong3 (Raymond Luong) wrote…

There are many nuances to this method. It really checks that a user has edit permission on at least 1 chapter in the range. The method comments make that explicit. I think the method name matches more with its intended use rather than its logic, so I think that is Ok.

I struggled to name this method. I am all ears if you can think of a better name!

@pmachapman
pmachapman deployed to screenshot_diff October 8, 2026 02:52 — with GitHub Actions Active
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

📸 Screenshot diff deployed! (1 change)

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

This branch was successfully deployed

1 active deployment
screenshot_diff — 08ddcc90 Deployed Oct 8, 2026 by pmachapman via Compare Screenshots #1052
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants