-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
#2492 - Groups filtering & retrieve single endpoint #3084
Conversation
…oup model Included in flarum#2492
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.
Fantastic work, thank you so much for taking up this issue and diving so deep into the codebase! I left a quick change comment in the code.
More generally speaking, I have mixed feelings about the "name" filter, as that feels more like a limited search than a filter. Seeing as it currently requires a precise match, is there a concrete use case? If not, maybe we should leave that one out for now.
Fixes flarum#2492 Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
Yeah, you are right, I'm probably lacking a use case for the exact match filter. I could rewrite it to LIKE match, but that can be a limited searcher as you said. I wanted to introduce further scoping, so that it can potentially cover use case described in flarum/issue-archive#280. But I guess that it's so rare 99.9% of forums won't need this. So I guess the question is whether full text match would have any benefits or not. Let me know & I will make the changes :) |
To be honest, I think that for now, it might be best to hold off on any name searching functionality until we better understand use cases. The PR is already pretty big, but more importantly, if we set an API now and decide to make changes later, that would be a breaking change. @flarum/core what are y'all thoughts?
flarum/issue-archive#280 is complicated because completely paginated groups has big implications across our architecture. This PR is definitely a huge step towards that direction, but I don't think we should aim to tackle it here. This all being said, thank you again so much for the contribution! |
I think that settles it - I have reverted changes related to name filter & marked this PR as ready to review. If there will be a need for name filtering or querying, it can be implemented anytime :). |
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.
Excellent, thank you very much!
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.
Thank you for the PR!
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.
Code looks good!
Thank you so much for the contribution! I think that code-size-wise, this might be the biggest PR from a first-time contributor we've had 😁 |
Fixes #2492
Changes proposed in this pull request:
GET /api/groups/{id}
endpoint for retrieving single recordGET /api/groups
endpointReviewers should focus on:
Confirmed
composer test
).Required changes: