Skip to content
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

Vault Without Purchase Analytics #258

Merged
merged 4 commits into from
Mar 1, 2024
Merged

Conversation

vradopp
Copy link
Contributor

@vradopp vradopp commented Feb 28, 2024

Reason for changes

We want to add analytics to the flow to track merchant’s usage of vault without purchase.

Summary of changes

Added analytics events for vault without purchase flows

PayPal Wallet Vault Events
paypal-web-payments:vault-wo-purchase:started
paypal-web-payments:vault-wo-purchase:canceled
paypal-web-payments:vault-wo-purchase:succeeded
paypal-web-payments:vault-wo-purchase:failed

Card Vault Events
card-payments:vault-wo-purchase:started
card-payments:vault-wo-purchase:challenge-required
card-payments:vault-wo-purchase:challenge:canceled
card-payments:vault-wo-purchase:succeeded
card-payments:vault-wo-purchase:failed

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

  • vradopp

@vradopp vradopp requested a review from a team as a code owner February 28, 2024 21:35
@@ -104,6 +104,10 @@ public class PayPalWebCheckoutClient: NSObject {
/// - Returns: PayPalVaultResult
/// - Throws: PayPalSDK error if vaulting could not complete successfully
public func vault(url: URL) {
Copy link
Collaborator

@scannillo scannillo Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside the scope of this PR - but why doesn't PayPalWebCheckoutClient.vault() accept a request object? The entry point to all of our other SDK features use the request object pattern (see CardClient.vault(), PayPalWebClient.start(), PayPalNativeCheckoutClient.start()).

The request object pattern helps us easily add request params as needed as features scale versus needing to add new overload methods.

cc: @KunJeongPark

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO a PayPalVaultRequest with a required setupTokenID & url would help future proof our analytics (in case the URL changes the approval_session_id param name), as well as align us better to the Card vaulting feature's public API.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we've never discussed this as a group. A PayPalVaultRequest object with a url property would be consistent with other methods. I'm personally 50 / 50 on adding a setupTokenID since it's an extra property, but if it's critical to analytics we should add it in.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Captured in DTPPCPSDK-1985.

@@ -104,6 +104,10 @@ public class PayPalWebCheckoutClient: NSObject {
/// - Returns: PayPalVaultResult
/// - Throws: PayPalSDK error if vaulting could not complete successfully
public func vault(url: URL) {
if let setupToken = getQueryStringParameter(url: url.absoluteString, param: "approval_session_id") {
analyticsService = AnalyticsService(coreConfig: config, setupToken: setupToken)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also slightly unrelated to analytics (but revealed by analytics 😆) - should we boot a merchant from the flow if their URL doesn't contain this value? Currently merchants can pass us any URL and we'll open it in an ASWebSession.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for being precautious. The only risk here would be if the format of the URL we get from PPaaS changed one day to something we don't expect, it could break existing integrations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, seems like a pretty big risk vector. You could pass in stealyourcreditcard.com and we'd be like "sounds good!" and open it right up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y'all know what we would need to allowlist so we can patch this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Luckily this hasn't been released yet on iOS, so we should be able to resolve this. We also should figure out when we want to release since our unreleased section is real long and we probably should be doing smaller releases along the way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah on second thought maybe restricting the TLD to paypal.com at least? That way we know for certain it's going to us over TLS.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Captured in DTPPCPSDK-1986.

@@ -44,6 +44,8 @@ public class CardClient: NSObject {
/// - Parameters:
/// - vaultRequest: The request containing setupTokenID and card
public func vault(_ vaultRequest: CardVaultRequest) {
analyticsService = AnalyticsService(coreConfig: config, setupToken: vaultRequest.setupTokenID)
analyticsService?.sendEvent("card-payments:vault-wo-purchase:started")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we extract analytics out into a constants file on PPCP: example from the BT SDK?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good call. Making DTPPCPSDK-1984 to address this project wide.

Copy link
Collaborator

@scannillo scannillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actual changes look good - comments left are outside the scope of analytics 👍

@vradopp vradopp merged commit c504258 into main Mar 1, 2024
4 checks passed
@vradopp vradopp deleted the vaultWithoutPurchaseAnalytics branch March 1, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants