Page MenuHomePhabricator

[web] Fix creating subchannels in chats in GENESIS
ClosedPublic

Authored by inka on Thu, Jul 4, 8:38 AM.
Tags
None
Referenced Files
F2322112: D12667.id42037.diff
Tue, Jul 23, 7:33 AM
F2319909: D12667.id42207.diff
Mon, Jul 22, 10:54 PM
F2319852: D12667.id42198.diff
Mon, Jul 22, 10:29 PM
Unknown Object (File)
Fri, Jul 19, 3:32 AM
Unknown Object (File)
Fri, Jul 19, 1:33 AM
Unknown Object (File)
Thu, Jul 18, 1:03 PM
Unknown Object (File)
Wed, Jul 17, 10:00 PM
Unknown Object (File)
Sun, Jul 14, 8:40 AM
Subscribers
None

Details

Summary

issue: ENG-8710
chats in GENESIS cannot be looking at the list of GENEISS members because every member sees just themselves in GENESIS

Test Plan

tested that it is possible to create a subchannel to a chat in GENESIS
Tested that we see the same list of users as on native both when there is no search text, and when there is some search text

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Thu, Jul 4, 8:59 AM

On Linear, you asked whether we should check the parent or the top ancestor under GENESIS for permissions. I suggested that the top ancestor made more sense, but I probably care more about consistency with native. What do we do there?

This revision is now accepted and ready to land.Thu, Jul 4, 11:31 AM
inka planned changes to this revision.Fri, Jul 5, 2:48 AM

On Linear, you asked whether we should check the parent or the top ancestor under GENESIS for permissions. I suggested that the top ancestor made more sense, but I probably care more about consistency with native. What do we do there?

I answer on linear, so maybe we decide there and update this diff if needed. I fill not land this until we make a decision

Use new approach - see the discussion in ENG-8710 - we want to unify web and native

This revision is now accepted and ready to land.Wed, Jul 10, 1:04 AM
inka requested review of this revision.Wed, Jul 10, 1:13 AM

For now it is possible to add any user that shows up in search results. On native we handle this in the UI by showing a popup if the user wants to add someone they cannot:

image.png (1×744 px, 306 KB)

On web we decided that for now we will just not display those users. This will be handled in the next diff

tomek added inline comments.
web/modals/threads/create/compose-subchannel-modal.react.js
90 ↗(On Diff #42198)

There's no need to memoize this - the values returned are constant and are cheap to compute.

web/settings/relationship/add-users-utils.js
198 ↗(On Diff #42198)
211 ↗(On Diff #42198)

Can we use a more descriptive name?

This revision is now accepted and ready to land.Wed, Jul 10, 4:25 AM

Shouldn't the test plan be expanded?

Please wrap the previouslySelectedUserIDs declaration is a useMemo before landing! If you disagree or if the feedback doesn't make sense, please re-request review

web/settings/relationship/add-users-utils.js
200–245 ↗(On Diff #42207)

Love seeing all this custom logic being removed in favor of a shared utility!

211–213 ↗(On Diff #42207)

You are creating a new array on every invocation here, which will prevent usePotentialMemberItems from being able to memoize effectively

This needs to be wrapped in a React.useMemo

223 ↗(On Diff #42207)

I noticed you replaced useSortedENSResolvedUsers with this. Are we sure that the sorting still happens? (Or perhaps userSearchResults does its own sorting that we prefer?)

Address review

web/settings/relationship/add-users-utils.js
223 ↗(On Diff #42207)

This was supposed to better match native, but I can use useSortedENSResolvedUsers