-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-28479] Clear premium interest on upgrade dialog open #17518
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
base: main
Are you sure you want to change the base?
[PM-28479] Clear premium interest on upgrade dialog open #17518
Conversation
|
Claude finished @amorask-bitwarden's task —— View job Code Review CompleteI've reviewed the changes in this PR and everything looks excellent. The refactoring successfully addresses the issue from the previous implementation. SummaryThe PR simplifies premium interest state management by moving the clearing logic from multiple dialog exit points (close, back, complete) into a single location:
This is a cleaner approach that eliminates the need to handle state clearing at every possible dialog exit point. Code QualityStrengths:
Implementation Details:
Test Coverage:
FindingsNo issues found. The code is well-architected, properly tested, and follows Bitwarden's coding standards. |
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17518 +/- ##
=======================================
Coverage 41.25% 41.26%
=======================================
Files 3546 3546
Lines 102040 102041 +1
Branches 15308 15309 +1
=======================================
+ Hits 42099 42103 +4
+ Misses 58177 58173 -4
- Partials 1764 1765 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks @amorask-bitwarden, here are the tests I ran through - everything seems to work just fine. The only scenario I couldn't test fully is the successfully payment scenario (the test card wouldn't work). But it really should work similarly to the rest since you are clearing the state on dialog open. Click Outsideclick-outside-dialog.movClick Xclick-x.movClick "Back"click-back.movPress ESCAPEpress-escape.mov |


🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28479
📔 Objective
The previous iteration of this change was not clearing the state for premium interest when a user clicked outside of the upgrade dialog.
@rr-bw made the excellent point that we can just clear the premium state when the dialog opens and track premium interest within dialog-scope as the state has served it's purpose. This PR accomplishes that and updates the associated test file.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes