PM-10276: Set up unlock: enable pin unlock #867
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #867 +/- ##
==========================================
- Coverage 88.77% 88.54% -0.24%
==========================================
Files 617 618 +1
Lines 30898 38908 +8010
==========================================
+ Hits 27430 34450 +7020
- Misses 3468 4458 +990 ☔ View full report in Codecov by Sentry. |
|
No New Or Fixed Issues Found |
| do { | ||
| try await services.authRepository.allowBioMetricUnlock(enabled) | ||
| return try await services.biometricsRepository.getBiometricUnlockStatus() | ||
| } catch { | ||
| services.errorReporter.log(error: error) | ||
| showAlert(.defaultAlert(title: Localizations.anErrorHasOccurred)) | ||
| return try? await services.biometricsRepository.getBiometricUnlockStatus() | ||
| } |
There was a problem hiding this comment.
🤔 This line return try? await services.biometricsRepository.getBiometricUnlockStatus() is the same as the above one but with try? so why not return nil directly at the end of the catch? Can getBiometricUnlockStatus() change somehow in between? If so, could a comment be added how that situation may happen?
There was a problem hiding this comment.
This is a bit of an edge case. If authRepository.allowBioMetricUnlock() throws an error, I'd still want to return the current unlock status. Returning nil will ultimately remove the biometrics toggle from the screen, since we use that to determine whether biometrics are available or not. I can add a comment.
| if enabled { | ||
| let pin = try await showEnterPinAlert(showAlert: showAlert) | ||
| guard !pin.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return false } | ||
|
|
||
| let userHasMasterPassword = try await services.stateService.getUserHasMasterPassword() | ||
| let biometricType = services.biometricsRepository.getBiometricAuthenticationType() | ||
| let requirePasswordAfterRestart = if userHasMasterPassword { | ||
| await showUnlockWithPinAlert(biometricType: biometricType, showAlert: showAlert) | ||
| } else { | ||
| false | ||
| } | ||
|
|
||
| try await services.authRepository.setPins( | ||
| pin, | ||
| requirePasswordAfterRestart: requirePasswordAfterRestart | ||
| ) | ||
| } else { | ||
| try await services.authRepository.clearPins() | ||
| } | ||
|
|
||
| return enabled |
There was a problem hiding this comment.
⛏️ Could we use guard to reduce nesting?
| if enabled { | |
| let pin = try await showEnterPinAlert(showAlert: showAlert) | |
| guard !pin.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return false } | |
| let userHasMasterPassword = try await services.stateService.getUserHasMasterPassword() | |
| let biometricType = services.biometricsRepository.getBiometricAuthenticationType() | |
| let requirePasswordAfterRestart = if userHasMasterPassword { | |
| await showUnlockWithPinAlert(biometricType: biometricType, showAlert: showAlert) | |
| } else { | |
| false | |
| } | |
| try await services.authRepository.setPins( | |
| pin, | |
| requirePasswordAfterRestart: requirePasswordAfterRestart | |
| ) | |
| } else { | |
| try await services.authRepository.clearPins() | |
| } | |
| return enabled | |
| guard enabled else { | |
| try await services.authRepository.clearPins() | |
| return false | |
| } | |
| let pin = try await showEnterPinAlert(showAlert: showAlert) | |
| guard !pin.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return false } | |
| let userHasMasterPassword = try await services.stateService.getUserHasMasterPassword() | |
| let biometricType = services.biometricsRepository.getBiometricAuthenticationType() | |
| let requirePasswordAfterRestart = if userHasMasterPassword { | |
| await showUnlockWithPinAlert(biometricType: biometricType, showAlert: showAlert) | |
| } else { | |
| false | |
| } | |
| try await services.authRepository.setPins( | |
| pin, | |
| requirePasswordAfterRestart: requirePasswordAfterRestart | |
| ) | |
| return true |
| @@ -90,7 +96,9 @@ final class AccountSecurityProcessor: StateProcessor< | |||
| case let .sessionTimeoutValueChanged(newValue): | |||
| setVaultTimeout(value: newValue) | |||
| case let .toggleUnlockWithPINCode(isOn): | |||
| toggleUnlockWithPIN(isOn) | |||
| Task { | |||
| await toggleUnlockWithPIN(isOn) | |||
There was a problem hiding this comment.
should this be an Effect?
There was a problem hiding this comment.
Yeah, I can change that.
| switch unlockMethod { | ||
| case .biometrics: | ||
| Task { | ||
| Task { |
There was a problem hiding this comment.
should this be an Effect instead?
| @@ -5,17 +5,19 @@ | |||
| class VaultUnlockSetupProcessor: StateProcessor<VaultUnlockSetupState, VaultUnlockSetupAction, VaultUnlockSetupEffect> { | |||
| // MARK: Types | |||
|
|
|||
| typealias Services = HasAuthRepository | |||
| typealias Services = DefaultVaultUnlockSetupHelper.Services | |||
| & HasBiometricsRepository | |||
There was a problem hiding this comment.
can we nuke HasBiometricsRepository since its within DefaultVaultUnlockSetupHelper.Services?
| /// A default implementation of `VaultUnlockSetupHelper` which is used to set up vault unlock methods. | ||
| /// | ||
| @MainActor | ||
| class DefaultVaultUnlockSetupHelper { |
There was a problem hiding this comment.
just curious, why not make this a service itself?
There was a problem hiding this comment.
We've used this concept of a helper elsewhere to extract logic from the processor or share logic between multiple processors. In this case, while not directly showing alerts, it does end up creating alerts that should be shown. So I feel like it belongs in the UI layer rather than the service layer.
🎟️ Tracking
PM-10276
PM-10277
📔 Objective
This enables toggling pin unlock on the setup up unlock screen as part of account setup.
📸 Screenshots
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-23.at.11.36.19.mp4
⏰ 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