-
Notifications
You must be signed in to change notification settings - Fork 69
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
Apply discounts to the ecommerce event if available #419
Apply discounts to the ecommerce event if available #419
Conversation
Right now only the purchase event will include discounts if available.
assets/js/src/utils/index.js
Outdated
typeof product.price_after_coupon_discount === 'number' && | ||
! isNaN( product.price_after_coupon_discount ) && |
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.
Why do we need these two? product.price_after_coupon_discount
seems to be always a number
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.
Since there were not only purchase event would call the method getProductFieldObject()
, for example begin_checkout
event would call this method as well. Initially I wanted to test if product.price_after_coupon_discount
is defined but since the price after discount could be 0
and 0
is treated as false
, that's why I added these two conditions here.
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.
Ah, I see. Thanks for clarifying. That might explain why we used
if ( typeof product.price_after_coupon_discount === 'number' )
But what about the NaN line check?
Also WDYT of doing something like:
if ( product?.price_after_coupon_discount < product.prices.price )
So we can save both checks number and nan
You can test it but in case product.price_after_coupon_discount
is undefined this would be false still.
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.
But what about the NaN line check?
I added it because I see typeof NaN
is also a number
, so added it for some prevention.
Also WDYT of doing something like:
if ( product?.price_after_coupon_discount < product.prices.price )
So we can save both checks number and nan
You can test it but in case product.price_after_coupon_discount is undefined this would be false still.
Ah good point, thanks for the info.
assets/js/src/utils/index.js
Outdated
if ( | ||
typeof product.price_after_coupon_discount === 'number' && | ||
! isNaN( product.price_after_coupon_discount ) && | ||
product.price_after_coupon_discount !== product.prices.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.
WDYT about changing this to product.price_after_coupon_discount < product.prices.price
I don't think we can have negative discounts.
assets/js/src/utils/index.js
Outdated
product.price_after_coupon_discount !== product.prices.price | ||
) { | ||
data.discount = | ||
product.prices.price - product.price_after_coupon_discount; |
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.
Thanks for the nice catch!
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.
Thanks for working on this @ianlin
The price is shown correctly but not the discount.
See my comments in the PR. Thanks
Hey @puntope, thanks for the review and nice suggestions. I've pushed more commits and it's ready for another round. |
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.
Thanks for the changes @ianlin
I left one more comment regarding the need for the number and nan check. But approved in advance
As the test result will still be false if the discount is undefined.
Changes proposed in this Pull Request:
Closes #417. This PR fixes a bug that the price in purchase events does not take coupon discounts into account by getting the price after discount using the method
get_total()
inWC_Order_Item_Product
class, and add thediscount
parameter to the ecommerce event.Only the
purchase
event will include discounts if available, since only the methodget_formatted_order()
will include the "price after coupon discounts" for each order line item.Checks:
Screenshots:
Detailed test instructions:
npm ci
andnpm run build
google-analytics
as the filterdt: Order Confirmation
anden: purchase
, which is the purchase eventpr1
andpr2
, e.g.id28~nmSingle~caMusic~qt1~pr12~ds12
id36~nmBeanie with Logo~caAccessories~qt1~pr10~ds8
pr
in the value is the price after coupon discounts, andds
is the amount of coupon discounts.ds
in the event parameters, and the price is just the regular price.Additional details:
Changelog entry