Page MenuHomePhabricator

[keyserver] Update changeRoleThreadQuery to use the Voiced role for announcement channels
AbandonedPublic

Authored by rohan on Nov 9 2023, 8:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 14, 2:30 PM
Unknown Object (File)
Wed, Dec 11, 9:23 PM
Unknown Object (File)
Mon, Nov 25, 7:48 AM
Unknown Object (File)
Nov 9 2024, 10:47 AM
Unknown Object (File)
Nov 9 2024, 8:29 AM
Unknown Object (File)
Nov 7 2024, 6:00 PM
Unknown Object (File)
Oct 10 2024, 7:57 PM
Unknown Object (File)
Oct 5 2024, 4:42 AM
Subscribers

Details

Reviewers
atul
ginsu
ashoat
Summary

Following introducing a new role for announcement subchannels, the first step I'd like to take is to give everyone added to the channel the Voiced role. This is not permanent, and will not be landed as-is, but it's an intermediate step in separating out Members and Voiced.

In changeRole, changeRoleThreadQuery(threadID, role) is called to get the intended role and permissions for the users being added to the thread. Naturally, this is the default_role column in MariaDB. We'll want to do some updates here to ultimately decide whether Voiced or Members is the appropriate role.

In this task, I'll make it so every new member gets Voiced instead of Members, and a later task will update this to check whether the user has 'Voiced in announcement channels' permission, and assign them a role accordingly.

Addresses ENG-5635

Test Plan

Tested this in three ways:

  • Confirmed that new users added to the announcement subchannel had voiced permissions in the memberships table in MariaDB
  • Confirmed that new users could automatically speak in announcement subchannels (since they have the Voiced role)
  • Changed the member pill that shows the role to use the current threadInfo (as opposed to the community thread info) just to see that:
    • in announcement subchannels, new members added had the Voiced role
    • in normal subchannels, new members added had the Members role still

Diff Detail

Repository
rCOMM Comm
Branch
combined_branch
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

keyserver/src/updaters/thread-permission-updaters.js
393

The alternative to using r.name = 'Voiced' is to store a voiced role ID in MariaDB in the same way we store default_role.

keyserver/src/updaters/thread-permission-updaters.js
393

(It should not be possible to rename the Voiced role since it's not user-facing and won't show up in the community roles modal. It's entirely just for the purposes of setting up a way for users with the 'Voiced in announcement channels' role to speak vs. users without the role)

rohan requested review of this revision.Nov 9 2023, 8:52 AM
ashoat requested changes to this revision.Nov 9 2023, 12:32 PM
ashoat added inline comments.
keyserver/src/updaters/thread-permission-updaters.js
381–384

These could be extracted to the top-level scope to avoid redefining in every function invocation

388

Can you add an indent here to match our query formatting conventions?

390

Is there a reason you stripped the r.thread = t.id condition from this INNER JOIN?

393

What happens if an admin creates a new role called "Voiced"?

The alternative to using r.name = 'Voiced' is to store a voiced role ID in MariaDB in the same way we store default_role.

This would be way better.

396

Can you undo the change to the indentation here please?

This revision now requires changes to proceed.Nov 9 2023, 12:32 PM

Abandoning this since I think it makes more sense to just do ENG-5637 immediately following D9827, rather than having this intermediate code change that will have to be changed again in this stack anyways.

Will reopen later if I change my mind, but I'll make sure to carry the feedback provided here when working on ENG-5637