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 3 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
1 change: 1 addition & 0 deletions BitwardenShared/Core/Auth/Services/AuthService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
try await stateService.setAccountSetupImportLogins(.incomplete)
}
try await stateService.setAccountSetupVaultUnlock(.incomplete)
await stateService.setLearnNewLoginActionCardStatus(.eligible)
Copy link
Collaborator

Choose a reason for hiding this comment

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

โ“ Should this action card appear only appear for the first new account created on a device or any new account created? If I've already seen/completed the walkthrough and I add a new account, should this overwrite the existing status?

Copy link
Member

@fedemkr fedemkr Jan 7, 2025

Choose a reason for hiding this comment

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

Additionally, this is only taking into consideration login in with master password. The user could login with other means like SSO, TDE or Device.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we only want to set this when creating a new account as opposed to logging into an existing account, which is why it was done here. I suppose you could use SSO + JIT to create a new account though? Do we know in the app though if signing into SSO created a new account or does that happen seamlessly in the backend?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

โ“ Should this action card appear only appear for the first new account created on a device or any new account created? If I've already seen/completed the walkthrough and I add a new account, should this overwrite the existing status?

I think it should not overwrite the existing status.

Copy link
Member

@fedemkr fedemkr Jan 7, 2025

Choose a reason for hiding this comment

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

๐Ÿค” From what I can see on the Jira ticket:

Given I am a first-time user creating a new login item

I understand it as either as I've just created an account on mobile, or I've created it elsewhere and I'm logging in the mobile app and want to start adding logins. Could we ask someone to confirm the scope of this?

} catch {
errorReporter.log(error: error)
}
Expand Down
1 change: 1 addition & 0 deletions BitwardenShared/Core/Auth/Services/AuthServiceTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,7 @@ class AuthServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body_
XCTAssertEqual(stateService.accountSetupAutofill["13512467-9cfe-43b0-969f-07534084764b"], .incomplete)
XCTAssertEqual(stateService.accountSetupImportLogins["13512467-9cfe-43b0-969f-07534084764b"], .incomplete)
XCTAssertEqual(stateService.accountSetupVaultUnlock["13512467-9cfe-43b0-969f-07534084764b"], .incomplete)
XCTAssertEqual(stateService.learnNewLoginActionCardStatus, .eligible)
XCTAssertEqual(
stateService.masterPasswordHashes,
["13512467-9cfe-43b0-969f-07534084764b": "hashed password"]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// MARK: - LearnNewLoginActionCardStatus

/// An enum to represent the status of the learn new login action card.
///
enum LearnNewLoginActionCardStatus: Int, Codable {
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
/// The user is new user and eligible to see the card.
case eligible

/// The user has interacted with the card and it is now dismissed.
case completed
}
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 -> LearnNewLoginActionCardStatus?

/// 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: LearnNewLoginActionCardStatus) 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 -> LearnNewLoginActionCardStatus? {
appSettingsStore.learnNewLoginActionCardStatus
}

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

func setLearnNewLoginActionCardStatus(_ status: LearnNewLoginActionCardStatus) 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_getLearnNewLoginActionCardShown() async {
var learnNewLoginActionCardStatus = await subject.getLearnNewLoginActionCardStatus()
XCTAssertNil(learnNewLoginActionCardStatus)

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

/// `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(.eligible)
XCTAssertEqual(appSettingsStore.learnNewLoginActionCardStatus, .eligible)

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

/// `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: LearnNewLoginActionCardStatus? { 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: LearnNewLoginActionCardStatus? {
get { fetch(for: .learnNewLoginActionCardStatus) }
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 `nil` if there isn't a previously stored value.
func test_learnNewLoginActionCardStatus_isInitiallyNil() {
XCTAssertNil(subject.learnNewLoginActionCardStatus)
}

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

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

/// `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: BitwardenShared.LearnNewLoginActionCardStatus?
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: LearnNewLoginActionCardStatus?
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 -> BitwardenShared.LearnNewLoginActionCardStatus? {
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: BitwardenShared.LearnNewLoginActionCardStatus) 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$@\".";
"LearnLogin" = "Learn about new logins";
"LearnLoginDescription" = "Weโ€™ll walk you through the key features to add a new login.";
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
"LearnOrg" = "Learn about organizations";
"CannotOpenApp" = "Cannot open the app \"%1$@\".";
"AuthenticatorAppTitle" = "Authenticator app";
Expand Down
7 changes: 6 additions & 1 deletion BitwardenShared/UI/Vault/Vault/VaultCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,17 @@ final class VaultCoordinator: Coordinator, HasStackNavigator {
case let .addItem(allowTypeSelection, group, newCipherOptions):
Task {
let hasPremium = try? await services.vaultRepository.doesActiveAccountHavePremium()
let learnNewLoginActionCardStatus = await services.stateService.getLearnNewLoginActionCardStatus()
let accountCount = try? await services.stateService.getAccounts().count
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
let isVaultEmpty = try? await services.vaultRepository.isVaultEmpty()
let shouldShowLearnNewLoginActionCard = learnNewLoginActionCardStatus == .eligible && isVaultEmpty == true && accountCount == 1
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
showVaultItem(
route: .addItem(
allowTypeSelection: allowTypeSelection,
group: group,
hasPremium: hasPremium ?? false,
newCipherOptions: newCipherOptions
newCipherOptions: newCipherOptions,
shouldShowLearnNewLoginActionCard: shouldShowLearnNewLoginActionCard
),
delegate: context as? CipherItemOperationDelegate
)
Expand Down
40 changes: 38 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,37 @@ 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,
shouldShowLearnNewLoginActionCard: false
)
)
}

/// `navigate(to:)` with `.addItem` presents the add item view onto the stack navigator.
@MainActor
func test_navigateTo_addItem_showLearnNewLoginActionCard() throws {
stateService.accounts = [Account.fixtureAccountLogin()]
stateService.learnNewLoginActionCardStatus = .eligible
vaultRepository.isVaultEmptyResult = .success(true)
let coordinator = MockCoordinator<VaultItemRoute, VaultItemEvent>()
module.vaultItemCoordinator = coordinator
subject.navigate(to: .addItem())

waitFor(!stackNavigator.actions.isEmpty)

let action = try XCTUnwrap(stackNavigator.actions.last)
XCTAssertEqual(action.type, .presented)
XCTAssertTrue(module.vaultItemCoordinator.isStarted)
XCTAssertEqual(
module.vaultItemCoordinator.routes.last,
.addItem(
hasPremium: true,
shouldShowLearnNewLoginActionCard: true
)
)
}

/// `.navigate(to:)` with `.editItem` presents the edit item screen.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ enum AddEditItemAction: Equatable, Sendable {
/// The remove passkey button was pressed.
case removePasskeyPressed

/// Show the learn new login guided tour.
case showLearnNewLoginGuidedTour

/// The username field was changed.
case usernameChanged(String)
}
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 Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
self.coordinator = coordinator
self.delegate = delegate
self.services = services

super.init(state: state)

if !state.configuration.isAdding {
Expand All @@ -134,6 +133,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.showLearnNewLoginActionCard = false
await services.stateService.setLearnNewLoginActionCardStatus(.completed)
case .fetchCipherOptions:
await fetchCipherOptions()
case .savePressed:
Expand Down Expand Up @@ -210,6 +212,12 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
)
}
}
case .showLearnNewLoginGuidedTour:
// TODO: PM-16154
state.showLearnNewLoginActionCard = false
Task {
await services.stateService.setLearnNewLoginActionCardStatus(.completed)
}
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
case let .sshKeyItemAction(sshKeyAction):
handleSSHKeyAction(sshKeyAction)
case let .toastShown(newValue):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,15 @@ class AddEditItemProcessorTests: BitwardenTestCase {
XCTAssertEqual(alert.alertActions[3].title, Localizations.cancel)
}

/// `receive(_:)` with `.showLearnNewLoginGuidedTour` sets `showLearnNewLoginActionCard` to `false`.
@MainActor
func test_receive_showLearnNewLoginGuidedTour() {
subject.state.showLearnNewLoginActionCard = true
subject.receive(.showLearnNewLoginGuidedTour)
XCTAssertFalse(subject.state.showLearnNewLoginActionCard)
waitFor(stateService.learnNewLoginActionCardStatus == .completed)
}

/// `receive(_:)` with `.customField(.removeCustomFieldPressed(index:))` will remove
/// the custom field from given index.
@MainActor
Expand Down Expand Up @@ -798,6 +807,16 @@ class AddEditItemProcessorTests: BitwardenTestCase {
XCTAssertTrue(delegate.itemSoftDeletedCalled)
}

/// `perform(_:)` with `.dismissNewLoginActionCard` will set `.showLearnNewLoginActionCard` to false
/// and updates `.learnNewLoginActionCardShown` via stateService.
@MainActor
func test_perform_dismissNewLoginActionCard() async {
subject.state.showLearnNewLoginActionCard = true
await subject.perform(.dismissNewLoginActionCard)
XCTAssertFalse(subject.state.showLearnNewLoginActionCard)
XCTAssertEqual(stateService.learnNewLoginActionCardStatus, .completed)
}

/// `perform(_:)` with `.fetchCipherOptions` fetches the ownership options for a cipher from the repository.
@MainActor
func test_perform_fetchCipherOptions() async {
Expand Down
Loading
Loading