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

WooCommerce Analytics: Queue add_to_cart Events #9052

Merged
merged 3 commits into from
Mar 26, 2018

Conversation

greenafrican
Copy link
Contributor

@greenafrican greenafrican commented Mar 14, 2018

Fixes #9049

Changes proposed in this Pull Request:

  • move to queueing all add_to_cart events and logging on the next page
  • prevent duplicate product_view events from product page

Testing instructions:

  • click any add to cart button and look for event to be logged in the next page load
  • add a link to a post or page with href like http://exampleshop.com/?add-to-cart=${product_id}
  • make sure page view and cart event logged in next page load

Proposed changelog entry for your changes:

WooCommerce Analytics: Start tracking all possible ways to add a product to a cart

@greenafrican greenafrican added [Type] Bug When a feature is broken and / or not performing as intended [Feature] WooCommerce Analytics labels Mar 14, 2018
@greenafrican greenafrican requested a review from psealock March 14, 2018 14:36
@greenafrican greenafrican requested a review from a team as a code owner March 14, 2018 14:36
@greenafrican greenafrican requested a review from timmyc March 14, 2018 14:36
* @param $cart_item_data
*/
public function capture_add_to_cart( $cart_item_key, $product_id, $quantity, $variation_id, $variation, $cart_item_data ) {
if ( ! is_single() ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_single() doesn't seem to work here so we always get a page_view even if we've already logged one on the single product page. Any ideas @psealock @timmyc ?

@greenafrican greenafrican changed the title New wc session to queue cart events WooCommerce Analytics: queue cart events Mar 14, 2018
@greenafrican greenafrican changed the title WooCommerce Analytics: queue cart events WooCommerce Analytics: Queue add_to_cart Events Mar 14, 2018
@ghost ghost removed the [Status] In Progress label Mar 15, 2018
@greenafrican greenafrican force-pushed the fix/missing-cart-events branch from 4793a0f to dacc670 Compare March 15, 2018 12:17
Copy link
Contributor

@timmyc timmyc left a comment

Choose a reason for hiding this comment

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

Some quick comments, will run some further tests shortly and report back.

);
// check for previous add-to-cart cart events
$data = WC()->session->get( 'wca_session_data' );
if ( !empty( $data ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor but space between ! empty

@@ -325,4 +303,89 @@ public function get_user_id() {
return 'null';
}

/**
* @param $cart_item_key
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is a little wonky right here.

public function capture_add_to_cart( $cart_item_key, $product_id, $quantity, $variation_id, $variation, $cart_item_data ) {
$referer_postid = isset( $_SERVER['HTTP_REFERER'] ) ? url_to_postid( $_SERVER['HTTP_REFERER'] ) : 0;
// if the referring post is not a product OR the product being added is not the same as post
// (eg. related product list on single product page) then include a product view event
Copy link
Contributor

Choose a reason for hiding this comment

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

Also noting from our convo today - how will this bit of logic respond to a quick add event from a product listing page?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. It captures the add_to_cart action properly -- but it does also fire off a view which might not be accurate in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, adding a product from the product page results in 2 product_views and one add-to-cart

if the referring post is not a product OR the product being added is not the same as post
(eg. related product list on single product page) then include a product view event

This is correct, and when I step through the code, this portion behaves as expected. Digging further, the extra page view is from the page refresh, and you can see that working in the master branch.

@greenafrican Since we are utilizing session, in capture_product_view we can check to see if a product_view is about to be dropped. This may depend on timing. Is it worth exploring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Digging further, the extra page view is from the page refresh

Yes, I had an extra check to prevent these but removed it yesterday as it can cause other issues down the line. For example, if the user continues to add_to_cart we log all the additional add_to_cart events but no further product_views. It is not ideal/perfect as is but I'd rather there were a few extra product_views than have a situation where there can be more add_to_carts than product_views. The more I think about this the more I wonder if we don't need to move to reporting on uniques rather than simple counts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how will this bit of logic respond to a quick add event from a product listing page?

@timmyc It will add both a product_view and an add_to_cart event, which is what we want.

It captures the add_to_cart action properly -- but it does also fire off a view which might not be accurate in this case.

@justinshreve My thoughts are that we do want the product_view here for 2 reasons: 1) we don't want a situation where we have more add_to_cart events than product_views in a funnel, 2) I feel as though, by clicking add_to_cart, the user has 'looked' at the product.


/**
* @param $product_id
* @param $qty
Copy link
Contributor

Choose a reason for hiding this comment

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

s/qty/quantity ?


// extract new event data
$new_data = array(
'event' => $event,
Copy link
Contributor

Choose a reason for hiding this comment

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

spacing a bit off here too.

@timmyc
Copy link
Contributor

timmyc commented Mar 15, 2018

/cc @justinshreve - would be keen to have you 👀 this PR if you have some time.

Copy link
Contributor

@justinshreve justinshreve left a comment

Choose a reason for hiding this comment

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

A few comments. Events seem to be firing correctly.

$product_id,
$quantity,
$event,
$cart_item_key = '' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: I'm not a fan of having each argument on it's own line here.


// check for existing data
$data = WC()->session->get( 'wca_session_data' );
if ( empty( $data ) || !is_array( $data ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space between ! and is_array

/**
* Gets product categories or varation attributes as a formatted concatenated string
*
* @param array $product WC_Product.
Copy link
Contributor

Choose a reason for hiding this comment

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

$product is an object, not an array.

public function capture_add_to_cart( $cart_item_key, $product_id, $quantity, $variation_id, $variation, $cart_item_data ) {
$referer_postid = isset( $_SERVER['HTTP_REFERER'] ) ? url_to_postid( $_SERVER['HTTP_REFERER'] ) : 0;
// if the referring post is not a product OR the product being added is not the same as post
// (eg. related product list on single product page) then include a product view event
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. It captures the add_to_cart action properly -- but it does also fire off a view which might not be accurate in this case.

$product_id,
$quantity,
$event,
$cart_item_key = '' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$cart_item_key is unused in this function

Robert Elliott added 2 commits March 16, 2018 11:32
send product view events for products added to cart from any list or non-product detail page
@timmyc
Copy link
Contributor

timmyc commented Mar 16, 2018

Any ideas why 88e8644 broke the build for this branch?

@zinigor
Copy link
Member

zinigor commented Mar 16, 2018

It probably just needs a restart - I restarted two failed jobs, let's see what happens.

@timmyc
Copy link
Contributor

timmyc commented Mar 16, 2018

Thanks @zinigor

@zinigor zinigor added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Mar 19, 2018
@greenafrican
Copy link
Contributor Author

Thanks for the reviews @timmyc @psealock @justinshreve

Fixes included in the latest squashed commit:

  • spaces between ! empty and ! array
  • indentation and spacing fixes
  • rename qty to quantity
  • moved arguments onto 1 line
  • documented $product as an object, not array
  • removed unused $cart_item_key

@greenafrican
Copy link
Contributor Author

@jeherve Would you mind reviewing this PR, to be included in v5.9.1 if possible?

@greenafrican greenafrican requested a review from jeherve March 20, 2018 14:07
@jeherve jeherve added this to the 6.0 milestone Mar 20, 2018
@greenafrican
Copy link
Contributor Author

@psealock @timmyc @justinshreve would you mind doing another review of this?

Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

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

Gave this another run through. Works as expected, didn't find anything odd.

@zinigor zinigor merged commit d876f73 into master Mar 26, 2018
@ghost ghost removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Mar 26, 2018
@zinigor zinigor deleted the fix/missing-cart-events branch March 26, 2018 13:20
oskosk added a commit that referenced this pull request Mar 26, 2018
dereksmart pushed a commit that referenced this pull request Mar 27, 2018
* Changelog 6.0: create base for changelog.

* Add #8938 to changelog

* Add #8962 to changelog

* Add #8974 to changelog

* Add #8975 to changelog

* Add #8978 to changelog

* Add #8867 to changelog

* Add #8937 to changelog

* Add #8961 to changelog

* Add #8855 to changelog

* Add #8944 to changelog

* Add #8973 to changelog

* Add #8977 to changelog

* Add #8979 to changelog

* Add #8980 to changelog

* Add #8982 to changelog

* Add #8983 to changelog

* Add #8984 to changelog

* Add #8986 to changelog

* Add #9005 to changelog

* Add #9010 to changelog

* Add #9012 to changelog

* Add #9021 to changelog

* Add #9022 to changelog

* Add #9056 to changelog

* Add #9061 to changelog

* Add #9079 to changelog

* Add #9080 to changelog

* Add #9088 to changelog

* Add #9096 to changelog

* Add #9097 to changelog

* Add #9100 to changelog

* Add #9107 to changelog

* Add #8969 to changelog

* Add #8993 to changelog

* Add #9003 to changelog

* Add #9031 to changelog

* Add #8945 to changelog

* Add #9052 to changelog

* Add #9058 to changelog

* Add #9066 to changelog

* Add #9076 to changelog

* Add #9053 to changelog

* Add #9108 to changelog

* Add #9135 to changelog

* Add #9148 to changelog

* Add #9125 to changelog

* Add #9137 to changelog

* Added testing instructions for 6.0.

* Added IS testing instructions, huge props to @tiagonoronha.

* Added #8498 to changelog.

* Added #8954 to changelog.

* Added #8985 to changelog.

* add #9027

* add #9112 to changelog

* add #9136 to changelog

* add #9102 to changelog

* add #9093 to changelog

* add #9062 to changelog

* add #9172 to changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] WooCommerce Analytics [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants