Fix inconsistencies with how as_unschedule_action works

[danieliser]

The as_unschedule_action function seems to have a few awkward qwerks in that it accepts clearing all by hook, all by group, but not both which seems inconsistent.

https://github.com/woocommerce/action-scheduler/blob/master/functions.php#L103-L121

Consider the following:

as_schedule_single_action( time(), 'hook1', [ '1', '2' ], 'group_1' );
as_schedule_single_action( time(), 'hook1', [ '3', '4' ], 'group_1' );
as_schedule_single_action( time(), 'hook1', [ '5', '6' ], 'group_2' );
as_schedule_single_action( time(), 'hook1', [ '7', '8' ], 'group_2' );

as_schedule_single_action( time(), 'hook2', [ '1', '2' ], 'group_1' );
as_schedule_single_action( time(), 'hook2', [ '3', '4' ], 'group_2' );


as_unschedule_action( 'hook1', null, null ); // Works to clear all for hook1
.
as_unschedule_action( null, null, 'group_1' ); // Works to clear all in group_1

as_unschedule_action( 'hook1', null, 'group_1' ); // Doesn't do anything because args won't match in the do while loop.

Two options:

1. Introduce a new method in abstract/ActionScheduler_Store.php

	/**
	 * Cancel pending actions by hook / group combination.
	 *
	 * @since 3.x.x
	 *
	 * @param string $hook Hook name.
	 * @param string $group Group slug.
	 *
	 * @return void
	 */
	public function cancel_actions_by_hook_for_group( $hook, $group ) {
		$action_ids = true;
		while ( ! empty( $action_ids ) ) {
			$action_ids = $this->query_actions(
				array(
					'hook' => $hook,
					'group' => $group,
					'status' => self::STATUS_PENDING,
					'per_page' => 1000,
				)
			);

			$this->bulk_cancel_actions( $action_ids );
		}
	}

2. Modify the existing method to allow specifying a hook optionally.

	/**
	 * Cancel pending actions by hook.
	 *
	 * @since 3.0.0
	 * @since 3.x.x Added the optional $group parameter.
         *
	 * @param string $hook Hook name.
	 * @param string|null $group Group slug.
	 *
	 * @return void
	 */
	public function cancel_actions_by_hook( $hook, $group = '' ) {
		$action_ids = true;
		while ( ! empty( $action_ids ) ) {
			$action_ids = $this->query_actions(
				array(
					'hook' => $hook,
					'group' => $group,
					'status' => self::STATUS_PENDING,
					'per_page' => 1000,
				)
			);

			$this->bulk_cancel_actions( $action_ids );
		}
	}

Then of course we need to update the helper function accordingly.

/**
 * Cancel all occurrences of a scheduled action.
 *
 * @param string $hook The hook that the job will trigger.
 * @param array $args Args that would have been passed to the job.
 * @param string $group The group the job is assigned to.
 */
function as_unschedule_all_actions( $hook, $args = array(), $group = '' ) {
	if ( ! ActionScheduler::is_initialized( __FUNCTION__ ) ) {
		return;
	}
	if ( empty( $args ) ) {
		if ( ! empty( $group ) && empty( $hook ) ) {
			ActionScheduler_Store::instance()->cancel_actions_by_group( $group );
			return;
		}

		if ( ! empty( $hook ) ) {
			ActionScheduler_Store::instance()->cancel_actions_by_hook( $hook, $group );
			return;
		}
	}
	do {
		$unscheduled_action = as_unschedule_action( $hook, $args, $group );
	} while ( ! empty( $unscheduled_action ) );
}

I reversed the order of the checks as we could simplify the && empty( $group ) by passing the empty value through to the cancel_actions_by_hook method.

[rrennick]

@danieliser Thanks for reporting this. There is an open PR for addressing this #475 which should be merged next week.

[danieliser]

@rrennick - I don't think that is correct. #475 is in regards to as_unschedule_action, my suggestion is in regards to as_unschedule_all_actions and allows unhooking all actions with a specific hook/group combination.

[danieliser]

Happy to submit a PR as well, just wasn't sure which way would be preferred.

[danieliser]

@rrennick can we reopen this given the differences above?

[rrennick]

@danieliser Reopening. But we need to be aware that AS supports custom data stores and this sort of change needs to be compatible with existing custom stores.

[danieliser]

@rrennick - Since we can't break the method signature due to existig child classes, either a new method or simply a new helper function are the options.

I'm happy to explore either, though for compatibility a function that simply queries based on hook/group combo and bulk deletes them might be simplest/best approach. That is use existing mechanics.

[roykho]

Hi @danieliser

though for compatibility a function that simply queries based on hook/group combo and bulk deletes them might be simplest/best approach

I would agree. Feel free to submit a PR for this. Thanks!