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

Add return url to analytics #1239

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Add return url to analytics #1239

merged 3 commits into from
Jan 15, 2025

Conversation

saperi22
Copy link
Contributor

@saperi22 saperi22 commented Dec 10, 2024

Summary of changes

  • Logging the return url on starting of handleReturnToApp.

Checklist

  • Added a changelog entry
  • Relevant test coverage
  • Tested and confirmed payment flows affected by this change are functioning as expected

Authors

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

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.

@saperi22 is this ready to review or do we need to hand it off before you are out?

@sairaghukiran
Copy link

@saperi22 is this ready to review or do we need to hand it off before you are out?

@jaxdesmarais do we want to send this info. at all?
Tim had raised a good question. Want your opinion on it too.

We are logging the url on individual success or failure events. I hoped logging it when handle return is called would be easier and would be a central place for success and failed events. However, Tim mentioned we'd have to search for the handle return to app event, find the session id and manually stitch through the other events. Given that, I was wondering if this would be useful or would it never be used at all.

@jaxdesmarais
Copy link
Contributor

👀 is the session ID different for the handle return events vs the success/failed events? If so that sounds like a bug that we should resolve. In the success case for example all of the following events should have the same session ID/correlation ID/PayPal context ID:

paypal:tokenize:app-switch:started
paypal:tokenize:app-switch:succeeded
paypal:tokenize:handle-return:started
paypal:tokenize:succeeded

@sairaghukiran
Copy link

👀 is the session ID different for the handle return events vs the success/failed events? If so that sounds like a bug that we should resolve. In the success case for example all of the following events should have the same session ID/correlation ID/PayPal context ID:

paypal:tokenize:app-switch:started
paypal:tokenize:app-switch:succeeded
paypal:tokenize:handle-return:started
paypal:tokenize:succeeded

I didnt mean there are separate sessions ids.
We didn't think there's an easy was to look at all the events in a given session.
I think we may have overlooked that we can filter by the session id, and that should work for us.

The concern was whether we'll be able to use the url sent in handleReturnToApp() and figure out whether the it succeeded or failed.

@jaxdesmarais
Copy link
Contributor

The concern was whether we'll be able to use the url sent in handleReturnToApp() and figure out whether the it succeeded or failed.

Ah gotcha. Agree, I do not think we will know the status based on the URL. But that is less the issue, the issue is that Venice does not ever log the full URL they send to us. This has been an issue on iOS because sometimes we cannot open the URL but we also don't know what the URL is. I don't think we have come across this same issue on Android yet, but the URL will help if we ever do come across a similar issue. This will allow us to hold onto the URL that we get back to allow us to troubleshoot that URL if we ever have issues and less to determine success/failure. That would be determined at a later point and the events could be linked via the session ID.

@saperi22 saperi22 marked this pull request as ready for review January 9, 2025 19:11
@saperi22 saperi22 requested a review from a team as a code owner January 9, 2025 19:11
@saperi22
Copy link
Contributor Author

saperi22 commented Jan 9, 2025

The current change is to add logging at handleReturnToApp which occurs before success and failure cases.

The other piece of work would be to remove logging from success and failure cases. This PR doesn't make the change, and I'm only trying to understand if that should happen.

the issue is that Venice does not ever log the full URL they send to us.

Yes, Venice doesn't log anything, but we are doing it. It shouldn't matter if we log early or on result.

This will allow us to hold onto the URL that we get back to allow us to troubleshoot that URL if we ever have issues and less to determine success/failure.

If I understand what you are saying correctly, we are both making the same statement.

Are you saying you are ok with the changes?

@jaxdesmarais
Copy link
Contributor

Yep, we are saying the same thing 😄

@saperi22 saperi22 merged commit 402d25a into main Jan 15, 2025
3 checks passed
@saperi22 saperi22 deleted the add-return-url-to-analytics branch January 15, 2025 16:37
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