988394 - User registration flow

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Loop, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

An user needs to be registered with Loop's server to be able to make calls, generate call URLs and receive call requests. For now, we just need to implement the basic registration UI and register a push endpoint. Loop's server authenticates the user with session cookies so far but in the future we will also need to do the Firefox Accounts and/or MSISDN registration flows.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Go for it.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Back to jaoo :)

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

The WIP patch adds an object and a handler that will be used to implement the registration flow. Still need to add tests and a simple UI as PoC.

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

First working version. It includes a basic UI that allows the user to signup/signin/logout. Next is to add unit tests but at this point and before moving forward it would be great to have some feedback. Fernando/Borja would you mind to take a quick look at the patch please? Thanks!

PS. This patch has been developed on top of the patch from bug 988396.

Comment 5

[Borja Salguero [:borjasalguero]]

Comment on attachment 8419479 [details] [diff] [review]
WIP

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

The main issue is to split the code, having a well-defined separation between the parts of the code. 'Controller' is the one who knows about the rest, but UI does not anything about the rest of the JS helpers. Keeping this will let is to change the UI completely without modifying any line of the rest of the Helpers, or reuse them in other apps.

Please take a look and ask to review again when ready! Thanks!

::: js/account.js
@@ +24,5 @@
> +     * @param {String} id Peer's id.
> +     * 
> +     * @return {Object} Alias object.
> +     */
> +    parse: function a_parse(id) {

Are we going to use this function outside? If not we should move it out of the prototype.

@@ +32,5 @@
> +       * @param {String} id The id to check.
> +       *
> +       * @return {Boolean} Result.
> +       */
> +      function _isPhoneNumberValid(id) {

Probably we should expose all of these functions to an Utils, Wdyt? I dont know it these 'checks' could be used outside actually...

@@ +121,5 @@
> +   * @param {String} id Peer's id.
> +   * 
> +   * @return {Account} New account object.
> +   */
> +  Account.create = function a_create(id) {

Why all these functions are outside the prototype?

@@ +135,5 @@
> +   * @param {Function} success A callback invoked when the transaction
> +   *                           completes successfully.
> +   * @param {Function} error A callback invoked if an operation fails.
> +   */
> +  Account.load = function a_load(onsuccess, onerror) {

Same

@@ +143,5 @@
> +        if (!alias) {
> +          onsuccess(null);
> +          return;
> +        }
> +        console.log('Loaded ' + JSON.stringify(alias));

Debug better! :)

::: js/account_helper.js
@@ +50,5 @@
> +       function onRegistered(error, endpoint) {
> +         if (error) {
> +           _callback(onerror, [error]);
> +         }
> +         // Register the peer.

We need a SimplePush.start right? And actually, this should be part of the 'onsuccess' method

@@ +75,5 @@
> +      Account.load(function(account) {
> +        if (!account) {
> +          _callback(onerror, [new Error('Unable to sign in. Sing up first')])
> +        }
> +        SimplePush.createChannel(

Same. This should be part of the 'onsuccess'

@@ +90,5 @@
> +
> +    /**
> +     * Log the user out. It clears the app account.
> +     */
> +    logOut: function logOut() {

We should receive a 'onsuccess/onerror' as well

::: js/ui.js
@@ +27,3 @@
>        shareUrlButton.addEventListener('click', this.onShareUrl);
> +
> +      Controller.getAccount(

Actually this is not supposed to be here! This must be part of the 'startup' or 'Controller'.
'Controller' is the main JS of our code. It is the only one which should know about UI, AccountHelper...
The rest of the JS (ui.js, accounts_helper.js...) should work only with 'callbacks', keeping the consistency and being agnostic about the rest of the functions to be called. They only know about 'callbacks', that's all.

This is the main change of the code, you should move it to 'Controller' instead.

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8419479 [details] [diff] [review]
WIP

Clearing review request at ferjm until I address the comments suggested by Borja. Sorry for the noise.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

Code structure as Borja suggested.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Pointer to Github PR https://github.com/mozilla-b2g/firefoxos-loop-client/pull/7

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8422428 [details] [diff] [review]
WIP

Borja provided feedback off-line so I cancel this request.

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8425575 [details] [review]
Pointer to Github PR https://github.com/mozilla-b2g/firefoxos-loop-client/pull/7

Off-line feedback comment addressed, would you mind to have a look again please? Thanks!

Comment 11

[Borja Salguero [:borjasalguero]]

Comment on attachment 8425575 [details] [review]
Pointer to Github PR https://github.com/mozilla-b2g/firefoxos-loop-client/pull/7

Thanks!!!! Great job and ready to add more code on it! R+ for sure.

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Borja Salguero (this week part-time in Gaia) [:borjasalguero] from comment #11)
> Comment on attachment 8425575 [details] [review]
> Pointer to Github PR
> https://github.com/mozilla-b2g/firefoxos-loop-client/pull/7
> 
> Thanks!!!! Great job and ready to add more code on it! R+ for sure.

Thanks for the review and for your help.

Landing patch at https://github.com/mozilla-b2g/firefoxos-loop-client/commit/be1d88763a3a66176337eae493d9f7b0c7ed347c