-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Granular level topic gating #9512
Conversation
…met by gated topic members
… required conditions are not met by gated topic members
…evel permissioning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FE code looks good to me. Didn't do proper CA testing though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Roger on this one. I think we can build this without creating a new enum. Additionally, I think a new table/model is also overkill. It seems like the only difference between the existing GroupPermissions
and the new GroupTopicPermissions
table is an additional topic_id
foreign key...
I would also like to get @kurtisassad's eyes on this PR since he is the original author of the more recent granular permissions model.
...es/commonwealth/server/migrations/20241010195545-migrate-existing-group-topic-permissions.js
Outdated
Show resolved
Hide resolved
...GroupsAndMembers/Groups/common/GroupForm/TopicPermissionsSubForm/TopicPermissionsSubForm.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, after addressing the comments
Link to Issue
Closes: #9063
Description of Changes
UPVOTE_AND_COMMENT_AND_POST
"How We Fixed It"
N/A
Test Plan
General steps
pnpm migrate-db
Test topic gating with non-group member accounts (i.e test original gating logic, works as expected)
Topic is gated
messages via a tooltip or on form controlsTest topic gating with existing topics in existing communities with group member accounts (i.e test original gating logic with full topic level permissions, works as expected)
Test topic gating with new topics in new communities
UPVOTE
,UPVOTE_AND_COMMENT
orUPVOTE_AND_COMMENT_AND_POST
UPVOTE
, verify you are only able to upvote and not able to create a thread or comment.UPVOTE_AND_COMMENT
, verify you are only able to upvote and comment on that thread and not able to create a thread.UPVOTE_AND_COMMENT_AND_POST
, verify you can upvote, comment, and create a thread on that topic.Topic members are only allowed to ${topicPermissionLevel}
tooltip in the block action CTA element/:communityId/overview
,/:communityId/discussions
,/:communityId/discussions/:topic
,/dashboard/for-you
,/dashboard/global
.Test permissions precedence with the same topics in different groups having different permission levels
UPVOTE
permission levelUPVOTE_AND_COMMENT_AND_POST
permission levelTest permissions change on gated topics are reflected
/:communityId/members?tab=groups
page, verify the group cards show correct permission levels with the gated topicsDeployment Plan
N/A
Other Considerations
N/A