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

Comments: add comment history endpoint #8222

Merged
merged 2 commits into from
Nov 27, 2017
Merged

Conversation

vindl
Copy link
Member

@vindl vindl commented Nov 21, 2017

Depends on D8410-code.

Add endpoint that will return comment history for given comment id.
This functionality relies on Akismet plugin.

Testing instructions

  1. Use your test Jetpack site with Akismet plugin activated.
  2. Point Jetpack to your permanent sandbox using define( ‘JETPACK__API_BASE’, ‘https://hostname.wordpress.com/jetpack.’ );
  3. Pick a comment and make some changes to it (approve, unapprove, spam etc).
  4. Set GET request to https://public-api.wordpress.com/rest/v1/sites/{site_slug}/comment-history/{comment_id}
  5. Verify that the comment history is correct.
  6. Verify that correct error message is returned if Akismet plugin is not active.

@vindl vindl added [Feature] Comments [Pri] BLOCKER [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 21, 2017
@vindl vindl added this to the 5.6 milestone Nov 21, 2017
@vindl vindl self-assigned this Nov 21, 2017
@vindl vindl requested a review from a team November 21, 2017 13:46
@vindl vindl requested a review from a team as a code owner November 21, 2017 13:46
@kwight
Copy link
Contributor

kwight commented Nov 21, 2017

What's the use-case for this? Why are we adding it?

@rodrigoi
Copy link
Contributor

rodrigoi commented Nov 21, 2017

What's the use-case for this? Why are we adding it?

This is for the Comment View: Automattic/wp-calypso#19772

@vindl
Copy link
Member Author

vindl commented Nov 21, 2017

@kwight here's some background:

}

if ( ! class_exists( 'Akismet' ) || ! method_exists( 'Akismet', 'get_comment_history' ) ) {
return new WP_Error( 'akismet_required', 'Akismet plugin must be active for this feature to work', 501 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a server or a client error in this case? Would it make more sense to choose a 4xx response since the plugin needs to be enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I see it - the client is sending a valid request that would otherwise succeed (503 might be more appropriate here). I'm open to other suggestions in terms of exact 4xx code that we should return.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for 503 (or 418, because it's fun 😜).

Copy link
Contributor

Choose a reason for hiding this comment

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

503 it is then 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in a38b95b.

@gwwar
Copy link
Contributor

gwwar commented Nov 21, 2017

As I noted in D8410 let's look into how the timestamp is being serialized.

While testing I noted that we appear to be auditing status changes (but not edits). Also not sure what check-ham and report-ham refer to.

screen shot 2017-11-21 at 11 53 50 am

@vindl
Copy link
Member Author

vindl commented Nov 22, 2017

As I noted in D8410 let's look into how the timestamp is being serialized.

These just look like the regular values returned from the PHP's microtime( true ).

While testing I noted that we appear to be auditing status changes (but not edits). Also not sure what check-ham and report-ham refer to.

These are custom Akismet events that also show up in wp-admin's history (so not user generated).

@vindl vindl requested a review from oskosk November 27, 2017 13:04
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Read, tested, seems to work as it should!

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 27, 2017
@dereksmart dereksmart merged commit e19c3e9 into master Nov 27, 2017
@dereksmart dereksmart deleted the add/comment-history-endpoint branch November 27, 2017 16:54
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 27, 2017
jeherve added a commit that referenced this pull request Nov 28, 2017
oskosk pushed a commit that referenced this pull request Nov 28, 2017
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
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.

8 participants