1374501 - Change fxa webchannel handshake and enable sync and credit-card engines if necessary

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

The FxA signup step will start to offer addresses (and at some stage credit-cards) to new sync accounts on desktop. In bug 1374500 we are adding these new engines, which will be disabled by default. This bug is to detect when those engines were offered but not declined and to enable the relevant engines.

The FxA issue to support this is at https://github.com/mozilla/fxa-content-server/issues/5087

Comment 1

[Shane Tomlinson [:stomlinson]]

WIP content-server code PR at https://github.com/mozilla/fxa-content-server/pull/5158.

:markh, :eoger - I'm copying Mark's comments over from the content server issue that I used as a basis for the implementation in PR 5158.

> So as a straw-man: desktop asserts it supports the "addresses" and "creditcards" capabilities - all 
> other engines remain implied.
> When FxA sends the "login" message, it should also list the engines it offered.

> When FxA sends a "fxaccounts:fxa_status" message, desktop currently replies with 
> {signedInUser: {email: "...", ...}. We extend that so it also includes a top-level "capabilities" 
> object, which looks something like, say, {capabilities: {engines: ["creditcards", "addresses"]}}. 
> As mentioned above, other engines remain implied.

> If the client supplied capabilities["engines"], the "fxaccounts:login" message sent back to the client 
> will include, say "enginesOffered" - in this case it would just return exactly what was passed in 
> capabilities. This is the signal to desktop that the engines were definitely offered, so it can be 
> assumed that the user left them selected if they don't appear in declined. 
> All other engines remain implied here too.


> I was wrong about the fact we can hard-code everything - the entire CC feature has some risk of not making 56. Addresses seems fine - can we maybe hard-code that to get the ball rolling and handle CC later?

I expect the response to fxaccounts:fxa_status to be of the form:

>  capabilities: {
>    engines: ['creditcards', 'addresses']
>  },
>  signedInUser: null


I'll send back an `enginesOffered` array in the `fxaccounts:login` message, of the same format as `engines`.

Comment 2

[Shane Tomlinson [:stomlinson]]

> I'll send back an `enginesOffered` array in the `fxaccounts:login` message, of the same format as `engines`.

:mhammond, :eoger - we already send a `declinedSyncEngines` field. Instead of sending `enginesOffered`, I'd like
to send `displayedSyncEngines`, the name is a bit clearer, and it's consistent with what already exists. Does that work for you?

Comment 3

[Mark Hammond [:markh] [:mhammond]]

I thought "offered" was a nice match for the existing "declined", but I'm not bothered by the spelling :)

Comment 4

[Mark Hammond [:markh] [:mhammond]]

I'd also be fine with "accepted" (which would be a semantic change - ie, accepted + declined would be the full set) - but whatever...

Comment 5

[Shane Tomlinson [:stomlinson]]

Thanks markh,

offeredSyncEngines WFM, my primary concern is to re-use the `SyncEngines` suffix. 

Would it be painful if I sent the full set of engines that are offered instead of only the subset that is conditional, e.g.:
> offeredSyncEngines: ['addons', 'tabs', 'bookmarks', 'passwords', 'history', 'prefs', 'addresses']

Comment 6

[Edouard Oger [:eoger]]

Attachment: Bug 1374501 - Allow extra sync engines to be activited using FxA Web Channels.

Review commit: https://reviewboard.mozilla.org/r/151380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151380/

Comment 7

[Edouard Oger [:eoger]]

> Would it be painful if I sent the full set of engines that are offered instead of only the subset that is conditional

As far as my implementation goes, it doesn't matter.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8880055 [details]
Bug 1374501 - Allow extra sync engines to be activited using FxA Web Channels.

https://reviewboard.mozilla.org/r/151380/#review156508

Looks great! Please ensure Shane signs off on how you are communicating those "capabilities" (I think it's what we agreed in the bug, but an explicit OK from him is worthwhile)

::: services/fxaccounts/FxAccountsWebChannel.jsm:41
(Diff revision 1)
>  const COMMAND_CHANGE_PASSWORD      = "fxaccounts:change_password";
>  const COMMAND_FXA_STATUS           = "fxaccounts:fxa_status";
>  
>  const PREF_LAST_FXA_USER           = "identity.fxaccounts.lastSignedInUserHash";
>  
> +const EXTRA_ENGINES = ["addresses", "creditcards"];

I think a brief comment for what's special about these engines would be nice.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Tomlinson [:stomlinson] from comment #5)
> Would it be painful if I sent the full set of engines that are offered
> instead of only the subset that is conditional, e.g.:
> > offeredSyncEngines: ['addons', 'tabs', 'bookmarks', 'passwords', 'history', 'prefs', 'addresses']

Sounds great!

Comment 10

[Shane Tomlinson [:stomlinson]]

Comment on attachment 8880055 [details]
Bug 1374501 - Allow extra sync engines to be activited using FxA Web Channels.

https://reviewboard.mozilla.org/r/151380/#review156670

r+ Works great for me when testing locally! If I set services.sync.engine.creditcards.available to true, the `creditcards` engine is displayed in CWTS.

Comment 11

[Edouard Oger [:eoger]]

Comment on attachment 8880055 [details]
Bug 1374501 - Allow extra sync engines to be activited using FxA Web Channels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151380/diff/1-2/

Comment 12

[Edouard Oger [:eoger]]

Thank, should I land this or wait a little bit?

Comment 13

[Mark Hammond [:markh] [:mhammond]]

I think we might as well wait a week or so, unless Shane would like it committed (for testing etc reasons), in which case I'd be fine with it landing now.

Comment 14

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #13)
> I think we might as well wait a week or so, unless Shane would like it
> committed (for testing etc reasons), in which case I'd be fine with it
> landing now.

I've already applied the patch locally, so no rush.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a0365361a9f
Allow extra sync engines to be activited using FxA Web Channels. r=markh,stomlinson

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a0365361a9f