-
Notifications
You must be signed in to change notification settings - Fork 29
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
Clean up Analytics Names #313
base: main
Are you sure you want to change the base?
Conversation
@@ -113,19 +113,19 @@ public class CardClient: NSObject { | |||
return | |||
} | |||
|
|||
analyticsService?.sendEvent("card-payments:3ds:confirm-payment-source:challenge-required") | |||
analyticsService?.sendEvent("card-payments:confirm-payment-source:challenge-required") |
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.
On Android we do have every event related to approveOrder()
named with approve-order
. Here's an example in CardAnalytics.kt.
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.
Yes, I had opened a PR yesterday to rename these events but closed it and wanted to just clean up things that were mislabeled as "3ds" in this PR. I wanted to hold off other changes until we have alignment in iOS/Android on analytic events.
completion(nil, error) | ||
} | ||
|
||
private func notifyCheckoutCancelWithError(with error: CoreSDKError, completion: (CardResult?, CoreSDKError?) -> Void) { | ||
analyticsService?.sendEvent("card-payments:3ds:challenge:user-canceled") | ||
analyticsService?.sendEvent("card-payments:approve-order:challenge:user-canceled") |
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 one seems like it deviates a bit from the rest. Everything else seems to have the format <MODULE>:<FEATURE>:<EVENT_NAME>
. Would card-payments:approve-order:user-canceled-challenge
work?
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 was thinking of changing both "card-payments:vault-wo-purchase:challenge:canceled" and this one to
"card-payments: approve-order:auth-challenge:canceled"
"card-payments: vault-wo-purchase:auth-challenge:canceled"
But I wanted to do overhaul in another PR after we have resolution between iOS and Android analytics.
This PR is correcting incorrectly named "3ds" 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.
Ok cool. This makes sense for 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.
Ty, Steven. Yeah, checking with analytics folks to coordinate name changing before merging anything. It totally makes sense to use approve-order.
But I wanted to make all the final changes in a different PR.
Summary of changes
Changes:
card-payments:3ds:started
=>card-payments:approve-order:started
card-payments:3ds:confirm-payment-source:challenge-required
->card-payments:confirm-payment-source:challenge-required
card-payments:3ds:confirm-payment-source:succeeded
->card-payments:confirm-payment-source:succeeded
card-payments:3ds:confirm-payment-source:failed
->card-payments:confirm-payment-source:failed
(I think intention was to have separate success and fail for 3DS also, but we don't have those events in iOS)
card-payments:3ds:succeeded
->card-payments:approve-order:succeeded
card-payments:3ds:failed
->card-payments:approve-order:failed
(these were just misnamed, they are being called for both contingency and non-contingency flows in iOS through notifyCheckoutSuccess/Failure functions)
In Android, there are different distinct success events for contingency and non-contingency flows,
no common end events)
card-payments:3ds:challenge:user-canceled
->card-payments:approve-order:challenge:user-canceled
We decided to take inventory of iOS and Android events and discuss possibility of sending common begin and end events in iOS and Android, along with agreement on naming conventions.
This ticket just cleans up misnamed events.
Checklist
- [ ] Added a changelog entryAuthors