1034570 - FindMyDevice says enabled when server rejects connections

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[:gerard-majax]

It looks like error states are not properly handled with Find My Device. Specifically, I noticed error in my logcat:

> 07-04 13:26:54.933  1036  1036 I GeckoDump: [findmydevice] findmydevice alarm!
> 07-04 13:26:54.933  1036  1036 I GeckoDump: [findmydevice] findmydevice attempting registration.
> 07-04 13:26:54.933  1036  1036 I GeckoDump: [findmydevice] registering: false
> 07-04 13:26:54.933  1036  1036 I GeckoDump: [findmydevice] refreshing client id if registered and logged in: {"registered":false,"loggedIn":true}
> 07-04 13:26:55.193  1036  1036 I GeckoDump: [findmydevice] findmydevice received push endpoint!
> 07-04 13:26:55.913  1036  1036 I GeckoDump: [findmydevice] findmydevice request failed with status: 401

But in the same time, in the Settings, we say the feature is enabled and we do not warn the user that we are having issues to communicate with the server. This happens on Flame, master, connected to Mozilla Guest. It was okay yesterday.

Comment 1

[bhavana bajaj [:bajaj]]

NI :ggp here to get started on the investigation here. ggp, something we changed recently ? Description say's it was working okay yesterday.

What is the user expected to see in situations like these ?

Comment 2

[Guilherme Gonçalves [:ggp]]

The issue actually goes a little deeper than that. What :gerard-majax refers to as "okay yesterday" are the 401 errors, which we'll investigate in bug 1034612. He's getting these errors when attempting to register for FMD for the first time, which prevents FMD from working at all, but on the other hand, I'd expect these errors to result from temporary server failures, from which we can recover in the background.

When 401's happen, we don't notify the user in any way, and the UI keeps reporting FMD as Enabled in the Settings. I don't think we ever got UX input on error cases like this, so this was actually my own decision -- we keep reporting as Enabled to the user, but attempt to resolve the problem in the background by contacting the server periodically, since I expect most errors of this type to be transient. My thinking was that, even though FMD is not working at the moment due to the 401, it will still attempt to ping the server while enabled, and only stop if disabled, so we should present its state as Enabled to the user.

I definitely agree the UX could be much, much better, and I'll be glad to change this, either by reporting it Disabled right now as a stopgap, and/or introducing proper error strings in the future. :vishy, :jsavory, what do you think?

Comment 3

[Jacqueline Savory [:jsavory] UX]

I think that for these errors we should try to recover for a period of time in the background, if it then fails, the FMD toggle should be disabled. The user should then receive the notification from bug 1029706 and when they return to the FMD settings screen they can try again by enabling FMD.

Comment 4

[Jason Smith [:jsmith]]

Vishy - Can you make a blocking call here from a product perspective?

Comment 5

[vkrishnamoorthy]

teasing out comment 3 into steps
1. User enables FMD on the settings app
2. FMD text changes to "Enabled"
3. In the background FMD client will connect with the server(s) to register the device
4. After 5 tries (~ 15mins) if the client is not able to connect to the server, client will put up a toast notification as shown in 1029706
5. When user clicks on notification, it will take the user to the FMD settings
6. At this point FMD text should read "Disabled"
7. User can try to enable FMD as in Step 1.

I am still keeping the NI on my name regarding the block/non block as I want to get a sense of the probability of occurrence for the STR

NI:ggp, jsavory to confirm steps 1 - 7 on the experience

Comment 6

[Jacqueline Savory [:jsavory] UX]

Yes, I believe that this set of steps is correct.

Comment 7

[Guilherme Gonçalves [:ggp]]

Looks good to me.

Comment 8

[vkrishnamoorthy]

NI: Bryan for the icon and myself for the decision

Bryan, can we get an icon for FMD? please see the image https://people.mozilla.org/~ggoncalves/notification.png to see where the icon will be used

Comment 9

[bryan]

Attachment: FMD_Notifications.zip

Attached is the icons needed and an example of how it should look in practice.

Comment 10

[vkrishnamoorthy]

I recommend we block on this for 2.0 as it is more than a user enhancement. It provides the user feedback on their action of enabling FMD has failed. 

NI:bajaj can we move this to a blocker?

Comment 11

[bhavana bajaj [:bajaj]]

vishy is already looked into this being string free, lets live upto that when landing it as well.

Comment 12

[Francesco Lodolo [:flod]]

(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #11)
> vishy is already looked into this being string free, lets live upto that
> when landing it as well.

Per IRC talk with Mark, keeping an eye on the bug from l10n side. 

FMD app has currently no strings, necessary strings are in system.app, so the approach for 2.0 should be like bug 1032502 in order to avoid breaking string freeze
https://github.com/mozilla-b2g/gaia/commit/6a2be886df047682d897d1c9c560009021c01951

Comment 13

[Mark Goodwin [:mgoodwin]]

From discussion with :ggp, it seems it's only desirable to notify the user in the event of a registration failure.

Comment 14

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1034570 - Notify the user if initial registration fails

Some explanations:

1) index.html - I moved the script element to sit next to the locales - for consistency with other apps I looked ad

2) settings; I've used separate settings for findmydevice.enableFailed and findmydevice.retryCount so we keep as much FMD logic as possible outside of the system app

Comment 15

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8460908 [details] [review]
Bug 1034570 - Notify the user if initial registration fails

Thanks for this! I left some notes on GitHub, please f? again when they're fixed. And as usual, don't hesitate to ping me if you want to discuss anything.

Comment 16

[Mark Goodwin [:mgoodwin]]

One question re. the L10n stuff; should the system app's strings be moved to /shared rather than importing as per https://github.com/mozmark/gaia/commit/685438b66a2f3fd0e02a7ad5ce7c7fb28508a886#apps-findmydevice-index-html-P6 ?

Comment 17

[Francesco Lodolo [:flod]]

If this needs to land on 2.0 (based on the flags), you can't add new strings. Moving around strings is basically deleting strings from one place, and adding them to another, which is not allowed at this point for 2.0

Comment 18

[Mark Goodwin [:mgoodwin]]

(In reply to Francesco Lodolo [:flod] from comment #17)
> If this needs to land on 2.0 (based on the flags), you can't add new
> strings. Moving around strings is basically deleting strings from one place,
> and adding them to another, which is not allowed at this point for 2.0

To make sure I understand; sharing is OK for 2.0, and moving is not?

Comment 19

[Francesco Lodolo [:flod]]

(In reply to Mark Goodwin [:mgoodwin] from comment #18)
> To make sure I understand; sharing is OK for 2.0, and moving is not?

Yes. In this case sharing is a necessary compromise to avoid breaking string freeze.

Comment 20

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8460908 [details] [review]
Bug 1034570 - Notify the user if initial registration fails

Addressed ggp's comments on https://github.com/mozmark/gaia/commit/685438b66a2f3fd0e02a7ad5ce7c7fb28508a886

Comment 21

[:gerard-majax]

Comment on attachment 8460908 [details] [review]
Bug 1034570 - Notify the user if initial registration fails

Thanks, I did a couple of comments on GitHub. Please flag me again once everything is addressed :)

Comment 22

[Mark Goodwin [:mgoodwin]]

(In reply to Alexandre LISSY :gerard-majax from comment #21)
> I did a couple of comments on GitHub. Please flag me again once
> everything is addressed :)

Thanks for looking at this. I have asked some questions in response to some of your comments; in the mean time I'll make a start on the other things.

Thanks again.

Comment 23

[:gerard-majax]

Thanks, replied to some.

Comment 24

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8460908 [details] [review]
Bug 1034570 - Notify the user if initial registration fails

The only thing I've not addressed is testing for 200s - this is Requester functionality and I think if we want to test that we should file a follow up bug to add unit tests to Requester (there are currently none).

Also, I noticed when working on this that the findmydevice_launcher tests were failing for unrelated reasons: see bug 1044368

Comment 25

[:gerard-majax]

Your PR is against v2.0. Please make it against master and request uplift after.

Comment 26

[:gerard-majax]

I'm partly unhappy of the behavior that I am observing right now: I configured my device to fake find.firefox.com (and redirected it to google). This obviously makes the connection failing. That may, for example, match the case of a WiFi Captive Portal or SIM card running out of data and getting redirected to a captive portal.

I already saw two failures in the logs, but the UI is still lying to the user by saying the feature is already enabled. Can't we have an intermediate state instead ?

Comment 27

[:gerard-majax]

After 15 mins I could get the notification, but the checkbox is still enabled.

Comment 28

[:gerard-majax]

Comment on attachment 8460908 [details] [review]
Bug 1034570 - Notify the user if initial registration fails

Works not bad but please do the PR against master and make sure we properly update the checkbox. Also, after closing the settings app, tapping the notification does nothing. And I still see the checkbox as enabled.

Comment 29

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #28)
> Comment on attachment 8460908 [details] [review]
> Bug 1034570 - Notify the user if initial registration fails
> 
> Works not bad but please do the PR against master and make sure we properly
> update the checkbox. Also, after closing the settings app, tapping the
> notification does nothing. And I still see the checkbox as enabled.

Okay, part of the issues there were because of bug 1044406: tapping indeed works. But I'm quite disturbed by the behavior (expected, according to your reply on github): we only turn off the checkbox when the user taps on the notification error.

We should reflect the real state there.

Comment 30

[:gerard-majax]

And yet, it makes sense to keep the setting enabled because it can be a transient error: user not yet logged in captive portal, bad network, server-side issue. I really think the proper way would be to distinguish between feature activation and current status.

Comment 31

[Mark Goodwin [:mgoodwin]]

(In reply to Alexandre LISSY :gerard-majax from comment #25)
> Your PR is against v2.0. Please make it against master and request uplift
> after.

OK, I'll do that.

Comment 32

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1034570 - Notify the user if initial registration fails - PR on master

This is just the changes from the v2.0 PR applied to master

Comment 33

[Jacqueline Savory [:jsavory] UX]

I don't think we need to include different states for the FMD toggle since FMD isn't something that the user will be able to act upon and knowing these states doesn't help the user. I think it is likely that the user will enable FMD and then navigate elsewhere, unless there is a problem and the user needs to re-enable the toggle, it is better to keep it in its current state. 

For now, I think that even though the experience is certainly not ideal, it is the best we can do without being able to add strings. For 2.1 I am currently working on a proposal to improve error cases like this one.

Comment 34

[:gerard-majax]

(In reply to jsavory from comment #33)
> I don't think we need to include different states for the FMD toggle since
> FMD isn't something that the user will be able to act upon and knowing these
> states doesn't help the user. I think it is likely that the user will enable
> FMD and then navigate elsewhere, unless there is a problem and the user
> needs to re-enable the toggle, it is better to keep it in its current state. 
> 
> For now, I think that even though the experience is certainly not ideal, it
> is the best we can do without being able to add strings. For 2.1 I am
> currently working on a proposal to improve error cases like this one.

That's your call, but I really feel this is a badly broken UX that will lead to misunderstanding.

Comment 35

[Jacqueline Savory [:jsavory] UX]

After talking with Alexandre I've realized that I misunderstood the exact problem. To clarify:

If a major error occurs and the notification to inform the user is sent, the FMD toggle switch should be disabled whether the user taps on the notification or not.  The toggle should disable as the notification is sent not because the user tapped on it. Sorry for any confusion.

Comment 36

[Mark Goodwin [:mgoodwin]]

OK, so to summarize, I'll make the following changes:
1) Notifications will no longer be cleared on restart
2) FMD will be disabled as soon as the limit (currently 5 retries) is reached - and a notification will be shown
3) Clicking the notification still takes you to settings (FMD section)
4) dismissing the notification does nothing.

to be clear, this means that if a user dismisses a notification (unlike the current implementation) FMD will be disabled, they will not be taken to settings (so they may not know it's disabled) and they will get no further warning that something went wrong.

Comment 37

[Mark Goodwin [:mgoodwin]]

Ok, I think I'm ready for another look at https://github.com/mozilla-b2g/gaia/pull/22276 (r? is already set).

Thanks

Comment 38

[:gerard-majax]

(In reply to Mark Goodwin [:mgoodwin] from comment #37)
> Ok, I think I'm ready for another look at
> https://github.com/mozilla-b2g/gaia/pull/22276 (r? is already set).
> 
> Thanks

You still have some code in tests for FMD that references Notification while you are using it in the System app. Also, your notification system message handler should probably check that you have been tapped on a FMD notification.

Comment 39

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8464210 [details] [review]
Bug 1034570 - Notify the user if initial registration fails - PR on master

Removed notifications from the FMD test,

Added a check for the FMD tag on the system message handler.

Comment 40

[:gerard-majax]

Comment on attachment 8464210 [details] [review]
Bug 1034570 - Notify the user if initial registration fails - PR on master

That looks good, thanks!

Comment 41

[Mark Goodwin [:mgoodwin]]

Attachment: PR on v2.0 for uplift

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/abd1624d173dbe730ac07c9598d634024a2bf824
v2.0:   https://github.com/mozilla-b2g/gaia/commit/4ab7384db7aee130be165a699472cc19405a4456