Refactored groupPermissions #8528

kurtisassad · 2024-07-18T16:41:19Z

Link to Issue

Closes: #7951

Description of Changes

Removes Topics.group_ids, and migrates them to a unique GroupPermission record
Refactors all application code to use the new model. On the backend this includes create_group, update_group, delete_group, as well as the unused CreateGroup.command.ts.
Refactors front end to use the new fine grained permissions model to gate UI features.

Test Plan

Turn the Users.isAdmin on for your account for a community
Create a group with some member on the allowlist
Update this group, make sure it works. Delete this group, make sure it works.
Re-add the group, ensure it is associated with at least one topic. Make yourself not a User.isAdmin. Now try creating a thread/commenting/reacting on a thread with a specific topic that is part of the group. You should be gated from performing these actions.
Check the gating on the /discussions, /discusssion/[thread-id], and /new/discussion pages. When gated it should look like the following (Depending on the forum actions gated and the specific topics gated for):

Discussions page:

Discussion page:

Create thread page:

Overview page:

packages/commonwealth/server/migrations/20240718135145-group-permission-refactor.js

# Conflicts: # packages/commonwealth/client/scripts/utils/Permissions.ts # packages/commonwealth/client/scripts/views/components/NewThreadForm/NewThreadForm.tsx # packages/commonwealth/client/scripts/views/pages/discussions/DiscussionsPage.tsx # packages/commonwealth/client/scripts/views/pages/view_thread/ViewThreadPage.tsx

masvelio

FE code looks good to me

timolegros

Good work. Would like to remove the unknown type casts and optimize queries where possible. Will run through test plan after comments are handled.

Why is the column removal not included in the migration?

libs/model/src/community/CreateGroup.command.ts

packages/commonwealth/client/scripts/hooks/useForumActionGated.ts

packages/commonwealth/server/controllers/server_groups_methods/create_group.ts

packages/commonwealth/server/controllers/server_groups_methods/refresh_membership.ts

packages/commonwealth/server/controllers/server_groups_methods/update_group.ts

timolegros · 2024-08-05T14:46:23Z

packages/commonwealth/server/migrations/20240718135145-group-permission-refactor.js

+
+        for (const t of topics) {
+          for (const g of t.group_ids) {
+            await queryInterface.sequelize.query(


bulkInsert?

bulkInsert won't work here because we have an array of enums, but we need to specify the field as a string[] in sequelize otherwise the tests will fail when trying to sync. As a result Sequelize tries to cast them into an array of strings which sql complains about.

I don't exactly understand but is this not something we can work-around with type assertions? Also, this is already a raw query so we should be able to just prepare all the values at once (and not use bulkInsert). A nested for-loop query is a bad idea.

packages/commonwealth/server/migrations/20240718135145-group-permission-refactor.js

packages/commonwealth/server/util/requirementsModule/validateTopicGroupsMembership.ts

mzparacha · 2024-08-06T14:22:29Z

Found a bug with group updates

Create a group with no members in allow list (adds requirement that allow some existing users)
Now verify you have some community members who are part of that group
Update that group and add some members to allow list
Now notice in the /members page when you filter by that specific group, only the member added in the allow list is displayed, and other members aren't (you'd have to rebase master to test the filter on /members as it got changes in Fix UI issues with community members page #8471)

Screen.Recording.2024-08-06.at.7.18.44.PM.mov

packages/commonwealth/client/scripts/models/Group.ts

packages/commonwealth/test/unit/server_controllers/server_threads_controller.spec.ts

# Conflicts: # packages/commonwealth/client/scripts/views/pages/discussions/DiscussionsPage.tsx

rbennettcw

Some small comments. Lmk when tests/conflicts are fixed and I'll test locally.

rbennettcw · 2024-08-29T16:52:19Z

packages/commonwealth/client/scripts/views/components/feed.tsx

+  const allowedActions = useForumActionGated({
+    communityId: app.activeChainId(),
+    address: user.activeAccount?.address || '',
+    topicId: thread?.topic?.id,
  });

-  const isTopicGated = !!(memberships || []).find((membership) =>
-    membership.topicIds.includes(thread?.topic?.id),
+  const { canCreateComment, canReactToThread } = canPerformAction(
+    allowedActions,
+    isAdmin,
+    thread?.topic?.id,
  );


The addition of the useForumActionGated hook is awesome to help simplify FE code. The canPerformAction call seems a bit redundant though– I think that should be called within the hook and return the result from the hook.

Good catch, I will merge this into the useForumActionGated

rbennettcw · 2024-08-29T17:24:54Z

packages/commonwealth/server/controllers/server_groups_methods/update_group.ts

+    models.sequelize.query(
+      `
+            INSERT INTO "GroupPermissions" (group_id, topic_id, allowed_actions, created_at, updated_at) VALUES
+            (:groupId, :topicId, Array[:allowedActions]::"enum_GroupPermissions_allowed_actions"[], NOW(), NOW())
+            ON CONFLICT (group_id, topic_id) DO UPDATE
+            SET
+                allowed_actions = EXCLUDED.allowed_actions,
+                updated_at = EXCLUDED.updated_at
+            RETURNING *;


We should always use a type for raw queries, e.g. models.sequelize.query<GroupPermissionInstance>(...), then there's no need to cast the type later.

Also, I think wildcards * should be avoided. It's easier to write but tricker to debug due to lack of transparency.

rbennettcw · 2024-08-29T17:26:26Z

packages/commonwealth/server/controllers/server_groups_methods/update_group.ts

+  return (await Promise.all(upsertPromises)) as unknown as Promise<
+    GroupPermissionInstance[]
+  >;


No need to await if you're returning a promise. It'll resolve either way. Also, if setting the type on the query, this would simply become return Promise.all(upsertPromises)

return await and return Promise... are not the same and I think return await is recommended now:
https://github.com/goldbergyoni/nodebestpractices/blob/master/sections/errorhandling/returningpromises.md

# Conflicts: # packages/commonwealth/client/scripts/views/pages/discussions/DiscussionsPage.tsx # packages/commonwealth/server/controllers/server_comments_methods/create_comment_reaction.ts # packages/commonwealth/server/controllers/server_threads_methods/create_thread.ts # packages/commonwealth/server/controllers/server_threads_methods/create_thread_comment.ts # packages/commonwealth/server/controllers/server_threads_methods/create_thread_reaction.ts # packages/commonwealth/test/integration/api/subscriptions.spec.ts # packages/commonwealth/test/unit/server_controllers/server_threads_controller.spec.ts

# Conflicts: # libs/model/src/middleware/authorization.ts # packages/commonwealth/server/controllers/server_groups_methods/delete_group.ts # packages/commonwealth/server/controllers/server_groups_methods/get_groups.ts # packages/commonwealth/server/controllers/server_groups_methods/refresh_membership.ts

masvelio · 2025-01-06T12:28:45Z

@kurtisassad any chance to make this PR mergable? Otherwise, let's close it to clean up the PR list

Refactored groupPermissions

a076462

rbennettcw reviewed Jul 22, 2024

View reviewed changes

packages/commonwealth/server/migrations/20240718135145-group-permission-refactor.js Outdated Show resolved Hide resolved

kurtisassad mentioned this pull request Jul 22, 2024

[Bug]: User having trouble posting a thread in https://commonwealth.im/stargatetoken/discussions #8544

Closed

Added group permission logic for all adding/updating/removing groups

2328c70

kurtisassad force-pushed the ka.groupPermissionsCleanup branch from f44fafe to 2328c70 Compare July 25, 2024 13:10

kurtisassad added 10 commits July 25, 2024 07:11

Fixed UI issue

c5230dc

Fixed lint errors

a585f93

Fixed UI for topic gating

c96c360

Fixed remaining bugs with UI

b941aab

Fixed logic

d2be5be

Fixed lint errors

77185db

Fixed lint errors

b4f87c9

Fixed lint errors

00cda1a

Fixed lint errors

38f69ec

kurtisassad requested a review from rbennettcw August 1, 2024 12:50

kurtisassad added 2 commits August 1, 2024 06:17

Fixed lint errors

71ed1cc

Fixed lint errors

f3686a2

kurtisassad marked this pull request as ready for review August 1, 2024 13:35

kurtisassad requested review from Rotorsoft, timolegros, masvelio, mzparacha and burtonator August 1, 2024 13:35

masvelio approved these changes Aug 5, 2024

View reviewed changes

timolegros requested changes Aug 5, 2024

View reviewed changes

mzparacha reviewed Aug 6, 2024

View reviewed changes

packages/commonwealth/client/scripts/models/Group.ts Outdated Show resolved Hide resolved

kurtisassad marked this pull request as draft August 7, 2024 12:01

Fixed PR comments

b7509e7

kurtisassad requested a review from ForestMars August 19, 2024 14:29

Fixed PR comments

2caadd7

kurtisassad force-pushed the ka.groupPermissionsCleanup branch from c20a1a7 to 2caadd7 Compare August 19, 2024 14:46

kurtisassad self-assigned this Aug 19, 2024

kurtisassad marked this pull request as draft August 19, 2024 14:48

Updated migration and fixed lint errors

099e810

kurtisassad marked this pull request as ready for review August 19, 2024 15:22

kurtisassad added 2 commits August 19, 2024 08:29

Fixed test failure

97380e3

Removed test

9e984aa

kurtisassad commented Aug 19, 2024

View reviewed changes

packages/commonwealth/test/unit/server_controllers/server_threads_controller.spec.ts Outdated Show resolved Hide resolved

kurtisassad added 3 commits August 19, 2024 11:41

Fixed lint errors

5eb64c4

Merge branch 'master' into ka.groupPermissionsCleanup

329f1b1

# Conflicts: # packages/commonwealth/client/scripts/views/pages/discussions/DiscussionsPage.tsx

Resolved merge conflicts

ab6393d

kurtisassad force-pushed the ka.groupPermissionsCleanup branch from 55b52b0 to ab6393d Compare August 22, 2024 13:44

rbennettcw reviewed Aug 29, 2024

View reviewed changes

kurtisassad added 12 commits September 3, 2024 12:28

Resolved merge conflicts

2020d4e

Fixed groups UI

f592b71

Fixed lint errors

3b81098

Fixed lint errors

fef29f6

Fixed lint errors

b9e8012

Fixed lint errors

34c1534

Fixed lint errors

39d0488

Fixed lint errors

c4579c5

Updated migration name

e289a20

Fixed lint errors

0f85378

timolegros marked this pull request as draft December 18, 2024 15:10

kurtisassad closed this Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored groupPermissions #8528

Refactored groupPermissions #8528

kurtisassad commented Jul 18, 2024 •

edited

Loading

masvelio left a comment

timolegros left a comment •

edited

Loading

timolegros Aug 5, 2024

kurtisassad Aug 7, 2024 •

edited

Loading

timolegros Aug 8, 2024

mzparacha commented Aug 6, 2024

rbennettcw left a comment

rbennettcw Aug 29, 2024

kurtisassad Sep 3, 2024

rbennettcw Aug 29, 2024

rbennettcw Aug 29, 2024

timolegros Aug 29, 2024

masvelio commented Jan 6, 2025

Refactored groupPermissions #8528

Refactored groupPermissions #8528

Conversation

kurtisassad commented Jul 18, 2024 • edited Loading

Link to Issue

Description of Changes

Test Plan

masvelio left a comment

Choose a reason for hiding this comment

timolegros left a comment • edited Loading

Choose a reason for hiding this comment

timolegros Aug 5, 2024

Choose a reason for hiding this comment

kurtisassad Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

timolegros Aug 8, 2024

Choose a reason for hiding this comment

mzparacha commented Aug 6, 2024

rbennettcw left a comment

Choose a reason for hiding this comment

rbennettcw Aug 29, 2024

Choose a reason for hiding this comment

kurtisassad Sep 3, 2024

Choose a reason for hiding this comment

rbennettcw Aug 29, 2024

Choose a reason for hiding this comment

rbennettcw Aug 29, 2024

Choose a reason for hiding this comment

timolegros Aug 29, 2024

Choose a reason for hiding this comment

masvelio commented Jan 6, 2025

kurtisassad commented Jul 18, 2024 •

edited

Loading

timolegros left a comment •

edited

Loading

kurtisassad Aug 7, 2024 •

edited

Loading