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

Add mechanism to report comment content modifications via sync #6929

Merged
merged 12 commits into from
Apr 6, 2017
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions sync/class.jetpack-sync-module-comments.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ public function init_listeners( $callable ) {
add_action( 'untrash_post_comments', $callable );
add_action( 'comment_approved_to_unapproved', $callable );
add_action( 'comment_unapproved_to_approved', $callable );
add_action( 'modified_comment_contents', $callable );

Copy link
Member

Choose a reason for hiding this comment

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

Lets remove this space.

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 had left the space to differentiate/set it apart it from the sync listener callbacks, but done!

add_filter( 'wp_update_comment_data', array( $this, 'handle_comment_contents_modification' ), 10, 2 );

// even though it's messy, we implement these hooks because
// the edit_comment hook doesn't include the data
Expand All @@ -40,6 +43,23 @@ public function init_listeners( $callable ) {
$this->init_meta_whitelist_handler( 'comment', array( $this, 'filter_meta' ) );
}

/**
* @param array $data The new, processed comment data.
* @param array $comment The old, unslashed comment data.
*/
public function handle_comment_contents_modification( $data, $comment ) {
Copy link
Member

Choose a reason for hiding this comment

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

$data is a very generic variable name. Can we make it more explicit $new_comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!


if ( $data['comment_content'] != $comment['comment_content'] ) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should compare more then just the comment content, in the ui you can change the name, email and the website as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

$action_args = array(
$data['comment_ID'],
$data,
$comment['comment_content'],
);
do_action( 'modified_comment_contents', $action_args );
Copy link
Member

Choose a reason for hiding this comment

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

Lets prefixs this with jetpack_ since it is something unique to jetpack.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to pass in all this data? Can we just pass in the comment ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! prefixed with 'jetpack' and passed before and after for content, author, author_email, and author_url if they changed.

}
return $data;
}

public function init_full_sync_listeners( $callable ) {
add_action( 'jetpack_full_sync_comments', $callable ); // also send comments meta
}
Expand Down
21 changes: 21 additions & 0 deletions tests/php/sync/test_class.jetpack-sync-comments.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,27 @@ public function test_update_comment() {
$this->assertEquals( "foo bar baz", $remote_comment->comment_content );
}

public function test_modify_comment_contents() {

//Confirm that 'modify_comment' action is set after changing comment contents
$this->comment->comment_content = "foo bar baz";
Copy link
Member

Choose a reason for hiding this comment

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

lets expand this to test also the changes to the name, email, website and content

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

wp_update_comment( (array) $this->comment );
$this->sender->do_sync();

$event = $this->server_event_storage->get_most_recent_event( 'modified_comment_contents' );
$this->assertTrue( (bool) $event );

$this->server_event_storage->reset();

//Confirm that 'modified_comment_contents' action is not set after unapproving comment
$this->comment->comment_approved = 0;
wp_update_comment( (array) $this->comment );
$this->sender->do_sync();

$event = $this->server_event_storage->get_most_recent_event( 'modified_comment_contents' );
$this->assertFalse( (bool) $event );
}

public function test_unapprove_comment() {

$this->assertEquals( 1, $this->server_replica_storage->comment_count( 'approve' ) );
Expand Down