In D4980#144730, @abosh wrote:

I'm a bit confused on what it means if uploadResult (or uploadThumbnailResult) fails. Is it just to push a failed 'upload' step to steps? @ashoat sort of mentioned this in D932, although he was referring to uploadThumbnailResult only.

It means we failed to upload the primary media or thumbnail media.

As of this diff we handle the success/failure of an upload as all or nothing (both primary media and thumbnail successfully uploaded = success, everything else is failure).

In the future we can break out more complex error cases (eg if only thumbnail upload fails, we can avoid re-uploading the primary media) and have separate entries in the media mission steps.

Breaking those steps apart will be addressed as part of related refactoring later in the stack. For now, we're "sneaking" the thumbnail upload step with the existing primary upload step.

If we assign uploadResult and uploadThumbnailResult in a try / catch block, one would assume any reason for it to fail would be caught in there. to be sure, though, we could add an invariant check after the try / catch block. But to have no checks seems...not great. Instead, we have a lot of conditionals that are lengthy and confusing, like on line 713. By this point, the media / related thumbnail, if any, should be uploaded, and if they aren't, we should capture this in error cases instead of having to wrap expected behavior in long conditionals. Even in the original code, there are long sections of code dependent on uploadResult being truthy.

Aren't we doing that?

...
} catch (e) {
      uploadExceptionMessage = getMessageForException(e);
      onUploadFailed(localMediaID, 'upload failed');
      mediaMissionResult = {
        success: false,
        reason: 'http_upload_failed',
        exceptionMessage: uploadExceptionMessage,
      };
    }
...

I realize that later on in uploadFile we push an 'upload' step to steps which contains a success property that is dependent on !!uploadResult. So perhaps we don't want to simply exit if the thumbnail upload/media upload failed. But for now, we can definitely refactor this better since the if statement on line 713 is quite confusing and long sections of indented code in a case that is 'expected' can be hard to read.

What is confusing about the if statement on line 713? We just break out the prerequisites for success for mediaType === 'video' and mediaType === 'photo'?

We definitely don't want to exit if the thumbnail upload/media upload failed because we have steps that occur afterwards regardless of success/failure (media report, cleaning up temporary files, etc).

That is, we expect the upload to occur correctly in uploadFile. So, we should only have conditional checks for if this doesn't occur (like if !uploadResult is true), and handle those error cases accordingly. As a reader, that makes more sense to me, since the non-indented, non-conditional portions of the code represent the "expected" behavior of uploadFile. Whereas currently we have long sections of indented and conditional code if uploadResult is true, which should just be expected behavior.

We have code that needs to run whether or not there is a failure (media mission, cleaning up temporary files, etc). Not sure there's a cleaner way to structure the changes here barring a more invasive refactor (which is "scheduled" for later in the stack)

There is an invariant on line 736, which makes sense there since if that if statement is entered the thumbnail should have been uploaded.

The invariant is just there to appease flow, it should already know a thumbnail exists as a consequence of the outer conditional

(processedMedia.mediaType === 'video' && uploadResult && uploadThumbnailResult)

But it feels like the differences between the upload succeeding vs. the upload failing need to be factored better.

Agree that there's plenty of room for refactoring once we have things working end-to-end.

fix

rebase around other diffs before landing