-
Notifications
You must be signed in to change notification settings - Fork 870
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
Add user-journey completion check for importing passwords via desktop sync #4698
base: feature/craig/import_passwords_via_sync_pixels
Are you sure you want to change the base?
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @CDRussell and the rest of your teammates on |
5fb09d4
to
5bc5361
Compare
9e5a998
to
ea4e47d
Compare
fa70e6d
to
c62b59a
Compare
ea4e47d
to
a4db5fc
Compare
c62b59a
to
0030112
Compare
a4db5fc
to
365dec6
Compare
2950ac3
to
963f332
Compare
365dec6
to
9df93ce
Compare
29ab853
to
6c69933
Compare
9df93ce
to
33646f3
Compare
6c69933
to
10e75a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just sharing a proposal for reorganizing the logic a bit, to keep the business logic together and viewmodel simpler.
|
||
// use appCoroutineScope as the call to get account state might not be quick; we want the call to succeed even if the user leaves the screen | ||
appCoroutineScope.launch(dispatchers.io()) { | ||
if (aUserJourneyIsOngoing()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to move all logic related to user journey checks into it's own class?
I see few benefits:
- ViewModel will be simplified of all logic about sync journey checks, which can be extracted. We can move all that business logic and just keep UI logic here.
- Dependencies can also be simplified: ViewModel has journey detecting logic which relies on
ImportPasswordsViaDesktopSyncDataStore
andUserJourneyEndRecorder
andDeviceSyncState
.UserJourneyEndRecoder
on a different class (with dependency toImportPasswordsViaDesktopSyncDataStore
).
I think the flow is easier if we just do: viewModel -> SyncImportUserJourney -> Business logic related to tracking journey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that is a good suggestion. I am going to do that as a follow-up as I don't think it's a blocker, tracking as Autofill: extract more of the business logic around importing passwords via desktop to its own class
...edential/management/importpassword/desktopapp/ImportPasswordsUserJourneyLifecycleObserver.kt
Outdated
Show resolved
Hide resolved
import javax.inject.Qualifier | ||
|
||
@ContributesTo(AppScope::class) | ||
@Module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Do we need to create modules if we rely on DS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly sure if needed, tbh; I was keeping it consistent with the other DS usage we have.
"Background the app and restore it. Verify logs indicate 48h clock was restarted when you re-entered the import passwords screen, by looking at the time remaining" This is the only step that didn't work for me. |
10e75a1
to
27adddb
Compare
33646f3
to
f57b2a3
Compare
27adddb
to
ec13e5f
Compare
I'm guessing I know why, and the instructions could have been clearer. Simply returning to the screen won't reset the clock. You have to come in with the Activity being created (basically, through the So you have to use the |
Task/Issue URL: https://app.asana.com/0/1203822806345703/1207149249611733/f
Description
Steps to test this PR
Suggest logcat filter:
message~:"user-journey" message~:"Pixel sent: .*autofill"
Testing the "immediate success" flow
Passwords
screen, then tap theImport Passwords
buttonStarting user-journey success clock for import passwords screen
in the logsm_autofill_logins_import_user_journey_started
in the logsSync with Desktop
buttonm_autofill_logins_import_success
Testing the timed out failure flow
Passwords
screen, then tap theImport Passwords
buttonm_autofill_logins_import_failure
Testing the delayed success flow
Passwords
screen, then tap theImport Passwords
buttonSettings
->Sync & Backup
and setup syncing with a desktop devicem_autofill_logins_import_success
Restarting the clock
Passwords
screen, then tap theImport Passwords
buttonStarting user-journey success clock for import passwords screen
Import Passwords
button againRestarting user-journey success clock for import passwords screen
m_autofill_logins_import_user_journey_restarted