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.

Sign up for GitHub

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

Jump to bottom

Add pixels for importing google passwords metrics #5284

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Add pixels for importing google passwords metrics #5284

merged 1 commit into from
Nov 21, 2024

Conversation

CDRussell
Copy link
Member

@CDRussell CDRussell commented Nov 18, 2024 •
edited by cmonfortep
Loading

Task/Issue URL: https://app.asana.com/0/608920331025315/1208787586139232/f

Description

Adds metrics around the import password flow. To support this in a flexible way, it also allows for the url mappings to be defined remotely for which stage of the flow a user dropped out at if they didn't go all the way through; if we ever need to, can override the default url mappings.

Logcat filter: message~:"Pixel sent: autofill_import_google_passwords"

Steps to test this PR

Setup

  • Fresh install

Cancelling the user journey

  • Visit Passwords screen, and verify autofill_import_google_passwords_import_button_shown in logs for the import password button being shown
  • Tap on Import Passwords From Google button; verifyautofill_import_google_passwords_import_button_tapped in logs
  • Verify autofill_import_google_passwords_preimport_prompt_displayed in logs
  • Dismiss the import dialog; verifyautofill_import_google_passwords_result_user_cancelled in logs
  • Tap on Import Passwords From Google button again.
  • This time, tap on the Open Google Passwords button; verifyautofill_import_google_passwords_preimport_prompt_confirmed in logs
  • Tap the ✖️ button to quit the import flow; verifyautofill_import_google_passwords_result_user_cancelled in logs, with stage=webflow-pre-login
  • Dismiss the dialog that is still showing, and verify there is not a further cancellation pixel in logs for when the dialog closes, as this is already covered by the ✖️ button cancellation
  • Launch the import flow again. This time tap on the Sign in button. Then ✖️ to exit the flow; verify autofill_import_google_passwords_result_user_cancelled with stage=webflow-authenticate
  • Launch the import flow again. This time, actually sign in, then ✖️ to exit the flow; verify autofill_import_google_passwords_result_user_cancelled with stage=webflow-post-login-landing
  • Launch the import flow again, and you'll be on the screen with the export button. tap ✖️ to exit the flow; verify autofill_import_google_passwords_result_user_cancelled with stage=webflow-export
  • Launch the import flow again. Tap the export button, and agree on the dialog. Then when prompted to authenticate, tap ✖️ to exit the flow; verify autofill_import_google_passwords_result_user_cancelled with stage=webflow-authenticate

Succeeding

  • Launch the import flow and fully complete it. Verify autofill_import_google_passwords_result_success, with the correct bucket for saved_credentials and skipped_credentials based on bucket rules defined here
  • Repeat that, and you should get duplicates this time. Verify again, ensuring that skipped_credentials bucket is accurate.

Overflow menu

  • Now that you have saved passwords, Import Passwords From Google will appear in the overflow menu. Tap on that. Verify autofill_import_google_passwords_overflow_menu_tapped and autofill_import_google_passwords_preimport_prompt_displayed in logs

Encrypted passphrase scenario

  • Apply patch
  • Launch import flow and you'll see the error screen. Tap ✖️ to exit flow. Verify autofill_import_google_passwords_result_user_cancelled with stage=webflow-passphrase-encryption

CSV parsing scenario

  • Discard local changes
  • Apply patch
  • Launch import flow and carry on until you've exported. The patch will simulate an error. Verify autofill_import_google_passwords_result_parsing in logs.

URL mapping via remote config

  • Discard local changes
  • Apply patch to override the URL mappings
  • Fresh install. Launch password import flow and then ✖️ to exit. Verify in logs with filter urlMappings that the mappings from the patched jsonblob are used and override the local defaults.

Selectively disabling JS injection

  • Discard local changes
  • Apply patch which simulates disabling remote config for JS injection specifically.
  • Fresh install
  • Launch password import flow; verify you don't see any UI hints on which button to press
  • Complete the flow; verify it still works as expected

@CDRussell CDRussell mentioned this pull request Nov 18, 2024
Merged
10 tasks
@CDRussell Graphite App
Copy link
Member Author

CDRussell commented Nov 18, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@CDRussell CDRussell mentioned this pull request Nov 18, 2024
Merged
22 tasks
@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_ui_to_launch_from_password_manager branch from 2a4356e to d9b11c1 Compare November 18, 2024 14:06
@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_kpis branch from f6fa1d8 to ba4e8c5 Compare November 18, 2024 14:06
@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_ui_to_launch_from_password_manager branch from d9b11c1 to 720a9dc Compare November 18, 2024 14:26
@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_kpis branch from ba4e8c5 to ef5e18b Compare November 18, 2024 14:26
@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_ui_to_launch_from_password_manager branch from 720a9dc to c1cc10b Compare November 18, 2024 18:03
@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_kpis branch 5 times, most recently from adc52be to 18d3de2 Compare November 19, 2024 16:54
CDRussell
CDRussell commented Nov 19, 2024
View reviewed changes
...in/java/com/duckduckgo/autofill/impl/importing/gpm/feature/AutofillImportPasswordSettings.kt
@@ -33,7 +33,14 @@ interface AutofillImportPasswordConfigStore {
data class AutofillImportPasswordSettings(
val canImportFromGooglePasswords: Boolean,
val launchUrlGooglePasswords: String,
val canInjectJavascript: Boolean,
Copy link
Member Author

Choose a reason for hiding this comment

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

as discussed, adding in ability to selectively disable just the JS injection

...in/java/com/duckduckgo/autofill/impl/importing/gpm/feature/AutofillImportPasswordSettings.kt
val javascriptConfigGooglePasswords: String,
val urlMappings: List<UrlMapping>,
Copy link
Member Author

Choose a reason for hiding this comment

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

for determining where in the webflow cancellation took place, we need to map URLs to a known string. there will be default mappings in place, but we can also override with remote config.

.../com/duckduckgo/autofill/impl/importing/gpm/webflow/ImportGooglePasswordsWebFlowViewModel.kt
} else {
terminateFlowAsCancellation(url ?: "unknown")
}
terminateFlowAsCancellation(url ?: "unknown")
Copy link
Member Author

Choose a reason for hiding this comment

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

handling ENCRYPTED_PASSPHRASE_ERROR_URL as another of the cancellation cases so don't need special treatment here for it

...l/src/main/java/com/duckduckgo/autofill/impl/engagement/store/AutofillEngagementBucketing.kt Show resolved Hide resolved
@CDRussell CDRussell marked this pull request as ready for review November 19, 2024 17:23
cmonfortep
cmonfortep approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

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

LGTM, everything works as expected.

Just a note, when reading in the latest scenario "Launch password import flow; verify you don't see any UI hints on which button to press", I was surprised because I didn't have hint in the previous scenarios. Let's review that tomorrow.

autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/pixel/AutofillPixelNames.kt
@@ -142,6 +142,15 @@ enum class AutofillPixelNames(override val pixelName: String) : Pixel.PixelName
AUTOFILL_TOGGLED_ON_SEARCH("m_autofill_toggled_on"),
AUTOFILL_TOGGLED_OFF_SEARCH("m_autofill_toggled_off"),

AUTOFILL_IMPORT_GOOGLE_PASSWORDS_EMPTY_STATE_CTA_BUTTON_TAPPED("autofill_import_google_passwords_import_button_tapped"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add them to remove atb or similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, keeping ATB et al.

@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_ui_to_launch_from_password_manager branch from c1cc10b to 3ae18f9 Compare November 20, 2024 23:02
@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_kpis branch from 18d3de2 to 097d049 Compare November 20, 2024 23:02
@CDRussell CDRussell mentioned this pull request Nov 20, 2024
Merged
1 task
@CDRussell
Copy link
Member Author

LGTM, everything works as expected.

Just a note, when reading in the latest scenario "Launch password import flow; verify you don't see any UI hints on which button to press", I was surprised because I didn't have hint in the previous scenarios. Let's review that tomorrow.

Isolated the problem and fixed in #5295. this branch has been restacked on top, meaning if you test again you should see the hints working now. good spot!

@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_ui_to_launch_from_password_manager branch from 3ae18f9 to d702bc0 Compare November 21, 2024 09:57
@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_kpis branch from 097d049 to f0972ee Compare November 21, 2024 09:57
@CDRussell Graphite App
Copy link
Member Author

CDRussell commented Nov 21, 2024 •
edited
Loading

Merge activity

  • Nov 21, 9:42 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Nov 21, 9:44 AM EST: Graphite rebased this pull request as part of a merge.
  • Nov 21, 9:59 AM EST: A user merged this pull request with Graphite.

@CDRussell CDRussell changed the base branch from feature/craig/autofill_gpm_ui_to_launch_from_password_manager to graphite-base/5284 November 21, 2024 14:42
@CDRussell CDRussell changed the base branch from graphite-base/5284 to develop November 21, 2024 14:43
@CDRussell CDRussell force-pushed the feature/craig/autofill_gpm_kpis branch from f0972ee to 90c8690 Compare November 21, 2024 14:43
@CDRussell CDRussell merged commit f213b31 into develop Nov 21, 2024
6 checks passed
@CDRussell CDRussell deleted the feature/craig/autofill_gpm_kpis branch November 21, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmonfortep cmonfortep cmonfortep approved these changes

Assignees

@cmonfortep cmonfortep

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CDRussell @cmonfortep