Page MenuHomePhabricator

[native] Replace logging with `Alert`s in `selectFromGalleryAndUpdateUserAvatar`
ClosedPublic

Authored by atul on Apr 19 2023, 12:03 PM.
Tags
None
Referenced Files
F3150512: D7530.diff
Mon, Nov 4, 11:24 PM
Unknown Object (File)
Sat, Nov 2, 12:26 PM
Unknown Object (File)
Fri, Nov 1, 9:13 AM
Unknown Object (File)
Fri, Nov 1, 9:13 AM
Unknown Object (File)
Fri, Nov 1, 9:13 AM
Unknown Object (File)
Fri, Nov 1, 9:12 AM
Unknown Object (File)
Fri, Nov 1, 9:09 AM
Unknown Object (File)
Oct 1 2024, 12:30 PM
Subscribers

Details

Summary

Replace console.log with Alerts so if something goes wrong with media selection/processing/uploading we can at least give the user an indication that the operation failed.

Proper error handling would include cleaning up temporary files + creating a media mission report, but deferring that for now.

NOTE: Currently if dispatchActionPromise of updateUserAvatarCall fails it'll fail silently. One possibility could be to wrap the call like we do in siwe-login-form to introspect (see below), or we could have a useEffect that Alert.alert(...)s if loadingStatus === 'error'?

046369.png (346×1 px, 64 KB)


Depends on D7529

Test Plan

Flipped boolean and observed that Alert appeared as expected:

Simulator Screenshot - Fresh iPhone 14 Pro - 2023-04-19 at 14.55.31.png (2×1 px, 573 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Apr 19 2023, 12:03 PM
atul edited the test plan for this revision. (Show Details)
atul edited the summary of this revision. (Show Details)
atul edited the summary of this revision. (Show Details)
atul edited the summary of this revision. (Show Details)

Can we replace the ALERT_TITLES with "Alert title", to make them more human-readable?

One possibility could be to wrap the call like we do in siwe-login-form to introspect (see below)

I think that's the best option. We do that in a bunch of other places in the app, wrapping the server call with a try-catch and showing an Alert if the catch blocks activates

This revision is now accepted and ready to land.Apr 19 2023, 12:20 PM

Can we replace the ALERT_TITLES with "Alert title", to make them more human-readable?

Will do

One possibility could be to wrap the call like we do in siwe-login-form to introspect (see below)

I think that's the best option. We do that in a bunch of other places in the app, wrapping the server call with a try-catch and showing an Alert if the catch blocks activates

Gotcha, and without having looked at the code will every scenario that leads to UPDATE_USER_AVATAR_FAILED result in exception being caught with a try/catch around the updateUserAvatarCall call? Was going to take a look, but just in case you know off the top of your head. (I'm guessing that's what happens internally in dispatchActionPromise but I haven't read it to verify)

This is much better than the loadingStatus approach because that'd require setting overrideKey and customKeyName to distinguish between types of avatar update request.

atul edited the summary of this revision. (Show Details)

address feedback (human readable alert titles)

This revision was landed with ongoing or failed builds.Apr 19 2023, 12:39 PM
This revision was automatically updated to reflect the committed changes.