PM-12565: Don't fail login if setting account setup progress fails #988
Conversation
|
No New Or Fixed Issues Found |
| func getAccountSetupAutofill(userId: String?) async throws -> AccountSetupProgress? | ||
| func getAccountSetupAutofill(userId: String) async -> AccountSetupProgress? |
There was a problem hiding this comment.
This is a slight change to the pattern that exists in StateService.
Our current pattern is that the protocol defines a throwing function which takes an optional userId. If the user ID parameter is nil, then we get the active user ID. And then we have an extension method which doesn't accept any parameters to essentially provide a default value of nil as the user ID to get the active user ID.
If we instead move the "default to ac 8000 tive user ID" logic into the extension method, we could make the protocol method require a user ID and in most cases make it not throw.
This won't affect most places where we call the extension method without a user ID. But in cases where we already have the user ID (some service/repository methods), we could avoid throwing. If we like this pattern, I can update the rest of StateService to match.
There was a problem hiding this comment.
Awesome! I really like the approach but I wouldn't perform the changes right now. We're working on a proposal to reduce most of the boilerplate in StateService and AppSettingsStore, so reducing the effort to maintain the code and tests. So I'll add this there and share the proposal with you as well to revise it if you agree.
There was a problem hiding this comment.
Sounds good, thanks!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #988 +/- ##
==========================================
+ Coverage 88.77% 88.81% +0.04%
==========================================
Files 638 641 +3
Lines 40129 40217 +88
==========================================
+ Hits 35625 35720 +95
+ Misses 4504 4497 -7 ☔ View full report in Codecov by Sentry. |
| func getAccountSetupAutofill(userId: String?) async throws -> AccountSetupProgress? | ||
| func getAccountSetupAutofill(userId: String) async -> AccountSetupProgress? |
There was a problem hiding this comment.
Awesome! I really like the approach but I wouldn't perform the changes right now. We're working on a proposal to reduce most of the boilerplate in StateService and AppSettingsStore, so reducing the effort to maintain the code and tests. So I'll add this there and share the proposal with you as well to revise it if you agree.
🎟️ Tracking
PM-12565
📔 Objective
This addresses a comment @fedemkr made here: https://github.com/bitwarden/ios/pull/972/files#r1777654377
If setting the user's account setup progress fails as part of login, we should log the error rather than throw which would cause the login to fail.
⏰ 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