[native] Process and upload image for image avatar in `EditAvatar`
ClosedPublic
Actions

Authored by atul on Apr 17 2023, 6:44 PM.

Details

Reviewers

ashoat
ginsu

Commits

rCOMM944f05c38c6a: [native] Process and upload image for image avatar in `EditAvatar`

Summary

We're able to process and upload image for image avatar and get back uploadID. However, before we're able to include uploadID in an image avatar update request we'll need additional information about thread/user passed into EditAvatar. It looks like the actual Avatars are being displayed as child of EditAvatar... I wonder if it makes sense to move UserAvatar and ThreadAvatar "within` EditAvatar and display them conditionally based on props?

Depends on D7476

Test Plan

Logged result of processMedia and observed that it was as expected.

Checked uploads table and saw uploaded images:

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D7481 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Apr 17 2023, 6:44 PM

Herald added a subscriber: tomek. · View Herald TranscriptApr 17 2023, 6:44 PM

atul published this revision for review.Apr 17 2023, 6:44 PM

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

Harbormaster completed remote builds in B18433: Diff 25248.Apr 17 2023, 7:03 PM

atul added inline comments.Apr 17 2023, 7:37 PM

native/components/edit-avatar.react.js
53 ↗	(On Diff #25248)	Note that we aren't explicitly passing in `uploadBlob` as we are in `input-state-container`. This is currently using `lib/utils/upload-blob:uploadBlob(...)` instead of the one in `input-state-container` that uses `react-native-background-upload`. This worked as is... though we probably want to use the same `uploadBlob` as `input-state-container` after we get this working end-to-end?

You forgot to annotate the parent of this diff. This has come up before... please always try to annotate your diff dependencies. It would probably be easier for you if you keep a 1-to-1 correspondence between branches and diff stacks, but if you want to do things differently, it's fine as long as you are able to remember to annotate your diff dependencies going forward.
Accepting due to the state of this work, but normally I would request changes here. Please read through all of the comments carefully and address them before landing
- If you disagree with the comment about returning a Promise and awaiting it, please re-request review

native/components/edit-avatar.react.js
41 ↗	(On Diff #25248)	It would be good to explain why we're not using `dispatchActionPromise` here, as we typically don't allow "naked" server calls without it. In this case, we're following the convention introduced in `InputStateContainer`, where we don't dispatch Redux actions for these server calls
45 ↗	(On Diff #25248)	Why not destructure `dimensions` here too, given you're using it below?
47 ↗	(On Diff #25248)	Why aren't we returning the promise from here? Is it intentional?
53 ↗	(On Diff #25248)	Can you create a follow-up task before landing this diff and link it here? We could probably extract the `uploadBlob` function in `InputStateContainer` into a hook We definitely want to replace `react-native-background-upload`, but probably with `expo-file-system`'s upload functionality rather than just using `fetch` in JS. Would be easier to do if we have one shared callsite
53–54 ↗	(On Diff #25248)	Is it necessary to include `loop: undefined` here?
72 ↗	(On Diff #25248)	It continues to really frustrate me that you're iterating on a function that has no callsites in the codebase. I strongly, strongly feel that this is an anti-pattern and in any other scenario I would request changes
109 ↗	(On Diff #25248)	Why aren't you `await`ing this? Was it intentional? We'll probably eventually need to track the "loading state" somehow, and probably the easiest solution will be to have some React state that is unset after the upload finishes. Given that, I think you should be `await`ing here.

This revision is now accepted and ready to land.Apr 18 2023, 5:49 AM

atul edited the summary of this revision. (Show Details)Apr 18 2023, 8:01 AM

atul added a parent revision: D7476: [native] Introduce `openPhotoGallery` in `EditAvatar` component.

atul added inline comments.Apr 18 2023, 8:11 AM

native/components/edit-avatar.react.js
41 ↗	(On Diff #25248)	I was matching `InputStateContainer`. Should we be using `dispatchActionPromise` here?
45 ↗	(On Diff #25248)	Will update
47 ↗	(On Diff #25248)	Was going to return promise and `await` from `openPhotoGallery` in subsequent diff where we handle processing/uploading/avatar update progress + states (loading/error). I can update this diff.
53–54 ↗	(On Diff #25248)	It can be removed, will update
109 ↗	(On Diff #25248)	We handle progress of processing/upload/avatar update + error states in later diff. I'll just update to add an `await` here for now.