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

[Woo POS] Analytics: Make allowed event list for POS when decorating #15202

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Feb 19, 2025

Closes: #15058

Description

This PR switches from a exemptedEventList to an allowedEventList for events that we decorate with pos_ 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 and receipt_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

  • In POS, test through the normal flow, you should see no difference in the pos_-specific events tracked.
  • To test shared events you can go through some action we do in both app and POS, for example connecting the reader. Connect the reader in POS, observe the event is tracked with the prefix, disconnect, go back to app's order creation, reconnect the reader, and observe how the event is not decorated anymore.

Eg:

  • IPP
🔵 Tracked card_reader_connection_success, properties: [country: US, plan: , battery_level: 0.85, connection_type: user_initiated, was_ecommerce_trial: false, is_wpcom_store: false, card_reader_model: CHIPPER_2X, store_id: c5bd46cc-1804-4f7b-badb-bb98c449127f, plugin_slug: woocommerce-payments, blog_id: -1, site_url: https://indiemelon.mystagingwebsite.com]
  • POS:
🔵 Tracked pos_card_reader_connection_success, properties: [card_reader_model: CHIPPER_2X, plugin_slug: woocommerce-payments, country: US, connection_type: user_initiated, plan: , is_wpcom_store: false, store_id: c5bd46cc-1804-4f7b-badb-bb98c449127f, was_ecommerce_trial: false, blog_id: -1, battery_level: 0.85, site_url: https://indiemelon.mystagingwebsite.com]

  • I have considered if this change warrants user-facing release notes and have added them to 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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

@dangermattic
Copy link
Collaborator

dangermattic commented Feb 19, 2025

1 Warning
⚠️ This PR is assigned to the milestone 21.8. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Feb 19, 2025

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr15202-f2059fa
Version21.7
Bundle IDcom.automattic.alpha.woocommerce
Commitf2059fa
App Center BuildWooCommerce - Prototype Builds #13041
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@iamgabrielma iamgabrielma added type: task An internally driven task. category: tracks Related to analytics, including Tracks Events. feature: POS labels Feb 19, 2025
@iamgabrielma iamgabrielma added this to the 21.8 milestone Feb 19, 2025
@iamgabrielma iamgabrielma marked this pull request as ready for review February 19, 2025 12:52
@iamgabrielma iamgabrielma requested a review from staskus February 19, 2025 12:53
@iamgabrielma
Copy link
Contributor Author

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

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

@iamgabrielma iamgabrielma marked this pull request as draft February 19, 2025 13:48
@iamgabrielma
Copy link
Contributor Author

I found out is not tracking order_creation_success, switching it back to draft to test further

@iamgabrielma iamgabrielma marked this pull request as ready for review February 19, 2025 14:54
@iamgabrielma
Copy link
Contributor Author

@staskus this is ready for review when you have the chance, added the missed events on the last commit 🙇

Copy link
Contributor

@staskus staskus left a 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,
Copy link
Contributor

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?

Copy link
Contributor Author

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Get support will be with pos and support_new_request_viewed without POS. I think this is fine, since this functionality lives outside POS but I'm point it out.

image

Copy link
Contributor Author

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 👍

@iamgabrielma iamgabrielma merged commit d244894 into trunk Feb 20, 2025
14 checks passed
@iamgabrielma iamgabrielma deleted the task/15058-pos-events-allowed-list branch February 20, 2025 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tracks Related to analytics, including Tracks Events. feature: POS type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Woo POS] MVP Analytics: Make allowedList
4 participants