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

Rewrite Filterable trait used for Squad filters #658

Merged
merged 9 commits into from
Dec 24, 2023
Merged

Rewrite Filterable trait used for Squad filters #658

merged 9 commits into from
Dec 24, 2023

Conversation

recursivetree
Copy link
Contributor

@recursivetree recursivetree commented Nov 3, 2023

Problem:

@bluedagger on discord noticed that if a character is not in any alliance, and the squad contains a filter for alliance is not x, the user doesn't get added to the squad, even though he should.

How it has been fixed

While studying the Filterable trait code (https://github.com/eveseat/web/blob/c97b7cd1dd8cd5c063415ba0710d4b343dfdfef5/src/Models/Filterable.php), I noticed how messy and broken it was. I really struggled to first pin down the problem, and fixing it was even harder since it would have required to special case is not in multiple cases.

Therefore, I decided it's rewrite time! This allowed me to fix the original bug, and additionally:

  • Make it more readable: There is no 8 levels deep nested code anymore
  • Allow nesting of groups to arbitrary depths
  • Add a new test case covering the bug

All squad tests are passing.

Lastly, I added a few files to the gitignore that were generated while running tests, but don't belong on github.

@recursivetree
Copy link
Contributor Author

One difference to the old implementation is that we don't sort by relation. I don't quite understand why this has been done, and it also works without

@Crypta-Eve Crypta-Eve merged commit d05ca29 into eveseat:5.0.x Dec 24, 2023
2 checks passed
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.

2 participants