684420 - Make Service.{create|check}Account asynchronous and use SyncStorageRequest

Status: RESOLVED INVALID

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

We're reworking the setup wizard in bug 675826. While we're at it, we should also kill the use of Resource in Service.createAccount + Service.checkAccount and make them asynchronous methods.

Comment 1

[Allison Naaktgeboren :ally]

this is currently desktop only, since mobile doesnt currently create accounts

Comment 2

[Philipp von Weitershausen [:philikon]]

Note: RESTRequest is just a bare-bones request and would be a regression from Resource in at least the following ways:

* standard User-Agent instead of Sync's special one
* no timeout (Resource times out after 5 minutes)
* no X-Weave-Backoff processing (officially not part of the reg/user API spec, but being considered, and already supported by Resource)

So, we could either use SyncStorageRequest. It does a bit more than just the points above (e.g. the authentication header), but it should degrade gracefully for the reg/user API. (If I'm missing something and that's somehow not the case, we then want to introduce a SyncRegRequest subclass of RESTRequest along the lines of SyncStorageRequest, but geared towards the reg/user API.)

Comment 3

[Allison Naaktgeboren :ally]

this has been sent to the back burner for now. In the case that this is shifted over to someone else at some point, braindumping here:

What needs to happen:
  - the Resource object instead  createAccount()/checkAccount() should be changed from a synchrous resource into a RESTRequest.
    - lives in services/sync/modulesx/service.js
    - result of restrequest is accessed inside the callback
  - checkAccount/createAccount's function signature must change to
    take in a callback function.
  - all code which calls the checkAccount/createAccount must become async.
    - includes tests (mochi & possibly tps)
    - syncSetup.js (impacts onWizardAdance!)
      - note that onWizardAdvance will have to manually moved by all
        advancing code instead of wizard magic due to the async)
      - onWizardADvance should probably be renamed at some point to
        reflect that the movement must be manual now
  - all code which calls that code..
    - so on and so forth, unwinding the stack
Impact:
  - just about every function inside of syncSetup.js will have to be
    altered
  - may not be able to easily land in stages.
  - lot of test impacted (unconfirmed, but grep turns up a lot of calls)
  - be sure to get extra QA scrutiny. async bugs are a right royal pain.
Possibly useful resources:
  - services-central/services/sync/modules/rest.js <-- great comments at
    the top walking through basic invokation
  - services-central/services/sync/tests/unit/test_restrequest.js
  - services-central/services/sync/modules/jpakeclient.js#335
  - https://github.com/mozilla-services/services-central/commit/23b26ed0bbb5e9cf61a4d3cb1bc2e19241bf04b5#diff-0
  - rnewman

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to :Ally Naaktgeboren from comment #3)
> What needs to happen:
>   - the Resource object instead  createAccount()/checkAccount() should be
> changed from a synchrous resource into a RESTRequest.

Almost. Please see my earlier comment 2.

>     - lives in services/sync/modulesx/service.js
>     - result of restrequest is accessed inside the callback
>   - checkAccount/createAccount's function signature must change to
>     take in a callback function.
>   - all code which calls the checkAccount/createAccount must become async.
>     - includes tests (mochi & possibly tps)
>     - syncSetup.js (impacts onWizardAdance!)
>       - note that onWizardAdvance will have to manually moved by all
>         advancing code instead of wizard magic due to the async)
>       - onWizardADvance should probably be renamed at some point to
>         reflect that the movement must be manual now
>   - all code which calls that code..
>     - so on and so forth, unwinding the stack
> Impact:
>   - just about every function inside of syncSetup.js will have to be
>     altered

I highly doubt that. It will affect some of the input field verification logic, but that's essentially already async since it happens when the user stops typing for 1 second. Yes, onWizardAdvance is affected, but only for the NEW_ACCOUNT_START_PAGE case that's the last page of the wizard in that flow anyway! So it could easily just return false and then do the verification + account creation in an async return. It really wouldn't have to have that much impact on the rest.

Comment 5

[Richard Newman [:rnewman]]

We no longer use the wizard, and FxA is in charge of account creation.