SF-3306 Add initial draft history UI

[pmachapman]

This PR enables the draft history portion of the updated draft UI. This UI is hidden behind the feature flag "Preview new draft history interface", which should be enabled to access this UI. See the mockup in Balsamiq for details (linked to in the JIRA ticket).

I have not implemented the full UI, but the initial draft history portion, with the ability to download and view previous drafts. Other JIRA issues (SF-3354, SF-3355, SF-3356, and SF-3357) have been created for the rest of the tasks to complete this entire feature.

Viewing/applying/downloading previous drafts will only work fully for drafts created with this PR (this includes the displaying of training books/languages selected).

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 95.45455% with 13 lines in your changes missing coverage. Please review.

Project coverage is 81.84%. Comparing base (f99f04c) to head (934a198).
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	92.85%	0 Missing and 6 partials ⚠️
...draft-history-list/draft-history-list.component.ts	88.88%	0 Missing and 2 partials ⚠️
...aft-preview-books/draft-preview-books.component.ts	88.23%	0 Missing and 2 partials ⚠️
...late/editor/editor-draft/editor-draft.component.ts	94.11%	1 Missing and 1 partial ⚠️
...aft-history-entry/draft-history-entry.component.ts	98.03%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3180      +/-   ##
==========================================
+ Coverage   81.76%   81.84%   +0.08%     
==========================================
  Files         598      601       +3     
  Lines       34300    34501     +201     
  Branches     5551     5571      +20     
==========================================
+ Hits        28044    28237     +193     
- Misses       5431     5443      +12     
+ Partials      825      821       -4     

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pmachapman]

Dismissed @Github-advanced-security[bot] from 2 discussions.
Reviewable status: 0 of 41 files reviewed, all discussions resolved

[RaymondLuong3]

The front-end looks really good and is already relatively feature complete based on my observations. Thanks for the good work!

Reviewed 22 of 39 files at r1, 20 of 20 files at r2, all commit messages.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @pmachapman)

---

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

  describe('getStatus', () => {
    it('should handle known build state strings', () => {
      expect(component.getStatus(BuildStates.Active)).not.toBeUndefined();

Nit: Instead of the double negative, this can just be toBeDefined() here and below.

Code quote:

      expect(component.getStatus(BuildStates.Active)).not.toBeUndefined();

---

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

      // Build the list of book numbers, first checking the build, then the project document if that is null
      const books: number[] =

You will probably want to get a Set for the books, otherwise there is the chance of duplicate books when we do implement multiple translation sources.

Code quote:

const books: number[] =

---

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

    private readonly transloco: TranslocoService
  ) {
    this.activatedProject.projectId$.pipe(filterNullish(), take(1)).subscribe(projectId => {

You'll want to also pipe it through quietTakeUntilDestroyed(destroyRef). I have seen articles explaining why take(1) is not guaranteed to cause the subscript to complete if the first value never gets emitted from the observable and the user moves off the component.

Code quote:

    this.activatedProject.projectId$.pipe(filterNullish(), take(1)).subscribe(projectId => {

---

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

        return this.transloco.translate('draft_history_list.draft_faulted');
      default:
        // The latest build must abe a build that has finished

Typo

Code quote:

// The latest build must abe a build that has finished

---

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

      // SUT
      service.getBuildHistory(projectId).subscribe(result => {
        expect(result).toEqual(undefined);

Nit: You could use .toBeUndefined() here and below.

Code quote:

        expect(result).toEqual(undefined);

---

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

      // SUT
      service.getGeneratedDraftHistory(projectId, book, chapter).subscribe(result => {
        expect(result).toEqual(undefined);

Nit: This could be toBeUndefined() here and below.

Suggestion:

        expect(result).toEqual(undefined)

---

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

    "draft_unknown": "Unknown",
    "hide_model_training_data": "Hide model training data",
    "show_model_training_data": "Show model training data",

Nit: We should name this model training books to avoid confusion with training data files.

Code quote:

    "hide_model_training_data": "Hide model training data",
    "show_model_training_data": "Show model training data",

---

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

    "draft_canceled": "Your draft was canceled",
    "draft_completed": "Your draft is ready",
    "draft_faulted": "The draft failed",

We should be consistent and choose "your" or "the" for these strings.

Code quote:

    "draft_canceled": "Your draft was canceled",
    "draft_completed": "Your draft is ready",
    "draft_faulted": "The draft failed",

---

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

<ng-container *transloco="let t; read: 'draft_history_entry'">
  @if (entry != null) {
    <div class="draft-entry-heading" (click)="headerClicked()" [class.expandable]="!forceDetailsOpen">

Material does have an expansion panel component that we are already using on the draft generation page. Is there a reason for not using it?

Code quote:

<div class="draft-entry-heading" (click)="headerClicked()" [class.expandable]="!forceDetailsOpen">

---

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

      </div>
      <span class="spacer"></span>
      <span class="status" [style.color]="getStatus(entry.state).color">

You will want to add the colour as a class to this element, and then in the css you can specify what the colour should be if the element contains the class. I think that is better than specifically setting the color style. It adds a layer so that the colour is not as tightly coupled to status.

Code quote:

      <span class="status" [style.color]="getStatus(entry.state).color">

---

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

    @if (detailsOpen && hasDetails) {
      <div class="draft-entry-body">
        <ng-container *ngIf="canDownloadBuild">

We should probably avoid mixing the structural directives *ngIf and use the control flow version here and throughout this file.

Code quote:

        <ng-container *ngIf="canDownloadBuild">

---

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

          <p *ngIf="canDownloadBuild">
            <a
              href="javascript:;"

What is this link doing?

Code quote:

              href="javascript:;"

---

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

            </a>
          </p>
          @if (trainingDataOpen) {

We might want to call this training books so that we don't get this confused with training data files that is in addition to training books.

Code quote:

          </p>
          @if (trainingDataOpen) {

---

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

    constructor(buildHistory: BuildDto[] | undefined) {
      when(mockedActivatedProjectService.projectId$).thenReturn(of('project01'));
      when(mockedActivatedProjectService.changes$).thenReturn(of(undefined));

Nit: I don't think this line is necessary.

Code quote:

      when(mockedActivatedProjectService.changes$).thenReturn(of(undefined));

---

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

  }

  findClosestRevision(date: Date, revisions: Revision[]): Revision {

This can be a private method.

Code quote:

 findClosestRevision(date: Date, revisions: Revision[]): Revision {

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.html line 5 at r2 (raw file):

    <mat-button-toggle-group
      class="draft-book-option"
      [disabled]="book.chaptersWithDrafts.length === 0"

I wouldn't expect this to be possible since the book wouldn't have been selected if it didn't have an chapters with a draft. Is there something I am not understanding?

Code quote:

      [disabled]="book.chaptersWithDrafts.length === 0"

---

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

  @if (isDraftReady) {
    <div class="toolbar">
      <mat-form-field class="draft-select" appearance="outline">

This is a really neat addition. It works so seamlessly!

Code quote:

      <mat-form-field class="draft-select" appearance="outline">

---

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

              // The legacy format does not support a timestamp
              if (err.status === 405 && timestamp == null) {
                return this.getDraft(textDocId, { isDraftLegacy: true, timestamp });

If the legacy does not support time stamp, then we should just omit that here.

Code quote:

    return this.getDraft(textDocId, { isDraftLegacy: true, timestamp });

---

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

    }

    private static string GetTranslationId(

This should be GetTranslationEngineId to make it less ambiguous that it is looking for the engine.

Code quote:

    private static string GetTranslationId(

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.ts line 72 at r2 (raw file):

      } else {
        draftBooks = this.build.additionalInfo?.translationScriptureRanges
          .flatMap(range => booksFromScriptureRange(range.scriptureRange))

This will work currently, but once there are multiple translation sources, this could lead to duplicate books.

Code quote:

          .flatMap(range => booksFromScriptureRange(range.scriptureRange))

---

src/SIL.XForge.Scripture/Models/ServalBuildAdditionalInfo.cs line 12 at r2 (raw file):

    public DateTimeOffset? DateFinished { get; init; }
    public DateTimeOffset? DateRequested { get; set; }
    public DateTimeOffset? DateGenerated { get; set; }

I wonder if these should allow a setter, or if init would make more sense. Oh, nevermind, I see the use case for allowing the setter now. Good.

Code quote:

    public DateTimeOffset? DateRequested { get; set; }
    public DateTimeOffset? DateGenerated { get; set; }

---

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

    this._trainingData = [];

    // Get the training data

Nit: We will probably want to call this training books to not get this confused with training data files.

Code quote:

    // Get the training data

---

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

  get columnsToDisplay(): string[] {
    return ['bookNames', 'source', 'target'];
  }

Nit: This doesn't need to be a getter. It can just be a field variable.

Code quote:

  get columnsToDisplay(): string[] {
    return ['bookNames', 'source', 'target'];
  }

---

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

  get trainingData(): TrainingDataRow[] {
    return this._trainingData;
  }

Nit: This would be better if it was called training books

Code quote:

  private _trainingData: TrainingDataRow[] = [];
  get trainingData(): TrainingDataRow[] {
    return this._trainingData;
  }

[pmachapman]

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @RaymondLuong3)

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

You will probably want to get a Set for the books, otherwise there is the chance of duplicate books when we do implement multiple translation sources.

Done. Good idea.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: You could use .toBeUndefined() here and below.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: This could be toBeUndefined() here and below.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

If the legacy does not support time stamp, then we should just omit that here.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

You'll want to also pipe it through quietTakeUntilDestroyed(destroyRef). I have seen articles explaining why take(1) is not guaranteed to cause the subscript to complete if the first value never gets emitted from the observable and the user moves off the component.

Done. Thank you!

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Typo

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: I don't think this line is necessary.

It is required for DraftPreviewBooksComponent. I have added a comment stating this.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Material does have an expansion panel component that we are already using on the draft generation page. Is there a reason for not using it?

Done. I have rewritten this as a mat-expansion-panel.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

You will want to add the colour as a class to this element, and then in the css you can specify what the colour should be if the element contains the class. I think that is better than specifically setting the color style. It adds a layer so that the colour is not as tightly coupled to status.

Done. I have added dark mode support, too.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

We should probably avoid mixing the structural directives *ngIf and use the control flow version here and throughout this file.

Done. Old habits die hard!

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

What is this link doing?

That is the same as href="#", but doesn't trigger a navigate.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

We might want to call this training books so that we don't get this confused with training data files that is in addition to training books.

I have renamed this and similar variables to be training configuration rather than training data.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: We will probably want to call this training books to not get this confused with training data files.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: This doesn't need to be a getter. It can just be a field variable.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: This would be better if it was called training books

I have called it trainingConfiguration, as trainingBooks made it confusing when refactoring it.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: Instead of the double negative, this can just be toBeDefined() here and below.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.html line 5 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I wouldn't expect this to be possible since the book wouldn't have been selected if it didn't have an chapters with a draft. Is there something I am not understanding?

If this is showing the books for a historic draft, and the book has been removed or not included in the latest draft, this length can be zero.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.ts line 72 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

This will work currently, but once there are multiple translation sources, this could lead to duplicate books.

Yes. I am not sure how we will support this in the UI. I think we will tackle this and other similar challenges when multiple translation sources support is added.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

This can be a private method.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: We should name this model training books to avoid confusion with training data files.

I have renamed this to "training configuration"

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

We should be consistent and choose "your" or "the" for these strings.

I have renamed these to be "The" not "Your".

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

This should be GetTranslationEngineId to make it less ambiguous that it is looking for the engine.

Done. Good spotting.

[Nateowami]

(I'm sorry this is going to be barrage of comments, some of which are probably just uninformed, but I'm catching up from being completely away for a while). I got tripped up quite a bit by not running the webhook, and only towards the end figured out what was wrong.

@pmachapman Is it intentional that this select is not subject to the feature flag? It shows up all the time.

Some kind of heading is needed here to separate the history from the technical options, otherwise they run together and the UI looks broken.

I'm also not sure why I can't expand the older history items. I think there isn't sufficient data? Right now it feels like the UI is broken. Can we make it expand, but just state that exact information isn't available for older drafts?

The spacing on the left side of this button is way less than it should be.

[pmachapman]

Is it intentional that this select is not subject to the feature flag? It shows up all the time.

@Nateowami Yes. I figured that we could have it independent of the feature flag. That said, if you think it should be hidden behind the feature flag, I can do that.

Some kind of heading is needed here to separate the history from the technical options, otherwise they run together and the UI looks broken.

I've added a heading to separate this (only Serval admins will see the bottom two panels and the heading)

I'm also not sure why I can't expand the older history items. I think there isn't sufficient data? Right now it feels like the UI is broken. Can we make it expand, but just state that exact information isn't available for older drafts?

I will look into that. Maybe I will see if there is some data we can show, even if partial.

The spacing on the left side of this button is way less than it should be.

I have fixed it, but it wasn't a pretty fix (I had to duplicate the button HTML).

[Nateowami]

@pmachapman Yes, the dropdown should be subject to the feature flag. We had some discussions earlier today about the fact that just showing the date is confusing, and some potential improvements. It's going to be complicated enough that I think it would be far easier to just feature flag it until we figure that out.

[Nateowami]

Reviewable status: 21 of 43 files reviewed, 17 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-download-button/draft-download-button.component.html line 12 at r4 (raw file):

  >
    @if (downloadProgress === 0) {
      <mat-icon [ngClass]="flat ? '' : 'material-icons-outlined'">cloud_download</mat-icon>

It's no longer relevant given your latest changes, but this conditional class could have been written much more simply as [class.material-icons-outlined]="flat". It's also possible to use object syntax: [ngClass]="{'material-icons-outlined': flat}"

Code quote:

[ngClass]="flat ? '' : 'material-icons-outlined'"

[pmachapman]

Yes, the dropdown should be subject to the feature flag. We had some discussions earlier today about the fact that just showing the date is confusing, and some potential improvements. It's going to be complicated enough that I think it would be far easier to just feature flag it until we figure that out.

@Nateowami Done.

Reviewable status: 20 of 43 files reviewed, 17 unresolved discussions (waiting on @Nateowami and @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-download-button/draft-download-button.component.html line 12 at r4 (raw file):

Previously, Nateowami wrote…

It's no longer relevant given your latest changes, but this conditional class could have been written much more simply as [class.material-icons-outlined]="flat". It's also possible to use object syntax: [ngClass]="{'material-icons-outlined': flat}"

Done. Thanks for letting me know!

[RaymondLuong3]

Reviewed 6 of 19 files at r3, 15 of 16 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Nateowami and @pmachapman)

---

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

Previously, pmachapman (Peter Chapman) wrote…

It is required for DraftPreviewBooksComponent. I have added a comment stating this.

Excellent. thanks for the explanation.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.ts line 72 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Yes. I am not sure how we will support this in the UI. I think we will tackle this and other similar challenges when multiple translation sources support is added.

Agreed, we don't need to solve all the problems now. Let's leave a TODO here to remind us this needs work.

---

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

    "preview_last_draft_header": "Preview last draft",
    "report_problem": "Report problem",
    "serval_administration": "Serval administration",

We don't need to localize this. We should update the places where we have this internationalized.

Code quote:

    "serval_administration": "Serval administration",

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-draft/editor-draft.component.html line 21 at r6 (raw file):

  @if (isDraftReady) {
    <div class="toolbar">
      @if (featureFlags.newDraftHistory.enabled) {

Thanks for catching this!

Code quote:

     @if (featureFlags.newDraftHistory.enabled) {

[pmachapman]

I'm also not sure why I can't expand the older history items. I think there isn't sufficient data? Right now it feels like the UI is broken. Can we make it expand, but just state that exact information isn't available for older drafts?

@Nateowami More information should now show for older builds Let me know if this looks ok, especially the use of "Unknown" for the source project.

Reviewable status: 35 of 43 files reviewed, 2 unresolved discussions (waiting on @Nateowami and @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.ts line 72 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Agreed, we don't need to solve all the problems now. Let's leave a TODO here to remind us this needs work.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

We don't need to localize this. We should update the places where we have this internationalized.

Done. I have removed this string. It also occurs in the top right hand app menu - do you want me to remove it (and probably Hangfire Dashboard) from there too?

[RaymondLuong3]

Reviewed 21 of 21 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami)

---

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

Previously, pmachapman (Peter Chapman) wrote…

Done. I have removed this string. It also occurs in the top right hand app menu - do you want me to remove it (and probably Hangfire Dashboard) from there too?

Oh that's interesting. I am happy to remove it or leave it as is. Your call.

[pmachapman]

Dismissed @Nateowami from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Oh that's interesting. I am happy to remove it or leave it as is. Your call.

Let's leave it as it has been there a while.