Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
171 stars 69 forks source link

tracks events names are easier to manage inline (constants file is not helpful) #8066

Closed haszari closed 6 months ago

haszari commented 7 months ago

Describe the bug

Many of our tracks events names are stored as constants in /tracks/index.js.

This is unnecessary, and makes it more painful to iterate on tracks:

I recommend the following approach:

The example code in field guide Recording Events in Tracks page (PCYsg-4KT-p2) has event names inline, for example:

window._tkq = window._tkq || [];
window._tkq.push( [
  'recordEvent',
  'wpadmin_domain_search',
  { status: 'available' }
] );

Note also other Automattic code bases have event names inline as raw strings. I'd argue we should aim for broad consistency unless we have good reason to take a different approach.


I don't know the history and rationale for these constants, so if anyone is aware of benefits to these constants, please comment! One potential reason would be to ensure the event name is consistent everywhere (i.e. avoid typos). Any event typos should show up in PR reviews, testing, and tracks data, and can easily be rectified. If there is a typo in the constant value … see above about iterating on names! Looks like events was introduced early (#855), though that PR doesn't have specifics on why move constants out of line.

haszari commented 7 months ago

This has been bugging me for a while 😄 . I'm working on Helix-owned events in this PR at a background priority: #8069

zmaglica commented 7 months ago

@haszari do you maybe know, will team Helix work on this issue, or shall I move it to the Gamma backlog?

haszari commented 7 months ago

will team Helix work on this issue, or shall I move it to the Gamma backlog?

Thanks for checking @zmaglica . I had planned to work on part of this (move Helix event names out of constants) though at a low priority (#8069). If there's agreement that this is worthwhile, yes please shift it to Gamma backlog – I appreciate y'all taking on my idea :)

haszari commented 6 months ago

Fixed by @dmallory42 in #8166 🎉

Closing.