Closed Bug 995353 Opened 10 years ago Closed 10 years ago

[FindMyDevice] When Account is created but not verified, Find My Device shows Signin/Create Account button that cannot be used

Categories

(Firefox OS Graveyard :: FindMyDevice, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S1 (9may)

People

(Reporter: njpark, Assigned: spenrose)

References

Details

Attachments

(1 file, 4 obsolete files)

STR:
- In Settings -> Firefox accounts, create a new FxAccount, but do not verify the email yet
- Go to Find my device

Expected:
- It should have some message about waiting for account verification
Actual:
- It shows a button to sign in / create firefox account, but pressing it does not do anything.

Version Info:
Gaia      9d0b1bdf746823a94b13e6574c1d8304dc584763
Gecko     https://hg.mozilla.org/mozilla-central/rev/690c810c8e3e
BuildID   20140410040201
Version   31.0a1
Jared Says:
those prefs are present in the past release, so I assume they will be included in this one too

note also, switching to fxa from persona on native device might mean that some of those prefs aren't needed
 for instance, if they use the fxa iac helper, as i have done in the settings app, it probably will work anyway since find my device is certified
Flags: needinfo?(jparsons)
Flags: needinfo?(jparsons)
Removing fxa bug tree dependency. ggp, feel free to ping me (or sam or jed) if you have questions.
No longer blocks: 974121
Blocks: 987416
Assignee: nobody → spenrose
Hey Jed --

Tests pass. I need to reindent the if (!localOnly) block. As an alternative to localOnly, I could make signOutLocal a public method; didn't work on my first try but presumably could be done.
Attachment #8406464 - Flags: feedback?(jparsons)
Comment on attachment 8406464 [details] [diff] [review]
994934-split-FxAccounts.jsm-signOut.patch

Good patch (I hope), but wrong bug.
Attachment #8406464 - Attachment is obsolete: true
Attachment #8406464 - Flags: feedback?(jparsons)
I believe we have these issues:

    1) FMD needs to add onerror (and oncancel) callbacks to its mozId.watch() call.
      - Guilherme, could you please create a bug for this and have this bug as its blocker?
      - oncancel can be a no-op as FMD currently does the right thing, but you should implement it as a matter of principle.

    2) mozId is not currently giving onerror() anything useful. This is because DOMIdentity.js/RPWatchContext.doError is sending a message that is getting garbled. Looks like a missing JSON.stringify in or just before the message manager.
      - I'll take this one, which is in effect this bug.

    3) Once 2) is done, onerror() should sniff the "UNVERIFIED_ACCOUNT" message and provide copy to the effect of "Please verify your Firefox Account; see Firefox Accounts in Settings for more options and information."

    4) We should have product and UX mull over options such as having FMD jump over to the correct screen 
in Settings and jump back auto-magically; that screen might need to become part of the system app. This is unlikely to happen for 2.0. The general issue is that we have separation of concern issues for FxA UX.
Flags: needinfo?(ggoncalves)
Flags: needinfo?(ggoncalves)
Attached patch 995353-send-object-to-DOM.patch (obsolete) — Splinter Review
I'm pretty confident this is a simple fix: without it, nsDOMIdentity gets messages of the form "[Object object]" (a string); with it, it gets a proper JSON blob that looks like this:

 {"error":"UNVERIFIED_ACCOUNT",
  "details":{"user":
             {"accountId":"<email_address>",
              "verified":false}}}

See also my next comment
Attachment #8409172 - Flags: review?(jparsons)
Attached patch 995353-send-object-to-DOM.patch (obsolete) — Splinter Review
So my comment turned into a patch as I figured out what was going on. Passing the object received by nsDOMIdentity as msg.message here:

https://github.com/mozilla/gecko-dev/blob/master/dom/identity/nsDOMIdentity.js#L527

results in an empty object being received by the RP's onerror function. In this patch, I stringify it first. There are issues. Maybe I should just convert whatever funky nonsense DOMIdentity sends over the wall into a plain JS object. Alternately, if stringifying and parsing is the right move at the RP level, we probably need to fix it elsewhere. Thoughts?
Attachment #8409193 - Flags: feedback?(jparsons)
Comment on attachment 8409193 [details] [diff] [review]
995353-send-object-to-DOM.patch

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

::: dom/identity/nsDOMIdentity.js
@@ +523,5 @@
>            return;
>          }
>  
>          if (this._rpWatcher.onerror) {
> +          this._rpWatcher.onerror(JSON.stringify(msg.message));

Returning a string that requires JSON.parsing seems kind of clunky.

What if, instead of a string or json object, we were to return an Error object whose .name could be like "UNVERIFIED_ACCOUNT" and whose .message could be something like "<email address> is unverified"?
Attachment #8409193 - Flags: feedback?(jparsons)
(In reply to Sam Penrose from comment #5)
> 
>     4) We should have product and UX mull over options such as having FMD
> jump over to the correct screen 
> in Settings and jump back auto-magically; that screen might need to become
> part of the system app. This is unlikely to happen for 2.0. The general
> issue is that we have separation of concern issues for FxA UX.

Once we know what the RP patterns are, we could open a bug to add FxA code to the building blocks[1], so that we have an easy way for other RPs to integrate. Does this seem useful? Or maybe the better way is to ask RPs to lean on a test app as an example?

[1] http://buildingfirefoxos.com/building-blocks
Jared, that's a great idea. +1 on building blocks.
Jed:

1) When you have a chance, could you review the attachment from comment #6. I faked you out by giving the second patch the same filename; my apologies.

2) It appears that nsDOMIdentity.js can only send strings to web content callbacks. If at line 527 I do one of these:

          this._rpWatcher.onerror({error: msg.message.error});
          this._rpWatcher.onerror({error: "msg.message.error"});

the result as logged in UITest/js/API/fxa.js is an empty object. But this works:

          this._rpWatcher.onerror(JSON.stringify({error: msg.message.error}));

I note also that wherever nsDOMIdentity returns an assertion, it does so as a string (see the unpackAssertion utility in the UITest modules you wrote). Returning JSON-in-a-string works. Can you think of an alternative that also works, or tell me what I'm doing wrong?

3) With regard to .message, one challenge we have is centralizing strings for localization. I am hoping that whatever solution we come up with here plays nicely with existing constants files and the like. I am not sure what nsDOMIdentity can access, or ask RPs to access.

Thanks as always.
Flags: needinfo?(jparsons)
Shifting from Gaia blocker subtree to Gecko blocker subtree :beers:
Blocks: 989363
No longer blocks: 987416
Ah, ok.  I had forgotten that we could only send strings.  I apologize for the wild goose chase.

Yes, a string that can be JSON parsed sounds fine, then.  We could do something like {name: SOME_REASON, error: "Human-readable explanation"}.

I believe we do not ship localizations in gecko, so we won't be able to do anything nice for non-Enlglish-speaking developers here, which is a pity, but there it is.
Flags: needinfo?(jparsons)
Comment on attachment 8409172 [details] [diff] [review]
995353-send-object-to-DOM.patch

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

Looks good.  Thanks for bearing with me for having sent you down the wrong path.  My apologies.
Attachment #8409172 - Flags: review?(jparsons) → review+
Thanks for the review on the toolkit tweak. I still have no good answer for how to manage user-facing strings in the mozId API, so I'm going to punt and just send:

  {error: "UNVERIFIED_ACCOUNT"}

back to the RP for now, since FMD and MP are going to have to roll their own UX to handle it anyway. For 2.1 we should have an RP-friendlier experience IMHO. Alternately, I could send the whole blob through, but it raises security concerns in my mind -- we may not want every RP to have all the values returned by any arbitrary service/fxaccounts error message. Let me know what you think.
Attachment #8409193 - Attachment is obsolete: true
Attachment #8411358 - Flags: review?(jparsons)
Comment on attachment 8411358 [details] [diff] [review]
995353-fix-DOM-to-content-onerror.patch

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

That looks good to me.  Thanks, Sam.

Wondering whether the property should be called something other than 'error', just because the RP will probably end up querying error.error.  Maybe .name?  I don't really care, but I often find myself getting mixed up with things like message.message and error.error.
Attachment #8411358 - Flags: review?(jparsons) → review+
Thanks Jed, and sorry for forgetting the .name suggestion. I fixed that and unified the patches for checkin.
Attachment #8409172 - Attachment is obsolete: true
Attachment #8411358 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/38be37271c25
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
You need to log in before you can comment on or make changes to this bug.