Add hooks in push actions #417

avag-novembit · 2019-06-24T17:12:46Z

Makes possible to run post distribution and re-distribution in background, closes #416.

…cation_allow_in_background'

… after the filter

…ng cron'

adamsilverstein · 2019-07-08T15:51:50Z

Hi @avag-novembit, thank you for your contribution.

Looking at the code, if the user returns true from dt_push_allow_in_background, normal 'push' behavior is bypassed. I don't see code to handle actually pushing the content here, are you working on that separately?

If we are going to enable async push requests, we probably want to include full support for the actual pushing as well, and proper UI to show the (background) progress.

adamsilverstein · 2019-07-08T15:53:06Z

includes/push-ui.php

+	$result = push( $params );
+
+	wp_send_json_success(
+		array(


can we have push return the exact array we need and return it here directly?

Changed accordingly.

adamsilverstein · 2019-07-08T15:55:14Z

includes/push-ui.php

 						</div>
 					</div>

 					<div class="messages">
 						<div class="dt-success">
-							<?php esc_html_e( 'Post successfully distributed.', 'distributor' ); ?>
+							<?php echo esc_html( apply_filters( 'dt_successfully_distributed_message', esc_html__( 'Post successfully distributed.', 'distributor' ) ) ); ?>


Is this a new filter? if so, needs documenting.

The inner esc_html__ can change to __ - escaping should happen late, right before output.

Changed accordingly.

adamsilverstein · 2019-07-08T15:58:52Z

includes/subscriptions.php

+		 * @param bool  false   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 );


this will affect all subscription updates for updated posts, these 'notifications' are required for updating pulled posts. This filter seems unrelated to the overall purpose of the PR, why is it included here?

I'll take a look at this.

@adamsilverstein basically, we are trying to prevent sending any REST API request during post update and move them into background. What if we will just add a filter, somewhere on the top and prevent send notifications, maybe near ?

if ( ( defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE ) || wp_is_post_revision( $post_id ) ) { return; }

adamsilverstein · 2019-07-08T15:59:56Z

includes/subscriptions.php

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


can this stay as part of the logic check on L:237 https://github.com/10up/distributor/pull/417/files#diff-12f37ea7c389dfd5a6a70cc9992d8935R237 ?

We don't need this check when running in background as in cron there is no current in user.

adamsilverstein

Left some feedback; if we enable this it should fully support background processing.

avag-novembit · 2019-07-09T08:24:30Z

Hi @avag-novembit, thank you for your contribution.

Looking at the code, if the user returns true from dt_push_allow_in_background, normal 'push' behavior is bypassed. I don't see code to handle actually pushing the content here, are you working on that separately?

If we are going to enable async push requests, we probably want to include full support for the actual pushing as well, and proper UI to show the (background) progress.

Hi @adamsilverstein, thanks for the comments.

Yes, 'Push in background' implementation is planned to be a separate add-on, and it will have its UI. Currently working on it.

adamsilverstein · 2019-07-15T20:48:53Z

includes/subscriptions.php

@@ -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 ) ) {


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

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'.

adamsilverstein · 2019-07-15T20:50:55Z

includes/push-ui.php

 		 * @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 );


add a sentence describing how this filter could be used

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.

adamsilverstein · 2019-07-15T20:55:13Z

includes/push-ui.php

+							 *
+							 * @param string Success message
+							 */
+							echo esc_html( apply_filters( 'dt_successfully_distributed_message', __( 'Post successfully distributed.', 'distributor' ) ) );


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

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.

adamsilverstein · 2019-07-15T20:56:25Z

includes/subscriptions.php

+		 * @param bool  true    Whether run 'send notification' in background or not, default 'false'
+		 * @param array $params request data
+		 */
+		$allow_send_notification = apply_filters( 'dt_allow_send_notifications', true, $post_id );


needs a description and better naming for the filter.

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 ); }

adamsilverstein · 2019-07-15T20:59:17Z

@avag-novembit Thanks for your updates here. I left some additional feedback. I'm still not 100% clear on your use case and whether its something the code plugin should support directly, enable via filters or perhaps help you figure out how to implement using existing extensibility.

When you have code ready that is close or complete, can you link to your project so we can better evaluate the value of these changes. Thanks!

avag-novembit · 2019-07-17T12:18:50Z

@adamsilverstein thanks. Hope my last comments gives some idea how filters can be used.

jeffpaul · 2020-03-26T13:59:38Z

@avag-novembit we're looking to solve a similar issue related to reports on ACF issues as part of our 2.1.0 release so I'm moving this PR and related issue to that release as well.

jeffpaul · 2023-01-16T18:04:46Z

Unassigning reviewers here while we focus on the v2 refactoring and can then reassess how best to handle the root issue here.

arsendovlatyan · 2023-01-17T13:09:32Z

@jeffpaul we are maintaining this repo anymor, however, if you will decide to move forward, please let me know if we will review it on our end.

github-actions · 2024-02-09T15:42:20Z

@avag-novembit thanks for the PR! Could you please rebase your PR on top of the latest changes in the base branch?

avag-novembit added 8 commits June 20, 2019 18:51

Make possible to send 'notification' in background also

1191f19

Rename filter 'dt_send_notification_in_background' to 'dt_send_notifi…

f0eeb0b

…cation_allow_in_background'

Refactor - 'send notification' 'current_user_can' condition implement…

b001784

… after the filter

do not apply 'send notifiaction allow in background' filter when 'doi…

bed14b6

…ng cron'

Add hook to implement 'push' action in background

e7a5cbb

Fix linter errors

b3ba59f

Fix linter errors

f35b08b

Workaround to avoid linter errors

9297c1f

jeffpaul assigned avag-novembit Jun 24, 2019

jeffpaul added the type:enhancement New feature or request. label Jun 24, 2019

jeffpaul added this to the 1.5.0 milestone Jun 24, 2019

Fix comments

4141f7c

adamsilverstein self-requested a review July 8, 2019 15:38

adamsilverstein reviewed Jul 8, 2019

View reviewed changes

adamsilverstein suggested changes Jul 8, 2019

View reviewed changes

Eliminate check 'current_user_can' when 'wp_doing_cron'

3c8f2b4

avag-novembit added 4 commits July 9, 2019 12:46

return push return array from 'ajax_push()'

b9a7517

Document the 'dt_successfully_distributed_message' filter

710d4ca

Refactor filter names

0234163

Rename variables in push actions

e8dcd98

adamsilverstein reviewed Jul 15, 2019

View reviewed changes

jeffpaul added the needs:feedback This requires reporter feedback to better understand the request. label Jul 16, 2019

helen modified the milestones: 1.5.0, 1.6.0 Jul 17, 2019

jeffpaul requested a review from dkotter January 27, 2020 21:48

jeffpaul requested a review from dinhtungdu February 27, 2020 04:24

jeffpaul modified the milestones: 2.0.0, 2.1.0 Mar 26, 2020

dkotter removed the request for review from dinhtungdu August 23, 2022 22:12

jeffpaul removed the request for review from dkotter January 16, 2023 18:04

aaronware mentioned this pull request Apr 6, 2023

Return response from distribution request #524

Open

github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hooks in push actions #417

Add hooks in push actions #417

avag-novembit commented Jun 24, 2019

adamsilverstein commented Jul 8, 2019 •

edited

Loading

adamsilverstein Jul 8, 2019

avag-novembit Jul 9, 2019

adamsilverstein Jul 8, 2019

avag-novembit Jul 9, 2019

adamsilverstein Jul 8, 2019

avag-novembit Jul 9, 2019

arsendovlatyan Jul 10, 2019 •

edited

Loading

adamsilverstein Jul 8, 2019 •

edited

Loading

avag-novembit Jul 9, 2019

adamsilverstein left a comment

avag-novembit commented Jul 9, 2019

adamsilverstein Jul 15, 2019

avag-novembit Jul 17, 2019

adamsilverstein Jul 15, 2019

avag-novembit Jul 17, 2019 •

edited

Loading

adamsilverstein Jul 15, 2019

avag-novembit Jul 17, 2019

adamsilverstein Jul 15, 2019

avag-novembit Jul 17, 2019

adamsilverstein commented Jul 15, 2019

avag-novembit commented Jul 17, 2019

jeffpaul commented Mar 26, 2020

jeffpaul commented Jan 16, 2023

arsendovlatyan commented Jan 17, 2023

github-actions bot commented Feb 9, 2024

Add hooks in push actions #417

Are you sure you want to change the base?

Add hooks in push actions #417

Conversation

avag-novembit commented Jun 24, 2019

adamsilverstein commented Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsendovlatyan Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

adamsilverstein Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein left a comment

Choose a reason for hiding this comment

avag-novembit commented Jul 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avag-novembit Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein commented Jul 15, 2019

avag-novembit commented Jul 17, 2019

jeffpaul commented Mar 26, 2020

jeffpaul commented Jan 16, 2023

arsendovlatyan commented Jan 17, 2023

github-actions bot commented Feb 9, 2024

adamsilverstein commented Jul 8, 2019 •

edited

Loading

arsendovlatyan Jul 10, 2019 •

edited

Loading

adamsilverstein Jul 8, 2019 •

edited

Loading

avag-novembit Jul 17, 2019 •

edited

Loading