-
Notifications
You must be signed in to change notification settings - Fork 115
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
[Woo POS] Analytics: Make allowed event list for POS when decorating #15202
Conversation
|
Just noting that we'll be handling receipt-related events separately here: #15203, as there are name changes and success/failure cases are not being tracked in POS |
I found out is not tracking |
@staskus this is ready for review when you have the chance, added the missed events on the last commit 🙇 |
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! Works well. 👍
The main events on POS are tracked as expected.
WooAnalyticsStat.collectPaymentFailed, | ||
WooAnalyticsStat.collectPaymentSuccess, | ||
WooAnalyticsStat.collectInteracPaymentSuccess, | ||
WooAnalyticsStat.interacRefundSuccess, |
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.
As I understand, there's no way to reproduce refund-related events, correct?
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.
AFAIK you're right, I'm not very acquainted with Interac but I believe that's the case and it was pretty challenging just to make them work ( some context on pdfdoF-yL-p2 and pdfdoF-R4-p2 )
WooAnalyticsStat.pointOfSaleClearCartTapped, | ||
WooAnalyticsStat.pointOfSaleExitMenuItemTapped, | ||
WooAnalyticsStat.pointOfSaleExitConfirmed, | ||
WooAnalyticsStat.pointOfSaleGetSupportTapped, |
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.
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.
Yeah! I saw the same when testing. I left it out because as you mention we're already "outside POS" and it's not in the Android or the P2 list either 👍
Closes: #15058
Description
This PR switches from a
exemptedEventList
to anallowedEventList
for events that we decorate withpos_
prefix for POS, so we are sure that no sneaky events pass through the decorator if they shouldn't.I've moved all the shared events with IPP that are currently in Android for simplicity, and all payment/card-related. We can reduce the list a bit as some are not really used (printing receipts, COD, ...). Possibly can be reduced further, but we do not have a real list asides from the Android's file referenced above.
Ref: p1739367957933319-slack-C070SJRA8DP
Caveat
I've noticed we do not track
receipt_email_success
andreceipt_email_failed
in POS, I've asked in the same thread above if we want to do so, and if we do, I'll tackle it separately from this issue.Testing
pos_
-specific events tracked.Eg:
RELEASE-NOTES.txt
if necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: