Skip to content

Conversation

@pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Jan 26, 2026

This PR fixes a bug where the apply draft button appears in the editor for chapters that are not drafted.


This change is Reviewable

@pmachapman pmachapman added the will require testing PR should not be merged until testers confirm testing is complete label Jan 26, 2026
@codecov
Copy link

codecov bot commented Jan 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.30%. Comparing base (d46a077) to head (70903f8).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3650   +/-   ##
=======================================
  Coverage   83.29%   83.30%           
=======================================
  Files         611      611           
  Lines       37646    37650    +4     
  Branches     6182     6183    +1     
=======================================
+ Hits        31358    31364    +6     
+ Misses       5332     5331    -1     
+ Partials      956      955    -1     

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

@RaymondLuong3 RaymondLuong3 self-assigned this Jan 27, 2026
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.

Nice! Just one minor comment but otherwise it works well.

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


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

    if (this.targetProject == null || this.bookNum == null || this.chapter == null || this.draftDelta?.ops == null) {
      return false;
    }

Looks like this can be replaced with if (!this.hasDraftToApply)

Code quote:

    if (this.targetProject == null || this.bookNum == null || this.chapter == null || this.draftDelta?.ops == null) {
      return false;
    }

Copy link
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

@pmachapman made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3).


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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Looks like this can be replaced with if (!this.hasDraftToApply)

Yes, logically it is equivalent, but the linter forces me to change this method to:

  get canApplyDraft(): boolean {
    if (!this.hasDraftToApply) {
      return false;
    }
    return this.draftHandlingService.canApplyDraft(
      this.targetProject!,
      this.bookNum!,
      this.chapter!,
      this.draftDelta!.ops
    );
  }

If you think that is still OK, I can make the change, I just thought all of the bangs looked ugly so Ikept the == null's at the top of the getter.

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.

@RaymondLuong3 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman).


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

Previously, pmachapman (Peter Chapman) wrote…

Yes, logically it is equivalent, but the linter forces me to change this method to:

  get canApplyDraft(): boolean {
    if (!this.hasDraftToApply) {
      return false;
    }
    return this.draftHandlingService.canApplyDraft(
      this.targetProject!,
      this.bookNum!,
      this.chapter!,
      this.draftDelta!.ops
    );
  }

If you think that is still OK, I can make the change, I just thought all of the bangs looked ugly so Ikept the == null's at the top of the getter.

Yes, let's make that change. Since it is logical that this method depends on the value of hasDraftToApply and it is sensible that we first check if there is a draft to apply before we check permissions.

Copy link
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

@pmachapman made 1 comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3).


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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Yes, let's make that change. Since it is logical that this method depends on the value of hasDraftToApply and it is sensible that we first check if there is a draft to apply before we check permissions.

Done.

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.

3 participants