Closed
Bug 1047181
Opened 10 years ago
Closed 10 years ago
Change the Loop toolbar button when FxA sign in completes
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox34 verified, firefox35 verified)
People
(Reporter: MattN, Assigned: jaws)
References
()
Details
(Whiteboard: [p=1][loop-uplift][fig:wontverify])
User Story
As a user, I'd like to know that the FxA log in flow that I completed in a tab successfully logged me into Loop.
Attachments
(2 files, 6 obsolete files)
14.29 KB,
patch
|
MattN
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.27 KB,
patch
|
MattN
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Toolbar design is at https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#toolbar
Flags: firefox-backlog?
Comment 1•10 years ago
|
||
Is that a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1028894?
Reporter | ||
Comment 2•10 years ago
|
||
This is a subset of that which is specific to FxA integration so I will make it block that.
Blocks: 1028894
Reporter | ||
Comment 3•10 years ago
|
||
I mean that this bug is specific to FxA integration. I don't think it makes sense to implement all of different icon changes in the one bug since they are from different areas of code.
Updated•10 years ago
|
Target Milestone: --- → mozilla35
Updated•10 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Comment 6•10 years ago
|
||
After talking with Mike, I think we want this for Fx34, if possible.
Priority: -- → P3
Target Milestone: mozilla35 → mozilla34
Updated•10 years ago
|
Whiteboard: [qa+] → [qa+, p=1]
Comment 7•10 years ago
|
||
Darrin, Do we need any strings for this bug? I'm thinking specifically about tooltips. Or can we go just with icons (especially for Fx34)?
Flags: needinfo?(dhenein)
Comment 8•10 years ago
|
||
I think just the icon state is enough for 34, especially given that this will likely occur in short-relation to the user's action of signing in (the association should be there).
Flags: needinfo?(dhenein)
Reporter | ||
Updated•10 years ago
|
Points: --- → 2
Flags: qe-verify+
Whiteboard: [qa+, p=1] → [p=1]
Updated•10 years ago
|
Priority: P3 → P1
Whiteboard: [p=1] → [p=1][loop-uplift]
Target Milestone: mozilla34 → mozilla35
Updated•10 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 35.1
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8488230 [details] [diff] [review] Patch Review of attachment 8488230 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-loop.js @@ +78,5 @@ > state = "error"; > } else if (MozLoopService.doNotDisturb) { > state = "disabled"; > + } else if (MozLoopService.userProfile) { > + state = "active"; I think opening the panel should clear the "active" state so I think you have more to do here.
Attachment #8488230 -
Flags: review?(MattN+bmo) → review-
![]() |
||
Updated•10 years ago
|
QA Contact: anthony.s.hughes
Updated•10 years ago
|
Iteration: 35.1 → 35.2
Assignee | ||
Comment 11•10 years ago
|
||
I added a new "authenticated" state so that we could remove it when the panel opened, but not remove the "active" state if the panel is opened during a call.
Attachment #8488230 -
Attachment is obsolete: true
Attachment #8490271 -
Flags: review?(MattN+bmo)
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8490271 [details] [diff] [review] Patch v1.1 Review of attachment 8490271 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-loop.js @@ +32,5 @@ > }, true); > }; > > + if (this.toolbarButton.node.getAttribute("state") == "authenticated") { > + this.toolbarButton.node.setAttribute("state", ""); This shouldn't assume "" and should call updateToolbarState (after that function changes). This also doesn't work with multiple windows. It will only clear the state in one. @@ +82,5 @@ > state = "error"; > } else if (MozLoopService.doNotDisturb) { > state = "disabled"; > + } else if (MozLoopService.userProfile) { > + state = "authenticated"; This isn't going to work if we get an error (error state applies) and then we clear the error OR after a call completes (when that's implemented). I think we could pass info with notification and then to this function (via an optional argument) that indicates it's the result of a login. If reason = "login" (or whatever) then we set to active, otherwise we go down the existing if/else statements. That way we will never fallback to the login state later when .userProfile is truthy. e.g.: updateToolbarState: function(reason = null) { To clear the state in all windows, perhaps we would need another reason related to either the first panel open after a login (would require storing state in the JSM though which is not ideal) or just have a notification with a reason whenever the panel opens and in most cases we would do nothing in updateToolbarState for that reason but we would clear the authenticated state if it was the current one. Please add tests for the above issues: primarily so we never default back to "authenticated" after the panel opens. ::: browser/components/loop/test/mochitest/head.js @@ +48,5 @@ > loopPanel.hidePopup(); > + let frameId = btn.getAttribute("notificationFrameId"); > + let frame = document.getElementById(frameId); > + if (frame) { > + loopPanel.removeChild(frame); Is this related to some intermittent failure?
Attachment #8490271 -
Flags: review?(MattN+bmo) → review-
Assignee | ||
Comment 13•10 years ago
|
||
Updated based on review comments. (In reply to Matthew N. [:MattN] from comment #12) > ::: browser/components/loop/test/mochitest/head.js > @@ +48,5 @@ > > loopPanel.hidePopup(); > > + let frameId = btn.getAttribute("notificationFrameId"); > > + let frame = document.getElementById(frameId); > > + if (frame) { > > + loopPanel.removeChild(frame); > > Is this related to some intermittent failure? Yes, if promiseGetMozLoopAPI is called multiple times within a test (such as making multiple calls to loadLoopPanel, then multiple cleanup functions will be registered and only the first one would succeed.
Attachment #8490271 -
Attachment is obsolete: true
Attachment #8490942 -
Flags: review?(MattN+bmo)
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8490942 [details] [diff] [review] Patch v1.4 Review of attachment 8490942 [details] [diff] [review]: ----------------------------------------------------------------- Please add tests to browser/components/loop/test/mochitest/browser_toolbarbutton.js for the cases where we need to fallback to a non-default option (e.g. DND) after opening the panel to clear the login state. You should be able to reach in and manipulate gFxAOAuthProfile for this to avoid the actual login overhead. ::: browser/base/content/browser-loop.js @@ +79,5 @@ > + * > + * @param {string} aReason (optional) Some states are only > + * shown if a related reason is provided. > + */ > + updateToolbarState: function(aReason) { Please use default argument syntax to indicate that an argument in optional: e.g. aReason = null In JSDoc syntax, you would put the argument name in square brackets: [aReason] @@ +86,5 @@ > state = "error"; > } else if (MozLoopService.doNotDisturb) { > state = "disabled"; > + } else if (aReason == "login" && MozLoopService.userProfile) { > + state = "authenticated"; I was thinking this would have precedence (at least over things like DND but perhaps errors should be first). Consider someone with DND on and then logs in. @@ +89,5 @@ > + } else if (aReason == "login" && MozLoopService.userProfile) { > + state = "authenticated"; > + } else if (aReason == "panelshown" && > + this.toolbarButton.node.getAttribute("state") == "authenticated") { > + state = ""; Since this is in the last |else if| and the default value of state is "", this isn't doing anything AFAICT. Is that intentional so people can follow the flow? If so, I think a comment about why you're doing nothing would be clearer instead of setting |state = ""| again. I think setting it to "" might lead others to believe the state should always be "" when the conditions are met when the state should actually default to the normal calculations (which could, for example, lead to the "disabled" state for DND). I'm wondering if this if block can just be removed? Is the getAttribute check supposed to be doing something?
Attachment #8490942 -
Flags: review?(MattN+bmo) → feedback+
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8490942 -
Attachment is obsolete: true
Attachment #8491718 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8491718 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 16•10 years ago
|
||
No need to keep the "authenticated" state value since it was only needed in earlier versions of this patch to know that it could be removed. The aReason parameter to updateToolbarState obsoleted it.
Attachment #8491718 -
Attachment is obsolete: true
Attachment #8491738 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8491738 -
Attachment is obsolete: true
Attachment #8491738 -
Flags: review?(MattN+bmo)
Attachment #8491742 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8491742 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8491742 -
Attachment is obsolete: true
Attachment #8491746 -
Flags: review?(MattN+bmo)
Reporter | ||
Comment 19•10 years ago
|
||
Comment on attachment 8491746 [details] [diff] [review] Patch v1.6 Review of attachment 8491746 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-loop.js @@ +78,5 @@ > + /** > + * Updates the toolbar/menu-button state to reflect Loop status. > + * > + * @param {string} [aReason] (optional) Some states are only > + * shown if a related reason is provided. You can remove "(optional)" now and then wrap this so that the second line aligns with the start of the argument description (starting with "Some") like is more commonly done. ::: browser/components/loop/test/mochitest/browser_toolbarbutton.js @@ +30,5 @@ > + Assert.strictEqual(LoopUI.toolbarButton.node.getAttribute("state"), "", "Check button is in default state after opening panel"); > + let loopPanel = document.getElementById("loop-notification-panel"); > + loopPanel.hidePopup(); > + yield MozLoopService.doNotDisturb = true; > + Assert.strictEqual(LoopUI.toolbarButton.node.getAttribute("state"), "disabled", "Check button is in disabled state"); This isn't testing the case I mentioned where DND is set *before* login so the "active" state from login replaces the "disabled" state from DND. I think you can probably change this test to test that case as I'm not sure this current function is testing more than test_active and test_doNotDisturb do together.
Attachment #8491746 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5710731f09e9
Flags: in-testsuite+
Whiteboard: [p=1][loop-uplift] → [p=1][loop-uplift][fixed in fx-team]
I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/10a23714dd6a for browser-chrome-1 failures: Non-e10s was failing with https://tbpl.mozilla.org/php/getParsedLog.php?id=48406490&tree=Fx-Team#error0 e10s was failing with https://tbpl.mozilla.org/php/getParsedLog.php?id=48404430&tree=Fx-Team
Flags: needinfo?(jaws)
Whiteboard: [p=1][loop-uplift][fixed in fx-team] → [p=1][loop-uplift]
Assignee | ||
Comment 22•10 years ago
|
||
This fixes the test failure by caching the state of the offline flag once at the beginning of tests instead of each time a helper function is called.
Attachment #8491855 -
Flags: review?(MattN+bmo)
Flags: needinfo?(jaws)
Reporter | ||
Comment 23•10 years ago
|
||
Comment on attachment 8491855 [details] [diff] [review] 1047181-followup.patch Review of attachment 8491855 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/test/mochitest/head.js @@ +6,5 @@ > LOOP_SESSION_TYPE, > MozLoopServiceInternal, > } = Cu.import("resource:///modules/loop/MozLoopService.jsm", {}); > > +// Cache this value only once, at at the beginning of a s/at at/at/
Attachment #8491855 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 24•10 years ago
|
||
Relanded with typo fix and both patches folded, https://hg.mozilla.org/integration/fx-team/rev/f9a9f246b0d8
Whiteboard: [p=1][loop-uplift] → [p=1][loop-uplift][fixed in fx-team]
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f9a9f246b0d8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [p=1][loop-uplift][fixed in fx-team] → [p=1][loop-uplift]
Comment 26•10 years ago
|
||
Paul, please verify this in the latest Nightly and the Try-server build here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rjesup@wgate.com-f9eb2cbac352
Flags: needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes → paul.silaghi
Whiteboard: [p=1][loop-uplift] → [p=1][loop-uplift][fig:verifyme]
Comment 27•10 years ago
|
||
Upon further review of the issues flagged for pre-uplift verification, I do not think this needs verification before we uplift. Paul, please still verify this in the latest Nightly but skip verification in the Fig branch.
Whiteboard: [p=1][loop-uplift][fig:verifyme] → [p=1][loop-uplift][fig:wontverify]
Comment 28•10 years ago
|
||
After sign in, the Loop button color changes to blue. It gets back to the normal gray after click. 35.0a1 (2014-10-01) Please reopen if the blue color should persist.
Comment 29•10 years ago
|
||
(In reply to Paul Silaghi, QA [:pauly] from comment #28) > After sign in, the Loop button color changes to blue. It gets back to the > normal gray after click. > 35.0a1 (2014-10-01) > Please reopen if the blue color should persist. Yeah, that's what I see too. I personally think this state should persist as long as a user is authenticated. However, perhaps that's out of scope for the original intent of this bug.
Flags: needinfo?(MattN+bmo)
Reporter | ||
Comment 30•10 years ago
|
||
It's working as intended. What you're saying would make sense if the toolbar button change for login was different than the color used for other states such as an incoming call but since they're the same clearing make the most sense IMO.
Flags: needinfo?(MattN+bmo)
Comment 31•10 years ago
|
||
Comment on attachment 8491746 [details] [diff] [review] Patch v1.6 Approval Request Comment Uplift request for patches staged and tested on Fig
Attachment #8491746 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8491855 -
Flags: approval-mozilla-aurora?
Comment 32•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/13a9118e44e4 https://hg.mozilla.org/releases/mozilla-aurora/rev/340da2f01cc2
Updated•10 years ago
|
status-firefox34:
--- → fixed
Comment 33•10 years ago
|
||
Comment on attachment 8491746 [details] [diff] [review] Patch v1.6 I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.
Attachment #8491746 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8491855 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 34•10 years ago
|
||
I've seen this in Aurora testing over the last few days. Marking this verified fixed for Firefox 34.
You need to log in
before you can comment on or make changes to this bug.
Description
•