Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
16 changes: 16 additions & 0 deletions BitwardenResources/Localizations/en.lproj/Localizable.strings
Original file line number Diff line number Diff line change
Expand Up @@ -1251,3 +1251,19 @@
"ClearFieldName" = "Clear %1$@";
"SelectDate" = "Select date";
"PremiumRequiredTOTPDescriptionLong" = "Authenticator key (TOTP) is a Premium feature. Your current plan does not include access to this feature.";
/* Toast shown after a login item is saved. */
"LoginSaved" = "Login saved";
/* Toast shown after a card item is saved. */
"CardSaved" = "Card saved";
/* Toast shown after an identity item is saved. */
"IdentitySaved" = "Identity saved";
/* Toast shown after a secure note item is saved. */
"SecureNoteSaved" = "Secure note saved";
/* Toast shown after an SSH key item is saved. */
"SSHKeySaved" = "SSH key saved";
/* Toast shown after a bank account item is saved. */
"BankAccountSaved" = "Bank account saved";
/* Toast shown after a driver's license item is saved. */
"LicenseSaved" = "License saved";
/* Toast shown after a passport item is saved. */
"PassportSaved" = "Passport saved";
14 changes: 14 additions & 0 deletions BitwardenShared/Core/Vault/Models/Enum/CipherType.swift
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,18 @@ extension CipherType {
[.text, .hidden, .boolean]
}
}

/// The title of the toast shown after an item of this type is saved.
var savedToastTitle: String {
switch self {
case .bankAccount: Localizations.bankAccountSaved
case .card: Localizations.cardSaved
case .driversLicense: Localizations.licenseSaved
case .identity: Localizations.identitySaved
case .login: Localizations.loginSaved
case .passport: Localizations.passportSaved
case .secureNote: Localizations.secureNoteSaved
case .sshKey: Localizations.sshKeySaved
}
}
}
12 changes: 12 additions & 0 deletions BitwardenShared/Core/Vault/Models/Enum/CipherTypeTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,16 @@ class CipherTypeTests: BitwardenTestCase {
XCTAssertEqual(CipherType.passport.rawValue, 8)
XCTAssertTrue(CipherType.allCases.contains(.passport))
}

/// `savedToastTitle` returns the correct values.
func test_savedToastTitle() {
XCTAssertEqual(CipherType.bankAccount.savedToastTitle, Localizations.bankAccountSaved)
XCTAssertEqual(CipherType.card.savedToastTitle, Localizations.cardSaved)
XCTAssertEqual(CipherType.driversLicense.savedToastTitle, Localizations.licenseSaved)
XCTAssertEqual(CipherType.identity.savedToastTitle, Localizations.identitySaved)
XCTAssertEqual(CipherType.login.savedToastTitle, Localizations.loginSaved)
XCTAssertEqual(CipherType.passport.savedToastTitle, Localizations.passportSaved)
XCTAssertEqual(CipherType.secureNote.savedToastTitle, Localizations.secureNoteSaved)
XCTAssertEqual(CipherType.sshKey.savedToastTitle, Localizations.sshKeySaved)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,10 @@ extension VaultGroupProcessor: CipherItemOperationDelegate {
displayToastAndRefresh(toastTitle: Localizations.itemDeleted)
}

func itemSaved(type: CipherType) {
displayToastAndRefresh(toastTitle: type.savedToastTitle)
}

func itemSoftDeleted() {
displayToastAndRefresh(toastTitle: Localizations.itemSoftDeleted)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,16 @@ class VaultGroupProcessorTests: BitwardenTestCase { // swiftlint:disable:this ty
waitFor(vaultRepository.fetchSyncCalled)
}

/// `itemSaved(type:)` delegate method shows the toast for the saved item's type.
@MainActor
func test_delegate_itemSaved() {
XCTAssertNil(subject.state.toast)

subject.itemSaved(type: .driversLicense)
XCTAssertEqual(subject.state.toast, Toast(title: Localizations.licenseSaved))
waitFor(vaultRepository.fetchSyncCalled)
}

/// `itemSoftDeleted()` delegate method shows the expected toast.
@MainActor
func test_delegate_itemSoftDeleted() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,10 @@ extension VaultListProcessor: CipherItemOperationDelegate {
state.toast = Toast(title: Localizations.itemDeleted)
}

func itemSaved(type: CipherType) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ QUESTION: Was the more-options "Edit" path intentionally left without a toast?

Details

The vault list and group list reach the editor by two routes. Tapping a row goes through ViewItemProcessor, which now shows the toast. But the row's ellipsis β†’ Edit goes through DefaultVaultItemMoreOptionsHelper:

// VaultItemMoreOptionsHelper.swift:230
case let .edit(cipherView):
    self.coordinator.navigate(to: .editItem(cipherView), context: self)

VaultCoordinator resolves that context with context as? CipherItemOperationDelegate (VaultCoordinator.swift:232), and DefaultVaultItemMoreOptionsHelper does not conform to that protocol, so AddEditItemProcessor.delegate is nil and itemSaved(type:) never fires.

Net effect after this change: adding an item from the vault list shows a toast, editing via row β†’ view β†’ edit shows a toast, but editing via row β†’ ellipsis β†’ Edit β†’ Save shows nothing. Before this PR all three were silent, so the inconsistency is new.

The helper already has a handleDisplayToast channel it uses for copy/archive/unarchive, so wiring the save confirmation through it is one option β€” happy to defer if this path is planned as follow-up work.

state.toast = Toast(title: type.savedToastTitle)
}

func itemSoftDeleted() {
state.toast = Toast(title: Localizations.itemSoftDeleted)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,15 @@ class VaultListProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ
XCTAssertEqual(subject.state.toast, Toast(title: Localizations.itemDeleted))
}

/// `itemSaved(type:)` delegate method shows the toast for the saved item's type.
@MainActor
func test_delegate_itemSaved() {
XCTAssertNil(subject.state.toast)

subject.itemSaved(type: .driversLicense)
XCTAssertEqual(subject.state.toast, Toast(title: Localizations.licenseSaved))
}

/// `itemSoftDeleted()` delegate method shows the expected toast.
@MainActor
func test_delegate_itemSoftDeleted() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ protocol CipherItemOperationDelegate: AnyObject {
/// Called when the cipher item has been successfully restored.
func itemRestored()

/// Called when the cipher item has been successfully saved, whether it was added or updated.
///
/// - Parameter type: The type of the cipher item that was saved.
///
func itemSaved(type: CipherType)

/// Called when the cipher item has been successfully soft deleted.
func itemSoftDeleted()

Expand All @@ -49,6 +55,8 @@ extension CipherItemOperationDelegate {

func itemRestored() {}

func itemSaved(type _: CipherType) {}

func itemSoftDeleted() {}

func itemUnarchived() {}
Expand Down Expand Up @@ -917,6 +925,7 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
try await services.vaultRepository.addCipher(state.cipher)
coordinator.hideLoadingOverlay()

delegate?.itemSaved(type: state.type)
handleDismiss(didAddItem: true)
await services.reviewPromptService.trackUserAction(.addedNewItem)
}
Expand Down Expand Up @@ -1081,6 +1090,7 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
private func updateItem(cipherView: CipherView) async throws {
try await services.vaultRepository.updateCipher(cipherView.updatedView(with: state))
coordinator.hideLoadingOverlay()
delegate?.itemSaved(type: state.type)
let shouldDismissed = delegate?.itemUpdated() ?? true
if shouldDismissed {
coordinator.navigate(to: .dismiss())
Expand Down Expand Up @@ -1217,7 +1227,7 @@ extension AddEditItemProcessor: AuthenticatorKeyCaptureDelegate {

extension AddEditItemProcessor: EditCollectionsProcessorDelegate {
func didUpdateCipher() {
state.toast = Toast(title: Localizations.itemUpdated)
state.toast = Toast(title: state.type.savedToastTitle)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -609,14 +609,16 @@ class AddEditItemProcessorTests: BitwardenTestCase {
)
}

/// `didUpdateCipher()` displays a toast after the cipher is updated.
/// `didUpdateCipher()` displays the toast for the item's type after the cipher is updated.
@MainActor
func test_didUpdateCipher() {
subject.state.type = .driversLicense

subject.didUpdateCipher()

waitFor { subject.state.toast != nil }

XCTAssertEqual(subject.state.toast, Toast(title: Localizations.itemUpdated))
XCTAssertEqual(subject.state.toast, Toast(title: Localizations.licenseSaved))
}

/// `folderAdded(_:)` sets the selected folder to the folder that was added and shows the
Expand Down Expand Up @@ -1680,6 +1682,18 @@ class AddEditItemProcessorTests: BitwardenTestCase {
XCTAssertTrue(coordinator.routes.isEmpty)
}

/// `perform(_:)` with `.savePressed` notifies the delegate of the type of the item that was
/// added, so that a confirmation toast can be shown.
@MainActor
func test_perform_savePressed_new_notifiesDelegateItemSaved() async throws {
subject.state.type = .driversLicense
subject.state.name = "Bitwarden"

await subject.perform(.savePressed)

XCTAssertEqual(delegate.itemSavedType, .driversLicense)
}

/// `perform(_:)` with `.savePressed` forwards errors to the error reporter.
@MainActor
func test_perform_savePressed_existing_error() async throws {
Expand All @@ -1700,6 +1714,21 @@ class AddEditItemProcessorTests: BitwardenTestCase {
XCTAssertTrue(reviewPromptService.userActions.isEmpty)
}

/// `perform(_:)` with `.savePressed` notifies the delegate of the type of the item that was
/// updated, so that a confirmation toast can be shown.
@MainActor
func test_perform_savePressed_existing_notifiesDelegateItemSaved() async throws {
subject.state = try XCTUnwrap(
CipherItemState(existing: .fixture(type: .identity), hasPremium: true),
).addEditState
subject.state.name = "vault item"
vaultRepository.updateCipherResult = .success(())

await subject.perform(.savePressed)

XCTAssertEqual(delegate.itemSavedType, .identity)
}

/// `perform(_:)` with `.savePressed` notifies the delegate that the item was updated and
/// doesn't dismiss the view if it returns `false`.
@MainActor
Expand Down Expand Up @@ -2411,12 +2440,14 @@ class AddEditItemProcessorTests: BitwardenTestCase {
XCTAssertFalse(subject.state.guidedTourViewState.showGuidedTour)
}

/// `receive(_:)` with `.dismiss()` navigates to the `.dismiss()` route.
/// `receive(_:)` with `.dismiss()` navigates to the `.dismiss()` route without notifying the
/// delegate that the item was saved, so that cancelling doesn't show a confirmation toast.
Comment on lines +2443 to +2444
@MainActor
func test_receive_dismiss() {
subject.receive(.dismissPressed)

XCTAssertEqual(coordinator.routes.last, .dismiss())
XCTAssertNil(delegate.itemSavedType)
}

/// `receive(_:)` with `.guidedTourViewAction(.doneTapped)` completes the guided tour.
Expand Down Expand Up @@ -3499,6 +3530,7 @@ class MockCipherItemOperationDelegate: CipherItemOperationDelegate {
var itemArchivedCalled = false
var itemDeletedCalled = false
var itemRestoredCalled = false
var itemSavedType: BitwardenShared.CipherType?
var itemSoftDeletedCalled = false
var itemUpdatedCalled = false
var itemUpdatedShouldDismiss = true
Expand All @@ -3521,6 +3553,10 @@ class MockCipherItemOperationDelegate: CipherItemOperationDelegate {
itemRestoredCalled = true
}

func itemSaved(type: BitwardenShared.CipherType) {
itemSavedType = type
}

func itemSoftDeleted() {
itemSoftDeletedCalled = true
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,10 @@ private extension ViewItemProcessor {

newState.loadingState = .data(itemState)
}

// Carry over any toast, so that a toast shown in response to saving the item isn't
// cleared out from under the user by the cipher update that the save triggers.
newState.toast = state.toast
state = newState
}
} catch {
Expand Down Expand Up @@ -775,6 +779,10 @@ extension ViewItemProcessor: CipherItemOperationDelegate {
delegate?.itemRestored()
}

func itemSaved(type: CipherType) {
state.toast = Toast(title: type.savedToastTitle)
}

func itemSoftDeleted() {
coordinator.navigate(to: .dismiss(DismissAction(action: { [delegate] in delegate?.itemSoftDeleted() })))
}
Expand All @@ -788,7 +796,8 @@ extension ViewItemProcessor: CipherItemOperationDelegate {

extension ViewItemProcessor: EditCollectionsProcessorDelegate {
func didUpdateCipher() {
state.toast = Toast(title: Localizations.itemUpdated)
let title = state.loadingState.data?.type.savedToastTitle ?? Localizations.itemUpdated
state.toast = Toast(title: title)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,24 @@ class ViewItemProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
)
}

/// `didUpdateCipher()` displays a toast after the cipher is updated.
/// `didUpdateCipher()` displays the toast for the item's type after the cipher is updated.
@MainActor
func test_didUpdateCipher() {
func test_didUpdateCipher() throws {
let cipherState = try XCTUnwrap(
CipherItemState(existing: .fixture(type: .identity), hasPremium: true),
)
subject.state.loadingState = .data(cipherState)

subject.didUpdateCipher()

waitFor { subject.state.toast != nil }

XCTAssertEqual(subject.state.toast, Toast(title: Localizations.identitySaved))
}

/// `didUpdateCipher()` falls back to the generic toast if the item hasn't loaded yet.
@MainActor
func test_didUpdateCipher_notLoaded() {
subject.didUpdateCipher()

waitFor { subject.state.toast != nil }
Expand Down Expand Up @@ -173,6 +188,15 @@ class ViewItemProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
XCTAssertTrue(delegate.itemDeletedCalled)
}

/// `itemSaved(type:)` shows the toast for the saved item's type.
@MainActor
func test_itemSaved() {
XCTAssertNil(subject.state.toast)

subject.itemSaved(type: .driversLicense)
XCTAssertEqual(subject.state.toast, Toast(title: Localizations.licenseSaved))
}

/// `itemSoftDeleted()` presents the dismiss action and calls the delegate.
@MainActor
func test_itemSoftDeleted() async throws {
Expand Down Expand Up @@ -201,6 +225,23 @@ class ViewItemProcessorTests: BitwardenTestCase { // swiftlint:disable:this type
XCTAssertTrue(delegate.itemUnarchivedCalled)
}

/// `perform(_:)` with `.appeared` keeps a toast that was already displayed, so that saving the
/// item doesn't clear its own confirmation toast when the cipher update streams in.
@MainActor
func test_perform_appeared_keepsToast() {
vaultRepository.doesActiveAccountHavePremiumResult = true
subject.state.toast = Toast(title: Localizations.licenseSaved)
vaultRepository.cipherDetailsSubject.send(.fixture(id: "id", type: .identity))

let task = Task {
await subject.perform(.appeared)
}
waitFor(subject.state.loadingState != .loading(nil))
task.cancel()

XCTAssertEqual(subject.state.toast, Toast(title: Localizations.licenseSaved))
}

/// `perform(_:)` with `.appeared` starts listening for updates with the vault repository.
@MainActor
func test_perform_appeared() { // swiftlint:disable:this function_body_length
Expand Down
Loading