Closed Bug 1098517 Opened 6 years ago Closed 6 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal

Tracking

(firefox38 fixed)

RESOLVED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed
Blocking Flags:
backlog Fx38?

People

(Reporter: MattN, Assigned: lazybug, Mentored)

References

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 5 obsolete files)

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.
Flags: qe-verify-
Flags: firefox-backlog+
only impacts if we turn direct calling off.  with this sprint so full - putting to next sprint to consider.
backlog: Fx35? → Fx37?
backlog: Fx37? → Fx38?
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
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?
(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?
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
Attached patch created patch v1 (obsolete) — Splinter Review
created patch , used "hidden" attribute to hide
Attachment #8550386 - Flags: review?(MattN+bmo)
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.
Assignee: nobody → imnmfotmal
Status: NEW → ASSIGNED
Summary: The gear menu separator isn't hidden when loop.fxa.enabled is false → Fix hiding of FxA UI when loop.fxa.enabled is false
Attachment #8550386 - Attachment is obsolete: true
Attachment #8550386 - Flags: review?(MattN+bmo)
Attached patch patch_v1.patch (obsolete) — Splinter Review
Attachment #8553734 - Flags: review+
Attached patch patch_v2.patch (obsolete) — Splinter Review
added patch
Attachment #8553734 - Attachment is obsolete: true
Attachment #8553738 - Flags: review?(MattN+bmo)
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 "<")
Attachment #8553738 - Flags: review?(MattN+bmo) → feedback+
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.
(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.
Attached patch loopfxa_v3.patch (obsolete) — Splinter Review
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?
Flags: needinfo?(standard8)
Attachment #8560703 - Flags: review?(MattN+bmo)
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.
Attachment #8560703 - Flags: review?(MattN+bmo) → review+
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()}
Flags: needinfo?(standard8)
Attached patch loopfxa_v4.patch (obsolete) — Splinter Review
created new patch with a test and also incoprating Markk's suggestion
Attachment #8553738 - Attachment is obsolete: true
Attachment #8560703 - Attachment is obsolete: true
Attachment #8564085 - Flags: review?(MattN+bmo)
Attachment #8564085 - Flags: feedback?(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.
Attachment #8564085 - Flags: feedback?(standard8) → feedback+
so should i remove those spaces and submit a new patch ???
Flags: needinfo?(standard8)
Yes please - it makes it easier for the person landing the patch.
Flags: needinfo?(standard8)
Attached patch loopfxa_v4.patchSplinter Review
Attachment #8564085 - Attachment is obsolete: true
Attachment #8564085 - Flags: review?(MattN+bmo)
Attachment #8564979 - Flags: review?(MattN+bmo)
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.
Attachment #8564979 - Flags: review?(MattN+bmo) → review+
Keywords: checkin-needed
Can we please run this through Try to ensure that there are no test failures lurking in the weeds before it lands? :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/58f815545f09
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/58f815545f09
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → mozilla38
Iteration: --- → 38.3 - 23 Feb
You need to log in before you can comment on or make changes to this bug.