SF-3835 Do not allow uploading training data when offline

[pmachapman]

Rather than changing the error message, I took the same approach as the chapter audio dialog and disabled uploading when offline

---

This change is 

[github-actions]

📸 Screenshot diff deployed! (1 change)

View the visual diff at: https://pr-3985--sf-screenshot-diffs.netlify.app

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.06%. Comparing base (ed28d14) to head (5400b20).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ning-data/training-data-upload-dialog.component.ts	66.66%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3985   +/-   ##
=======================================
  Coverage   81.06%   81.06%           
=======================================
  Files         645      645           
  Lines       41551    41557    +6     
  Branches     6766     6772    +6     
=======================================
+ Hits        33684    33689    +5     
- Misses       6755     6756    +1     
  Partials     1112     1112           

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

[Nateowami]

Rather than changing the error message, I took the same approach as the chapter audio dialog and disabled uploading when offline

The problem is: All errors when uploading a file (regardless of cause; they could be network related, bad file, or something else) are attributed to a malformed file, and the user is shown this message:

Your file needs to have two columns, and every row must have a value in both columns. Please fill in any empty cells and try again.

The error message should be shown only when a specific error code (could be HTTP status code, or something returned via JSONRPC) indicates a problem with the file, and the error message should contemplate the same breadth of potential problem as the code that triggers it (for example, I don't know what happens when you upload an unsupported file type, but if it can trigger the message, then the message should specify to check that the file type is one of the supported types). Disabling upload when we know we're offline doesn't stop network errors from occurring; it only reduces their frequency.

Also, we generally avoid disabling buttons, except when a feature is unavailable due to the user being offline, and then only disabling them in the context where we would show a message saying the user is offline. So in general it would be better to let the user open the upload dialog, but show a message about the user being offline inside the dialog.

@Nateowami made 1 comment.
Reviewable status: 0 of 7 files reviewed, all discussions resolved.

[pmachapman]

The error message should be shown only when a specific error code (could be HTTP status code, or something returned via JSONRPC) indicates a problem with the file, and the error message should contemplate the same breadth of potential problem as the code that triggers it (for example, I don't know what happens when you upload an unsupported file type, but if it can trigger the message, then the message should specify to check that the file type is one of the supported types). Disabling upload when we know we're offline doesn't stop network errors from occurring; it only reduces their frequency.

I have updated this PR to catch offline and format errors, and display an appropriate error message. I tried to do it in a way that would not break the other code dependent on the upload returning undefined on error.

Also, we generally avoid disabling buttons, except when a feature is unavailable due to the user being offline, and then only disabling them in the context where we would show a message saying the user is offline. So in general it would be better to let the user open the upload dialog, but show a message about the user being offline inside the dialog.

I agree. I made it offline because all the rest of the form cannot be used if offline, and most of the form was already disabled if offline - uploading the training sources offline, then connecting to click submit seems like an odd user workflow. If connectivity is interrupted during upload, then an offline error will be displayed.

@pmachapman made 1 comment.
Reviewable status: 0 of 12 files reviewed, all discussions resolved (waiting on Nateowami).

[Nateowami]

@Nateowami made 1 comment.
Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on pmachapman).

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/file.service.ts line 129 at r2 (raw file):

   * @param returnUndefinedForErrors If true, errors will be rethrown and undefined will only be returned if offline.
   */
  async onlineUploadFileOrFail(

Wouldn't the canonical way to do this be to have onlineUploadFile and tryOnlineUploadFile (the latter of which wraps the former in try/catch)?

The current method signature creates code that does not make clear what is happening:

const audioUrl: string | undefined = await this.fileService.onlineUploadFileOrFail(
  FileType.Audio,
  this.data.projectId, // can't verify that this arg is in the correct position and didn't get swapped with another string arg
  TextAudioDoc.COLLECTION, // can't verify that this arg is in the correct position and didn't get swapped with another string arg
  objectId(), // can't verify that this arg is in the correct position and didn't get swapped with another string arg
  this.audio!.blob!, // probably correct position since it's a Blob so it would fail type check if it were in wrong position
  this.audio!.fileName!, // can't verify that this arg is in the correct position and didn't get swapped with another string arg
  true, // no idea what this is for unless I look up method
  true // no idea what this is for unless I look up method
);

[pmachapman]

@pmachapman made 1 comment.
Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on Nateowami).

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/file.service.ts line 129 at r2 (raw file):

Previously, Nateowami wrote…

Wouldn't the canonical way to do this be to have onlineUploadFile and tryOnlineUploadFile (the latter of which wraps the former in try/catch)?

The current method signature creates code that does not make clear what is happening:

const audioUrl: string | undefined = await this.fileService.onlineUploadFileOrFail(
  FileType.Audio,
  this.data.projectId, // can't verify that this arg is in the correct position and didn't get swapped with another string arg
  TextAudioDoc.COLLECTION, // can't verify that this arg is in the correct position and didn't get swapped with another string arg
  objectId(), // can't verify that this arg is in the correct position and didn't get swapped with another string arg
  this.audio!.blob!, // probably correct position since it's a Blob so it would fail type check if it were in wrong position
  this.audio!.fileName!, // can't verify that this arg is in the correct position and didn't get swapped with another string arg
  true, // no idea what this is for unless I look up method
  true // no idea what this is for unless I look up method
);

Done. Thank you!

[Nateowami]

@Nateowami reviewed 8 files and all commit messages, and made 3 comments.
Reviewable status: 8 of 12 files reviewed, 3 unresolved discussions (waiting on pmachapman).

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/file.service.ts line 129 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Done. Thank you!

Thanks for fixing this. Seems like "or fail" doesn't belong in the method name since it no longer swallows errors.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/training-data/training-data-multi-select.component.ts line 70 at r3 (raw file):

  openUploadDialog(): void {
    if (this.activatedProjectService.projectId == null) return;
    if (!this.appOnline) return;

Isn't this the wrong place for a check like this? Seems like the caller should be doing the check. Or alternatively the dialog should open and say it's not available offline. Doing nothing doesn't seem like the correct approach.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/file.service.ts line 145 at r3 (raw file):

      } catch {}
    }
    return undefined;

I really don't think defining undefined as meaning offline is good practice. It's a value you have to remember to check for, it's not obvious what it means, and it's the value that is most likely to accidentally pop up in this language, meaning findOrUpdateCache mistakenly returning undefined would not be improbable. If we want to create a value to check for instead of an exception, a string (that the type system is aware of) would not pop up accidentally, and would be obvious what it means.

That said, I really don't think this method should be checking if we're online at all. Network conditions are inherently unpredictable, and therefore we can't use guards to completely prevent requests that will fail for network reasons. Therefore I think it makes most sense to put such guards at the UI level (e.g. disabled buttons with explanatory text), and not have business logic methods named doThing that really mean doThingButOnlyUnderCertainCircumstances, and to consider such guards nice-to-have, not absolute requirements.

[pmachapman]

@pmachapman made 3 comments.
Reviewable status: 8 of 12 files reviewed, 3 unresolved discussions (waiting on Nateowami).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/training-data/training-data-multi-select.component.ts line 70 at r3 (raw file):

Previously, Nateowami wrote…

Isn't this the wrong place for a check like this? Seems like the caller should be doing the check. Or alternatively the dialog should open and say it's not available offline. Doing nothing doesn't seem like the correct approach.

You are right. I'm not sure what I was thinking, as the button will be disabled if offline anyway, and if the user is offline while using the dialog, a warning is shown when they save.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/file.service.ts line 129 at r2 (raw file):

Previously, Nateowami wrote…

Thanks for fixing this. Seems like "or fail" doesn't belong in the method name since it no longer swallows errors.

I've merged this with onlineUploadFile to hopefully reduce some of the kludge.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/file.service.ts line 145 at r3 (raw file):

Previously, Nateowami wrote…

I really don't think defining undefined as meaning offline is good practice. It's a value you have to remember to check for, it's not obvious what it means, and it's the value that is most likely to accidentally pop up in this language, meaning findOrUpdateCache mistakenly returning undefined would not be improbable. If we want to create a value to check for instead of an exception, a string (that the type system is aware of) would not pop up accidentally, and would be obvious what it means.

That said, I really don't think this method should be checking if we're online at all. Network conditions are inherently unpredictable, and therefore we can't use guards to completely prevent requests that will fail for network reasons. Therefore I think it makes most sense to put such guards at the UI level (e.g. disabled buttons with explanatory text), and not have business logic methods named doThing that really mean doThingButOnlyUnderCertainCircumstances, and to consider such guards nice-to-have, not absolute requirements.

You are right. The HttpClient will throw an exception if offline. I have updated the use case to test for that. I've moved the offline check to tryOnlineUploadFile as it seemed to only be required for the upload offline/move to cache workflow.

[Nateowami]

@Nateowami reviewed 1 file, made 2 comments, and resolved 1 discussion.
Reviewable status: 5 of 12 files reviewed, 4 unresolved discussions (waiting on pmachapman).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/training-data/training-data-multi-select.component.ts line 41 at r4 (raw file):

  ) {}

  get appOnline(): boolean {

This file and the spec file with it can be reverted.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/training-data/training-data-upload-dialog.component.ts line 134 at r4 (raw file):

        true
      );
      if (fileUrl === undefined) {

This calls onlineUploadFile and then checks if the result is undefined, something only tryOnlineUploadFile should ever return.