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

Fix WooCommerce comment type handling in the API #10786

Merged
merged 3 commits into from
Jan 3, 2019

Conversation

justinshreve
Copy link
Contributor

@justinshreve justinshreve commented Dec 3, 2018

Fixes Automattic/wp-calypso#28858

Changes proposed in this Pull Request:

There are issues with the general comment count endpoints, where reviews are included in the count of the comments page, but omitted from the actual. See Automattic/wp-calypso#28858.

This PR updates the comment-counts and total count responses in the API to not include WooCommerce comment types.

Testing instructions:

  • Apply this PR/change.
  • Make a request to GET /sites/:site/comments and verify reviews, store order notes, etc are not included in the list.
  • Make a request to GET /sites/:site/count-comments and verify reviews and other WC comment types are not included in the count.

Proposed changelog entry for your changes:

  • The comment-counts endpoint no longer includes comments not returned by the comments listing endpoint.

@justinshreve justinshreve added [Feature] WPCOM API [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 3, 2018
@justinshreve justinshreve self-assigned this Dec 3, 2018
@justinshreve justinshreve requested a review from a team December 3, 2018 19:21
@matticbot
Copy link
Contributor

D21737-code. (newly created revision)

@jetpackbot
Copy link

jetpackbot commented Dec 3, 2018

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 10, 2019.
Scheduled code freeze: January 3, 2019

Generated by 🚫 dangerJS against d78cfab

@jeherve
Copy link
Member

jeherve commented Dec 4, 2018

Related: Automattic/wp-calypso#28858

Make a request to GET /sites/:site/comments and verify the review is not included in the list.

Should we also update the comment-counts endpoints to make sure the comment count matches what will be displayed, and does not include reviews?

@justinshreve
Copy link
Contributor Author

Thanks for catching that. It looks like we will have to roll our own function because wp_count_comments doesn't include any methods for querying for specific types.

@justinshreve justinshreve added [Status] In Progress and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 4, 2018
@justinshreve justinshreve force-pushed the fix/product-review-comment-endpoint branch from 5674e26 to 65880c4 Compare December 4, 2018 18:24
@justinshreve
Copy link
Contributor Author

Actually @jeherve come to think of it -- could we try to get this in the next point release since it fixes the mobile apps, and fix comment-counts in a separate PR since it looks like that's an existing issue regardless?

@jeherve
Copy link
Member

jeherve commented Dec 4, 2018

@justinshreve No worries about the next release. We are not planning on a point release so you have time to come up with a fix for that issue. I'm totally ok with working on the fix in a different PR, too, if you'd like. Up to you! 👍

@justinshreve justinshreve changed the title Add 'review' comment type on GET/POST /comments/$comment_id Handle WooCommerce comment types on GET/POST /comments/$comment_id Dec 5, 2018
@justinshreve justinshreve changed the title Handle WooCommerce comment types on GET/POST /comments/$comment_id Fix WooCommerce comment type handling in the API Dec 5, 2018
@justinshreve
Copy link
Contributor Author

Okay - I've pushed a change that updates the comment counting. I've included a filter to make this easier for others in the future, since comment_types can sometimes be used for internal notes/logs. I've updated the title and description of this PR as well since it's doing a bit more now.

@justinshreve justinshreve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Dec 5, 2018
@jeherve jeherve added this to the 6.9 milestone Dec 6, 2018
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Just a quick change to the filter's docblock!

class.json-api.php Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 6, 2018
@justinshreve
Copy link
Contributor Author

justinshreve commented Dec 6, 2018

@jeherve I've updated the docblock for the filter to include the module and 6.9.0. I do want to note that without the 3 line fix here in json-endpoints/class.wpcom-json-api-comment-endpoint.php, the mobile apps will remain unable to moderate product reviews for WooCommerce 3.5+ until January.

I know that there is currently no point release planned at the moment, but if one ends up happening it might make sense to get that change in if we can (and leave the rest for 6.9.0).

cc @jleandroperez @aforcier

jeherve pushed a commit that referenced this pull request Dec 6, 2018
…mment_id (#10855)

#### Changes proposed in this Pull Request:

See p90Yrv-Tz-p2.

Prior to WooCommerce 3.5, product reviews were stored as comments with an empty comment type (''). After 3.5, these are now marked with the review type.

A few things relied on being able to update comment status using the WordPress.com REST API, such as the mobile apps and web notifications.

This PR whitelists the review type to restore that behavior. 

This was originally part of #10786 -- but separated out since it is a separate fix for the mobile apps.

#### Testing instructions:
* Apply this PR/change.
* Make sure you have WooCommerce with a product setup. Create a product review notification. Take note of the ID.
* Go to https://developer.wordpress.com/docs/api/console/ and point towards your test Jetpack site.
* Make a request to GET /sites/:site/comments/:comment (find via the dropdown) and verify that it does not 404.

#### Proposed changelog entry for your changes:
N/A
@justinshreve justinshreve added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 6, 2018
@justinshreve justinshreve force-pushed the fix/product-review-comment-endpoint branch from fcd4b9a to b9b2bfa Compare December 7, 2018 14:50
@justinshreve justinshreve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Dec 7, 2018
@justinshreve
Copy link
Contributor Author

I've rebased this PR since the other fix went in Jetpack 6.8.1. I also updated the main description and testing directions.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 13, 2018
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to work well in my tests, but I would have 2 small questions.

class.json-api.php Outdated Show resolved Hide resolved
class.json-api.php Outdated Show resolved Hide resolved
@justinshreve
Copy link
Contributor Author

Thanks @jeherve. I've address the feedback above.

@justinshreve
Copy link
Contributor Author

Do you have any idea why the wpcom tests are failing? It seems to have started after a rebase a week ago.. but the builds are not providing much info as far as I can see - they just instantly fail.

@jeherve
Copy link
Member

jeherve commented Dec 14, 2018

Do you have any idea why the wpcom tests are failing?

The patch could not be cleanly applied as the wpcom version of the file does not exactly match. I rebased the diff and applied the changes manually; the test now pass, all good there.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 14, 2018
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good now, and tests well. It should be good to merge.

@dereksmart
Copy link
Member

Code looks ok - but @justinshreve should we be caching wp_count_comments()?

I see that this method is very similar to the one in WC plugin WC_Comments::wp_count_comments that has caching.

Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeherve
Copy link
Member

jeherve commented Jan 3, 2019

Merging for now, we can revisit Derek's suggestion in the future if needed.

@jeherve jeherve merged commit 8eb61be into master Jan 3, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 3, 2019
@jeherve jeherve deleted the fix/product-review-comment-endpoint branch January 3, 2019 21:45
jeherve added a commit that referenced this pull request Jan 3, 2019
jeherve added a commit that referenced this pull request Jan 3, 2019
* Add first version of the Changelog and testing list for 6.9

* Changelog: add #10710

* changelog: add #10538

* changelog: add #10741

* changelog: add #10749

* changelog: add #10664

* changelog: add #10224

* changelog: add #10788

* Changelog: add #10560

* Chanegelog: add #10812

* changelog: add #10556

* Changelog: add #10668

* Changelog: add #10846

* Changelog: add #10947

* Changelog: add #10962

* Changelog: add #10956

* Changelog: add #10940

* Changelog: add #10934

* Changelog: add #10912

* changelog: add #10866

* changelog: add #10924

* Changelog: add #10936

* Changelog: add #10833

* changelog: add #10867

* Changelog: add #10960

* Changelog: add #10888

* changelog: add #10840

* changelog: add #10972

* Changelog: add #10979

* changelog: add #10909

* Changelog: add #10958

* Changelog: add #10981

* Changelog: add #10564

* Changelog: add #10809

* Changelog: add #10982

* Changelog: add #10706

* Changelog: add #10978

* Changelog: add #10132

* Changelog: add #11022

* Changelog: add #11024

* Changelog: add #10875

* Changelog: add #11030

* Changelog: add #11053

* Changelog: add #10880

* Changelog: add #9359

* Changelog: add #11037

* Update block list

* Changelog: add #11060

* Changelog: add #10755

* changelog: add #11000

* Changelog: add #10786

* Changelog: add #10945

* Changelog: add #10597
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.

6 participants