1098517 - Fix hiding of FxA UI when loop.fxa.enabled is false

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Matthew N. [:MattN]]

It may be as simple as adding the attribute `displayed={navigator.mozLoop.fxAEnabled}` to <div className="footer-signin-separator" /> at https://mxr.mozilla.org/mozilla-central/source/browser/components/loop/content/js/panel.jsx?rev=3e5a8432885b#777

Make sure to build-jsx so panel.js gets updated too.

Comment 1

[:shell escalante]

only impacts if we turn direct calling off.  with this sprint so full - putting to next sprint to consider.

Comment 2

[nithin[:lazybug]]

hi i would like to work on this bug.i have successfully build and ran firefox.

but here i cant figure out where to set loop.fxa.enabled. i checked in about:config but couldn't find any such setting.

Comment 3

[:Gijs (he/him)]

(In reply to nithin from comment #2)
> hi i would like to work on this bug.i have successfully build and ran
> firefox.
> 
> but here i cant figure out where to set loop.fxa.enabled. i checked in
> about:config but couldn't find any such setting.

You can set it yourself using about:config - right-click, and choose new boolean, then put "loop.fxa.enabled" as the name (without quotes) and set the value to true.

Comment 4

[nithin[:lazybug]]

i am not able to build the panel.jsx file beacause even when iam commenting out the code for the seperator "<div className="footer-signin-separator" />" its still showing up . but if i am changing it in the panel.js "React.createElement("div", {className: "footer-signin-separator"}), " its not showing up. 
now iam using " ./mach build  browser/components/loop/" 
can you help me in properly building it?

Comment 5

[:Gijs (he/him)]

(In reply to nithin from comment #4)
> i am not able to build the panel.jsx file beacause even when iam commenting
> out the code for the seperator "<div className="footer-signin-separator" />"
> its still showing up . but if i am changing it in the panel.js
> "React.createElement("div", {className: "footer-signin-separator"}), " its
> not showing up. 
> now iam using " ./mach build  browser/components/loop/" 
> can you help me in properly building it?

Comment 6

[Mike de Boer [:mikedeboer]]

Hi nithin, the panel.jsx should indeed be built to update panel.js, which is the script loaded by the Loop panel. To build the *.jsx files, you'll need to run the following:

$ npm install react-tools -g
$ cd <your-checkout-dir>/browser/components/loop
$ ./build-jsx -w

The -w flag will keep watching the loop source directory for changes in the *.jsx files and re-build them if necessary.

After all this you can do

$ ./mach build browser/components/loop/

and you'll see the changes you just made.

Comment 7

[nithin[:lazybug]]

Attachment: created patch v1

created patch , used "hidden" attribute to hide

Comment 8

[Matthew N. [:MattN]]

Sorry, the bug summary no longer reflects what's need to be done. The behaviour of the pref has regressed since it landed so it's no longer doing the right thing. The gear menu shouldn't be hiding anymore now that there are non-login items in it (Tour + Help) so the separator should stay visible but some of the entries in the dropdown should be hidden.

Do you want to update panel.jsx to only hide the "Account", and "Sign In"/"Sign Out" entries in the dropdown but leave the gear showing with the other entries. You can look at: 
https://mxr.mozilla.org/mozilla-central/source/browser/components/loop/content/js/panel.jsx?rev=37ca80281932#322

You can use `displayed={navigator.mozLoop.fxAEnabled}`

Thanks and sorry about the bug being stale.

Comment 9

[nithin[:lazybug]]

Attachment: patch_v1.patch

Comment 10

[nithin[:lazybug]]

Attachment: patch_v2.patch

added patch

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8553738 [details] [diff] [review]
patch_v2.patch

Review of attachment 8553738 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This patch fixes the problem but I guess you didn't see the instructions from comment 6 on how to generate the .js file from the .jsx. I also noticed that tests were failing:
AssertionError: browser/components/loop/test/desktop-local/index.html: 1 failure(s) encountered:
TEST-UNEXPECTED-FAIL | index.html | should be hidden if FxA is not enabled…

Could you test your patch with ./browser/components/loop/run-all-loop-tests.sh and fix the test?

Also, the User header of the patch seems to be malformed (missing "<")

Comment 12

[nithin[:lazybug]]

sorry, i created the patch using git, forgot to add js file.

I am not exactly sure where to look for what failed the test (the testing condition), i tried locating the python file used to run the test, but was not helpful. i tried searching the documentation , still couldn't find anything much useful.

Comment 13

[Mark Banner (:standard8)]

(In reply to nithin from comment #12)
> I am not exactly sure where to look for what failed the test (the testing
> condition), i tried locating the python file used to run the test, but was
> not helpful. i tried searching the documentation , still couldn't find
> anything much useful.

You can run that test by hand, I've just filed bug 1125775 for adding the following information into our readme.

=======
You can run these as part of the run-all-loop-tests.sh command above, or you can run these individually in Firefox. To run them individually, start the standalone client (see standalone/README.md) and load:

  http://localhost:3000/test/
=======

Once loaded, you should be able to scroll through the page, see the errors in more detail, and debug what's happening.

Comment 14

[nithin[:lazybug]]

Attachment: loopfxa_v3.patch

hi sorry for the delay, i tried the standalone client, but it was not running the local tests. but however i was able to figure out the test which failed and i removed it (As it was asserting exactly what we have changed , it was no longer needed i guess ) now patch is passing all the tests. but however,using standalone , in the shared testes 5 tests are failing, (they were failing even before the changes i made) do we need to take care of that?

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8560703 [details] [diff] [review]
loopfxa_v3.patch

Review of attachment 8560703 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/test/desktop-local/panel_test.js
@@ -269,5 @@
>        afterEach(function() {
>          navigator.mozLoop.fxAEnabled = true;
>        });
>  
> -      it("should be hidden if FxA is not enabled",

It's fine to delete this but a new test should be added to test that the Account menu item is hidden though.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8560703 [details] [diff] [review]
loopfxa_v3.patch

Review of attachment 8560703 [details] [diff] [review]:
-----------------------------------------------------------------

Matt's right, we should get a test added for the account display functionality - you should just be able to base it on the existing "should show an account entry when user is authenticated" test.

::: browser/components/loop/content/js/panel.jsx
@@ +337,1 @@
>                                     displayed={this._isSignedIn()} />

This might work, but probably would be better as:

displayed={navigator.mozLoop.fxAEnabled || this._isSignedIn()}

Comment 17

[nithin[:lazybug]]

Attachment: loopfxa_v4.patch

created new patch with a test and also incoprating Markk's suggestion

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8564085 [details] [diff] [review]
loopfxa_v4.patch

Review of attachment 8564085 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, the test is the right thing to do.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +258,5 @@
>            expect(view.getDOMNode()).to.be.null;
>        });
>      });
> +    
> +    it("should hide the account entry when FxA is not enabled", function() {

Small nit: there's a little bit of whitespace on the line before the test that isn't needed.

Comment 19

[nithin[:lazybug]]

so should i remove those spaces and submit a new patch ???

Comment 20

[Mark Banner (:standard8)]

Yes please - it makes it easier for the person landing the patch.

Comment 21

[nithin[:lazybug]]

Attachment: loopfxa_v4.patch

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8564979 [details] [diff] [review]
loopfxa_v4.patch

Review of attachment 8564979 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

By the way, you can use "r=MattN" in the future as we usually don't use spaces for r=… so that we can parse the reviewer out of the message.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Can we please run this through Try to ensure that there are no test failures lurking in the weeds before it lands? :)

Comment 24

[Matthew N. [:MattN]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78cf5584af08

Comment 25

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/58f815545f09

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58f815545f09