Skip to content

Conversation

@pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Nov 10, 2025

This PR deduplicates permissions by:

  • Not storing chapter permissions for users if they are the same as that user's book permissions.
  • Not storing read permissions for users that have access to resources.
  • Not storing "none" permissions (which were ignored anyway) for SF (i.e. non-PT) users.

For some projects on my dev machine, this has reduced the document size by a third. I think for popular resources, like the NIV on production, the document size reduction will be much more substantial.


This change is Reviewable

@pmachapman pmachapman added will require testing PR should not be merged until testers confirm testing is complete e2e Run e2e tests for this pull request labels Nov 10, 2025
@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

❌ Patch coverage is 92.85714% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.89%. Comparing base (12c8136) to head (7efc714).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...c/SIL.XForge.Scripture/Services/ParatextService.cs 50.00% 1 Missing and 1 partial ⚠️
...ture/ClientApp/src/app/core/permissions.service.ts 90.00% 1 Missing ⚠️
...SIL.XForge.Scripture/Services/MachineApiService.cs 83.33% 0 Missing and 1 partial ⚠️
.../SIL.XForge.Scripture/Services/SFProjectService.cs 96.55% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3566      +/-   ##
==========================================
- Coverage   82.90%   82.89%   -0.01%     
==========================================
  Files         605      605              
  Lines       36974    36992      +18     
  Branches     6058     6064       +6     
==========================================
+ Hits        30652    30665      +13     
+ Misses       5408     5401       -7     
- Partials      914      926      +12     

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

@pmachapman pmachapman marked this pull request as draft November 10, 2025 19:40
@pmachapman pmachapman changed the title SF-3637 Deduplicate permissions WIP: SF-3637 Deduplicate permissions Nov 10, 2025
@pmachapman pmachapman marked this pull request as ready for review November 11, 2025 01:07
@pmachapman pmachapman changed the title WIP: SF-3637 Deduplicate permissions SF-3637 Deduplicate permissions Nov 11, 2025
@RaymondLuong3 RaymondLuong3 self-assigned this Nov 13, 2025
Copy link
Collaborator

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

I did some testing and when I updated the roles in PT for books and chapters they were working as expected.

@RaymondLuong3 reviewed 15 of 17 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @pmachapman)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 524 at r2 (raw file):

  get showNoEditPermissionMessage(): boolean {
    return this.userHasGeneralEditRight && this.hasChapterEditPermission === false;
  }

This message doesn't work when the user has no permission on the book. Since it is assumed that books have an implicit read level permission, then hasChapterEditPermission is undefined. I think textDocService should be updated.

Code quote:

  get showNoEditPermissionMessage(): boolean {
    return this.userHasGeneralEditRight && this.hasChapterEditPermission === false;
  }

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 530 at r2 (raw file):

  }

  get hasSourceViewRight(): boolean {

It is so nice to see redundant code cleaned up!

Code quote:

 get hasSourceViewRight(): boolean {

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

    }

    public async Task<SyncMetricInfo> UpdateParatextPermissionsForNewBooksAsync(

Could you add a description here. That would help better understand what the different parameters mean. It isn't clear to me what "writeToParatext" means.

Code quote:

 public async Task<SyncMetricInfo> UpdateParatextPermissionsForNewBooksAsync(

src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.ts line 71 at r2 (raw file):

    // Ensure the user has project level permission to view the text
    if (
      textDocId != null &&

I don't see a reason why textDocId should be optional. At a minimum the textDocId should be defined

Code quote:

    if (
      textDocId != null &&

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e Run e2e tests for this pull request will require testing PR should not be merged until testers confirm testing is complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants