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

fix: LW-10549 add missing onboarding and multiwallet posthog events, improve type-safety #1160

Merged
merged 2 commits into from
May 21, 2024

Conversation

szymonmaslowski
Copy link
Contributor

Checklist

  • JIRA - <link>
  • Proper tests implemented
  • Screenshots added.

Proposed solution

Extracted onboarding and multi-wallet posthog events from the PosthogAction enum to be separate entities. Improved type safety around those events.

Testing

Onboarding and multi-wallet flows send identical posthog events (same number of events, same time of sending) - same event texts except for the prefix which is either onboarding or multiwalet.

@szymonmaslowski szymonmaslowski self-assigned this May 15, 2024
@szymonmaslowski szymonmaslowski requested a review from a team as a code owner May 15, 2024 15:35
Copy link

github-actions bot commented May 15, 2024

Allure Report

allure-report-publisher generated test report!

smokeTests: ✅ test report for 769f294a

passed failed skipped flaky total result
Total 30 0 0 0 30

Copy link
Contributor

@shawnbusuttil shawnbusuttil left a comment

Choose a reason for hiding this comment

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

Left just one comment.


export type SendOnboardingAnalyticsEvent = (
postHogAction: PostHogAction,
postHogAction: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldnt this lower the safety?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is dead. It is old onboarding and I have it removed on a different branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More context: as you can see this PR is based on the codebase unification PR where on ording implementation was swapped with the adjusted multi wallet implementation and all the old code resides there without being used including this file.

Copy link
Contributor

@greatertomi greatertomi left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@szymonmaslowski szymonmaslowski force-pushed the feat/LW-10194-unify-multiwallet-and-onboarding-codebases branch from d2009df to bc54849 Compare May 20, 2024 12:58
@szymonmaslowski szymonmaslowski force-pushed the feat/LW-10549-add-wallet-added-events branch from e532b7d to 92fe71e Compare May 20, 2024 13:27
Base automatically changed from feat/LW-10194-unify-multiwallet-and-onboarding-codebases to main May 20, 2024 17:20
@szymonmaslowski szymonmaslowski force-pushed the feat/LW-10549-add-wallet-added-events branch from 92fe71e to 23b3f3f Compare May 21, 2024 14:21
Copy link

@szymonmaslowski szymonmaslowski enabled auto-merge (squash) May 21, 2024 15:11
@szymonmaslowski szymonmaslowski merged commit ed79fcb into main May 21, 2024
9 of 11 checks passed
@szymonmaslowski szymonmaslowski deleted the feat/LW-10549-add-wallet-added-events branch May 21, 2024 15:36
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