Skip to content

Conversation

RaymondLuong3
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 commented Oct 2, 2025

This PR hides the option for formatting drafts when the latest draft is finished before serval 1.11.0 was released. On the editor tabs, the options to format drafts is hidden. On the draft generation page, the option for formatting is not visible on the latest draft if the draft was finished before the cutoff date.

image

This change is Reviewable

@RaymondLuong3 RaymondLuong3 added the will require testing PR should not be merged until testers confirm testing is complete label Oct 2, 2025
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.32%. Comparing base (f38ae20) to head (35fd6c5).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3487   +/-   ##
=======================================
  Coverage   82.32%   82.32%           
=======================================
  Files         613      613           
  Lines       36879    36887    +8     
  Branches     6047     6049    +2     
=======================================
+ Hits        30359    30366    +7     
- Misses       5628     5641   +13     
+ Partials      892      880   -12     

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

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion


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

  get formatOptionsMessage(): string {
    if (!this.formattingOptionsPossible) {
      return translate('editor_draft_tab.format_draft_outdated_draft');

Please just hide the button when it's not available. We don't need to support this as a feature into the future; the feature already doesn't exist on live, and the idea is that it will just appear on the first draft that's done after the cutoff date.

@RaymondLuong3 RaymondLuong3 force-pushed the improvement/sf-3592-older-drafts branch from 5066519 to 4fd2cf4 Compare October 3, 2025 16:24
Copy link
Collaborator Author

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

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @Nateowami)


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

Previously, Nateowami wrote…

Please just hide the button when it's not available. We don't need to support this as a feature into the future; the feature already doesn't exist on live, and the idea is that it will just appear on the first draft that's done after the cutoff date.

Done.

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions


src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 418 at r2 (raw file):

    "format_draft_can": "Customize formatting options for the draft",
    "format_draft_cannot": "You can only change formatting for books from the latest draft",
    "format_draft_outdated_draft": "This draft is outdated and formatting options cannot be applied",

This should be removed


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

    }));

    it('should not guide user to select formatting options before specified date', fakeAsync(() => {

The title of this test is wrong

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@Nateowami reviewed 4 of 8 files at r1, 2 of 4 files at r2, all commit messages.
Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @RaymondLuong3)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-utils.ts line 7 at r2 (raw file):

// Corresponds to Serval 1.11.0 release
export const FORMATTING_OPTIONS_RELEASE_DATE: Date = new Date('2025-09-25T00:00:00Z');

The name of this variable is incorrect.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 275 at r2 (raw file):

  get formattingOptionsSelected(): boolean {
    return (
      !this.formattingOptionsPossible ||

The logic in this getter does not match the name of the getter.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.spec.ts line 397 at r2 (raw file):

    }));

    it('should not show the USFM format option before a specified date', fakeAsync(() => {

The title of this test is wrong

@Nateowami Nateowami self-assigned this Oct 3, 2025
Copy link
Collaborator Author

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

Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-utils.ts line 7 at r2 (raw file):

Previously, Nateowami wrote…

The name of this variable is incorrect.

How is this?


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 275 at r2 (raw file):

Previously, Nateowami wrote…

The logic in this getter does not match the name of the getter.

I have improved this.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.spec.ts line 397 at r2 (raw file):

Previously, Nateowami wrote…

The title of this test is wrong

How is this?


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

Previously, Nateowami wrote…

The title of this test is wrong

How is this?


src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 418 at r2 (raw file):

Previously, Nateowami wrote…

This should be removed

Done.

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@Nateowami reviewed 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @RaymondLuong3)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-utils.ts line 7 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

How is this?

Much better.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 275 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I have improved this.

Much better.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.spec.ts line 397 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

How is this?

It's now ambiguous whether "before the supported date" refers to "show" or "drafts". I think it would be clearer with "drafts created before the supported date"


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

Previously, RaymondLuong3 (Raymond Luong) wrote…

How is this?

It's better, but ambiguous. I read it as meaning the hiding is before the date, whereas you're meaning the drafts being completed before the date. I think it would be clearer if it read "drafts created before supported date"


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

  }

  get doesLatestHaveDraft(): boolean {

Latest what? Latest draft is what comes to mind, but that would make this mean "doesLatestDraftHaveDraft", which doesn't make sense. I think it means "Does the latest draft build have a draft for this chapter?"


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

  set draftRevisions(value: Revision[]) {
    this._draftRevisions = value;
    const latestRevisionDate = value.length > 0 ? new Date(value[0].timestamp) : new Date(0);

Personally I think it makes a lot more sense to use null to represent a lack of date, than to use new Date(0). There are just more ways for a 0 date to go subtly wrong.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.spec.ts line 36 at r3 (raw file):

const mockedFeatureFlagsService = mock(FeatureFlagService);

const dateBeforeFormattingSupported = '2025-09-01T12:00:00.000Z';

Personally I would write:

const oneDay = 1000 * 60 * 60 * 24;
const dateBeforeFormattingSupported = new Date(FORMATTING_OPTIONS_SUPPORTED_DATE.getTime() - oneDay);
const dateAfterFormattingSupported = new Date(FORMATTING_OPTIONS_SUPPORTED_DATE.getTime() + oneDay);

Copy link
Collaborator Author

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.spec.ts line 397 at r2 (raw file):

Previously, Nateowami wrote…

It's now ambiguous whether "before the supported date" refers to "show" or "drafts". I think it would be clearer with "drafts created before the supported date"

Done.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.spec.ts line 36 at r3 (raw file):

Previously, Nateowami wrote…

Personally I would write:

const oneDay = 1000 * 60 * 60 * 24;
const dateBeforeFormattingSupported = new Date(FORMATTING_OPTIONS_SUPPORTED_DATE.getTime() - oneDay);
const dateAfterFormattingSupported = new Date(FORMATTING_OPTIONS_SUPPORTED_DATE.getTime() + oneDay);

I like that too. Done.


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

Previously, Nateowami wrote…

Latest what? Latest draft is what comes to mind, but that would make this mean "doesLatestDraftHaveDraft", which doesn't make sense. I think it means "Does the latest draft build have a draft for this chapter?"

Yes, that is the correct interpretation for this. I've updated it to specify build.


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

Previously, Nateowami wrote…

Personally I think it makes a lot more sense to use null to represent a lack of date, than to use new Date(0). There are just more ways for a 0 date to go subtly wrong.

That is probably reasonable. I updated this check to be a little more robust.

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

:lgtm:

@Nateowami reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)

@Nateowami Nateowami added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Oct 3, 2025
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