1150285 - [Settings] When signed into Firefox Account no toggle appears on Find My Device Page

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Brogan Zumwalt [Inactive]]

Description:
After user signs into Firefox Account in Settings, Find My Device page only shows "Create account or sign in" button. No toggle switch for enabling or disabling FMD is visible. Despite this, status in Settings shows FMD as "enabled" and user can remotely interact with phone on find.firefox.com

Repro Steps:
1) Update a Flame to 20150401001203
2) Launch settings and sign into valid Firefox account
3) Navigate back to Settings and open Find My Device menu

Actual:
No toggle for enabling or disabling Find My Device appears despite being logged into FxA

Expected:
When logged into Firefox Account, Find My Device page shows toggle to enable and disable Find My Device.

Environmental Variables:
Device: Flame 2.1
Build ID: 20150401001203
Gaia: 63c89e001010a9a43ea55afd39333ded3d5e83fc
Gecko: 30ac9a03343a
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Repro frequency: 3/3, 100%
See attached: Youtube video clip - http://youtu.be/GiY1j56D4Co

Comment 1

[Brogan Zumwalt [Inactive]]

Issue does NOT reproduce on Flame 2.0, 2.2, or 3.0

When logged into Firefox Account, Find My Device page shows toggle to enable and disable Find My Device.

Device: Flame 2.0
Build ID: 20150401000204
Gaia: 896803174633fc6acd3fd105f81c349b8e9b9633
Gecko: 9c12f28cc73f
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 32.0 (2.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.2
Build ID: 20150401002624
Gaia: 8b3086ad3963f1707e2bee9094baccafffe161c4
Gecko: 20b67213a047
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 3.0
Build ID: 20150401010204
Gaia: 03164bd160809747e6a198e0dba1b7c3ee7789f5
Gecko: 18a8ea7c2c62
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Let's get a regression window here.

Comment 3

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Bug 1062558 seems to have caused this issue.

B2g-34 Regression Window:

Last Working 
Environmental Variables:
Device: Flame 2.1
BuildID: 20150122140947
Gaia: 2119de79b6edfbcd4f112b2f797dbd7dba17ea69
Gecko: 804697da948c
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First Broken 
Environmental Variables:
Device: Flame 2.1
BuildID: 20150122204031
Gaia: 234ec27050481f4787f1f4750ec26f62ce5cc2c0
Gecko: 4cdcc0e85fc0
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Last Working gaia / First Broken gecko - Issue does NOT occur
Gaia: 2119de79b6edfbcd4f112b2f797dbd7dba17ea69
Gecko: 4cdcc0e85fc0

First Broken gaia / Last Working gecko - Issue DOES occur
Gaia: 234ec27050481f4787f1f4750ec26f62ce5cc2c0
Gecko: 804697da948c

Gaia Pushlog: https://github.com/mozilla-b2g/gaia/compare/2119de79b6edfbcd4f112b2f797dbd7dba17ea69...234ec27050481f4787f1f4750ec26f62ce5cc2c0

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

Sherman, can you take a look at this please? This looks to have been caused by the uplift for bug 1062558.

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

Comment 6

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8588948 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

Guilherme, could you review this patch? We may need to toggle login panel after fxa login state changed.

Comment 7

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8588948 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

Thanks! In light of the current state of the code, this patch makes sense.

You may have seen that, on master, we rely on the 'findmydevice.logged-in' setting to toggle the panel in the settings app [1]. Can you please remove that listener from the settings app, as it is no longer needed in this branch?

I think, however, that if we ever land bug 1062558 on master, then we should consider using the setting instead of this solution, so that we track FxA's state from a single place across FMD.

Regardless, I can't really review this, as it's a change in the Settings app and I'm not a peer. Arthur Chen has done these kinds of reviews in the past, can you please try flagging him instead?

1- https://github.com/shamenchens/gaia/blob/cc10966/apps/settings/js/findmydevice.js#L30

Comment 8

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8588948 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

Hi Arthur, per ggp's comment we could also remove 'findmydevice.logged-in' settings observer to toggle the panel, could you review this patch? thanks!

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8588948 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

Thanks for providing the fix. I tried to find the difference between v2.1 and master and found that in v2.1 the event handler updating `findmydevice.logged-in` is only added when findmydevice is enabled (findmydevice_launcher.js). Although I did not figure out the reason but make the UI rely on the events from mozId seems much more robust to me compared with using settings value.

With this patch applied there is no one using the value under `findmydevice.logged-in`, please also remove all related code.

Comment 10

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8588948 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

Hi Arthur, I've removed `findmydevice.logged-in` related code, would you review it again? thanks!

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8588948 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

There are still scripts in system app accessing to `findmydevice.logged-in` and they should also be removed, thanks!

Comment 12

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8588948 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

Guilherme, since `findmydevice.logged-in` is no longer used, I'm going to remove settings helper in system app, could you review system app part? thanks!

Comment 13

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8588948 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

Actually, I can't, sorry -- I'm not a peer of the System app either. However, after taking another look at this, I think we may want to keep 'findmydevice.logged-in' and its listener.

This setting caches the login state so we display the correct panel when offline. IIRC, the FxA API doesn't give us a way to distinguish whether we're logged in or out while offline, and just errors out in that case. Please see bug 1034088, in particular bug 1034088 comment 12.

Sorry for the back-and-forth on this, and if you find that you can remove that setting without regressing bug 1034088, I'm fine with it.

Comment 14

[Guilherme Gonçalves [:ggp]]

So to clarify this, my suggestion for solving this is to keep the system app code that sets 'findmydevice.logged-in', and make sure the setting is updated even when FMD is disabled. But we only want to launch FMD when logging in or out if it is enabled, as usual.

Again, sorry I missed this at first.

Comment 15

[GitHub Autolander Bot]

Attachment: [gaia] shamenchens:Bug1150285-FMDSettings-2 > mozilla-b2g:master

Comment 16

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8590290 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-2 > mozilla-b2g:master

Thanks Guilherme, I know you are not system/settings owner, but since most of findmydevice related code was committed by you, you would be the most suitable one to give some feedback on this. 

And I think we get back to where we start, Arthur and Guilherme would both of you review/feedback this patch? thanks!

Comment 17

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8590290 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-2 > mozilla-b2g:master

This PR is based on master, I think you meant to base it on v2.1 right?

Anyway, I think we should only _togglePanel() in response to 'findmydevice.logged-in', otherwise we risk regressing bug 1034088. The important thing is that we store our login state somewhere so we can reuse it if FxA fails on us the next time we're offline.

My suggestion in comment 14 does that -- we'll be tracking the login state at all times from the system app, even when the log in didn't happen directly from the FMD panel in the settings. That's what we already do on master. The only difference is that we shouldn't call _wakeUpFindMyDevice if 'findmydevice.enabled' is false.

Can you try that please? Let me know if this is still unclear, and feel free to ping me for clarifications!

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8588948 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings > mozilla-b2g:v2.1

Clear the r+ flag per comment 13. The listener and the key should not be removed or we regress bug 1034088. I think the key is in v2.1, we should always update the value of `findmydevice.logged-in` no matter find my device is enabled or not, just like what we did in master.

And I was wondering why to provide a patch against if it is unaffected there?

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

missing a word: And I was wondering why to provide a patch against "master" if it is unaffected there?

Comment 20

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8590290 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-2 > mozilla-b2g:master

It was a mistake, my intention is to patch 2.1.

Comment 21

[GitHub Autolander Bot]

Attachment: [gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1s

Comment 22

[GitHub Autolander Bot]

Attachment: [gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1

Comment 23

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8593774 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1

Arthur and Guilherme, this patch updates `findmydevice.logged-in` when fxa login status changes, would you like to take another look?

Comment 24

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8593774 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1

Thanks! This is almost correct, but you'll need to do this from the System app instead of the Settings app, because we need to capture FxA events even when the Settings app is closed.

Note that there's already some code to do it in the System app [1], but after bug 1062558, we only set the setting when FMD is enabled. We now want to always set the setting, but only launch FMD if it is enabled.

1- https://github.com/mozilla-b2g/gaia/blob/d5f0b24894cb5848d6cf7f199f0bde508f7f2e7a/apps/system/js/findmydevice_launcher.js#L106,L114

Comment 25

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8593774 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1

Hi Chens, we need to update the value of `findmydevice.logged-in` no matter findmydevice is enabled or not just like we did in master (details please refer to comment 9). The changes should be made in system app as ggp suggested.

Comment 26

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8593774 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1

Move around onFxAChromeEvent, does this makes more sense?

Comment 27

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8593774 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1

Great, thank you! Can you please add a unit test to make sure the setting is set even when disabled?

Oh, and if you need a system peer to review this, :kgrandon has usually helped us on FMD bugs like this.

Comment 28

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8593774 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1

Hi Kevin, this patch fixes regression caused by bug 1062558, `findmydevice.logged-in` should always reflect to fxa login status so we can have correct view in settings app panel. Could you review it? thanks!

Comment 29

[Kevin Grandon :kgrandon]

Comment on attachment 8593774 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1

I will take a quick look, but basically just review stamping this based on  Guilherme's feedback. Thanks.

Comment 30

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8593774 [details] [review]
[gaia] shamenchens:Bug1150285-FMDSettings-v21 > mozilla-b2g:v2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1062558
[User impact] if declined: Not able to use FMD panel in settings app if login at FxA account panel for the first time.
[Testing completed]: Manual and unit test.
[Risk to taking this patch] (and alternatives if risky): Changes only related in findmydevice, will not affect system behavior, should be relatively low.
[String changes made]: None

Comment 31

[Josh Cheng [:josh]]

Hi Lancy,
Can you check whether the issue also happen on 2.1S dolphin device?

Also NI Steven EPM of Dolphin for following up.

Comment 32

[Lancy(Leave from Mozilla)]

Attachment: Verify1_Dolphin2.1S_Fail.mp4

Hi Josh,
This issue also happen on latest Dolphin 2.1s build.
See attachment:Verify1_Dolphin2.1S_Fail.mp4 and Verify1_log_Dolphin2.1s.txt.
Reproducing rate:3/3
Occurrence time:23:38

Device: Dolphin 2.1S build(512M)
Build ID               20150511001203
Gaia Revision          e4c6fee5a08daddf2d9dc90be7c1516e0f371f9d
Gaia Date              2015-05-06 21:48:08
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/c1903564bbe6
Gecko Version          34.0
Device Name            scx15_sp7715ea
Firmware(Release)      4.4.2
Firmware(Incremental)  122
Firmware Date          Thu Feb  5 12:42:58 CST 2015

Comment 33

[Lancy(Leave from Mozilla)]

Attachment: Verify1_log_Dolphin2.1s.txt

Comment 34

[Steven Yang [:styang]]

a critical regression, let's fix it in 2.1S as well.

Comment 35

[GitHub Autolander Bot]

There was an error creating the taskgraph, please try again. If the issue persists please contact someone in #taskcluster.

Comment 36

[Sherman Chen [:chens] (inactive)]

2.1: https://github.com/mozilla-b2g/gaia/commit/c80865cb0bf73f1b97defbc646083b404feb3ac4

Comment 37

[Verson Xiong (Leave from Mozilla)]

Attachment: Verify.mp4

This bug has been verified as pass on latest build of Flame v2.1 and 2.1s_512mb by the STR in Comment 0.
Actually Result: When logged into Firefox Account, Find My Device page shows toggle to enable and disable Find My Device.
Reproduce rate: 0/10
See attachment: Verify.mp4


Device: Flame v2.1 build(pass)
Build ID               20150601001204
Gaia Revision          2304a1f6327c2ccf35d6995ee16f2231ed1f22a3
Gaia Date              2015-05-26 13:30:41
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e52807dee101
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150601.035649
Firmware Date          Mon Jun  1 03:57:00 EDT 2015
Bootloader             L1TC000118D0

Device: 2.1s_512mb build(pass)
Build ID               20150601001204
Gaia Revision          0493b4dbb59f9617c1c1a45f422303c2aff32a9a
Gaia Date              2015-05-27 19:33:12
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/1aae9fc735b5
Gecko Version          34.0
Device Name            scx15_sp7715ea
Firmware(Release)      4.4.2
Firmware(Incremental)  122
Firmware Date          Thu Feb  5 12:42:58 CST 2015