-
Notifications
You must be signed in to change notification settings - Fork 378
Fix totalValue test measurement
#2865
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
Conversation
totalValue test measurement
35ab53a to
aad4b27
Compare
aad4b27 to
aa28a44
Compare
| expect(events[1]).toEqual( | ||
| getExpectedPayload(pageViewPayload, { | ||
| event_name: 'product_page_rendered', | ||
| total_value: pageViewPayload.totalValue, |
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.
total_value is actually expected to not exist when product quantity is 0 or undefined:
hydrogen/packages/hydrogen-react/src/analytics-schema-custom-storefront-customer-tracking.ts
Lines 206 to 211 in e7db0a5
| addDataIf( | |
| { | |
| event_name: PRODUCT_ADDED_TO_CART_EVENT_NAME, | |
| customerId: addToCartPayload.customerId, | |
| cart_token: cartToken?.id ? `${cartToken.id}` : null, | |
| total_value: addToCartPayload.totalValue, |
^ removes total_value if totalValue is 0/undefined.
This is test case where totalValue is 0, since all products here are quantity undefined. products array is derived from BASE_PRODUCT_PAYLOAD, which does not define quantity.
I add a test below to test a payload where products has a product with quantity 1.
Enhances the tests to ensure the totalValue is more accurate. Establishes that totalValue is indeed the total value of the products in the event.
aa28a44 to
f098607
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.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/hydrogen-react/src/analytics-utils.ts:125
- If product.quantity is undefined, the result defaults to 0. If a missing quantity should be interpreted as 1, consider adjusting the logic to use a default of 1.
export const getProductValue = (product: ShopifyAnalyticsProduct): number => parseFloat(product.price) * (product.quantity || 0);
If a product has no quantity, there is no quantity. The quantity is 0. |
|
Dear shopify devs, may I get another test run and review, please? Your review approval will clarify what an expected event should have, which is very valuable for me. A disapproval will also be helpful in clarification. |
|
Hello @juanpprieto , please let me know if I can change something to make this look good. |
|
Hey @xav-ie! Thanks for the PR and apologies for the delay. I am still investigating internally what this value should be. In the liquid domain the reference Horizon theme's add to cart event appears to set You can test here: I will talk to the analytics team and try to confirm why we don't seem to be sending the combined value. |
|
Thank you for investigating. I appreciate it a lot. This is the reason I made the PR. I was also confused and thought this was wrong. Hopefully, it will become clear at conclusion of this PR :) |
|
This pull request has been marked as stale due to inactivity for 60 days. If no further activity occurs, it will be closed in 7 days. |

Fix
totalValuetest measurementWHY are these changes introduced?
In my limited experience, I notice many Shopify stores are sending an incorrect
totalValue(in analytics lib)/total_value(in network request) measurement intheir analytics. Most Shopify storefronts default to sending just the
product.value.I am pretty sure this is incorrect:
hydrogen/packages/hydrogen-react/src/analytics-types.ts
Lines 116 to 117 in e7db0a5
I went to verify this, but the tests do not rigourously test the
totalValue. Itjust sends placeholder value, not based on the products at all. Currently, it
is not clear what expected
totalValueis for different product payloads.WHAT is this pull request doing?
Enhances the tests to ensure the totalValue is more accurate. Establishes
that
totalValueis indeed the total value of the products in the event.totalValuemeasurement, making itclear
totalValueis based onproductstotal value.HOW to test your changes?
npm run testChecklist