SF-1873 Add messages to sync progress

[kylebuss]

This PR adds sync messages to the sync progress component to provide the user with better visibility into the status of where the sync is at.

---

This change is 

[kylebuss]

src/SIL.XForge.Scripture/Models/ProgressState.cs line 10 at r1 (raw file):

    public double ProgressValue { get; set; }
    public int SyncPhase { get; set; }
    public double SyncProgress { get; set; }

Just realized, I could format the stage_phase_progress string on the back-end in the ProgressString if that would be preferred?

Code quote:

    public int SyncPhase { get; set; }
    public double SyncProgress { get; set; }

[codecov]

Codecov Report

Attention: Patch coverage is 90.62500% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.19%. Comparing base (b41e9d0) to head (cb5e7b4).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
.../app/sync/sync-progress/sync-progress.component.ts	88.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2837   +/-   ##
=======================================
  Coverage   79.18%   79.19%           
=======================================
  Files         531      531           
  Lines       30874    30902   +28     
  Branches     5040     5044    +4     
=======================================
+ Hits        24447    24472   +25     
- Misses       5654     5656    +2     
- Partials      773      774    +1     

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

[RaymondLuong3]

I like the feel of this update. It makes me think that I can see into the things that are being updated. Nice touch! I have some comments. Let me know if you would like to pair to discuss some of this.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kylebuss)

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 18 at r1 (raw file):

    public progressValue: number,
    public progressString?: string,
    public syncPhase?: number,

The syncPhase property should be and enum type similar to the SyncPhase enum in ParatextSyncRunner.

Code quote:

    public syncPhase?: number,

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 31 at r1 (raw file):

  @Output() inProgress: EventEmitter<boolean> = new EventEmitter<boolean>();

  private progressPercent$ = new BehaviorSubject<number>(0);

This private field should be put with the others below the public fields.

Code quote:

  private progressPercent$ = new BehaviorSubject<number>(0);

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 35 at r1 (raw file):

  syncProgress: number | undefined;
  stagePercentage: string = '0.0';
  projectType: string = '';

We have a TextType type which can by either source or target. It is not quite the same thing here, but FYI.

Code quote:

  projectType: string = '';

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 132 at r1 (raw file):

    this.syncProgress = progressState.syncProgress != null ? Math.floor(progressState.syncProgress) : 0;
    this.stagePercentage =
      progressState.syncProgress != null ? ((progressState.syncProgress - this.syncProgress) * 100).toFixed(2) : '0.00';

I would not use decimals for the progress. Integers if better here.

Code quote:

    this.stagePercentage =
      progressState.syncProgress != null ? ((progressState.syncProgress - this.syncProgress) * 100).toFixed(2) : '0.00';

---

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

    "supported_back_translation_languages_dialog_header": "Supported Back Translation Languages"
  },
  "sync": {

I think this is too fine-grained and not likely to be as helpful as it could be, but I can see that these align nicely with the expected output from the backend. I don't like how tightly coupled this is to the expected report from the backend since it means that any changes to the backend reported will require front-end translations to handle it.
What do you think about just adding a since label for each SyncPhase and for the ones that benefit from a percentage value, we can include that. So then the stages keys would look like stage_0, stage_1, etc. I am open to discussion

---

src/SIL.XForge.Scripture/Models/ProgressState.cs line 9 at r1 (raw file):

    public string? ProgressString { get; set; }
    public double ProgressValue { get; set; }
    public int SyncPhase { get; set; }

I wonder if this should be the SyncPhase enum type? int is probably fine here.

Code quote:

   public int SyncPhase { get; set; }

---

src/SIL.XForge.Scripture/Models/ProgressState.cs line 10 at r1 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

Just realized, I could format the stage_phase_progress string on the back-end in the ProgressString if that would be preferred?

If we leave this as is, I think it gives us some flexibility in how we report things to the user (i.e. if we wanted to skip some reporting steps). I am happy to leave it as is.

[kylebuss]

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

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 18 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

The syncPhase property should be and enum type similar to the SyncPhase enum in ParatextSyncRunner.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 31 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

This private field should be put with the others below the public fields.

Done moved above syncProgress observables that rely on it being initialized.

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 132 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I would not use decimals for the progress. Integers if better here.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

I think this is too fine-grained and not likely to be as helpful as it could be, but I can see that these align nicely with the expected output from the backend. I don't like how tightly coupled this is to the expected report from the backend since it means that any changes to the backend reported will require front-end translations to handle it.
What do you think about just adding a since label for each SyncPhase and for the ones that benefit from a percentage value, we can include that. So then the stages keys would look like stage_0, stage_1, etc. I am open to discussion

Done That was a concern of mine as well. I've cleaned it up a bit, let me know what you think!

[RaymondLuong3]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kylebuss)

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 31 at r1 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

Done moved above syncProgress observables that rely on it being initialized.

Oh yes, that's right. Can you move it above the comment for syncProgressPercent$

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 149 at r2 (raw file):

    this.phasePercentage =
      progressState.syncProgress != null ? Math.round((progressState.syncProgress - this.syncProgress) * 100) : 0;
    console.log(progressState.syncPhase);

This should be removed?

Code quote:

    console.log(progressState.syncPhase);

---

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

Previously, kylebuss (Kyle Buss) wrote…

Done That was a concern of mine as well. I've cleaned it up a bit, let me know what you think!

Nice. That looks much better.

---

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

    "log_in_to_paratext": "Log in to Paratext",
    "never_been_synced": "Never been synced",
    "phase_1": "Connecting to {{projectType}} project",

This should be starting at phase_0 just to keep it in line with the phases on the backend. No need to modify it to start at 1. Also, you can lowercase the percent

Code quote:

    "phase_1": "Connecting to {{projectType}} project",

---

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

    "phase_2_Percent": "Checking Paratext updates to biblical terms {{progressPercent}}%",
    "phase_3": "Update Paratext biblical terms",
    "phase_3_Percent": "Update Paratext biblical terms {{progressPercent}}%",

Nit: You should keep the language consistent (i.e. us 'Updating ...")

Code quote:

    "phase_2": "Update {{projectType}} Paratext books and notes",
    "phase_2_Percent": "Checking Paratext updates to biblical terms {{progressPercent}}%",
    "phase_3": "Update Paratext biblical terms",
    "phase_3_Percent": "Update Paratext biblical terms {{progressPercent}}%",

[kylebuss]

Thanks. Code updated.

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

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 31 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Oh yes, that's right. Can you move it above the comment for syncProgressPercent$

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/sync/sync-progress/sync-progress.component.ts line 149 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

This should be removed?

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

This should be starting at phase_0 just to keep it in line with the phases on the backend. No need to modify it to start at 1. Also, you can lowercase the percent

Done.

[Nateowami]

This is great. A couple issues I see:

I'm seeing this logged to the console during a sync:

Missing translation for 'sync.phase_4'
Missing translation for 'sync.phase_5'
Missing translation for 'sync.phase_6'

It's a little bit odd that there are two messages, one above the progress indicator, and one below. Perhaps the detailed message should be moved up to replace "Your project is being synchronized..."? Also, it would be nice if it indicated which project is syncing. It may seem obvious, but syncing projects syncs the source as well, and there's no clear indication of that.

[kylebuss]

I think it's cleaner to just have one message. Also updated to show the name of the source/target project as it syncs.

[RaymondLuong3]

I like the way that looks. This is a sweet improvement. I am not seeing any console errors on this current iteration.

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kylebuss)