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 9 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
88 changes: 65 additions & 23 deletions includes/push-ui.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,52 @@ function ajax_push() {
exit;
}

$connection_map = get_post_meta( intval( $_POST['postId'] ), 'dt_connection_map', true );
// Maybe we need $_POST stored for background task
$params = $_POST;

if ( ! wp_doing_cron() ) {
/**
* Add possibility to send notification in background
*
* @param bool false 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 );

if ( true === $push_in_background ) {
wp_send_json_success(
array(
'results' => 'Success!!',
)
);

exit;
}
}

$result = push( $params );

wp_send_json_success(
array(
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed accordingly.

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

exit;
}

/**
* Performs 'push' action
*
* @param array $params Parameters from 'post' request
*
* @return array
*/
function push( $params ) {
$connection_map = get_post_meta( intval( $params['postId'] ), 'dt_connection_map', true );
if ( empty( $connection_map ) ) {
$connection_map = array();
}
Expand All @@ -104,7 +149,7 @@ function ajax_push() {
$external_push_results = array();
$internal_push_results = array();

foreach ( $_POST['connections'] as $connection ) {
foreach ( $params['connections'] as $connection ) {
if ( 'external' === $connection['type'] ) {
$external_connection_type = get_post_meta( $connection['id'], 'dt_external_connection_type', true );
$external_connection_url = get_post_meta( $connection['id'], 'dt_external_connection_url', true );
Expand All @@ -127,11 +172,11 @@ function ajax_push() {
$push_args['remote_post_id'] = (int) $connection_map['external'][ (int) $connection['id'] ]['post_id'];
}

if ( ! empty( $_POST['postStatus'] ) ) {
$push_args['post_status'] = $_POST['postStatus'];
if ( ! empty( $params['postStatus'] ) ) {
$push_args['post_status'] = $params['postStatus'];
}

$remote_id = $external_connection->push( intval( $_POST['postId'] ), $push_args );
$remote_id = $external_connection->push( intval( $params['postId'] ), $push_args );

/**
* Record the external connection id's remote post id for this local post
Expand All @@ -149,7 +194,7 @@ function ajax_push() {
'status' => 'success',
);

$external_connection->log_sync( array( $remote_id => $_POST['postId'] ) );
$external_connection->log_sync( array( $remote_id => $params['postId'] ) );
} else {
$external_push_results[ (int) $connection['id'] ] = array(
'post_id' => (int) $remote_id,
Expand All @@ -166,11 +211,11 @@ function ajax_push() {
$push_args['remote_post_id'] = (int) $connection_map['internal'][ (int) $connection['id'] ]['post_id'];
}

if ( ! empty( $_POST['postStatus'] ) ) {
$push_args['post_status'] = esc_attr( $_POST['postStatus'] );
if ( ! empty( $params['postStatus'] ) ) {
$push_args['post_status'] = esc_attr( $params['postStatus'] );
}

$remote_id = $internal_connection->push( intval( $_POST['postId'] ), $push_args );
$remote_id = $internal_connection->push( intval( $params['postId'] ), $push_args );

/**
* Record the internal connection id's remote post id for this local post
Expand All @@ -179,7 +224,7 @@ function ajax_push() {
$origin_site = get_current_blog_id();
switch_to_blog( intval( $connection['id'] ) );
$remote_url = get_permalink( $remote_id );
$internal_connection->log_sync( array( $_POST['postId'] => $remote_id ), $origin_site );
$internal_connection->log_sync( array( $params['postId'] => $remote_id ), $origin_site );
restore_current_blog();

$connection_map['internal'][ (int) $connection['id'] ] = array(
Expand All @@ -203,18 +248,12 @@ function ajax_push() {
}
}

update_post_meta( intval( $_POST['postId'] ), 'dt_connection_map', $connection_map );
update_post_meta( intval( $params['postId'] ), 'dt_connection_map', $connection_map );

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

exit;
}

/**
Expand Down Expand Up @@ -367,7 +406,7 @@ function menu_content() {
$current_post_type = get_post_type();

if ( ! empty( $_GET['post_type'] ) ) { // @codingStandardsIgnoreLine nonce not required
$current_post_type = sanitize_key( $_GET['post_type'] );
$current_post_type = sanitize_key( $_GET['post_type'] ); // @codingStandardsIgnoreLine nonce not required
}

if ( empty( $current_post_type ) ) {
Expand Down Expand Up @@ -498,13 +537,16 @@ function menu_content() {
*/
$as_draft = apply_filters( 'dt_allow_as_draft_distribute', $as_draft, $connection, $post );
?>
<button class="syndicate-button"><?php esc_html_e( 'Distribute', 'distributor' ); ?></button> <?php if ( $as_draft ) : ?><label class="as-draft" for="dt-as-draft"><input type="checkbox" id="dt-as-draft" checked> <?php esc_html_e( 'As draft', 'distributor' ); ?></label><?php endif; ?>
<button class="syndicate-button"><?php esc_html_e( 'Distribute', 'distributor' ); ?></button>
<?php if ( $as_draft ) : ?>
<label class="as-draft" for="dt-as-draft"><input type="checkbox" id="dt-as-draft" checked> <?php esc_html_e( 'As draft', 'distributor' ); ?></label>
<?php endif; ?>
</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' ) ) ); ?>
Copy link

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed accordingly.

</div>
<div class="dt-error">
<?php esc_html_e( 'There was an issue distributing the post.', 'distributor' ); ?>
Expand Down
20 changes: 19 additions & 1 deletion includes/subscriptions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,25 @@ function delete_subscriptions( $post_id ) {
* @since 1.0
*/
function send_notifications( $post_id ) {
if ( ( defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE ) || wp_is_post_revision( $post_id ) || ! current_user_can( 'edit_post', $post_id ) ) {
if ( ( defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE ) || wp_is_post_revision( $post_id ) ) {
return;
}

if ( ! wp_doing_cron() ) { // @codingStandardsIgnoreLine `wp_doing_cron(..)` is a WP function
/**
* Add possibility to send notification in background
*
* @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 );
Copy link

Choose a reason for hiding this comment

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

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?

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'll take a look at this.

Copy link
Contributor

@arsendovlatyan arsendovlatyan Jul 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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


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

if ( ! current_user_can( 'edit_post', $post_id ) ) {
Copy link

@adamsilverstein adamsilverstein Jul 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return;
}

Expand Down