-
-
Notifications
You must be signed in to change notification settings - Fork 840
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
[mentions] feat: group mentions #3658
Conversation
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.
Looks neat! a few comments:
- Groups are loaded from the store for autocomplete, do we know if all groups are within the store at that point? part of the requirements suggests an API search endpoint.
- I have yet to test locally, but I think we might be missing taking into consideration hidden groups.
- To avoid abuse, I reckon we need a
mention groups
permission,but since it wasn't listed in the initial requirements, we can add it inanother PR outside of this.
...nsions/mentions/migrations/2022_10_21_000001_change_post_mentions_group_add_foreign_keys.php
Outdated
Show resolved
Hide resolved
extensions/mentions/migrations/2022_10_21_000002_add_created_at_to_post_mentions_group.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Sami Mazouz <[email protected]>
Co-authored-by: Sami Mazouz <[email protected]>
🤦♂️ whoops, yes I missed that. I feel the search endpoint should probably be in core, rather than mentions, what is your opinion?
As it stands, yes, hidden groups are not returned for users who do not have permission to view them.
I'm happy to include that in this PR, as it makes complete sense to do that at the same time. Again, probably for core directly, rather than in mentions? |
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 feel the search endpoint should probably be in core, rather than mentions, what is your opinion?
Yup. core makes sense.
I'm happy to include that in this PR, as it makes complete sense to do that at the same time. Again, probably for core directly, rather than in mentions?
mention groups
permission is only relevant in the mentions extension, so it makes more sense in mentions than in core imo.
extensions/mentions/migrations/2022_10_21_000000_create_post_mentions_groups_table.php
Outdated
Show resolved
Hide resolved
…entions_groups_table.php Co-authored-by: Sami Mazouz <[email protected]>
Co-authored-by: Sami Mazouz <[email protected]>
Co-authored-by: Sami Mazouz <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
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.
Added a few more tests, one of them actually caught a bug where the editing of posts doesn't have the actor so group mentions don't work. pushed a fix at the same time.
I believe this is ready now.
Ah nice one, thank you! |
**Addresses https://discuss.flarum.org/d/31473-mentioning-groups-within-posts **
Changes proposed in this pull request:
Allows mentioning of groups
Reviewers should focus on:
Screenshot
![image](https://user-images.githubusercontent.com/16573496/197233323-8bc7e2f0-4d45-4543-b7c3-14b31aae84f4.png)
User notification settings:
Autocomplete:
![image](https://user-images.githubusercontent.com/16573496/197327696-f4d420c3-c9a7-43c3-9877-74b0b8251b09.png)
Preview:
![image](https://user-images.githubusercontent.com/16573496/197327889-002b2d02-b404-486e-842a-d1c30db59a74.png)
Post content:
![image](https://user-images.githubusercontent.com/16573496/197259659-c3c20a46-f5c9-458b-9b7a-90804637580f.png)
Web notification:
![image](https://user-images.githubusercontent.com/16573496/197257932-2416ac93-6c2d-4dda-960f-07e859371b6d.png)
Email notification:
Necessity
Confirmed
composer test
).Required changes: