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 2 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 whether the learn new login action card has been shown.
///
/// - Returns: Whether the learn new login action card has been shown.
///
func getLearnNewLoginActionCardShown() async -> Bool

/// 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 whether the learn new login action card has been shown.
///
/// - Parameter shown: Whether the learn new login action card has been shown.
///
func setLearnNewLoginActionCardShown(_ shown: Bool) 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 getLearnNewLoginActionCardShown() async -> Bool {
appSettingsStore.isLearnNewLoginActionCardShown
}

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

func setLearnNewLoginActionCardShown(_ shown: Bool) async {
appSettingsStore.isLearnNewLoginActionCardShown = shown
}

func setLastActiveTime(_ date: Date?, userId: String?) async throws {
let userId = try userId ?? getActiveAccountUserId()
appSettingsStore.setLastActiveTime(date, userId: userId)
Expand Down
18 changes: 18 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)
}

/// `getLearnNewLoginActionCardShown()` returns whether the learn new login action card has been shown.
func test_getLearnNewLoginActionCardShown() async {
var hasShownLearnNewLoginActionCard = await subject.getLearnNewLoginActionCardShown()
XCTAssertFalse(hasShownLearnNewLoginActionCard)

appSettingsStore.isLearnNewLoginActionCardShown = true
hasShownLearnNewLoginActionCard = await subject.getLearnNewLoginActionCardShown()
XCTAssertTrue(hasShownLearnNewLoginActionCard)
}

/// `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,14 @@ class StateServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body
)
}

func test_setLearnNewLoginActionCardShown() async {
await subject.setLearnNewLoginActionCardShown(true)
XCTAssertTrue(appSettingsStore.isLearnNewLoginActionCardShown)

await subject.setLearnNewLoginActionCardShown(false)
XCTAssertFalse(appSettingsStore.isLearnNewLoginActionCardShown)
}

/// `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 @@ -32,6 +32,9 @@
/// Whether the intro carousel screen has been shown.
var introCarouselShown: Bool { get set }

/// Whether the learn new login action card has been shown.
var isLearnNewLoginActionCardShown: Bool { get set }

/// The last value of the connect to watch setting, ignoring the user id. Used for
/// sending the status to the watch if the user is logged out.
var lastUserShouldConnectToWatch: Bool { get set }
Expand Down Expand Up @@ -709,6 +712,7 @@
case encryptedUserKey(userId: String)
case events(userId: String)
case introCarouselShown
case isLearnNewLoginActionCardShown
case lastActiveTime(userId: String)
case lastSync(userId: String)
case lastUserShouldConnectToWatch
Expand Down Expand Up @@ -783,6 +787,8 @@
key = "events_\(userId)"
case .introCarouselShown:
key = "introCarouselShown"
case .isLearnNewLoginActionCardShown:
key = "isLearnNewLoginActionCardShown"

Check warning on line 791 in BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift

View check run for this annotation

Codecov / codecov/patch

BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift#L791

Added line #L791 was not covered by tests
case let .lastActiveTime(userId):
key = "lastActiveTime_\(userId)"
case let .lastSync(userId):
Expand Down Expand Up @@ -876,6 +882,11 @@
set { store(newValue, for: .introCarouselShown) }
}

var isLearnNewLoginActionCardShown: Bool {
get { fetch(for: .isLearnNewLoginActionCardShown) }
set { store(newValue, for: .isLearnNewLoginActionCardShown) }

Check warning on line 887 in BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift

View check run for this annotation

Codecov / codecov/patch

BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift#L886-L887

Added lines #L886 - L887 were not covered by tests
}

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 @@ -16,6 +16,7 @@ class MockAppSettingsStore: AppSettingsStore { // swiftlint:disable:this type_bo
var cachedActiveUserId: String?
var disableWebIcons = false
var introCarouselShown = false
var isLearnNewLoginActionCardShown = false
var lastUserShouldConnectToWatch = false
var loginRequest: LoginRequestNotification?
var migrationVersion = 0
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 learnNewLoginActionCardShown = false
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 getLearnNewLoginActionCardShown() async -> Bool {
learnNewLoginActionCardShown
}

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 setLearnNewLoginActionCardShown(_ shown: Bool) async {
learnNewLoginActionCardShown = shown
}

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 isLearnNewLoginCardShown = await services.stateService.getLearnNewLoginActionCardShown()
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 = !isLearnNewLoginCardShown && isVaultEmpty == true && accountCount == 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we want to show this action card for new users only, not returning users, I came up with this checks, if you have better suggestions I am all open for it, even for moving this check to a service, tried to put this in StateService, but can't do it because of CipherService rely on StateService.

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.learnNewLoginActionCardShown = false
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.setLearnNewLoginActionCardShown(true)
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.setLearnNewLoginActionCardShown(true)
}
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.learnNewLoginActionCardShown)
}

/// `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)
XCTAssertTrue(stateService.learnNewLoginActionCardShown)
}

/// `perform(_:)` with `.fetchCipherOptions` fetches the ownership options for a cipher from the repository.
@MainActor
func test_perform_fetchCipherOptions() async {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ protocol AddEditItemState: Sendable {
/// If master password reprompt toggle should be shown.
var showMasterPasswordReprompt: Bool { get set }

/// If the Learn New Login Action Card should be shown.
var showLearnNewLoginActionCard: Bool { get set }

/// The SSH key item state.
var sshKeyState: SSHKeyItemState { get set }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,20 @@ struct AddEditItemView: View {
.accessibilityIdentifier("PersonalOwnershipPolicyLabel")
}

if case .add = store.state.configuration, store.state.type == .login,
store.state.showLearnNewLoginActionCard {
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
ActionCard(
title: Localizations.learnLogin,
message: Localizations.learnLoginDescription,
actionButtonState: ActionCard.ButtonState(title: Localizations.getStarted) {
store.send(.showLearnNewLoginGuidedTour)
},
dismissButtonState: ActionCard.ButtonState(title: Localizations.dismiss) {
await store.perform(.dismissNewLoginActionCard)
}
)
}

informationSection
miscellaneousSection
notesSection
Expand Down Expand Up @@ -343,7 +357,8 @@ struct AddEditItemView_Previews: PreviewProvider {
store: Store(
processor: StateProcessor(
state: CipherItemState(
hasPremium: true
hasPremium: true,
showLearnNewLoginActionCard: false
).addEditState
)
)
Expand Down Expand Up @@ -372,7 +387,8 @@ struct AddEditItemView_Previews: PreviewProvider {
processor: StateProcessor(
state: CipherItemState(
addItem: .card,
hasPremium: true
hasPremium: true,
showLearnNewLoginActionCard: false
)
.addEditState
)
Expand Down
Loading
Loading