Page MenuHomePhabricator

[web] Introduce member action menu items in `ThreadMember` component
ClosedPublic

Authored by jacek on Mar 9 2022, 4:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 2:04 AM
Unknown Object (File)
Thu, Nov 7, 8:02 PM
Unknown Object (File)
Thu, Nov 7, 9:07 AM
Unknown Object (File)
Tue, Nov 5, 8:58 PM
Unknown Object (File)
Fri, Oct 25, 7:32 PM
Unknown Object (File)
Thu, Oct 24, 2:35 AM
Unknown Object (File)
Sat, Oct 19, 2:38 AM
Unknown Object (File)
Thu, Oct 17, 9:48 PM

Details

Summary

Introduce thread member actions - to remove member or promote him to an admin. The conditions are the same as in native app.

Screenshot_Google Chrome_2022-03-09_144517.png (158×198 px, 8 KB)

Test Plan

Can be tested after introducing members modal.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Mar 10 2022, 8:41 AM
ashoat added inline comments.
web/modals/threads/members/member.react.js
61–73 ↗(On Diff #10201)

I find this sort of ternary syntax to not be very readable. I think it should be easy to see conditionals when scanning through code at-a-glance.

In my opinion, clean ternary syntax should be short and sweet:

const something = cond ? 'val1' : 'val2';

const somethingLonger = isSomethingBiggerThanTen > 10
  ? someValueWhenItsBiggerThanTen
  : someOtherValueWhenItsLessThanOrEqualToTen;

As soon as the conditions are spanning multiple lines, it's hard to tell that there is a condition there at all. See my suggested edit

tomek requested changes to this revision.Mar 15 2022, 10:32 AM
tomek added inline comments.
web/modals/threads/members/member.react.js
61 ↗(On Diff #10201)

It's strange to see memberInfo.username in this condition. Why do we need to check here if username is set?

This revision now requires changes to proceed.Mar 15 2022, 10:32 AM
web/modals/threads/members/member.react.js
61 ↗(On Diff #10201)

I agree. It's strange, but it's the way it works for react-native app currently. I'm not sure if there are any cases that require this condition to be here.

Replace ternaty operator with if

tomek added 1 blocking reviewer(s): ashoat.
tomek added inline comments.
web/modals/threads/members/member.react.js
61 ↗(On Diff #10201)

This condition was introduced in a commit 020bdd0d8d870a2a19c949c270036bd6679a27fb from 2017 which describes the purpose:

It should be impossible to make an anonymous user the admin of a thread

So we need to keep this condition. It might be a good idea to either, create a function with descriptive name e.g. canBeMadeAnAdmin or to add a comment describing why this condition is needed.

web/modals/threads/members/member.react.js
61 ↗(On Diff #10201)

If a user deletes their account, they become an "anonymous" user. Their messages still exist, and the authorID has an ID field. But there is no username for them because the entry in the users MySQL table has been deleted. (This is the difference between UserInfo, which can be a deleted user, and AccountUserInfo, which can't.)

(The real question is whether we should show anonymous users in the member list at all, but that's outside the scope of this diff, I think.)

This revision is now accepted and ready to land.Mar 17 2022, 1:31 PM