[PM-27047] Segregate out a BiometricsStateService into BitwardenKit #2192
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
KatherineInCode
requested review from
a team and
matt-livefront
as code owners
|
Claude finished @KatherineInCode's task in 3m 3s —— View job Code Review Complete ✓Review completed and posted inline. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if let setBiometricAuthenticationEnabledError { | ||
| throw setBiometricAuthenticationEnabledError | ||
| } | ||
| biometricAuthenticationEnabledResult = .success(isEnabled ?? false) |
There was a problem hiding this comment.
📝 Observation: The mock implementation stores the result of setBiometricAuthenticationEnabled in biometricAuthenticationEnabledResult, which creates bidirectional coupling between the setter and getter.
While this works for simple test scenarios, it means that setting isEnabled = nil will be stored as false, which may not accurately reflect the behavior of the real implementation. Consider whether tests need to verify the actual value passed vs. the stored state separately.
This is fine for current test needs, but worth noting for future test scenarios.
There was a problem hiding this comment.
We do this elsewhere in our mocks, I believe, so I'm comfortable with this for now. In the PR where I bring the BiometricsRepository into BitwardenKit, I might do a re-look through all the tests and decide to change this then.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2192 +/- ##
==========================================
- Coverage 85.53% 84.22% -1.32%
==========================================
Files 1737 1979 +242
Lines 147232 162342 +15110
==========================================
+ Hits 125942 136734 +10792
- Misses 21290 25608 +4318 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // MARK: Biometrics | ||
|
|
||
| extension DefaultStateService { | ||
| extension DefaultStateService: BiometricsStateService { |
There was a problem hiding this comment.
🤔 Do we want to remove the getBiometricAuthenticationEnabled()/setBiometricAuthenticationEnabled() definitions from the StateService protocol now that these are covered by BiometricsStateService? Maybe we haven't done that elsewhere, but it seems like it would be better to?
There was a problem hiding this comment.
I'm comfortable with that, especially as we have the pattern here of a separate extension for the segregated protocol. I'll make the change.
|
Overall Assessment: APPROVE Reviewed interface segregation refactoring moving BiometricsStateService into BitwardenKit across 6 files. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27047
📔 Objective
Following the Interface Segregation Principle, this segregates out the Biometrics-relevant parts of
StateServiceinto aBiometricsStateService, which is then pulled intoBitwardenKit.The PM
BiometricsRepositoryhas been updated to use this; BWA has not, as in a future PR the PM one will also be pulled intoBitwardenKit, and used in BWA, which will result in some changed functionality. That is, theBiometricsRepositoryclasses in PM and BWA have diverged due to changes being made in PM but not BWA, and will eventually re-merge following the newer implementation. As updating BWA now would require building out functions inBiometricsStateServicethat will just be taken out at that time, it didn't seem the prudent thing to do.⏰ 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