Skip to content

SF-3905 Use new apply draft logic in the editor draft tab - #4072

Open
pmachapman wants to merge 1 commit into
masterfrom
fix/SF-3905
Open

SF-3905 Use new apply draft logic in the editor draft tab#4072
pmachapman wants to merge 1 commit into
masterfrom
fix/SF-3905

Conversation

@pmachapman

@pmachapman pmachapman commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

This PR updates the editor draft tab to use the new draft application logic, but retaining the same interface as the draft tab had previously. This resolves in particular an issue that prevented the old editor draft tab form adding a draft to a chapter of a book that was not in the project already.

I also took the opportunity to add/correct some dates in [Obsolete] attributes, so we can make a decision on when to remove these obsolete functions in future.


This change is Reviewable

@pmachapman pmachapman added the will require testing PR should not be merged until testers confirm testing is complete label Aug 26, 2026
@pmachapman
pmachapman deployed to screenshot_diff August 26, 2026 02:01 — with GitHub Actions Active
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.56604% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.08%. Comparing base (2f7542b) to head (2c99c32).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ripture/ClientApp/src/app/core/text-doc.service.ts 57.14% 3 Missing ⚠️
...late/editor/editor-draft/editor-draft.component.ts 91.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4072      +/-   ##
==========================================
- Coverage   81.09%   81.08%   -0.02%     
==========================================
  Files         666      666              
  Lines       43258    43273      +15     
  Branches     7060     7063       +3     
==========================================
+ Hits        35079    35086       +7     
- Misses       7013     7019       +6     
- Partials     1166     1168       +2     

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

@pmachapman
pmachapman deployed to screenshot_diff August 26, 2026 02:15 — with GitHub Actions Active
@RaymondLuong3 RaymondLuong3 self-assigned this Aug 27, 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.

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


src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.spec.ts line 358 at r1 (raw file):

    });

    it('should return false if the user not have the permission', () => {

Typo: if the user does not have the permission

Code quote:

the user not have the permission'

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 590 at r1 (raw file):

        const textDoc = await this.projectService.getText(this._id);
        this.onCreateSub?.unsubscribe();
        this.onCreateSub = textDoc.create$.subscribe(() => this.bindQuill());

It looks like this code can be refactored into its own method and with a name like subscribeToTextDocCreation

Code quote:

        const textDoc = await this.projectService.getText(this._id);
        this.onCreateSub?.unsubscribe();
        this.onCreateSub = textDoc.create$.subscribe(() => this.bindQuill());

src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 1220 at r1 (raw file):

        const textDoc = await this.projectService.getText(this._id);
        this.onCreateSub?.unsubscribe();
        this.onCreateSub = textDoc.create$.subscribe(() => this.bindQuill());

This can be replaced with the method suggested above.

Code quote:

        const textDoc = await this.projectService.getText(this._id);
        this.onCreateSub?.unsubscribe();
        this.onCreateSub = textDoc.create$.subscribe(() => this.bindQuill());

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-draft/editor-draft.component.ts line 417 at r1 (raw file):

      this.draftCheckState = 'draft-present';
      this.isDraftApplied = true;
      this.userAppliedDraft = true;

It seems like this.isDraftApplied and this.userAppliedDraft should not be true if applying the draft failed. Maybe I am missing the reason we still want those values to be true here?

Code quote:

      this.isDraftApplied = true;
      this.userAppliedDraft = true;

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

    }

    [Obsolete("Use ApplyPreTranslationToProject instead. Deprecated 2026-03")]

This can be safely removed, it is almost 6 months old. But we can do that in a follow up too.

Code quote:

    [Obsolete("Use ApplyPreTranslationToProject instead. Deprecated 2026-03")]

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

Labels

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.

2 participants