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.

Sign up for GitHub

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

Jump to bottom

Add hooks in push actions #417

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

Add hooks in push actions #417

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1191f19
Make possible to send 'notification' in background also
avag-novembit Jun 19, 2019
f0eeb0b
Rename filter 'dt_send_notification_in_background' to 'dt_send_notifi…
avag-novembit Jun 20, 2019
b001784
Refactor - 'send notification' 'current_user_can' condition implement…
avag-novembit Jun 20, 2019
bed14b6
do not apply 'send notifiaction allow in background' filter when 'doi…
avag-novembit Jun 21, 2019
e7a5cbb
Add hook to implement 'push' action in background
avag-novembit Jun 24, 2019
b3ba59f
Fix linter errors
avag-novembit Jun 24, 2019
f35b08b
Fix linter errors
avag-novembit Jun 24, 2019
9297c1f
Workaround to avoid linter errors
avag-novembit Jun 24, 2019
4141f7c
Fix comments
avag-novembit Jul 8, 2019
3c8f2b4
Eliminate check 'current_user_can' when 'wp_doing_cron'
avag-novembit Jul 9, 2019
b9a7517
return push return array from 'ajax_push()'
avag-novembit Jul 9, 2019
710d4ca
Document the 'dt_successfully_distributed_message' filter
avag-novembit Jul 9, 2019
0234163
Refactor filter names
avag-novembit Jul 12, 2019
e8dcd98
Rename variables in push actions
avag-novembit Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions includes/push-ui.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,12 @@ function ajax_push() {
/**
* Add possibility to send notification in background
*
* @param bool false Whether run 'push' action in background or not, default 'false'
* @param bool true Whether run 'push' action in background or not, default 'false'
* @param array $params request data
*/
$push_in_background = apply_filters( 'dt_push_allow_in_background', false, $params );
$allow_push = apply_filters( 'dt_allow_push', true, $params );
Copy link

Choose a reason for hiding this comment

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

add a sentence describing how this filter could be used

Copy link
Contributor Author

@avag-novembit avag-novembit Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In case the 'push' action is consuming a lot of resources, we can use the dt_allow_push filter to terminate function execution, and run the function in background. Here is simple use case example:

add_filter( 'dt_allow_push', 'schedule_push_action', 10, 2 );

function schedule_push_action( $allow_push, $params ) {
if ( ! wp_next_scheduled( 'dt_push_posts_hook' ) ) {
	wp_schedule_single_event( time(), 'dt_push_posts_hook', [ $params ] );
}

return false;
}

And then simply we can run it by hooking to scheduled event:

add_action( 'dt_push_posts_hook', 'push_action', 10, 1 );

function push_action( $params ) {
 \Distributor\PushUI\push( $params );
}

Or we can call custom function depending on use case.


if ( true === $push_in_background ) {
if ( false === $allow_push ) {
wp_send_json_success(
array(
'results' => 'Success!!',
Expand All @@ -111,15 +111,8 @@ function ajax_push() {
}
}

$result = push( $params );

wp_send_json_success(
array(
'results' => array(
'internal' => $result['internal_push_results'],
'external' => $result['external_push_results'],
),
)
push( $params )
);

exit;
Expand Down Expand Up @@ -251,8 +244,10 @@ function push( $params ) {
update_post_meta( intval( $params['postId'] ), 'dt_connection_map', $connection_map );

return array(
'internal_push_results' => $internal_push_results,
'external_push_results' => $external_push_results,
'results' => array(
'internal' => $internal_push_results,
'external' => $external_push_results,
),
);
}

Expand Down Expand Up @@ -546,7 +541,14 @@ function menu_content() {

<div class="messages">
<div class="dt-success">
<?php echo esc_html( apply_filters( 'dt_successfully_distributed_message', esc_html__( 'Post successfully distributed.', 'distributor' ) ) ); ?>
<?php
/**
* Filter distribution success message
*
* @param string Success message
*/
echo esc_html( apply_filters( 'dt_successfully_distributed_message', __( 'Post successfully distributed.', 'distributor' ) ) );
Copy link

Choose a reason for hiding this comment

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

this filter is related to the success message, can you add a sentence describing how it can be used/your use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case when we are terminating 'push' function execution, it would be more descriptive to show another status message instead of "Post successfully distributed". For example: "Scheduled action to distribute post". Then show scheduled action status in a separate UI.

?>
</div>
<div class="dt-error">
<?php esc_html_e( 'There was an issue distributing the post.', 'distributor' ); ?>
Expand Down
16 changes: 8 additions & 8 deletions includes/subscriptions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,24 +238,24 @@ function send_notifications( $post_id ) {
return;
}

if ( ! wp_doing_cron() ) { // @codingStandardsIgnoreLine `wp_doing_cron(..)` is a WP function
if ( ! wp_doing_cron() ) { //phpcs:ignore
if ( ! current_user_can( 'edit_post', $post_id ) ) {
Copy link

Choose a reason for hiding this comment

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

Subscriptions are run in the cron context, will this user check work in cron? I suspect not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what do you mean, there is no 'current user' in cron, so we need the current_user_can(..) check when 'is not doing cron'.

return;
}

/**
* Add possibility to send notification in background
*
* @param bool false Whether run 'send notification' in background or not, default 'false'
* @param bool true Whether run 'send notification' in background or not, default 'false'
* @param array $params request data
*/
$send_notification_in_background = apply_filters( 'dt_send_notification_allow_in_background', false, $post_id );
$allow_send_notification = apply_filters( 'dt_allow_send_notifications', true, $post_id );
Copy link

Choose a reason for hiding this comment

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

needs a description and better naming for the filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we can change it to dt_allow_subscription_update, which is more descriptive perhaps.

By using this filter we can terminate function redistribution process and run it in background. It can be helpful in cases when resource consumption is high for that process. Here is a simple example how it can be implemented:

add_filter( 'dt_allow_subscription_update', 'schedule_send_notifications', 10, 2 );

function schedule_send_notifications( $allow_send_notifications, $post_id ) {
if ( ! wp_next_scheduled( 'dt_redistribute_posts_hook' ) ) {
	wp_schedule_single_event( time(), 'dt_redistribute_posts_hook', [ $post_id ] );
}

return false;
}

Here we are scheduling an event to run in background, and by returning false terminating post redistribution function execution. Then we can hook the post redistribution function to scheduled action or write a custom function to redistribute post.

add_action( 'dt_redistribute_posts_hook', 'redistribute_post', 10, 1 );

function redistribute_posts( $post_id ) {
 \Distributor\Subscriptions\send_notifications( $post_id );
}


if ( true === $send_notification_in_background ) {
if ( false === $allow_send_notification ) {
return;
}
}

if ( ! current_user_can( 'edit_post', $post_id ) ) {
return;
}

$subscriptions = get_post_meta( $post_id, 'dt_subscriptions', true );

if ( empty( $subscriptions ) ) {
Expand Down