Skip to content
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

New permission controlling 2+ recipients PDs #184

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

clarkwinkelmann
Copy link
Member

Changes proposed in this pull request:
A new permission controls whether the user can add more than 2 user recipients to a discussion. This allows restricting the feature to simple direct messages and control who can create group discussions separately.

The texts in the recipients modal have not been modified, but if you try adding a third recipient to a new discussion and don't have the permission, the other recipient will be removed and the newly selected recipient added.

The permission only affects the user count. If the actor has permission to add groups, those are still unlimited and unaffected. It probably doesn't make sense to restrict user count while allowing groups but it'll work.

Reviewers should focus on:
This first version is not "backward compatible", users who update would lose functionality until they customize the new permission. I guess we should copy the value of discussion.startPrivateDiscussionWithUsers into discussion.addMoreThanTwoUserRecipients with a migration? The challenge is that if users forget to run the migrations they will encounter the issue anyway, and the migration needs to do nothing if run later, but has no way of knowing if the lack of value is indication of a choice for "admin" or that no choice was made...

I think there are also edge cases regarding leaving. You could add a "third" recipient at the same time as you remove yourself, which still meets the 2 recipients rule.

This PR was sponsored by a client.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

@imorland imorland merged commit d85c4c3 into master Jun 1, 2023
@imorland imorland deleted the cw/more-than-two-recipients-permission branch June 1, 2023 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants