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

Updated GroupFilterGambit to prevent hidden groups being visible wher… #2657

Merged
merged 6 commits into from
Mar 4, 2021

Conversation

ajaypayne
Copy link
Contributor

…e they shouldn't be and to ensure that only the selected groups are returned on a search. #2559

**Fixes #2559 **

Changes proposed in this pull request:
modify db query to use and instead of or

Screenshot
n/a

Confirmed

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

…e they shouldn't be and to ensure that only the selected groups are returned on a search. flarum#2559
Copy link
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.

Thank you very much for this! The code looks good, I just have a few comments on test cases. It might also be good to test that multiple comma-separated groups can be included in the search.

tests/integration/api/users/GroupSearchTest.php Outdated Show resolved Hide resolved
tests/integration/api/users/GroupSearchTest.php Outdated Show resolved Hide resolved
tests/integration/api/users/GroupSearchTest.php Outdated Show resolved Hide resolved
Copy link
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.

Any chance we could include a test case for multiple groups in the same query? IE:

'q' => 'group:admin,mod'

or even
'q' => 'group:admin,4'

@ajaypayne
Copy link
Contributor Author

Sorry, I missed that comment the first time around. Adding now.

Copy link
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 so much!

@askvortsov1 askvortsov1 merged commit 8eef723 into flarum:master Mar 4, 2021
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.

Hidden Groups Visible in GroupGambit
3 participants