-
Notifications
You must be signed in to change notification settings - Fork 805
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
Vdimitrakis/add price in posts endpoint #35066
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @therocket-gr!
I checked the response and I am getting some scaped HTML code on it. I would be nice if we can clean that up. Just to be more compatible with other clients like Mobile apps.
We are already cleaning up all HTML tags, maybe we can also run html_entity_decode
to decode the rest of the code. That function should convert $
to the correct $
projects/packages/blaze/src/class-dashboard-rest-controller.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. I only have a minor question, and I believe this will need a rebase.
$posts[ $key ]['price'] = html_entity_decode( | ||
wp_strip_all_tags( | ||
wc_price( $product->get_price() ) | ||
), | ||
ENT_QUOTES | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it there is no simple function to retrieve a formatted and escaped product price?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- created a function to reuse the logic - added the same functionality in get_dsp_blaze_posts
a032d70
to
b375a5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @therocket-gr - As we discussed over DMs, you followed the same approach as what wc_price
is doing.
I like the defensive coding, maybe you're overdoing it with checking all the functions, but better safe than sorry :)
Pre-approving this in order not to block you, my comments are minor
FYI - didn't actually tested the endpoint, I just reviewed the PHP side of getting the prices
projects/packages/blaze/src/class-dashboard-rest-controller.php
Outdated
Show resolved
Hide resolved
projects/packages/blaze/src/class-dashboard-rest-controller.php
Outdated
Show resolved
Hide resolved
# Conflicts: # projects/packages/blaze/package.json # projects/packages/blaze/src/class-dashboard.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeherve i think we are good to go!! I refactored a bit the code on how the price is fetched. I would appreciate your thoughts and if you think we are ok i will merge this one. Thanks in advance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests well for me. I only have one small remark to ensure compatibility with other Woo extensions.
continue; | ||
} | ||
$product = wc_get_product( $item['ID'] ); | ||
if ( $product ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may have to be more defensive here, just like in #35566, to avoid fatals in some scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that's not going to be enough. In some scenarios, Woo extensions may change what's returned there. That's something we ran into a few times in the past, and why we ended up checking for $product instanceof WC_Product
in the PR I linked to earlier. Also see #11301 for another example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok fixed, added the extra condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being extra defensive doesn't hurt :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code reviewed - being extra defensive is good!
Thank you both @PanosSynetos and @jeherve for reviewing and approving it... |
We need to expose the WooCommerce price in the blaze dashboard list (in Advertising - or in the new Marketing page)
This change will check if the
wc_get_product
method is available and will call for each of the items/posts, it will retrieve the price of each post (if a product) and will inject it in the final results.This will only work for self-hosted or pressable sites.
Fixes #
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
You need to have Woo installed in your testing site and to have (at least) a product created.
Navigate to the advertising page and check your network inspect tab. Filter requests by /blaze/posts
In the request result, you should see the price data when the post is a product.
Then click on the Promote button (on the top right corner of your page). Click continue to load all posts again and check the network for the same call.
you should see the price entry in the posts/results array
posts that are not of product type should have the property but with an empty value (empty string)