From f04283b0a98a5571e0dc846ef4106c4cdc27e373 Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Tue, 21 May 2024 14:23:58 +0200 Subject: [PATCH 1/4] New save / update password prompt pixels for alignment with iOS --- .../View/SaveCredentialsViewController.swift | 103 ++++++++++++++++++ DuckDuckGo/Statistics/GeneralPixel.swift | 44 ++++++++ 2 files changed, 147 insertions(+) diff --git a/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift b/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift index 02a5595240..8c7566c511 100644 --- a/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift +++ b/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift @@ -63,6 +63,12 @@ final class SaveCredentialsViewController: NSViewController { @IBOutlet var fireproofCheck: NSButton! @IBOutlet weak var fireproofCheckDescription: NSTextFieldCell! + private enum Action { + case displayed + case confirmed + case dismissed + } + weak var delegate: SaveCredentialsDelegate? private var credentials: SecureVaultModels.WebsiteCredentials? @@ -139,6 +145,9 @@ final class SaveCredentialsViewController: NSViewController { // Only use the non-editable state if a credential was automatically saved and it didn't already exist. let condition = credentials.account.id != nil && !(credentials.account.username?.isEmpty ?? true) && automaticallySaved updateViewState(editable: !condition) + + let existingCredentials = getExistingCredentialsFrom(credentials) + firePixels(for: .displayed, credentials: existingCredentials) } private func updateViewState(editable: Bool) { @@ -208,6 +217,7 @@ final class SaveCredentialsViewController: NSViewController { domain: domainLabel.stringValue) account.id = credentials?.account.id let credentials = SecureVaultModels.WebsiteCredentials(account: account, password: passwordData) + var existingCredentials = getExistingCredentialsFrom(credentials) do { if passwordManagerCoordinator.isEnabled { @@ -231,6 +241,8 @@ final class SaveCredentialsViewController: NSViewController { PixelKit.fire(DebugEvent(GeneralPixel.secureVaultError(error: error))) } + firePixels(for: .confirmed, credentials: existingCredentials) + PixelKit.fire(GeneralPixel.autofillItemSaved(kind: .password)) if passwordManagerCoordinator.isEnabled { @@ -250,6 +262,9 @@ final class SaveCredentialsViewController: NSViewController { @IBAction func onDontUpdateClicked(_ sender: Any) { delegate?.shouldCloseSaveCredentialsViewController(self) + + let existingCredentials = getExistingCredentialsFrom(credentials) + firePixels(for: .dismissed, credentials: existingCredentials) } @IBAction func onNotNowSegmentedControlClicked(_ sender: Any) { @@ -306,6 +321,8 @@ final class SaveCredentialsViewController: NSViewController { notifyDelegate() } + let existingCredentials = getExistingCredentialsFrom(credentials) + firePixels(for: .dismissed, credentials: existingCredentials) } @objc func onNeverPromptClicked() { @@ -365,4 +382,90 @@ final class SaveCredentialsViewController: NSViewController { } } + private func getExistingCredentialsFrom(_ credentials: SecureVaultModels.WebsiteCredentials?) -> SecureVaultModels.WebsiteCredentials? { + guard let credentials = credentials, let id = credentials.account.id else { + return nil + } + + var existingCredentials: SecureVaultModels.WebsiteCredentials? + + if passwordManagerCoordinator.isEnabled { + guard !passwordManagerCoordinator.isLocked else { + os_log("Failed to access credentials: Password manager is locked") + return existingCredentials + } + + passwordManagerCoordinator.websiteCredentialsFor(accountId: id) { credentials, _ in + existingCredentials = credentials + } + } else { + if let idInt = Int64(id) { + existingCredentials = try? AutofillSecureVaultFactory.makeVault(reporter: SecureVaultReporter.shared).websiteCredentialsFor(accountId: idInt) + } + } + + return existingCredentials + } + + private func isUsernameUpdated(credentials: SecureVaultModels.WebsiteCredentials) -> Bool { + if credentials.account.username != self.usernameField.stringValue.trimmingWhitespace() { + return true + } + return false + } + + private func isPasswordUpdated(credentials: SecureVaultModels.WebsiteCredentials) -> Bool { + if credentials.password != self.passwordData { + return true + } + return false + } + + private func firePixels(for action: Action, credentials: SecureVaultModels.WebsiteCredentials?) { + switch action { + case .displayed: + if let credentials = credentials { + if isPasswordUpdated(credentials: credentials) { + PixelKit.fire(GeneralPixel.autofillLoginsUpdatePasswordInlineDisplayed) + } else { + PixelKit.fire(GeneralPixel.autofillLoginsUpdateUsernameInlineDisplayed) + } + } else { + if usernameField.stringValue.trimmingWhitespace().isEmpty { + PixelKit.fire(GeneralPixel.autofillLoginsSavePasswordInlineDisplayed) + } else { + PixelKit.fire(GeneralPixel.autofillLoginsSaveLoginInlineDisplayed) + } + } + case .confirmed, .dismissed: + if let credentials = credentials { + if isUsernameUpdated(credentials: credentials) { + firePixel(for: action, + confirmedPixel: GeneralPixel.autofillLoginsUpdateUsernameInlineConfirmed, + dismissedPixel: GeneralPixel.autofillLoginsUpdateUsernameInlineDismissed) + } + if isPasswordUpdated(credentials: credentials) { + firePixel(for: action, + confirmedPixel: GeneralPixel.autofillLoginsUpdatePasswordInlineConfirmed, + dismissedPixel: GeneralPixel.autofillLoginsUpdatePasswordInlineDismissed) + } + } else { + if usernameField.stringValue.trimmingWhitespace().isEmpty { + firePixel(for: action, + confirmedPixel: GeneralPixel.autofillLoginsSavePasswordInlineConfirmed, + dismissedPixel: GeneralPixel.autofillLoginsSavePasswordInlineDismissed) + } else { + firePixel(for: action, + confirmedPixel: GeneralPixel.autofillLoginsSaveLoginInlineConfirmed, + dismissedPixel: GeneralPixel.autofillLoginsSaveLoginInlineDismissed) + } + } + } + } + + private func firePixel(for action: Action, confirmedPixel: PixelKitEventV2, dismissedPixel: PixelKitEventV2) { + let pixel = action == .confirmed ? confirmedPixel : dismissedPixel + PixelKit.fire(pixel) + } + } diff --git a/DuckDuckGo/Statistics/GeneralPixel.swift b/DuckDuckGo/Statistics/GeneralPixel.swift index d5846e38a7..fd7e8dafa3 100644 --- a/DuckDuckGo/Statistics/GeneralPixel.swift +++ b/DuckDuckGo/Statistics/GeneralPixel.swift @@ -40,11 +40,27 @@ enum GeneralPixel: PixelKitEventV2 { case formAutofilled(kind: FormAutofillKind) case autofillItemSaved(kind: FormAutofillKind) + case autofillLoginsSaveLoginInlineDisplayed + case autofillLoginsSaveLoginInlineConfirmed + case autofillLoginsSaveLoginInlineDismissed + + case autofillLoginsSavePasswordInlineDisplayed + case autofillLoginsSavePasswordInlineConfirmed + case autofillLoginsSavePasswordInlineDismissed + case autofillLoginsSaveLoginModalExcludeSiteConfirmed case autofillLoginsSettingsResetExcludedDisplayed case autofillLoginsSettingsResetExcludedConfirmed case autofillLoginsSettingsResetExcludedDismissed + case autofillLoginsUpdatePasswordInlineDisplayed + case autofillLoginsUpdatePasswordInlineConfirmed + case autofillLoginsUpdatePasswordInlineDismissed + + case autofillLoginsUpdateUsernameInlineDisplayed + case autofillLoginsUpdateUsernameInlineConfirmed + case autofillLoginsUpdateUsernameInlineDismissed + case bitwardenPasswordAutofilled case bitwardenPasswordSaved @@ -361,6 +377,20 @@ enum GeneralPixel: PixelKitEventV2 { case .autofillItemSaved(kind: let kind): return "m_mac_save_\(kind)" + case .autofillLoginsSaveLoginInlineDisplayed: + return "m_mac_autofill_logins_save_login_inline_displayed" + case .autofillLoginsSaveLoginInlineConfirmed: + return "m_mac_autofill_logins_save_login_inline_confirmed" + case .autofillLoginsSaveLoginInlineDismissed: + return "m_mac_autofill_logins_save_login_inline_dismissed" + + case .autofillLoginsSavePasswordInlineDisplayed: + return "m_mac_autofill_logins_save_password_inline_displayed" + case .autofillLoginsSavePasswordInlineConfirmed: + return "m_mac_autofill_logins_save_password_inline_confirmed" + case .autofillLoginsSavePasswordInlineDismissed: + return "m_mac_autofill_logins_save_password_inline_dismissed" + case .autofillLoginsSaveLoginModalExcludeSiteConfirmed: return "m_mac_autofill_logins_save_login_exclude_site_confirmed" case .autofillLoginsSettingsResetExcludedDisplayed: @@ -370,6 +400,20 @@ enum GeneralPixel: PixelKitEventV2 { case .autofillLoginsSettingsResetExcludedDismissed: return "m_mac_autofill_settings_reset_excluded_dismissed" + case .autofillLoginsUpdatePasswordInlineDisplayed: + return "m_mac_autofill_logins_update_password_inline_displayed" + case .autofillLoginsUpdatePasswordInlineConfirmed: + return "m_mac_autofill_logins_update_password_inline_confirmed" + case .autofillLoginsUpdatePasswordInlineDismissed: + return "m_mac_autofill_logins_update_password_inline_dismissed" + + case .autofillLoginsUpdateUsernameInlineDisplayed: + return "m_mac_autofill_logins_update_username_inline_displayed" + case .autofillLoginsUpdateUsernameInlineConfirmed: + return "m_mac_autofill_logins_update_username_inline_confirmed" + case .autofillLoginsUpdateUsernameInlineDismissed: + return "m_mac_autofill_logins_update_username_inline_dismissed" + case .bitwardenPasswordAutofilled: return "m_mac_bitwarden_autofill_password" From a4a779026390a2ef8df9208551d091f79482d6ce Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Tue, 21 May 2024 14:33:40 +0200 Subject: [PATCH 2/4] Update when .dismissed is fired on clicking Don't Save --- .../SecureVault/View/SaveCredentialsViewController.swift | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift b/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift index 8c7566c511..99c40fc44c 100644 --- a/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift +++ b/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift @@ -295,6 +295,9 @@ final class SaveCredentialsViewController: NSViewController { delegate?.shouldCloseSaveCredentialsViewController(self) } + let existingCredentials = getExistingCredentialsFrom(credentials) + firePixels(for: .dismissed, credentials: existingCredentials) + guard DataClearingPreferences.shared.isLoginDetectionEnabled else { notifyDelegate() return @@ -321,8 +324,6 @@ final class SaveCredentialsViewController: NSViewController { notifyDelegate() } - let existingCredentials = getExistingCredentialsFrom(credentials) - firePixels(for: .dismissed, credentials: existingCredentials) } @objc func onNeverPromptClicked() { From 801d9fecdcc3471af03d269c4df52710b363ea0d Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Thu, 23 May 2024 16:12:54 +0200 Subject: [PATCH 3/4] Renamed function to be clearer --- .../View/SaveCredentialsViewController.swift | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift b/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift index 99c40fc44c..486b1b4673 100644 --- a/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift +++ b/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift @@ -147,7 +147,7 @@ final class SaveCredentialsViewController: NSViewController { updateViewState(editable: !condition) let existingCredentials = getExistingCredentialsFrom(credentials) - firePixels(for: .displayed, credentials: existingCredentials) + evaluateCredentialsAndFirePixels(for: .displayed, credentials: existingCredentials) } private func updateViewState(editable: Bool) { @@ -241,7 +241,7 @@ final class SaveCredentialsViewController: NSViewController { PixelKit.fire(DebugEvent(GeneralPixel.secureVaultError(error: error))) } - firePixels(for: .confirmed, credentials: existingCredentials) + evaluateCredentialsAndFirePixels(for: .confirmed, credentials: existingCredentials) PixelKit.fire(GeneralPixel.autofillItemSaved(kind: .password)) @@ -264,7 +264,7 @@ final class SaveCredentialsViewController: NSViewController { delegate?.shouldCloseSaveCredentialsViewController(self) let existingCredentials = getExistingCredentialsFrom(credentials) - firePixels(for: .dismissed, credentials: existingCredentials) + evaluateCredentialsAndFirePixels(for: .dismissed, credentials: existingCredentials) } @IBAction func onNotNowSegmentedControlClicked(_ sender: Any) { @@ -296,7 +296,7 @@ final class SaveCredentialsViewController: NSViewController { } let existingCredentials = getExistingCredentialsFrom(credentials) - firePixels(for: .dismissed, credentials: existingCredentials) + evaluateCredentialsAndFirePixels(for: .dismissed, credentials: existingCredentials) guard DataClearingPreferences.shared.isLoginDetectionEnabled else { notifyDelegate() @@ -422,7 +422,7 @@ final class SaveCredentialsViewController: NSViewController { return false } - private func firePixels(for action: Action, credentials: SecureVaultModels.WebsiteCredentials?) { + private func evaluateCredentialsAndFirePixels(for action: Action, credentials: SecureVaultModels.WebsiteCredentials?) { switch action { case .displayed: if let credentials = credentials { From 3cc5a70f5010758a1fb0049c8e158a03824a1755 Mon Sep 17 00:00:00 2001 From: Anya Mallon Date: Thu, 23 May 2024 16:16:01 +0200 Subject: [PATCH 4/4] Updated var to let --- DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift | 2 +- UnitTests/Tab/ViewModel/TabViewModelTests.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift b/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift index 486b1b4673..e9f565bb9b 100644 --- a/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift +++ b/DuckDuckGo/SecureVault/View/SaveCredentialsViewController.swift @@ -217,7 +217,7 @@ final class SaveCredentialsViewController: NSViewController { domain: domainLabel.stringValue) account.id = credentials?.account.id let credentials = SecureVaultModels.WebsiteCredentials(account: account, password: passwordData) - var existingCredentials = getExistingCredentialsFrom(credentials) + let existingCredentials = getExistingCredentialsFrom(credentials) do { if passwordManagerCoordinator.isEnabled { diff --git a/UnitTests/Tab/ViewModel/TabViewModelTests.swift b/UnitTests/Tab/ViewModel/TabViewModelTests.swift index c2e83d6fbb..4828527900 100644 --- a/UnitTests/Tab/ViewModel/TabViewModelTests.swift +++ b/UnitTests/Tab/ViewModel/TabViewModelTests.swift @@ -294,7 +294,7 @@ final class TabViewModelTests: XCTestCase { let filteredCases = DefaultZoomValue.allCases.filter { $0 != AccessibilityPreferences.shared.defaultPageZoom } let randomZoomLevel = filteredCases.randomElement()! AccessibilityPreferences.shared.updateZoomPerWebsite(zoomLevel: randomZoomLevel, url: hostURL) - var tab = Tab(url: url) + let tab = Tab(url: url) var tabVM = TabViewModel(tab: tab) // WHEN