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

Send PayPal correlation_id in analytics payload #1108

Merged
merged 12 commits into from
Oct 5, 2023

Conversation

scannillo
Copy link
Contributor

@scannillo scannillo commented Oct 5, 2023

Summary

Currently, we cannot tie our analytics to a PayPal correlationID (aka clientMetadataID) because we are not sending the value in our analytics batch payload. We recently had a Slack inbound where this data would be extremely helpful for handoff to the underlying PP Web or PP NXO team to do further investigation on the specific PayPal transaction.

Changes

  • Update FPTIBatchData to send correlation_id at the event level (I verified that I can see these values populate in FPTI event explorer)
  • Update BTAnalyticsService to accept an optional correlationID param
  • Send already-existant clientMetadataID in BTPayPalWebClient
  • Collect clientMetadataID & send in analytics for BTPayPalNativeCheckoutClient

Feedback/Questions

  • We don't have a testing infrastructure for BTPayPalNativeCheckoutClient set up right now, which I left a TODO for. It will be a handful of work, since MXO is hard to test (ticket DTBTSDK-3076)
  • We don't have testing in place for our analytics (ticket DTBTSDK-2441)
  • We could block this PR until the above ^ are taken care of, if folks feel strongly? What do you think?

Checklist

  • Added a changelog entry

Authors

@scannillo

@scannillo scannillo requested a review from a team as a code owner October 5, 2023 15:11
@jaxdesmarais
Copy link
Contributor

We could block this PR until the above ^ are taken care of, if folks feel strongly? What do you think?

I personally don't think we need to wait on these. I feel like MXO being easily testable is a long ways off. With the analytics ticket maybe we can prioritize that in the next few sprints? I feel like that one may be a bit easier to tackle sooner but still not a blocker.

@scannillo
Copy link
Contributor Author

I personally don't think we need to wait on these. I feel like MXO being easily testable is a long ways off. With the analytics ticket maybe we can prioritize that in the next few sprints? I feel like that one may be a bit easier to tackle sooner but still not a blocker.

Cool, yeh I'm down for not blocking this!

The testing story on MXO won't change, realistically. So we'll have to do something like PPCP with a wrapper protocol. It's possible, but a pain :/. We could also explore if OCMock makes things easier too (I captured this in the ticket).

@scannillo
Copy link
Contributor Author

UI Test failures are unrelated :/

Copy link
Contributor

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

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

🔥

@scannillo scannillo merged commit 431f1c9 into main Oct 5, 2023
5 of 6 checks passed
@scannillo scannillo deleted the send-correlation-id-analytics branch October 5, 2023 21:35
@jaxdesmarais jaxdesmarais mentioned this pull request Oct 10, 2023
1 task
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.

3 participants