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

PM-16153: Draw new login action card #1238

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions BitwardenShared/Core/Platform/Services/StateService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,12 @@
///
func getShouldTrustDevice(userId: String) async -> Bool?

/// Gets the status of Learn New Login Action Card.
///
/// - Returns: The status of Learn New Login Action Card.
///
func getLearnNewLoginActionCardStatus() async -> AccountSetupProgress?

/// Get whether to show the website icons.
///
/// - Returns: Whether to show the website icons.
Expand Down Expand Up @@ -513,6 +519,12 @@
///
func setIntroCarouselShown(_ shown: Bool) async

/// Sets the status of Learn New Login Action Card.
///
/// - Parameter status: The status of Learn New Login Action Card.
///
func setLearnNewLoginActionCardStatus(_ status: AccountSetupProgress) async

/// Sets the last active time within the app.
///
/// - Parameters:
Expand Down Expand Up @@ -1334,7 +1346,7 @@
showWebIconsSubject = CurrentValueSubject(!appSettingsStore.disableWebIcons)

Task {
for await activeUserId in self.appSettingsStore.activeAccountIdPublisher().values {

Check warning on line 1349 in BitwardenShared/Core/Platform/Services/StateService.swift

View workflow job for this annotation

GitHub Actions / Test

expression is 'async' but is not marked with 'await'; this is an error in the Swift 6 language mode
errorReporter.setUserId(activeUserId)
}
}
Expand Down Expand Up @@ -1560,6 +1572,10 @@
appSettingsStore.shouldTrustDevice(userId: userId)
}

func getLearnNewLoginActionCardStatus() async -> AccountSetupProgress? {
appSettingsStore.learnNewLoginActionCardStatus
}

func getShowWebIcons() async -> Bool {
!appSettingsStore.disableWebIcons
}
Expand Down Expand Up @@ -1769,6 +1785,10 @@
appSettingsStore.introCarouselShown = shown
}

func setLearnNewLoginActionCardStatus(_ status: AccountSetupProgress) async {
appSettingsStore.learnNewLoginActionCardStatus = status
}

func setLastActiveTime(_ date: Date?, userId: String?) async throws {
let userId = try userId ?? getActiveAccountUserId()
appSettingsStore.setLastActiveTime(date, userId: userId)
Expand Down
19 changes: 19 additions & 0 deletions BitwardenShared/Core/Platform/Services/StateServiceTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,16 @@ class StateServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body
XCTAssertTrue(hasShownCarousel)
}

/// `getLearnNewLoginActionCardStatus()` returns the status of the learn new login action card.
func test_getLearnNewLoginActionCardStatus() async {
var learnNewLoginActionCardStatus = await subject.getLearnNewLoginActionCardStatus()
XCTAssertEqual(learnNewLoginActionCardStatus, .incomplete)

appSettingsStore.learnNewLoginActionCardStatus = .complete
learnNewLoginActionCardStatus = await subject.getLearnNewLoginActionCardStatus()
XCTAssertEqual(learnNewLoginActionCardStatus, .complete)
}

/// `getLastActiveTime(userId:)` gets the user's last active time.
func test_getLastActiveTime() async throws {
await subject.addAccount(.fixture(profile: .fixture(userId: "1")))
Expand Down Expand Up @@ -1762,6 +1772,15 @@ class StateServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body
)
}

/// `setLearnNewLoginActionCardStatus(_:)` sets the learn new login action card status.
func test_setLearnNewLoginActionCardStatus() async {
await subject.setLearnNewLoginActionCardStatus(.incomplete)
XCTAssertEqual(appSettingsStore.learnNewLoginActionCardStatus, .incomplete)

await subject.setLearnNewLoginActionCardStatus(.complete)
XCTAssertEqual(appSettingsStore.learnNewLoginActionCardStatus, .complete)
}

/// `setLoginRequest()` sets the pending login requests.
func test_setLoginRequest() async {
let loginRequest = LoginRequestNotification(id: "1", userId: "10")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ protocol AppSettingsStore: AnyObject {
/// sending the status to the watch if the user is logged out.
var lastUserShouldConnectToWatch: Bool { get set }

/// The status of the learn new login action card.
var learnNewLoginActionCardStatus: AccountSetupProgress { get set }

/// The login request information received from a push notification.
var loginRequest: LoginRequestNotification? { get set }

Expand Down Expand Up @@ -709,6 +712,7 @@ extension DefaultAppSettingsStore: AppSettingsStore {
case encryptedUserKey(userId: String)
case events(userId: String)
case introCarouselShown
case learnNewLoginActionCardStatus
case lastActiveTime(userId: String)
case lastSync(userId: String)
case lastUserShouldConnectToWatch
Expand Down Expand Up @@ -783,6 +787,8 @@ extension DefaultAppSettingsStore: AppSettingsStore {
key = "events_\(userId)"
case .introCarouselShown:
key = "introCarouselShown"
case .learnNewLoginActionCardStatus:
key = "learnNewLoginActionCardStatus"
case let .lastActiveTime(userId):
key = "lastActiveTime_\(userId)"
case let .lastSync(userId):
Expand Down Expand Up @@ -876,6 +882,11 @@ extension DefaultAppSettingsStore: AppSettingsStore {
set { store(newValue, for: .introCarouselShown) }
}

var learnNewLoginActionCardStatus: AccountSetupProgress {
get { fetch(for: .learnNewLoginActionCardStatus) ?? .incomplete }
set { store(newValue, for: .learnNewLoginActionCardStatus) }
}

var lastUserShouldConnectToWatch: Bool {
get { fetch(for: .lastUserShouldConnectToWatch) }
set { store(newValue, for: .lastUserShouldConnectToWatch) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,29 @@ class AppSettingsStoreTests: BitwardenTestCase { // swiftlint:disable:this type_
XCTAssertFalse(subject.isBiometricAuthenticationEnabled(userId: "1"))
}

/// `learnNewLoginActionCardStatus` returns `.incomplete` if there isn't a previously stored value.
func test_learnNewLoginActionCardStatus_isInitiallyIncomplete() {
XCTAssertEqual(subject.learnNewLoginActionCardStatus, .incomplete)
}

/// `learnNewLoginActionCardStatus` can be used to get and set the persisted value in user defaults.
func test_learnNewLoginActionCardStatus_withValues() {
subject.learnNewLoginActionCardStatus = .complete
XCTAssertEqual(subject.learnNewLoginActionCardStatus, .complete)

try XCTAssertEqual(
JSONDecoder().decode(
AccountSetupProgress.self,
from: XCTUnwrap(
userDefaults
.string(forKey: "bwPreferencesStorage:learnNewLoginActionCardStatus")?
.data(using: .utf8)
)
),
AccountSetupProgress.complete
)
}

/// `lastUserShouldConnectToWatch` returns `false` if there isn't a previously stored value.
func test_lastUserShouldConnectToWatch_isInitiallyFalse() {
XCTAssertFalse(subject.lastUserShouldConnectToWatch)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class MockAppSettingsStore: AppSettingsStore { // swiftlint:disable:this type_bo
var disableWebIcons = false
var introCarouselShown = false
var lastUserShouldConnectToWatch = false
var learnNewLoginActionCardStatus: AccountSetupProgress = .incomplete
var loginRequest: LoginRequestNotification?
var migrationVersion = 0
var overrideDebugFeatureFlagCalled = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class MockStateService: StateService { // swiftlint:disable:this type_body_lengt
var isAuthenticated = [String: Bool]()
var isAuthenticatedError: Error?
var lastActiveTime = [String: Date]()
var learnNewLoginActionCardStatus: AccountSetupProgress?
var loginRequest: LoginRequestNotification?
var logoutAccountUserInitiated = false
var getAccountEncryptionKeysError: Error?
Expand Down Expand Up @@ -250,6 +251,10 @@ class MockStateService: StateService { // swiftlint:disable:this type_body_lengt
introCarouselShown
}

func getLearnNewLoginActionCardStatus() async -> AccountSetupProgress? {
learnNewLoginActionCardStatus
}

func getLastActiveTime(userId: String?) async throws -> Date? {
let userId = try unwrapUserId(userId)
return lastActiveTime[userId]
Expand Down Expand Up @@ -505,6 +510,10 @@ class MockStateService: StateService { // swiftlint:disable:this type_body_lengt
isAuthenticated[account.profile.userId] = true
}

func setLearnNewLoginActionCardStatus(_ status: AccountSetupProgress) async {
learnNewLoginActionCardStatus = status
}

func setLastActiveTime(_ date: Date?, userId: String?) async throws {
let userId = try unwrapUserId(userId)
lastActiveTime[userId] = timeProvider.presentTime
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,8 @@
"PossibleMatchingItems" = "Possible matching items";
"Search" = "Search";
"BitwardenAutofillServiceSearch" = "You are searching for an autofill item for \"%1$@\".";
"LearnAboutNewLogins" = "Learn about new logins";
"WeWillWalkYouThroughTheKeyFeaturesToAddANewLogin" = "Weโ€™ll walk you through the key features to add a new login.";
"LearnOrg" = "Learn about organizations";
"CannotOpenApp" = "Cannot open the app \"%1$@\".";
"AuthenticatorAppTitle" = "Authenticator app";
Expand Down Expand Up @@ -1055,6 +1057,7 @@
"SomethingWentWrong" = "Something went wrong";
"UnableToMoveTheSelectedItemPleaseTryAgain" = "Unable to move the selected item. Please try again.";
"Done" = "Done";
"Next" = "Next";
"CopyPublicKey" = "Copy public key";
"CopyPrivateKey" = "Copy private key";
"CopyFingerprint" = "Copy fingerprint";
Expand Down
15 changes: 13 additions & 2 deletions BitwardenShared/UI/Vault/Vault/VaultCoordinatorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class VaultCoordinatorTests: BitwardenTestCase {
var errorReporter: MockErrorReporter!
var module: MockAppModule!
var stackNavigator: MockStackNavigator!
var stateService: MockStateService!
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
var subject: VaultCoordinator!
var vaultRepository: MockVaultRepository!

Expand All @@ -23,13 +24,18 @@ class VaultCoordinatorTests: BitwardenTestCase {
errorReporter = MockErrorReporter()
delegate = MockVaultCoordinatorDelegate()
module = MockAppModule()
stateService = MockStateService()
stackNavigator = MockStackNavigator()
vaultRepository = MockVaultRepository()
subject = VaultCoordinator(
appExtensionDelegate: MockAppExtensionDelegate(),
delegate: delegate,
module: module,
services: ServiceContainer.withMocks(errorReporter: errorReporter, vaultRepository: vaultRepository),
services: ServiceContainer.withMocks(
errorReporter: errorReporter,
stateService: stateService,
vaultRepository: vaultRepository
),
stackNavigator: stackNavigator
)
}
Expand Down Expand Up @@ -103,7 +109,12 @@ class VaultCoordinatorTests: BitwardenTestCase {
let action = try XCTUnwrap(stackNavigator.actions.last)
XCTAssertEqual(action.type, .presented)
XCTAssertTrue(module.vaultItemCoordinator.isStarted)
XCTAssertEqual(module.vaultItemCoordinator.routes.last, .addItem(hasPremium: true))
XCTAssertEqual(
module.vaultItemCoordinator.routes.last,
.addItem(
hasPremium: true
)
)
}

/// `.navigate(to:)` with `.editItem` presents the edit item screen.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ enum AddEditItemEffect {
/// The delete option was pressed.
case deletePressed

/// The user tapped the dismiss button on the new login action card.
case dismissNewLoginActionCard

/// Any options that need to be loaded for a cipher (e.g. organizations and folders) should be fetched.
case fetchCipherOptions

Expand All @@ -24,4 +27,7 @@ enum AddEditItemEffect {

/// The setup totp button was pressed.
case setupTotpPressed

/// Show the learn new login guided tour.
case showLearnNewLoginGuidedTour
}
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
typealias Services = HasAPIService
& HasAuthRepository
& HasCameraService
& HasConfigService
& HasErrorReporter
& HasEventService
& HasFido2UserInterfaceHelper
Expand Down Expand Up @@ -111,11 +112,15 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
self.coordinator = coordinator
self.delegate = delegate
self.services = services

super.init(state: state)
Task {
if await services.configService.getFeatureFlag(.nativeCreateAccountFlow),
appExtensionDelegate == nil {
self.state.isLearnNewLoginActionCardEligible = await services.stateService
.getLearnNewLoginActionCardStatus() == .incomplete
}
Copy link
Member

Choose a reason for hiding this comment

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

๐Ÿค” Shouldn't this be done in perform -> appeared? Additionally the if !state.configuration.isAdding is not meant to be done inside the Task. Can't say for sure that it will match the same behavior as before so I wouldn't change that part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if we do it in appeared, it would slide in the action card after the view has appeared.
as far as configuration.isAdding I don't see why it will not match the same behavior, the original code was like this:

if !state.configuration.isAdding {
     Task {
           await self.services.rehydrationHelper.addRehydratableTarget(self)
      }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tested doing it on appear? I think that's what we're doing on other views that show the action card and I don't recall it being an issue.


if !state.configuration.isAdding {
Task {
if !state.configuration.isAdding {
await self.services.rehydrationHelper.addRehydratableTarget(self)
}
}
Expand All @@ -134,6 +139,9 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
guard let key = state.loginState.authenticatorKey else { return }
services.pasteboardService.copy(key)
state.toast = Toast(title: Localizations.valueHasBeenCopied(Localizations.authenticatorKeyScanner))
case .dismissNewLoginActionCard:
state.isLearnNewLoginActionCardEligible = false
await services.stateService.setLearnNewLoginActionCardStatus(.complete)
case .fetchCipherOptions:
await fetchCipherOptions()
case .savePressed:
Expand All @@ -142,6 +150,10 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
await setupTotp()
case .deletePressed:
await showSoftDeleteConfirmation()
case .showLearnNewLoginGuidedTour:
// TODO: PM-16154
state.isLearnNewLoginActionCardEligible = false
await services.stateService.setLearnNewLoginActionCardStatus(.complete)
}
}

Expand Down
Loading
Loading