-
Notifications
You must be signed in to change notification settings - Fork 217
Track the reconnect notice and button clicks #4717
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
@wjrosa
EDIT: |
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.
This is working as indicated, my only nitpick is that we are not actually tracking the click in the admin notice button, but the page load it triggers (&highlight=account-details), so page refreshes will count towards it 🤷🏼

Also, maybe we should also track the Re-authenticate
button to get the full picture:

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.
Code looks good, agreeing with Diego's feedback.
Thanks for the review, folks!
That's a good point. I refactored the code in f348c35 to actually track the click instead of the page scroll. I had to use raw JS since I could not import the existing Tracks component there.
Added tracking for that in 79025c6 |
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.
Changes look good to me
$onclick .= 'window.wcTracks.recordEvent( \'wcstripe_reconnect_button_click\', { source: \'admin_notice\', mode: ' . ( $testmode ? '\'test\'' : '\'live\'' ) . ' } );'; | ||
$onclick .= 'window.location = \'admin.php?page=wc-settings&tab=checkout§ion=stripe&panel=settings&highlight=account-details\'; return 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.
Not a fan of this series of inline JS, but I also don't see a significantly better approach compared to this.
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 we not add a JS snippet that wraps all of this logic? It feels like manually generating the onclick handling here is going to be really messy to get right in the long run. Also, is it possible for us to keep the href
and simply let the onclick trigger the tracking?
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.
This is working as indicated, my only nitpick is that we are not actually tracking the click in the admin notice button, but the page load it triggers (&highlight=account-details), so page refreshes will count towards it 🤷🏼
~ Diego
Another option to address the original issue that this in-line JS is trying to solve is that we pass in a URL fragment here, and in the frontend record the event if the fragment is present. Once the event is recorded, remove the fragment so that page refreshes don't result in duplicate events.
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.
Good point, folks. I refactored it again in b18cc9c, using Malith's suggestion above (thanks!)
…/github.com/woocommerce/woocommerce-gateway-stripe into add/tracking-account-reconnect-notice-click
Co-authored-by: Diego Curbelo <[email protected]>
Co-authored-by: Diego Curbelo <[email protected]>
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 fixes @wjrosa. It's working as expected now!
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.
I removed the reconnection logic yesterday in #4730. I think we need to take a step back to assess how and when we show the reconnection notices due to some of the issues we saw in the 10.0.0 release.
I removed all code related to the admin notice yesterday. I kept only the code tracking the existing buttons and links. Still, do you want to put this on hold anyway? |
Fixes STRIPE-740
Changes proposed in this Pull Request:
To complement #4669, this PR will add tracking for the "Reconnect to Stripe" button clicks (for both the admin notice and the main button in the settings screen).
Testing instructions
add/tracking-account-reconnect-notice-click
)wcstripe_reconnect_button_click
eventChangelog entry
Changelog Entry Comment
Comment
Post merge