Page MenuHomePhabricator

[web] Create a community roles modal to display all roles and member counts
ClosedPublic

Authored by rohan on Jul 21 2023, 9:03 AM.
Tags
None
Referenced Files
F3388668: D8593.diff
Fri, Nov 29, 3:33 PM
Unknown Object (File)
Sat, Nov 23, 11:36 PM
Unknown Object (File)
Sat, Nov 23, 4:24 AM
Unknown Object (File)
Sat, Nov 23, 4:24 AM
Unknown Object (File)
Mon, Nov 11, 4:14 AM
Unknown Object (File)
Sat, Nov 9, 11:26 PM
Unknown Object (File)
Sat, Nov 9, 11:15 PM
Unknown Object (File)
Sat, Nov 9, 10:54 PM

Details

Summary

For a community when the "Roles" option is clicked in the community actions menu, we want to display a modal that lists each of the member counts and role names for that given community. The logic is similar to what was done on native in D8359.

Something to note about the reduxThreadInfo line of code is that it's used so after creating a role, then returning back to this modal, we want to pull in the latest ThreadInfo from the redux store so the modal can show the newly created role in the list of roles. Without this (as was a similar case in native), the new role will only show once the modal is closed and re-opened, since the props have not changed. The effect of this can be seen later in the stack in D8595, where after creating a new role it appears in the community roles modal.

Additional note: any common / shared logic between web and native are tracked to be extracted to a shared utils file in https://linear.app/comm/issue/ENG-4443/[follow-up]-extract-shared-logic-between-web-and-native-to-utils to not delay this goal.

ENG-4426

Depends on D8592

Test Plan

Checked that the community roles modal showed the correct role names and member counts for a given community

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan edited the test plan for this revision. (Show Details)
rohan edited the summary of this revision. (Show Details)
rohan edited the summary of this revision. (Show Details)
rohan edited the summary of this revision. (Show Details)
rohan requested review of this revision.Jul 21 2023, 9:20 AM
atul requested changes to this revision.Jul 26 2023, 11:45 AM

Something to note about the reduxThreadInfo line of code is that it's used so after creating a role, then returning back to this modal, we want to pull in the latest ThreadInfo from the redux store so the modal can show the newly created role in the list of roles. Without this (as was a similar case in native), the new role will only show once the modal is closed and re-opened, since the props have not changed.

I'm not sure I understand this?

First, I'm not sure why the threadInfo prop being passed into CommunityRolesModal isn't being updated once it changes? We should investigate why the threadInfo being passed to CommunityRolesModal is stale and not a function of "state"?

Second, the following block:

const { threadInfo } = props;

const [updatedThreadInfo, setUpdatedThreadInfo] =
  React.useState<ThreadInfo>(threadInfo);

const threadID = threadInfo.id;
const reduxThreadInfo: ?ThreadInfo = useSelector(
  state => threadInfoSelector(state)[threadID],
);

React.useEffect(() => {
  if (reduxThreadInfo) {
    setUpdatedThreadInfo(reduxThreadInfo);
  }
}, [reduxThreadInfo]);

is pretty confusing to me...

Can't we just pass in threadID as a prop to CommunityRolesModal and pull the threadInfo from Redux directly? It seems weird to handle this updating manually when the whole point of React is that it handles updates to state? We should first figure out why the threadInfo being passed into CommunityRolesModal isn't changing + triggering a re-render

This revision now requires changes to proceed.Jul 26 2023, 11:45 AM
In D8593#254333, @atul wrote:

Can't we just pass in threadID as a prop to CommunityRolesModal and pull the threadInfo from Redux directly? It seems weird to handle this updating manually when the whole point of React is that it handles updates to state? We should first figure out why the threadInfo being passed into CommunityRolesModal isn't changing + triggering a re-render

Yeah, I think that is definitely the better move + makes the whole threadInfo logic way simpler and conventional.

As for why the threadInfo isn't changing, I think it might have to do with perhaps how CommunityRolesModal remains open while the role is created. I'm not certain, but I will update this diff to pull the threadInfo directly from Redux to not block it, and meanwhile investigate locally

atul requested changes to this revision.Jul 26 2023, 3:44 PM

Much cleaner!

Let's use .map() instead of .forEach() for rolePanelList and this diff should be good.

web/roles/community-roles-modal.react.js
29–43 ↗(On Diff #29074)

Can we use .map() instead of forEach():

const rolePanelList = React.useMemo(() => 
  Object.keys(roleNamesToMembers).map(roleName => 
    <RolePanelEntry
      key={roleName}
      roleName={roleName}
      memberCount={roleNamesToMembers[roleName]}
    />
  ), 
[roleNamesToMembers]);
This revision now requires changes to proceed.Jul 26 2023, 3:44 PM

Use map instead of forEach

ashoat requested changes to this revision.Jul 28 2023, 7:26 AM

The point of this code:

React.useEffect(() => {
  if (reduxThreadInfo) {
    setUpdatedThreadInfo(reduxThreadInfo);
  }
}, [reduxThreadInfo]);

Is to make sure that the threadInfo we're using is always set, EVEN IF the thread is deauthorized. It protects against an error case where somebody is editing a community's roles, and gets deauthorized from that community in that moment. As currently written, that case would crash the entire app, as it would result in an undefined threadInfo being passed to useRoleMemberCountsForCommunity.

In the future, @rohan it's your responsibility to understand the change you're making, and to explain it to @atul (with references to eg. ThreadSettings) if he doesn't immediately understand why you're doing it. If I recall correctly, I explicitly requested that you write it this way, and explained in the moment why it should be written this way.

This sort of situation is frankly deeply concerning to me... it's another case (following this one) where my review appears to "required". We need to get to a place where the rest of the team is able to put code out (and review code) without needing me to step in. Ultimately, it's on the diff author to understand the changes they're making, and to explain them thoroughly / thoughtfully.

This revision now requires changes to proceed.Jul 28 2023, 7:26 AM

The point of this code:

React.useEffect(() => {
  if (reduxThreadInfo) {
    setUpdatedThreadInfo(reduxThreadInfo);
  }
}, [reduxThreadInfo]);

Is to make sure that the threadInfo we're using is always set, EVEN IF the thread is deauthorized. It protects against an error case where somebody is editing a community's roles, and gets deauthorized from that community in that moment. As currently written, that case would crash the entire app, as it would result in an undefined threadInfo being passed to useRoleMemberCountsForCommunity.

In the future, @rohan it's your responsibility to understand the change you're making, and to explain it to @atul (with references to eg. ThreadSettings) if he doesn't immediately understand why you're doing it. If I recall correctly, I explicitly requested that you write it this way, and explained in the moment why it should be written this way.

This sort of situation is frankly deeply concerning to me... it's another case (following this one) where my review appears to "required". We need to get to a place where the rest of the team is able to put code out (and review code) without needing me to step in. Ultimately, it's on the diff author to understand the changes they're making, and to explain them thoroughly / thoughtfully.

Sorry for the misunderstanding, I thought it was for a different reason (ensuring that on native, since the React Navigation props for ThreadInfo doesn't update when navigating back from the create roles screen to the community roles screen, so the solution to that was to pull in the latest ThreadInfo (if it exists) from redux.

Here on web, if there's a situation where say a user who has permissions to create/edit roles is in the process of editing a role, and someone changes their role, I didn't experience any crashes when testing before making the change. Instead, all that happened was the expected error message occurred where it displayed that there was an unknown error, please try again (since they no longer had role change permissions).

I'm probably missing the method you're talking about to re-create the crash, and if so it makes sense that we should definitely avoid that. If that is the case, I can bring back the original method of pulling from Redux if it exists

Address feedback on threadInfo

I'm probably missing the method you're talking about to re-create the crash, and if so it makes sense that we should definitely avoid that. If that is the case, I can bring back the original method of pulling from Redux if it exists

Looking at the code here, in order to cause a crash you would need to deauthorize the user for this community. So eg. you could open up CommunityRolesModal on web as an admin of a community, and then have that same user leave the community on a different device. At that point, the CommunityRolesModal would crash because the ThreadStore would no longer have the community in it, but CommunityRolesModal needs to have a ThreadInfo.

This revision is now accepted and ready to land.Jul 31 2023, 11:48 AM
This revision was landed with ongoing or failed builds.Jul 31 2023, 6:15 PM
This revision was automatically updated to reflect the committed changes.