Move registrationMode-related logic from `setUserAvatar` to `nativeSetUserAvatar`
ClosedPublic
Actions

Authored by atul on Jun 27 2023, 5:19 PM.

Details

Reviewers

ashoat
ginsu
rohan

Commits

rCOMM24e5fa169ea0: Move registrationMode-related logic from `setUserAvatar` to…

Summary

The registrationMode-related code is native-specific and was pretty interwoven with the logic in setUserAvatar/updateImageUserAvatar.

This diff begins to untangle the two and move registrationMode-related logic to native/.../avatar-hooks.js.

As of this diff, setUserAvatar(request: UpdateUserAvatarRequest) is fully platform-agnostic and can be used on both native (via nativeSetUserAvatar) and web (via upcoming webSetUserAvatar).

Depends on D8342

Test Plan

As of D8341, all native codepaths go through nativeSetUserAvatar instead of setUserAvatar.

Continue to be able to set non-image avatars + remove avatars on both native and web as expected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Jun 27 2023, 5:19 PM

Herald added a subscriber: tomek. · View Herald TranscriptJun 27 2023, 5:19 PM

atul added a child revision: D8344: [lib] Rename `setUserAvatar` to `baseSetUserAvatar` in `*EditUserAvatarProvider`.Jun 27 2023, 5:33 PM

atul published this revision for review.Jun 27 2023, 5:34 PM

Harbormaster completed remote builds in B20561: Diff 28196.Jun 27 2023, 5:36 PM

atul mentioned this in D8345: Move registrationMode-related logic from `updateImageUserAvatar` to `nativeUpdateUserImageAvatar`.Jun 27 2023, 5:59 PM

rohan accepted this revision.Jun 28 2023, 10:09 AM

This revision is now accepted and ready to land.Jun 28 2023, 10:09 AM

I'm confused – what was not platform-agnostic about this before? It seems like you still need the state in lib, so I'm not really sure what this diff is achieving

This revision now requires changes to proceed.Jun 28 2023, 6:20 PM

In D8343#246573, @ashoat wrote:

I'm confused – what was not platform-agnostic about this before? It seems like you still need the state in lib, so I'm not really sure what this diff is achieving

updateImageUserAvatar previously took (selection: NativeMediaSelection) as an argument to satisfy the mediaSelection: NativeMediaSelection field of the object passed to registrationModeRef.current.successCallback when needsUpload is true.

As of D8346 the argument is changed to (imageAvatarUpdateRequest: ImageAvatarDBContent) so updateUserImageAvatar can be used by both web and native. It becomes the responsibility of platform-specific avatar-hooks to provide platform-agnostic ImageAvatarDBContent to *EditUserAvatarProvider.

Because I was moving the registrationMode stuff out of updateImageUserAvatar, I decided to do the same for setUserAvatar for consistency/symmetry. I thought it'd be best if the registrationMode stuff for image and non-image avatars were handled in the same file in nativeUpdateUserImageAvatar(...) and nativeSetUserAvatar(...) instead of in nativeUpdateUserImageAvatar(...) in avatar-hooks and setUserAvatar(...) in *EditUserAvatarProvider.

registrationMode is only relevant on native because there's no registration flow on web (and based on my understanding there won't be in the foreseeable future). We are leaving setRegistrationMode, getRegistrationModeEnabled, etc here because it doesn't affect anything on web and I wanted to limit the amount of refactor work.