FindMyDevice says enabled when server rejects connections

RESOLVED FIXED in Firefox OS v2.0

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gerard-majax, Unassigned)

Tracking

unspecified
2.1 S2 (15aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
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 ?
Flags: needinfo?(ggoncalves)
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?
Flags: needinfo?(vkrishnamoorthy)
Flags: needinfo?(jsavory)
Flags: needinfo?(ggoncalves)
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.
Flags: needinfo?(jsavory)

Updated

4 years ago
Flags: needinfo?(vkrishnamoorthy)
Vishy - Can you make a blocking call here from a product perspective?
Flags: needinfo?(vkrishnamoorthy)

Comment 5

4 years ago
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
Flags: needinfo?(vkrishnamoorthy)
Flags: needinfo?(jsavory)
Flags: needinfo?(ggp)
Yes, I believe that this set of steps is correct.
Flags: needinfo?(jsavory)
Looks good to me.
Flags: needinfo?(ggp)

Comment 8

4 years ago
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
Flags: needinfo?(vkrishnamoorthy)
Flags: needinfo?(bryan)

Comment 9

4 years ago
Created attachment 8453905 [details]
FMD_Notifications.zip

Attached is the icons needed and an example of how it should look in practice.
Flags: needinfo?(bryan)

Comment 10

4 years ago
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?
Flags: needinfo?(vkrishnamoorthy) → needinfo?(bbajaj)
vishy is already looked into this being string free, lets live upto that when landing it as well.
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(bbajaj)
Assignee: nobody → mgoodwin

Updated

4 years ago
Target Milestone: --- → 2.0 S6 (18july)
(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
Depends on: 1041593

Updated

4 years ago
Target Milestone: 2.0 S6 (18july) → ---

Updated

4 years ago
Target Milestone: --- → 2.1 S1 (1aug)
From discussion with :ggp, it seems it's only desirable to notify the user in the event of a registration failure.
Created attachment 8460908 [details] [review]
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
Attachment #8460908 - Flags: feedback?(ggoncalves)
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.
Attachment #8460908 - Flags: feedback?(ggoncalves)
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 ?
Flags: needinfo?(francesco.lodolo)
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
Flags: needinfo?(francesco.lodolo)
(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?
Flags: needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)
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
Attachment #8460908 - Flags: review?(lissyx+mozillians)
(Reporter)

Comment 21

4 years ago
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 :)
Attachment #8460908 - Flags: review?(lissyx+mozillians)
(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.
Flags: needinfo?(lissyx+mozillians)
(Reporter)

Comment 23

4 years ago
Thanks, replied to some.
Flags: needinfo?(lissyx+mozillians)
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
Attachment #8460908 - Flags: review?(lissyx+mozillians)
(Reporter)

Comment 25

4 years ago
Your PR is against v2.0. Please make it against master and request uplift after.
Flags: needinfo?(mgoodwin)
(Reporter)

Comment 26

4 years ago
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 ?
(Reporter)

Comment 27

4 years ago
After 15 mins I could get the notification, but the checkbox is still enabled.
(Reporter)

Comment 28

4 years ago
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.
Attachment #8460908 - Flags: review?(lissyx+mozillians)
(Reporter)

Comment 29

4 years ago
(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.
(Reporter)

Comment 30

4 years ago
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.
(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.
Flags: needinfo?(mgoodwin)
Created attachment 8464210 [details] [review]
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
Attachment #8464210 - Flags: review?(lissyx+mozillians)
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.
(Reporter)

Comment 34

4 years ago
(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.
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.
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.
Ok, I think I'm ready for another look at https://github.com/mozilla-b2g/gaia/pull/22276 (r? is already set).

Thanks
Flags: needinfo?(lissyx+mozillians)
(Reporter)

Comment 38

4 years ago
(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.
Flags: needinfo?(lissyx+mozillians)
(Reporter)

Updated

4 years ago
Attachment #8464210 - Flags: review?(lissyx+mozillians)
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.
Attachment #8464210 - Flags: review?(lissyx+mozillians)
(Reporter)

Comment 40

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

That looks good, thanks!
Attachment #8464210 - Flags: review?(lissyx+mozillians) → review+
Created attachment 8466713 [details] [review]
PR on v2.0 for uplift
Attachment #8460908 - Attachment is obsolete: true
Attachment #8466713 - Flags: review+
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/abd1624d173dbe730ac07c9598d634024a2bf824
v2.0:   https://github.com/mozilla-b2g/gaia/commit/4ab7384db7aee130be165a699472cc19405a4456
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Assignee: mgoodwin → nobody
(Reporter)

Updated

3 years ago
Blocks: 1189758
You need to log in before you can comment on or make changes to this bug.