[RaymondLuong3]

If a user had applied a draft to a chapter and synchronizes with Paratext, those chapters will appear as not editable since the permission in Paratext is not granted for new books or chapters. We bypass this to allow the user to add a new books or chapter chapters to their project.

This PR improves this behaviour so that users who add chapters to existing books (when the user does not have book edit permission) will successfully apply those chapters. Previously, when a user applied a draft chapters to an existing book the whole book would be marked editable.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 89.33333% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.31%. Comparing base (c429a36) to head (3bb2f00).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...SIL.XForge.Scripture/Services/MachineApiService.cs	84.61%	2 Missing and 4 partials ⚠️
...c/SIL.XForge.Scripture/Services/ParatextService.cs	94.44%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4236      +/-   ##
==========================================
+ Coverage   81.30%   81.31%   +0.01%     
==========================================
  Files         671      671              
  Lines       43020    43083      +63     
  Branches     7138     7142       +4     
==========================================
+ Hits        34976    35035      +59     
- Misses       6877     6879       +2     
- Partials     1167     1169       +2     

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

[Nateowami]

@Nateowami made 1 comment.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).

---

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

                    bool isExistingBook =
                        addedChapters is not null && addedChapters.TryGetValue(bookNum, out addedChaptersInBook);
                    if (!hasPermissionInMongo && isExistingBook && !hasPermissionInParatext)

Have not verified, but Devin claims this gives permission even to non-admins: https://app.devin.ai/review/sillsdev/web-xforge/pull/4236#security-SEC_pr-review-job-cc5723243c1b424baf8a59a02016d195_0001

[pmachapman]

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

---

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

                    bool isExistingBook =
                        addedChapters is not null && addedChapters.TryGetValue(bookNum, out addedChaptersInBook);
                    if (!hasPermissionInMongo && isExistingBook && !hasPermissionInParatext)

Should we check for currentUserIsAdministrator here? I could not get a non-admin user to get write permissions, as they are blocked earlier in the add draft workflow, but perhaps if this funciton gets used by other code in the future, it might not make that check before calling this function?

Code quote:

 if (!hasPermissionInMongo && isExistingBook && !hasPermissionInParatext)

---

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

                                        p => p.Texts[textIndex].Chapters[chapterIndex].Permissions[user.SFUserId],
                                        TextInfoPermission.Write
                                    );

These variables captured into the lambda are modified in their outer scopes, which can sometimes cause their values to be unexpected due to how the C# compiler builds lambdas at runtime (it passes the variables ByRef, not ByVal). It should be something along the lines of:

int i = textIndex;
int j = chapterIndex;
op.Set(
	p => p.Texts[i].Chapters[j].Permissions[user.SFUserId],
	TextInfoPermission.Write
);

Code quote:

                                    op.Set(
                                        p => p.Texts[textIndex].Chapters[chapterIndex].Permissions[user.SFUserId],
                                        TextInfoPermission.Write
                                    );

---

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

                            for (int chapterIndex = 0; chapterIndex < text.Chapters.Count; chapterIndex++)
                            {
                                int chapterNumber = text.Chapters[chapterIndex].Number;

Same bug here - the two variable declarations that were removed were necessary to stop the outer scope modifying the variables in the lambda.

Code quote:

                        ];
                        await projectDoc.SubmitJson0OpAsync(op =>
                        {
                            op.Set(p => p.Texts[textIndex].Permissions[user.SFUserId], TextInfoPermission.Write);
                            for (int chapterIndex = 0; chapterIndex < text.Chapters.Count; chapterIndex++)
                            {
                                int chapterNumber = text.Chapters[chapterIndex].Number;

[RaymondLuong3]

@RaymondLuong3 made 4 comments.
Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on Nateowami and pmachapman).

---

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

Previously, Nateowami wrote…

Have not verified, but Devin claims this gives permission even to non-admins: https://app.devin.ai/review/sillsdev/web-xforge/pull/4236#security-SEC_pr-review-job-cc5723243c1b424baf8a59a02016d195_0001

Thanks for pointing that out. I'll add the additional guard to ensure that this path is only available to admin users.

---

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

Previously, pmachapman (Peter Chapman) wrote…

Should we check for currentUserIsAdministrator here? I could not get a non-admin user to get write permissions, as they are blocked earlier in the add draft workflow, but perhaps if this funciton gets used by other code in the future, it might not make that check before calling this function?

Thanks for looking into this. I have added the additional guard.

---

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

Previously, pmachapman (Peter Chapman) wrote…

These variables captured into the lambda are modified in their outer scopes, which can sometimes cause their values to be unexpected due to how the C# compiler builds lambdas at runtime (it passes the variables ByRef, not ByVal). It should be something along the lines of:

int i = textIndex;
int j = chapterIndex;
op.Set(
	p => p.Texts[i].Chapters[j].Permissions[user.SFUserId],
	TextInfoPermission.Write
);

Thanks for catching that. I am surprised that the lambdas are constructed by ref rather than by value. I'm glad I know that now!

---

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

Previously, pmachapman (Peter Chapman) wrote…

Same bug here - the two variable declarations that were removed were necessary to stop the outer scope modifying the variables in the lambda.

Done.

[pmachapman]

@pmachapman reviewed 1 file and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Nateowami).