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

Split scheduler to separate classes for easier testing #2991

Merged
merged 16 commits into from
Mar 31, 2020
Merged

Conversation

jom
Copy link
Member

@jom jom commented Mar 26, 2020

This PR looks big, but it is a lot of testing.

Changes proposed in this Pull Request:

  • In testing https://github.com/Automattic/sensei-wc-paid-courses/pull/452 I discovered an issue with how I am using Action Scheduler's as_next_scheduled_action(). I've fixed it so it properly reschedules when there is currently an action in-progress.
  • I took this opportunity (prior to beta) to do the separation of the different scheduling methods (WP Cron vs Action Scheduler). Most of this PR is actually tests for the new classes.

Testing instructions:

With WooCommerce installed/activated...

  • Ensure you have more than 20 learner users.
  • Reset the site enrolment salt option (sensei_course_enrolment_site_salt).
  • Load a WP admin page.
  • Make sure the action is scheduled in Tools > Scheduled Actions.
  • Make sure it is rescheduled for each batch.

New Filters

  • sensei_background_job_actions - Get a list of background job actions that are handled by the class.
  • sensei_scheduler_class - Override the default class that implements Sensei_Scheduler_Interface.

@jom jom added this to the 3.0.0 milestone Mar 26, 2020
@jom jom requested a review from a team March 26, 2020 22:05
@jom jom self-assigned this Mar 26, 2020
@jom jom changed the title Fix issue preventing batch jobs from getting rescheduled in Action Scheduler [WIP] Fix issue preventing batch jobs from getting rescheduled in Action Scheduler Mar 27, 2020
@jom jom removed the request for review from a team March 27, 2020 07:27
@jom jom changed the title [WIP] Fix issue preventing batch jobs from getting rescheduled in Action Scheduler [WIP] Split scheduler to seperate classes for easier testing Mar 27, 2020
@jom jom changed the title [WIP] Split scheduler to seperate classes for easier testing Split scheduler to seperate classes for easier testing Mar 27, 2020
@jom jom requested a review from a team March 27, 2020 19:44
Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

This looks pretty good! Congrats! =)

@donnapep donnapep added the Hooks This change adds or modifies one or more hooks. label Mar 30, 2020
@jom jom requested review from renatho and a team March 31, 2020 19:19
Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

I tested now and it didn't work for me. I have 21 users and I removed the salt option from the database directly.

It ran the 2 first batches and after that, it continued infinitely. I checked and it entered the cancel_scheduled_job, but it wasn't canceling.

@jom jom requested a review from renatho March 31, 2020 21:35
Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

LGTM! Tested and working well!

@jom jom merged commit d7e7b5a into master Mar 31, 2020
@jom jom deleted the fix/scheduler branch March 31, 2020 23:11
@jom jom changed the title Split scheduler to seperate classes for easier testing Split scheduler to separate classes for easier testing Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hooks This change adds or modifies one or more hooks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants