Paths

Table of Contentst

[native] Make `failed` `boolean` in `PendingMultimediaUpload`
ClosedPublic
Actions

Authored by • abosh on Aug 19 2022, 1:46 PM.

Details

Reviewers

atul
tomek
• jacek
ashoat

Commits

rCOMM73e1804de16e: [native] Make `failed` `boolean` in `PendingMultimediaUpload`

Summary

Change failed from ?string to boolean in PendingMultimediaUpload. This diff also adds an optional failureMessage prop to PendingMultimediaUpload to store the error message in case failed is true.

This is part of an effort to refactor InputStateContainer since some of its logic is confusing. More context here. The main reason why failed was confusing is highlighted in the linked comment, but this section made little sense when failed was a string:

input-state-container.react.js

uploadInProgress = () => {
  if (this.props.ongoingMessageCreation) {
    return true;
  }
  for (const localMessageID in this.state.pendingUploads) {
    const messagePendingUploads = this.state.pendingUploads[localMessageID];
    for (const localUploadID in messagePendingUploads) {
      const { failed } = messagePendingUploads[localUploadID];
      if (!failed) {
        return true;
      }
    }
  }
  return false;
}

Now that failed is a boolean, if !failed is true, that means that there is a non-failed upload (since failed is false), which means it is in progress. If every single pending upload's failed prop is true, then no uploads are in progress because they all failed.

This same refactoring of failed for web will be the next diff.

Test Plan

Made sure sending photos and videos worked as expected on iOS Simulator and added console.log(...) statements to log pendingUploads to view failed as a boolean.

Searched through the codebase for all usages of PendingMultimediaUpload to do this refactor.

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• abosh created this revision.Aug 19 2022, 1:46 PM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 19 2022, 1:46 PM

• abosh edited the summary of this revision. (Show Details)Aug 19 2022, 1:49 PM

• abosh edited the test plan for this revision. (Show Details)

• abosh edited the summary of this revision. (Show Details)

• abosh added inline comments.

native/input/input-state-container.react.js
983–985 ↗	(On Diff #15795)	Works because `failed` is a boolean!

• abosh added inline comments.Aug 19 2022, 1:53 PM

native/input/input-state-container.react.js
948 ↗	(On Diff #15795)	This is the only case where `failureMessage` is used. It stores the `errorMessage` `uploadFile` passes to `handleUploadFailure`, but `uploadFile` also returns the error message to the caller (`uploadFiles`).

• abosh edited the test plan for this revision. (Show Details)Aug 19 2022, 1:54 PM

Harbormaster completed remote builds in B11483: Diff 15795.Aug 19 2022, 1:56 PM

• abosh requested review of this revision.Aug 19 2022, 1:56 PM

Remove ? from failureMessage's type, making it only string.

• abosh mentioned this in D4891: [web] Make `failed` `boolean` in `PendingMultimediaUpload`.Aug 19 2022, 2:03 PM

• abosh added a child revision: D4891: [web] Make `failed` `boolean` in `PendingMultimediaUpload`.Aug 19 2022, 2:04 PM

Harbormaster completed remote builds in B11484: Diff 15796.Aug 19 2022, 2:07 PM

• abosh added a child revision: D4892: [native] Use `Array.some(...)` in `uploadInProgress`.Aug 19 2022, 2:13 PM

• abosh edited the summary of this revision. (Show Details)Aug 23 2022, 6:58 AM

• abosh edited the summary of this revision. (Show Details)

tomek requested changes to this revision.Aug 24 2022, 5:18 AM

tomek added inline comments.

native/input/input-state-container.react.js
948 ↗	(On Diff #15795)	I wound't say it is used here. We only set it as a prop of an object, but this prop doesn't seem to be used. Do we really need this prop at all? I'm asking, because if there was a code which used `failed` message content, this diff should have a place where `failed` is replaced by `failureMessage`, but I don't see thing like that.
native/input/input-state.js
12–15	This type doesn't fully express what happens here. Actually, it seems like we have either `{ failed: false }` or `{ failed: true, failureMessage: string }`

This revision now requires changes to proceed.Aug 24 2022, 5:18 AM

I wound't say it is used here. We only set it as a prop of an object, but this prop doesn't seem to be used. Do we really need this prop at all? I'm asking, because if there was a code which used failed message content, this diff should have a place where failed is replaced by failureMessage, but I don't see thing like that.

@tomek you're right, I should have been clearer in my original description. The failureMessage prop is unused and should be removed.

The reason for this is as follows:
For native, the only place where failed is truthy in InputStateContainer is on line 947. handleUploadFailure` sets failed to message for the upload that failed in the current state for the pendingUploads.

The only place handleUploadFailure is called (in the entire codebase) is in uploadFile which is also in InputStateContainer. handleUploadFailure is called in the fail method in uploadFile, which is called whenever an exception is thrown in uploadFile.

After fail is called, finish is called (another uploadFile method), which returns the same error message failed is set to in handleUploadFailure. This error message is returned to uploadFiles, which is the caller of uploadFile.

uploadFiles then gathers all return values from calling uploadFile on each file and filters out non-truthy return values. Remember that the return value from uploadFile is an error message, which is either truthy (since failed was truthy) or falsey (undefined) if no failures occurred.

Then, if there were any truthy error messages (which means an upload failure occurred), uploadFiles will display the errors to the user (displayActionResultModal):

To be doubly sure failed isn't used for it's error message and instead only for its truthiness, I searched for usages of failed throughout the whole codebase. Most of it is unrelated (maybe failed should be renamed to uploadFailed because it's more accurate), and didn't find any usages of failed for its error message.

Additionally, in my Test Plan, I note that I searched for all usages of PendingMultimediaUpload. (This is definitely much easier than searching for failed through the whole codebase.) There are no usages of failed's content, only its truthiness. So, I remade it into a boolean. I will remove the failureMessage prop in the following update, however.

This investigation makes me think that failed is a string because of debugging purposes. For example, console.log(...)ing pendingUploads will show the exact error message that caused the upload to fail. So maybe we should keep it this way, but the code becomes confusing since the way failed is actually used in the codebase is as a boolean, not a string.

Remove failureMessage prop

• abosh attached a referenced file: F148338: image.png. (Show Details)Aug 24 2022, 8:29 AM

Harbormaster completed remote builds in B11589: Diff 15934.Aug 24 2022, 8:37 AM

Thanks for the explanation! I think that if the exact content of failed was important to us, there would be some code that looked at it, and not just the truthiness.

This investigation makes me think that failed is a string because of debugging purposes. For example, console.log(...)ing pendingUploads will show the exact error message that caused the upload to fail. So maybe we should keep it this way, but the code becomes confusing since the way failed is actually used in the codebase is as a boolean, not a string.

This makes sense, but maybe we should spend some time verifying this. One thing to check is a diff / commit that introduced this property - there might be some context there. @ashoat do you remember why this prop was a string instead of a boolean?