SF-3335 Display additional text when syncing source project

[josephmyers]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.93%. Comparing base (6e96876) to head (c83d3b7).

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3220   +/-   ##
=======================================
  Coverage   81.92%   81.93%           
=======================================
  Files         604      604           
  Lines       34657    34657           
  Branches     5620     5597   -23     
=======================================
+ Hits        28393    28395    +2     
  Misses       5438     5438           
+ Partials      826      824    -2     

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

[Nateowami]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions

---

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

      ops.set<number>(p => p.sync.queuedCount, 1);
    }, false);
    this.host.syncProgress['updateProgressState'](projectId, new ProgressState(percentCompleted));

Marking this protected and then cheating the type system seems to me to be counterproductive. If this is ever renamed, or someone tries to find all references, it won't work properly. Much better in my opinion to just leave it public.

---

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

    "synchronize": "Sync with Paratext",
    "synchronize_project": "Synchronize {{ projectName }} with Paratext",
    "synchronize_reference_project": "Synchronizing reference project:",

Nowhere else in the UI do we call this a "reference project". The only place we use the term "reference project" is on the draft sources page. Maybe we should change our wording, but we definitely shouldn't add a description here that's inconsistent with the rest of the application.

---

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

  public isActiveProjectSource(): boolean {
    return this.activeSyncProjectName !== undefined && this.activeSyncProjectName === this.sourceProjectDoc?.data?.name;

Why are we using the project name as a identifier? Are we assuming the name can never change?

[josephmyers]

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

---

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

Previously, Nateowami wrote…

Why are we using the project name as a identifier? Are we assuming the name can never change?

Short answer: no, the code in this class is attempting to keep it up to date as the sync changes.

The field is only ever set from the sourceProjectDoc or projectDoc, and it's resynced each time the sync progress changes. If the project name changes on the project doc as part of a sync then the field will update accordingly and the logic will continue to work. Even in the case where the name is out of date on the project doc, we're only ever checking this field, which is sourced from the project doc, against what came from the project doc.

If the project doc name changed outside of a sync, then technically this method could produce wrong results outside of a sync (though we're only using it during sync), but at that point the whole field is wrong, and likely other parts of this component. As soon as you sync, the field will fix itself.

---

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

Previously, Nateowami wrote…

Marking this protected and then cheating the type system seems to me to be counterproductive. If this is ever renamed, or someone tries to find all references, it won't work properly. Much better in my opinion to just leave it public.

Both renaming and Find All References work perfectly with this, at least in VS Code. It doesn't seem right having this public. We shouldn't be, and aren't, relying on class consumers to update its properties for it (and we shouldn't make API concessions just for tests).

---

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

Previously, Nateowami wrote…

Nowhere else in the UI do we call this a "reference project". The only place we use the term "reference project" is on the draft sources page. Maybe we should change our wording, but we definitely shouldn't add a description here that's inconsistent with the rest of the application.

Done

[pmachapman]

Reviewed 4 of 5 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @josephmyers and @Nateowami)

---

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

Previously, josephmyers wrote…

Short answer: no, the code in this class is attempting to keep it up to date as the sync changes.

The field is only ever set from the sourceProjectDoc or projectDoc, and it's resynced each time the sync progress changes. If the project name changes on the project doc as part of a sync then the field will update accordingly and the logic will continue to work. Even in the case where the name is out of date on the project doc, we're only ever checking this field, which is sourced from the project doc, against what came from the project doc.

If the project doc name changed outside of a sync, then technically this method could produce wrong results outside of a sync (though we're only using it during sync), but at that point the whole field is wrong, and likely other parts of this component. As soon as you sync, the field will fix itself.

There is at least one project with the same name as its back translation source on live. Can we use a different differentiator (i.e. id) to determine if the source project is syncing?

---

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

Previously, josephmyers wrote…

Both renaming and Find All References work perfectly with this, at least in VS Code. It doesn't seem right having this public. We shouldn't be, and aren't, relying on class consumers to update its properties for it (and we shouldn't make API concessions just for tests).

I agree with Nathaniel on this one - if we were exporting our TypeScript module to another system/codebase/npm/etc, then yes, it would make sense to keep it protected, but as we don't, and TypeScript doesn't have an internal modifier, I think that functions called from unit tests should be public, unless they are particularly obscure.

@Nateowami I can't find any docs on coding standards in TypeScript about public/private and tests (unlike C# which has a lot of material on this question). Maybe we should look at a form of TypeScript coding standards?

---

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

    "synchronize": "Sync with Paratext",
    "synchronize_project": "Synchronize {{ projectName }} with Paratext",
    "synchronize_reference_project": "Synchronizing source text:",

Can we rename this key to synchronizing_source_project or synchronize_source_project or similar? We don't use the phrase reference project anywhere else to refer to the source project.

Code quote:

synchronize_reference_project

[josephmyers]

Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @Nateowami and @pmachapman)

---

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

Previously, pmachapman (Peter Chapman) wrote…

There is at least one project with the same name as its back translation source on live. Can we use a different differentiator (i.e. id) to determine if the source project is syncing?

Done. Thanks, I didn't know that was possible!

---

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

Previously, pmachapman (Peter Chapman) wrote…

I agree with Nathaniel on this one - if we were exporting our TypeScript module to another system/codebase/npm/etc, then yes, it would make sense to keep it protected, but as we don't, and TypeScript doesn't have an internal modifier, I think that functions called from unit tests should be public, unless they are particularly obscure.

@Nateowami I can't find any docs on coding standards in TypeScript about public/private and tests (unlike C# which has a lot of material on this question). Maybe we should look at a form of TypeScript coding standards?

I'm confused. Both of you guys are recommending against pretty basic OOP principles, even when we have an alternative we're using in many other places.

The issue here at least is less about coding standards and more about ease of testing. The only reason this test needs this method is because the notification service doesn't have an actual, i.e. usable in tests, way to notify its users. You'll notice it has no tests. It's a direct line to SignalR, and so both end up really difficult to test. Which is exactly why bypassing the access makes sense here.

If bypassing access in tests is cheating, then that's really good for me to know. But we should probably fix all the other tests that are using it, too.

---

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

Previously, pmachapman (Peter Chapman) wrote…

Can we rename this key to synchronizing_source_project or synchronize_source_project or similar? We don't use the phrase reference project anywhere else to refer to the source project.

Done.

[Nateowami]

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

---

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

Previously, josephmyers wrote…

Done. Thanks, I didn't know that was possible!

@josephmyers Not only can projects have the same name, projects can have the same shortName, which is actually used as an identifier in a lot of cases.