Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[MBL-2110] [MBL-2113] Fixes for PPO analytics events #2289

Merged
merged 10 commits into from
Feb 19, 2025

Conversation

stevestreza-ksr
Copy link
Contributor

📲 What

Two fixes:

  1. Make sure that analytics events are only firing once; if the page reloads, they currently send all events another time.
  2. Add a missing event for the user tapping "confirm" on the confirm address popup

🤔 Why

  1. In order to support the counts properties we need, we use Combine to grab the most recent results from the Paginator. However, when the user pulls to refresh, this causes the Paginator's results publisher to fire again, triggering all analytics events a subsequent time.
  2. This event was added but never connected.

🛠 How

  1. Instead of using combineLatest, this was changed to a flatMap + first. This causes the Paginator's results method to fire immediately, and only one time. Since this is a flatMap, this process repeats every time the flatMap happens (e.g. every time the relevant button is clicked). This gets us the behavior we need.
  2. The analytics for PPO are handled in PPOViewModel, but the confirm address button happens in the PPOContainerViewModel. So to support this, I added a PPOActionState closure to the confirmAddress case, similar to 3DS, and listen to it in the PPOViewModel. This also needed a new case added to PPOActionState, confirmed, when the user has confirmed their intent but processing is still happening.

@stevestreza-ksr stevestreza-ksr changed the title [MBL-2110] Fixes for PPO analytics events [MBL-2110] [MBL-2113] Fixes for PPO analytics events Feb 13, 2025
Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter left a comment

Choose a reason for hiding this comment

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

I've got two non-blocking suggestion for making this more self-documenting - but the fix itself LGTM.

Thanks for the excellent explanation in the review notes!

@stevestreza-ksr stevestreza-ksr force-pushed the stevestreza/ppo/analytics-fixes branch from 34e9235 to 5906a7f Compare February 18, 2025 23:13
@stevestreza-ksr stevestreza-ksr merged commit 0f11a8e into main Feb 19, 2025
5 checks passed
@stevestreza-ksr stevestreza-ksr deleted the stevestreza/ppo/analytics-fixes branch February 19, 2025 01:58
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants