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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[No-Jun Park [:njpark]]

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

Comment 1

[No-Jun Park [:njpark]]

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

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Removing fxa bug tree dependency. ggp, feel free to ping me (or sam or jed) if you have questions.

Comment 3

[Sam Penrose]

Attachment: 994934-split-FxAccounts.jsm-signOut.patch

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.

Comment 4

[Sam Penrose]

Comment on attachment 8406464 [details] [diff] [review]
994934-split-FxAccounts.jsm-signOut.patch

Good patch (I hope), but wrong bug.

Comment 5

[Sam Penrose]

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.

Comment 6

[Sam Penrose]

Attachment: 995353-send-object-to-DOM.patch

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

Comment 7

[Sam Penrose]

Attachment: 995353-send-object-to-DOM.patch

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?

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :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"?

Comment 9

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(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

Comment 10

[Sam Penrose]

Jared, that's a great idea. +1 on building blocks.

Comment 11

[Sam Penrose]

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.

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Shifting from Gaia blocker subtree to Gecko blocker subtree :beers:

Comment 13

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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.

Comment 14

[Jed Parsons (use needinfo, please) [:jedp, :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.

Comment 15

[Sam Penrose]

Attachment: 995353-fix-DOM-to-content-onerror.patch

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.

Comment 16

[Jed Parsons (use needinfo, please) [:jedp, :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.

Comment 17

[Sam Penrose]

Attachment: 995353-unified.patch

Thanks Jed, and sorry for forgetting the .name suggestion. I fixed that and unified the patches for checkin.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/38be37271c25

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38be37271c25