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

Conversation

gititon
Copy link
Contributor

@gititon gititon commented Apr 5, 2017

Adds non-sync listener for comment update data, and fires action sync listener is listening for if the comment contents is modified

Changes proposed in this Pull Request:

  • Adds non-sync listener for comment update data, and fires action sync listener is listening for if the comment contents is modified

Testing instructions:

  • phpunit --filter comment (added test_modify_comment_contents())

Proposed changelog entry for your changes:

@gititon gititon added [Team] Poseidon [Package] Sync [Status] Ready to Merge Go ahead, you can push that green button! [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 5, 2017
Copy link
Member

@enejb enejb left a comment

Choose a reason for hiding this comment

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

Good approach.

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

*/
public function handle_comment_contents_modification( $data, $comment ) {

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!

* @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!

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!

$new_comment['comment_ID'],
$changes,
);
do_action( 'jetpack_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 document this do_action like we do others.

Copy link
Member

Choose a reason for hiding this comment

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

Lets call the do action like this
do_action( 'jetpack_modified_comment_contents',$new_comment['comment_ID'], $changes );
This adds some a bit more consistency

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!

@@ -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( 'jetpack_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!

@@ -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( 'jetpack_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.

add_action( 'jetpack_modified_comment_contents', $callable ); will need to be updated to
add_action( 'jetpack_modified_comment_contents', $callable, 10, 2 );

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!

@enejb
Copy link
Member

enejb commented Apr 5, 2017

Great work! :shipit: After the tests pass

… 4.7 since it lacks wp_update_comment_data filter
@gititon gititon merged commit 363dffc into master Apr 6, 2017
@matticbot matticbot removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 6, 2017
@gititon gititon deleted the add/edit_comment branch April 7, 2017 14:01
jeherve added a commit that referenced this pull request Apr 7, 2017
jeherve added a commit that referenced this pull request Apr 24, 2017
eliorivero pushed a commit that referenced this pull request Apr 25, 2017
* Changelog: initial commit for 4.9 release.

* Changelog: add #6929

* Changelog: move old changelogs to changelog.txt

* Readme: restore deleted release post link.

The post is now live.

* Changelog: add #6853

* Changelog: add #6856

* Changelog: add #6857

* Changelog: add #6884

* Changelog: add #6885

* Changelog: add #6892

* Changelog: add #6894

* Changelog: add #6898

* Changelog: add #6899

* Changelog: add #6900

* Changelog: add #6909

* Changelog: add #6927

* Changelog: add #6947

* Chagelog: add #6958

* Changelog: add #6961

* Changelog: add #6963

* Changelog: add #6965

* Changelog: add #6986

* Changelog: add #7000

* Changelog: add #7013

* Changelog: add #7015

* Changelog: add #7019

* Changelog: add #7028

* Changelog: add #6998

* Changelog: add #6999

* Changelog: add #7044

* Changelog: add #6881

* Changelog: add #6922

* Changelog: add #6940

* Changelog: add #6962

* Changelog: add #6942

* Changelog: add #6959

* Changelog: add #7018

* Changelog: add #6948

* Changelog: add #6657

* Changelog: add #7030

* Changelog: add #7048

* Changelog: add #7031

* Changelog: add #6990

* Changelog: add #6957

* Changelog: add #7027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants