Groups GET API endpoint ignores parameters

[franga2000]

Bug Report

Current Behavior
No matter the GET params, /api/groups always return all groups. None of the usual parameters like filter[q] and page work. There is also no way to get one group by its ID (GET /groups/{id} doesn't exist like it does for users and discussions).

Steps to Reproduce

1. Open https://discuss.flarum.org/api/groups?page[limit]=1
2. See more than 1 entry

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[MatusMak]

Hello, is this issue open for PRs? I'd like to work on it.

If so, please confirm that as part of it, the following needs to be implemented:

• filtering for GET /api/groups endpoint, similar to other parts of the system
• dedicated GET /api/groups/{id} endpoint for retrieving single record
• and, naturally, tests for both changes

Thanks!

[askvortsov1]

Absolutely, thank you for expressing interest! We'd probably want to follow a similar pattern to the User and Discussion endpoints, with searchers/filterers.

One potential challenge is pagination, as that would require a lot of frontend changes. That's something we want to eventually move towards, and closing this issue would pave the road for that.

In the meantime, if you hardcode a negative limit as the default (probably with the ability to override), that should disable limiting: https://github.com/flarum/core/blob/d2c953b7ee7c2644ab33314f9966da800b08d1a6/src/Query/ApplyQueryParametersTrait.php#L58-L67

[MatusMak]

Hello,

I have created a draft PR #3084, so that you can have a preliminary look into my work. As of right now, I only implemented dedicated GET /api/groups/{id} endpoint, so that I could get myself familiar with project and workflow. Feel free to have a look and comment on anything that needs to be changed/added.

I will push changes related to filtering in the upcoming days, once I will understand how it is implemented for other entities.

[askvortsov1]

Feel free to have a look and comment on anything that needs to be changed/added.

It looks great to me, thank you very much for the contribution!

I will push changes related to filtering in the upcoming days, once I will understand how it is implemented for other entities.

Please don't hesitate to reach out here, on discuss.flarum.org, or on our Discord if you have any question! Thanks again!

[MatusMak]

Hey, I pushed filter & pagination changes!

After thinking about it, I decided to leave out searcher, as using same gambits as in filtering wouldn't make sense and implementing full-text searcher that would require migration seemed like an overkill for groups, but if you think otherwise, let me know - if this is okay though, I will mark PR as ready for review.

Also, some other notes:

• because default limit is -1 as you requested to keep changes backwards compatible, specifying offset in parameters without limit will fail, as it will produce invalid SQL query.
• I didn't implement any includes (users nor permissions), as it wasn't implemented before either, and I wasn't even sure how I would handle permissions. It is beyond the scope of this ticket anyway, but I'm leaving this for the record.

---

PS: Sorry that it took so long, I didn't have much free time & I spent extra time digging deeper into the code, nice job with the architecture, love it 😍👍.

[askvortsov1]

Hey, I pushed filter & pagination changes!

After thinking about it, I decided to leave out searcher, as using same gambits as in filtering wouldn't make sense and implementing full-text searcher that would require migration seemed like an overkill for groups, but if you think otherwise, let me know - if this is okay though, I will mark PR as ready for review.

Also, some other notes:

• because default limit is -1 as you requested to keep changes backwards compatible, specifying offset in parameters without limit will fail, as it will produce invalid SQL query.
• I didn't implement any includes (users nor permissions), as it wasn't implemented before either, and I wasn't even sure how I would handle permissions. It is beyond the scope of this ticket anyway, but I'm leaving this for the record.

PS: Sorry that it took so long, I didn't have much free time & I spent extra time digging deeper into the code, nice job with the architecture, love it heart_eyes+1.

Left a comment on the PR, thank you so much for your hard work on this!