HomePhabricator
Diffusion Comm 61eb0e026466

[native] Match changes made to `EditUserAvatar` for `EditThreadAvatar`

Description

[native] Match changes made to EditUserAvatar for EditThreadAvatar

Summary:
As stated in D7527:

For convenience, I'm going to have a multiple diffs where I update the EditUserAvatar flow, and then a single diff where I update the EditThreadAvatar flow to match. There's a lot of repetition, but I have ideas on how to consolidate things once we have a shippable experience.

This is that diff that brings thread avatars "up to speed" with user avatars. In hindsight it may have been good to make the changes simultaneously so this diff isn't so difficult to review.

But effectively this diff handles:

D7527: Updating useSelectFromGalleryAndUpdateThreadAvatar avatar with new processingOrUploadInProgress state and combining that with info from updateThreadAvatarLoadingStatusSelector to determine whether avatar update is "in progress" and to set inProgress accordingly. EditThreadAvatar pulls the inProgress boolean from useSelectFromGalleryAndUpdateThreadAvatar to (partially) determine whether to display a spinner. The changes here that correspond to D7527 are in useSelectFromGalleryAndUpdateThreadAvatar (we also change how we call the hook in EditThreadAvatar, but that's just wrapping in brackets). Almost all the changes here were copy/pasted from useSelectFromGalleryAndUpdateUserAvatar with slight differences for the endpoint and loading selector.

D7528: Updating useRemoveThreadAvatar hook to return isLoading. This is similar to the changes made to match D7527 described above, but effectively getting the avatar update status from updateThreadAvatarLoadingStatusSelector and exposing whether or not it's "loading" by returning inProgress. The changes here are in useRemoveThreadAvatar. The changes, again, were effectively copy/pasted from useRemoveUserAvatar.

D7529: Updating EditThreadAvatar to display a spinner based on the isLoading status from useSelectFromGalleryAndUpdateThreadAvatar and useRemoveThreadAvatar we get from the previous two changes. The code here is identical to the code in EditUserAvatar. Definitely some room for refactoring after this is shippable... I have a list of changes I want to make.

D7530: Basically replace the console.logs with Alert.alerts. The changes here are entirely in useSelectFromGalleryAndUpdateThreadAvatar and are identical to those in useSelectFromGalleryAndUpdateUserAvatar.

Apologies in advance to the reviewer, I probably should've just included the changes to threads along with the changes to users to make things easier to review. For what it's worth the changes are almost identical and effectively copy/paste. The only differences are the loading status selectors and the endpoints being called by dispatchActionPromise.

Test Plan:
Tested these changes thoroughly.

  1. Set and unset avatar to images/emoji/etc and observed that spinner appeared as expected.
  2. Set and unset other thread settings and observed that spinner did not appear... again as expected.
  3. Logged the value of isLoading from both useSelectFromGalleryAndUpdateThreadAvatar and useRemoveThreadAvatar and ensured that they were exactly as expected.
  4. Toggled various booleans and ensures that the Alert.alerts appeared as expected.
  5. "Broke" various endpoints on keyserver (throw new ServerError() at entrypoint) and ensured that failures were displayed on client as expected (broke upload and threadUpdateResponder and made sure correct Alerts were displayed)
  6. Made a bunch of simultaneous thread avatar change requests from different users and ensured that things remained in a consistent state (not really in the scope of this diff though)
  7. Went carefully line-by-line and compared to EditUserAvatar (and accompanying user avatar hooks) and made sure that the two were identical except for loading status selectors and endpoints.

Reviewers: ashoat, ginsu

Reviewed By: ashoat

Subscribers: tomek

Differential Revision: https://phab.comm.dev/D7547