-
Notifications
You must be signed in to change notification settings - Fork 59
Implemented setEcommerceView #699
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
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.
Looks good to me, I will do a functional test and get back to you 👍
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.
Nice work! I only had few couple comments.
Edit: I did some functional testing and things worked as I expected. After updating the migration to use the NewTagParameterMigrator
class, it worked correctly.
Updates/5.0.0-rc4.php
Outdated
/** | ||
* Update for version 5.0.0-rc4. | ||
*/ | ||
class Updates_5_0_0_rc4 extends PiwikUpdates |
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.
Matomo 5.0.0-rc4 has already been released. Could you please refactor this for 5.0.0-rc5?
lang/en.json
Outdated
"MatomoTagEcommerceViewIsEcommerceView": "Is Ecommerce View", | ||
"MatomoTagEcommerceViewIsEcommerceViewHelp": "Used to record that the current page view is an item (product) page view, or a Ecommerce Category page view. On a category page, you can set the parameter category, and set the other parameters to empty strings. Tracking Product/Category page views will allow Matomo to report on Product & Categories conversion rates (Conversion rate = Ecommerce orders containing this product or category / Visits to the product or category). Requires the Ecommerce plugin to be active.", | ||
"MatomoTagEcommerceViewPrice": "Price", | ||
"MatomoTagEcommerceViewPriceHelp": "Item's display price, not use in standard Matomo reports, but output in API product reports.", |
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.
Was this meant to say used
instead of use
?
Updates/5.0.0-rc4.php
Outdated
$updater->executeMigrations(__FILE__, $this->getMigrations($updater)); | ||
|
||
// Migrate the MatomoConfiguration type variables to all include the newly configured fields. | ||
$migrator = new NewVariableParameterMigrator(MatomoTag::ID, 'isEcommerceView', false); |
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.
Can you please update this to use NewTagParameterMigrator
instead, since you're adding parameters to a Tag and not a Variable?
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.
Thank you for making those changes. It looks good to me and I didn't find any issues during functional testing 👍
I thought Ecommerce support would be nice (at least to a certain extend) for the Tag Manager.
This PR focuses on the
setEcommerceView
of the Matomo Tracker.Documentation is mostly copied from the corresponding function in
piwik.js
.I'll open an seperate issue about my thoughts of the other ecommerce tracking functions.
Feedback and discussion is highly appreciated 😄.