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

finished_at_start/end job filters #111

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Xeitor
Copy link
Contributor

@Xeitor Xeitor commented Apr 5, 2024

Changelog

  • added finished_at range filter support for jobs

Issue: #30

PD: I'll split the different filters in different PRs for easier review

@Xeitor Xeitor marked this pull request as draft April 5, 2024 21:57
@Xeitor Xeitor marked this pull request as ready for review April 7, 2024 14:38
@Xeitor
Copy link
Contributor Author

Xeitor commented Apr 9, 2024

Hi @rosa! Don't mean to bother you but in case you missed this this PR is ready for review :)

@rosa
Copy link
Member

rosa commented Apr 10, 2024

@Xeitor thanks for letting me know, I'll try to review soon!

@@ -23,9 +23,9 @@ class ActiveJob::JobsRelation
include Enumerable

STATUSES = %i[ pending failed in_progress blocked scheduled finished ]
FILTERS = %i[ queue_name job_class_name ]
FILTERS = %i[ queue_name job_class_name finished_at_start finished_at_end ]
Copy link
Contributor Author

@Xeitor Xeitor Apr 10, 2024

Choose a reason for hiding this comment

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

I am not sure if we should add this to filters given that the filtering is done natively for solid_queue and i'ts not supported for resque (there isn't a finished status)

Suggested change
FILTERS = %i[ queue_name job_class_name finished_at_start finished_at_end ]
FILTERS = %i[ queue_name job_class_name ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rosa what are your thoughts on this?

@Xeitor
Copy link
Contributor Author

Xeitor commented May 15, 2024

Hi @rosa, hope you are doing great

Sorry to bother you, wanted to know if you were able to take a look at the PR :)

@job_filters = { job_class_name: params.dig(:filter, :job_class_name).presence, queue_name: params.dig(:filter, :queue_name).presence }.compact
@job_filters = { job_class_name: params.dig(:filter, :job_class_name).presence, queue_name: params.dig(:filter, :queue_name).presence,
finished_at_start: params.dig(:filter, :finished_at_start).presence, finished_at_end: params.dig(:filter, :finished_at_end) }.compact
finished_at_timezoned_filters
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be in this method, inlined, and have another helper method to do the parsing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i pushed some changes about this, I could move date_with_time_zone method to the helpers and include here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @rosa

just wanted to ping you on this one, no worries however, I know you are probably super bussy with other stuff

@Xeitor Xeitor requested a review from rosa August 18, 2024 13:34
@rosa
Copy link
Member

rosa commented Sep 13, 2024

Hey @Xeitor, so sorry about this huge delay! I haven't had time to focus on Mission Control lately, but it's definitely in the plans for October, I need to spend a week or so just working on all pending PRs and missing features 😳

@Xeitor
Copy link
Contributor Author

Xeitor commented Sep 25, 2024

Hey @Xeitor, so sorry about this huge delay! I haven't had time to focus on Mission Control lately, but it's definitely in the plans for October, I need to spend a week or so just working on all pending PRs and missing features 😳

for sure no problem

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