Issue metadata
Sign in to add a comment
|
Prevent MultiDevice "new user" event from firing immediately after forgetting a host on new Chromebook |
||||||||||||||||||||||||
Issue descriptionThis is expected behavior given the implementation of https://chromium-review.googlesource.com/c/chromium/src/+/1205495 but it should be prevented.
,
Sep 10
,
Sep 10
Jordy, please provide more context on this issue. Specifically, what is the actual user experience and what should it be instead? What changes are necessary in the codebase to affect this change?
,
Sep 12
TLDR: We should prevent the 'new user' event which triggers a notification prompting the user to go through Unified Setup if the user has ever seen the 'Chromebook added' notification before on that machine. It should only involve making the method AccountStatusChangeDelegateNotifierImpl::CheckForNewUserPotentialHostExistsEvent() (https://cs.chromium.org/chromium/src/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.cc?q=%22CheckForNewUserPotentialHostExistsEvent(%22+f:chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.cc&sq=package:chromium&g=0&l=163) return early if there is a non-zero timestamp for that event. Full detail/context: In the Settings UI subpage, a click on the 'Forget device' line will prevent the Chromebook from connecting to the host until it is set up again. Under the hood, this changes the account's host status (i.e. chromeos::multidevice_setup::mojom::HostStatus) from one of the 'host set' statuses (i.e. waiting for verification, waiting for server, or verified) to the 'eligible hosts exist' mode. One result of https://chromium-review.googlesource.com/c/chromium/src/+/1205495 is that any change in the account's host status triggers a check for MultiDevice notifications. At this stage, the notification check sees that the host status is 'eligible hosts exist' so the user is a candidate for the Setup Flow so it can trigger the 'new user' notification. If the user has has ever received the 'new user' on the Chromebook they're currently using (call it Chromebook A), the currently logic prevents. However, if setup took place on a different Chromebook (call it Chromebook B) so they were never shown the 'new user' notification on Chromebook A, it will come up (on Chromebook A) when the host is forgotten. So basically the problem case is: 1) User goes through Unified Setup on Chromebook B 2) User logs onto Chromebook A 3) User clicks 'forget device on Chromebook A' The simplest solution is just to have the check prevent the 'new user' notification from showing if the 'Chromebook added' notification has ever appeared on Chromebook A because, if setup took place on Chromebook B, the 'Chromebook added' notification should have appeared on Chromebook A the next time it was logged in.
,
Sep 13
Is it not simpler to have a 'host set' status of 'forgotten' or similar?
,
Sep 14
Issue 884088 has been merged into this issue.
,
Sep 14
,
Sep 20
Issue 887597 has been merged into this issue.
,
Sep 20
,
Oct 4
Jordy's suggestion of "making the method AccountStatusChangeDelegateNotifierImpl::CheckForNewUserPotentialHostExistsEvent()... return early if there is a non-zero timestamp for that event" has actually already been implemented, see [1]. Has anyone repro'd this issue recently? I personally cannot (on HEAD), and from my reading of the code [1], I can't see how it can happen. However, I'm sure the issue he described of multiple client devices still occurs, that is: 1) User goes through Unified Setup on Chromebook B 2) User logs onto Chromebook A 3) User clicks 'forget device' on Chromebook A The best way to solve this particular issue is probably to follow James's suggestion in #5. AFAICT, this bug can only cause issues for users with multiple Chromebooks, or who remove their account and sign back in. That's common for us, but definitely an edge case for most users -- I think it's best if we lower the priority of this for now. If I'm wrong about not being able to repro this in the single-client case, please correct me and fix the priority. 1) https://cs.chromium.org/chromium/src/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.cc?q=%22CheckForNewUserPotentialHostExistsEvent(%22+f:chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.cc&sq=package:chromium&g=0&l=175
,
Oct 4
Currently, our logic is to show the notification the first time that the Chromebook notices that the user has an eligible phone but no active host phone. In this case, we go through setup flow on Chromebook A, then log into Chromebook B and forget the device. Since it is the first time that Chromebook B has seen an eligible device without any host set, it shows the notification. We should change the logic so that we simply never show the promo notification after the user has set a host, regardless of what Chromebook the user set the host on. The easiest way to do this would be to modify SetupFlowCompletionRecorder to observer HostStatusProvider; if at any time HostStatusProvider has a status of kHostVerified, SetupFlowCompletionRecorder should record this event. Then, when AccountStatusChangeDelegateNotifier checks to see if it should show the "new user" notification, it should ask SetupFlowCompletionRecorder if the host has ever been verified. If it has, it should not show the notification. The two changes involved are: (1) Modify SetupFlowCompletionRecorderImpl so that it observes HostStatusProvider, and remove SetupFlowCompletionRecorder's RecordCompletion() function which is currently unused. (2) Modify AccountStatusChangeDelegateNotifierImpl to check for whether the host has ever been verified within CheckForNewUserPotentialHostExistsEvent().
,
Oct 4
Yeah, I'm not seeing the original bug either. I'm not sure off the top of my head why it stopped, but I'm about to dive into that code for crbug.com/884373 so I'll double check there if it makes sense. Kyle, could you put the issue in comment 11 in a separate bug?
,
Oct 4
The bug still is that you get the "new user" notification after forgetting a host, so I don't think it warrants a separate bug. I adjusted the naming of this bug to be more specific, though :)
,
Oct 5
,
Oct 8
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by jordynass@chromium.org
, Sep 6Owner: jordynass@chromium.org
Status: Assigned (was: Untriaged)