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

#2492 - Groups filtering & retrieve single endpoint #3084

Merged
merged 17 commits into from
Oct 25, 2021

Conversation

MatusMak
Copy link
Contributor

@MatusMak MatusMak commented Sep 28, 2021

Fixes #2492

Changes proposed in this pull request:

  • Added dedicated GET /api/groups/{id} endpoint for retrieving single record
  • Added basic filtering, sorting and pagination to GET /api/groups endpoint

Reviewers should focus on:

  • Overal code style to confirm compliance with established conventions

Confirmed

  • Backend changes: tests are green (run composer test).

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a 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.

src/Api/Controller/ListGroupsController.php Outdated Show resolved Hide resolved
Fixes flarum#2492

Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
@MatusMak
Copy link
Contributor Author

MatusMak commented Oct 8, 2021

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 :)

@askvortsov1
Copy link
Sponsor Member

askvortsov1 commented Oct 10, 2021

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?

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.

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!

@MatusMak MatusMak marked this pull request as ready for review October 12, 2021 20:20
@MatusMak
Copy link
Contributor Author

if we set an API now and decide to make changes later, that would be a breaking change.

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 :).

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a 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!

src/Group/Filter/GroupFilterer.php Outdated Show resolved Hide resolved
Copy link
Member

@SychO9 SychO9 left a 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!

src/Group/Query/HiddenFilterGambit.php Outdated Show resolved Hide resolved
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good!

@askvortsov1 askvortsov1 merged commit 5a1bf08 into flarum:master Oct 25, 2021
@askvortsov1
Copy link
Sponsor Member

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 😁

@MatusMak MatusMak deleted the mm/groups-filtering branch October 31, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Groups GET API endpoint ignores parameters
4 participants