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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guarding against Jirafe false positive conversions #2273

Closed
wants to merge 1 commit into
base: 1-1-stable
from

Conversation

Projects
None yet
2 participants
@jumph4x
Member

jumph4x commented Dec 4, 2012

Heyyo,

Whoever pulls this one in should cherry pick this to 1-2-stable as well as master, AFAIK they all suffer from this.

Should be pretty obvious, but basically after cross-referencing actual collected conversions server-side against client-side GA and Jirafe, it was obvious Jirafe was way too optimistic.

Cheers 馃帀

@jumph4x

This comment has been minimized.

Show comment
Hide comment
@jumph4x

jumph4x Dec 4, 2012

Member

Please either mention # or explicitly drop SHA1s :)

Or else you will get two more pull requests :trollface:

Member

jumph4x commented Dec 4, 2012

Please either mention # or explicitly drop SHA1s :)

Or else you will get two more pull requests :trollface:

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Dec 4, 2012

Member

Is it possible to write a regression test for this also?

Also: could you better explain the problem that this is fixing? Is it showing too many analytic tags?

Member

radar commented Dec 4, 2012

Is it possible to write a regression test for this also?

Also: could you better explain the problem that this is fixing? Is it showing too many analytic tags?

@jumph4x

This comment has been minimized.

Show comment
Hide comment
@jumph4x

jumph4x Dec 4, 2012

Member

I'll look into the regression test.

Context

The semantics of tracking a conversion is as this: 'a customer has given us money'

Problem

Existing Jirafe implementation tracks conversions, or in the case of Jirafe, sends order data whenever the Spree::OrdersController#show renders the view.

This is a problem because viewing your order history does not constitute a conversion. Similarly, if one refreshes the success\completion page multiple times, only a single conversion should be tracked.

Approach

Current Spree approach utilizes the expiring flash pseudo-hash, so that the conversions only run once.

The Google Analytics implementation of Spree already does this correctly.

Member

jumph4x commented Dec 4, 2012

I'll look into the regression test.

Context

The semantics of tracking a conversion is as this: 'a customer has given us money'

Problem

Existing Jirafe implementation tracks conversions, or in the case of Jirafe, sends order data whenever the Spree::OrdersController#show renders the view.

This is a problem because viewing your order history does not constitute a conversion. Similarly, if one refreshes the success\completion page multiple times, only a single conversion should be tracked.

Approach

Current Spree approach utilizes the expiring flash pseudo-hash, so that the conversions only run once.

The Google Analytics implementation of Spree already does this correctly.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Dec 4, 2012

Member

It sounds like it should be easy enough to give a list of steps that would reproduce this issue in some kind of visible context. Once I can see the issue for myself, then I can go about fixing it.

Member

radar commented Dec 4, 2012

It sounds like it should be easy enough to give a list of steps that would reproduce this issue in some kind of visible context. Once I can see the issue for myself, then I can go about fixing it.

@jumph4x

This comment has been minimized.

Show comment
Hide comment
@jumph4x

jumph4x Dec 4, 2012

Member

Sure thing, just that you asked for an explanation.

Steps [observed]

  1. Go through check out process until you hit the success page at Spree::OrdersController#show
  2. View Source
  3. Observe both Google Analytics and Jirafe reporting order details via JS in the document head.

The above is correct. Now:

  1. Hit 'my account' at the top.
  2. Hit any order
  3. View Source
  4. Observer Jirafe reporting order details via JS as if this order has just been completed.

Jirafe just reported 2 sales. Google Analytics reported 1. Jirafe is wrong. GA is correct.

Member

jumph4x commented Dec 4, 2012

Sure thing, just that you asked for an explanation.

Steps [observed]

  1. Go through check out process until you hit the success page at Spree::OrdersController#show
  2. View Source
  3. Observe both Google Analytics and Jirafe reporting order details via JS in the document head.

The above is correct. Now:

  1. Hit 'my account' at the top.
  2. Hit any order
  3. View Source
  4. Observer Jirafe reporting order details via JS as if this order has just been completed.

Jirafe just reported 2 sales. Google Analytics reported 1. Jirafe is wrong. GA is correct.

@jumph4x

This comment has been minimized.

Show comment
Hide comment
@jumph4x

jumph4x Dec 5, 2012

Member

I read the older bug report trails that @schof referenced and fail to see how this has since been addressed in 1-2-stable or master.

Google Analytics, AFAIK, will actually reject duplicate submissions based on the (unique) transaction id, in our case Spree::Order#number. Now Jirafe, from what is confirmed in our production environment, does NOT do this.

IMHO this bug is extremely simple and this is an issue of miscommunication or something. If I can further clarify anything, let me know.

Member

jumph4x commented Dec 5, 2012

I read the older bug report trails that @schof referenced and fail to see how this has since been addressed in 1-2-stable or master.

Google Analytics, AFAIK, will actually reject duplicate submissions based on the (unique) transaction id, in our case Spree::Order#number. Now Jirafe, from what is confirmed in our production environment, does NOT do this.

IMHO this bug is extremely simple and this is an issue of miscommunication or something. If I can further clarify anything, let me know.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Dec 5, 2012

Member

I am not sure I understand "the success page at Spree::ProductsController#show". I don't see that page at any point in the checkout that isn't the "Add to cart" button. Perhaps you meant OrdersController#show?

Member

radar commented Dec 5, 2012

I am not sure I understand "the success page at Spree::ProductsController#show". I don't see that page at any point in the checkout that isn't the "Add to cart" button. Perhaps you meant OrdersController#show?

@jumph4x

This comment has been minimized.

Show comment
Hide comment
@jumph4x

jumph4x Dec 5, 2012

Member

Absolutely, my bad!

Correcting my comments to reflect the mistake, not sure why I keep mixing em up.

Member

jumph4x commented Dec 5, 2012

Absolutely, my bad!

Correcting my comments to reflect the mistake, not sure why I keep mixing em up.

@radar radar closed this in f8755e4 Dec 5, 2012

radar added a commit that referenced this pull request Dec 5, 2012

radar added a commit that referenced this pull request Dec 5, 2012

radar added a commit that referenced this pull request Dec 5, 2012

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Dec 5, 2012

Member

Looks like this finally fixes this issue. Thanks @jumph4x!

Member

radar commented Dec 5, 2012

Looks like this finally fixes this issue. Thanks @jumph4x!

@jumph4x

This comment has been minimized.

Show comment
Hide comment
@jumph4x

jumph4x Dec 5, 2012

Member

Brofist

Member

jumph4x commented Dec 5, 2012

Brofist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment