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

Adjust filters for long term events #3410

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

JoeyStk
Copy link
Contributor

@JoeyStk JoeyStk commented Feb 7, 2025

Short description

This PR refactors the event filters, adjusts the filters for long term events and adds a test.

Proposed changes

  • Refactor event list
  • Adjust filters
  • Add test

Side effects

  • Please check if I didn't break anything with the refactoring
  • I deleted all comments as I think the code is more speaking now - but this might be "author blindness", so please let me know if you want to restore any comments

Resolved issues

Fixes: #3379


Pull Request Review Guidelines

@JoeyStk JoeyStk added the help wanted Extra attention is needed label Feb 10, 2025
@JoeyStk
Copy link
Contributor Author

JoeyStk commented Feb 10, 2025

If some knows how to fix the tests, I'm open to suggestions! @jonbulz Mizuki gave me the hint that you might something about this :)

@jonbulz
Copy link
Contributor

jonbulz commented Feb 10, 2025

@JoeyStk alright seems that renaming the test-event has a lot of implications 😅
Maybe this is good use for test specific fixtures? Why did you re-name the events anyway?

@JoeyStk
Copy link
Contributor Author

JoeyStk commented Feb 10, 2025

Why did you re-name the events anyway?

My idea was to be explicit. The test event is a weekly recurring event and we use it for this case and while refactoring I thought it would be the cleaner way to rename it and to be clear about what the test is for. I wouldn't mind it too much if I had to name it back, but don't think it's ideal :)

@jonbulz
Copy link
Contributor

jonbulz commented Feb 17, 2025

@JoeyStk it took longer than anticipated, but I finally fixed the tests. Could you have a look at my changes?

@JoeyStk
Copy link
Contributor Author

JoeyStk commented Feb 17, 2025

Thank you soooo much! 😍 I'll check it out in a bit :)

@JoeyStk
Copy link
Contributor Author

JoeyStk commented Feb 17, 2025

To me it looks and works as expected! So this PR is ready to have it's second review :)

@JoeyStk JoeyStk removed the help wanted Extra attention is needed label Feb 17, 2025
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.

Adjust event filter for long-term event
2 participants