1190978 - Add Sign in and Sign Up links to the panel

Status: RESOLVED INCOMPLETE

(Hello (Loop) :: Client, defect, P2)

Description

[Andrei Oprea [:andreio]]

Add Sign In and Sign Up as two different links in the panel footer.

Comment 1

[Mark Banner (:standard8)]

Andrei, can you list the problems that you were having in the other bug, that caused this to be split out please?

Comment 2

[Ed Lee :Mardak]

See Bug 1183636 comment 24 for :flod's suggestion of "signin_or_signup_message=#1 or #2"

Comment 3

[Andrei Oprea [:andreio]]

(In reply to Ed Lee :Mardak from comment #2)
> See Bug 1183636 comment 24 for :flod's suggestion of
> "signin_or_signup_message=#1 or #2"

The issue is not about how to get the links in the right locale it's about not being able to generate a signup link easily.

Comment 4

[Andrei Oprea [:andreio]]

Attachment: Add login and signup links to Loop panel

Comment 5

[Andrei Oprea [:andreio]]

Attachment: Add login and signup links to Loop panel

Comment 6

[Andrei Oprea [:andreio]]

I've added a working patch that shows the changes needed to add two different links.
The reason I've spin this off is 

```
    gFxAOAuthClientPromise = null;
```

If I understood everything correctly: There is a caching system implemented for the FxA promise. If you click on signin and never complete the step it won't go an request the OAuth token it will just resume. However if you want to signup you need a new token and the previous promise has to be invalidated. I guess we could check if you click on the same link twice and not invalidate. Or create a different promise cache for signup but that seems like to much code/time spent and a very small win. I'm sure there are other options.

Comment 7

[Mark Banner (:standard8)]

MattN, could you help us out with comment 6? I'm not sure I understand enough to be able to give a good answer.

Comment 9

[Sevaan Franks [:sevaan]]

We should remove the account sign-in/sign-up links from the product completely until we have some better user value. At the moment user research has shown that the links make users belief that an account is required to use Hello, which it is not.

Also, there is no messaging or any indicators of why sync is useful with Hello. It feels like a clinger-on that we should remove, like the contacts section.

Comment 10

[Fernando Campo (:fcampo)]

(In reply to Sevaan Franks [:sevaan] from comment #9)
> We should remove the account sign-in/sign-up links from the product
> completely until we have some better user value. At the moment user research
> has shown that the links make users belief that an account is required to
> use Hello, which it is not.
> 
> Also, there is no messaging or any indicators of why sync is useful with
> Hello. It feels like a clinger-on that we should remove, like the contacts
> section.

is this the final decision? to remove Sing In / Sign up appearances on the the product?
In that case, I'll rename the bug (when confirmed) and close the dependencies

Comment 11

[Mark Banner (:standard8)]

(In reply to Sevaan Franks [:sevaan] from comment #9)
> Also, there is no messaging or any indicators of why sync is useful with
> Hello. It feels like a clinger-on that we should remove, like the contacts
> section.

I have no idea what this refers to. The reason we are still using *FxA* (!= Firefox sync) is that it allows you to sync your rooms across instances of Firefox.

Comment 12

[Pau Masiá [:Pau]]

(In reply to Fernando Campo (:fcampo) from comment #10)
> 
> is this the final decision? to remove Sing In / Sign up appearances on the
> the product?
> In that case, I'll rename the bug (when confirmed) and close the dependencies

Yes, for now we'll go with anonymous only until we can provide better value to the user.

Comment 13

[Romain Testard [:RT]]

There seems to be some confusion.

The issue was that users feel they are forced to sign-in/up to FxA in order to start using Hello.
We agreed to implement bug 1217412 which makes the sign-in/up link less "in your face" in order to fix the issue although we want to keep the ability to sign-in/up to FxA since it provides value (persistency of rooms created across devices).
Let's keep the FxA integration and just implement bug 1217412 to fix the user perception that signing-in is mandatory. We'll then validate the impact of the change and see if this works better with users.

Comment 14

[Mark Banner (:standard8)]

Note: we should still use this bug to provide the two separate links, rather than the single one we have currently.

Comment 15

[Fernando Campo (:fcampo)]

Attachment: Add Sign in and Sign Up links to the panel

Added style changes and tests to original patch from :andreio as it works nicely.
Still pending on ni? from :MattN in case there's a better approach on solving the problem

As links wording includes a separator 'or', asking for string review/advice on wording to :flod

Comment 16

[Francesco Lodolo [:flod]]

Comment on attachment 8688548 [details] [diff] [review]
Add Sign in and Sign Up links to the panel

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

Not really the right person for advice on wording ;-)

About strings: while the current structure is safe-ish, I would like a more elastic approach (unless it proves to be technically too complex).

panel_footer_signin_link=Sign In
panel_footer_signup_link=Sign Up
panel_footer_signin_signup_message={{panel_footer_signin_link}} or {{panel_footer_signup_link}}

Using a string for the separator means you're concatenating strings, adding arbitrary spaces and excluding the chance of having some text after the links.

Please also add a comment explaining what is going to happen with these strings.

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8688548 [details] [diff] [review]
Add Sign in and Sign Up links to the panel

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

::: browser/components/loop/modules/MozLoopService.jsm
@@ +1699,5 @@
> +        MozLoopServiceInternal.clearError("profile");
> +        return MozLoopServiceInternal.fxAOAuthTokenData;
> +      });
> +    });
> +  },

What about:

```js
signupToFxA: Task.async(function* () {
  gFxAOAuthClientPromise = null;

  let {code, state} = yield MozLoopServiceInternal.promiseFxAOAuthAuthorization(false, "signup");
  let tokenData = yield MozLoopServiceInternal.promiseFxAOAuthToken(code, state);
  MozLoopServiceInternal.fxAOAuthTokenData = tokenData;

  yield MozLoopServiceInternal.promiseRegisteredWithServers(LOOP_SESSION_TYPE.FXA);
  MozLoopServiceInternal.clearError("login");
  MozLoopServiceInternal.clearError("profile");

  return MozLoopServiceInternal.fxAOAuthTokenData;
}),
```

(I'm leaving the correctness of implementation to Matt, just showing a style that requires less curlies)

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8688548 [details] [diff] [review]
Add Sign in and Sign Up links to the panel

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

I think MattN wants to be reviewer here; removing myself.

Comment 19

[Fernando Campo (:fcampo)]

(In reply to Francesco Lodolo [:flod] from comment #16)
> Comment on attachment 8688548 [details] [diff] [review]
> Add Sign in and Sign Up links to the panel
> 
> Review of attachment 8688548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not really the right person for advice on wording ;-)
Yeah, I meant help with the strings for localizers, sorry
> 
> About strings: while the current structure is safe-ish, I would like a more
> elastic approach (unless it proves to be technically too complex).
> 
> panel_footer_signin_link=Sign In
> panel_footer_signup_link=Sign Up
> panel_footer_signin_signup_message={{panel_footer_signin_link}} or
> {{panel_footer_signup_link}}
> 
> Using a string for the separator means you're concatenating strings, adding
> arbitrary spaces and excluding the chance of having some text after the
> links.
Problem with this approach is that we need two different links, and I don't see a proper way of adding the tags in place.
Even if we add them on the localization file (e.g. panel_footer_signup_link=<a>Sign Up</a>) we would need to add a different onclick event for each one of them, and it's not clear with React

If you have any idea on how to do so, I'm all ears :)
> 
> Please also add a comment explaining what is going to happen with these
> strings.
Not sure on what do you mean on 'what is going to happen' to the strings. As in how are they gonna be displayed? Or do you mean the same explanation for the different links that I gave before?

Comment 20

[Francesco Lodolo [:flod]]

(In reply to Fernando Campo (:fcampo) from comment #19)
> Problem with this approach is that we need two different links, and I don't
> see a proper way of adding the tags in place.
> Even if we add them on the localization file (e.g.
> panel_footer_signup_link=<a>Sign Up</a>) we would need to add a different
> onclick event for each one of them, and it's not clear with React

Ah, I only checked the strings and assumed you were using actual links and not onClick events. With URLs you could replace placeholders with code, assuming you can inject HTML into a paragraph element (sadly I don't know anything about React), but the code would look pretty ugly.

Let's stick with the current strings, and consider it a f+ with a comment added.

> Not sure on what do you mean on 'what is going to happen' to the strings. As
> in how are they gonna be displayed? Or do you mean the same explanation for
> the different links that I gave before?
An explanation like "These strings will be concatenated and displayed on a single line as "Sign In or Sign Up". Both "Sign In" and "Sign Up" will be active links.

Comment 21

[Mark Banner (:standard8)]

(In reply to Fernando Campo (:fcampo) from comment #19)
> (In reply to Francesco Lodolo [:flod] from comment #16)
> > Comment on attachment 8688548 [details] [diff] [review]
> > Add Sign in and Sign Up links to the panel
> > 
> > Review of attachment 8688548 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Not really the right person for advice on wording ;-)
> Yeah, I meant help with the strings for localizers, sorry
> > 
> > About strings: while the current structure is safe-ish, I would like a more
> > elastic approach (unless it proves to be technically too complex).
> > 
> > panel_footer_signin_link=Sign In
> > panel_footer_signup_link=Sign Up
> > panel_footer_signin_signup_message={{panel_footer_signin_link}} or
> > {{panel_footer_signup_link}}
> > 
> > Using a string for the separator means you're concatenating strings, adding
> > arbitrary spaces and excluding the chance of having some text after the
> > links.
> Problem with this approach is that we need two different links, and I don't
> see a proper way of adding the tags in place.
> Even if we add them on the localization file (e.g.
> panel_footer_signup_link=<a>Sign Up</a>) we would need to add a different
> onclick event for each one of them, and it's not clear with React
> 
> If you have any idea on how to do so, I'm all ears :)

We've done something similar before with the ToS links:

http://hg.mozilla.org/mozilla-central/annotate/a523d4c7efe2/browser/components/loop/standalone/content/js/standaloneRoomViews.jsx#l26

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8688548 [details] [diff] [review]
Add Sign in and Sign Up links to the panel

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

::: browser/components/loop/modules/MozLoopService.jsm
@@ +1068,2 @@
>      // We must make sure to have only a single client otherwise they will have different states and
>      // multiple channels. This would happen if the user clicks the Login button more than once.

This comment explains why we have gFxAOAuthClientPromise as a global. With this change, running the following after clearing `loop.hawk-session-token.fxa` (like a new profile) will open two tabs with different `state` arguments meaning that only the last tab will work.

LoopUI.MozLoopService.logInToFxA(); LoopUI.MozLoopService.signupToFxA()

In practice this seems hard to do unless the network is slow since the panel closes after the first click so maybe it's not worth worrying about?

I thought this was doing much worse things and creating a new `state` argument in the common case but the server gives us the same `state` each time and FxAccountsOAuthClient simply uses the clientID for the webchannel ID which is more lenient then I expected though I'm not sure if that's a good thing.

I guess I wonder if stale gFxAOAuthClientPromise's will call their callbacks if I open multiple tabs and login in one? I just tested this and it seemed like they did all get called so r- for that (though maybe it was possible to be in that state before too?)

I guess I'm not clear on what the lifetime of a FxAccountsOAuthClient should be… ideally I think we should be using the same one and have the possibility to vary the tab opened after construction time e.g. arguments to launchWebFlow or modifying some/the parameters some other way.

ckarlof/vladikoff, what do you think? Do you understand what is going on or do you want to have a meeting about this with Fernando?

@@ +1692,5 @@
> +      return MozLoopServiceInternal.promiseFxAOAuthToken(response.code, response.state);
> +    }).then(tokenData => {
> +      MozLoopServiceInternal.fxAOAuthTokenData = tokenData;
> +      return tokenData;
> +    }).then(tokenData => {

This method is almost identical to logInToFxA so I would rather we didn't duplicate it. How about factoring the two identical (IIUC) .then functions into one helper used for both logInToFxA and signupToFxA. e.g. _handleSignInSignUp? It may even be worth just keeping one method and adding an extra argument to logInToFxA to indicate whether to pass "signup" to promiseFxAOAuthAuthorization.

Please don't change the style if refactoring it so I can use reviewboard's code move detection to review. You can change the style in a follow-up.

Comment 23

[Chris Karlof [:ckarlof]]

I'm not going to get to this before US Thanksgiving (I'm off Wed). Vlad should be able to handle it if he has time. Don't wait on me, but I can weigh in next week if this is still pending then.

Comment 24

[Vlad Filippov (:vladikoff)]

(In reply to Matthew N. [:MattN] from comment #22)
> Comment on attachment 8688548 [details] [diff] [review]
> Add Sign in and Sign Up links to the panel
> 
> Review of attachment 8688548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/modules/MozLoopService.jsm
> @@ +1068,2 @@
> >      // We must make sure to have only a single client otherwise they will have different states and
> >      // multiple channels. This would happen if the user clicks the Login button more than once.
> 
> This comment explains why we have gFxAOAuthClientPromise as a global. With
> this change, running the following after clearing
> `loop.hawk-session-token.fxa` (like a new profile) will open two tabs with
> different `state` arguments meaning that only the last tab will work.
> 
> LoopUI.MozLoopService.logInToFxA(); LoopUI.MozLoopService.signupToFxA()
> 
> In practice this seems hard to do unless the network is slow since the panel
> closes after the first click so maybe it's not worth worrying about?
> 
> I thought this was doing much worse things and creating a new `state`
> argument in the common case but the server gives us the same `state` each
> time and FxAccountsOAuthClient simply uses the clientID for the webchannel
> ID which is more lenient then I expected though I'm not sure if that's a
> good thing.
> 
> I guess I wonder if stale gFxAOAuthClientPromise's will call their callbacks
> if I open multiple tabs and login in one? I just tested this and it seemed
> like they did all get called so r- for that (though maybe it was possible to
> be in that state before too?)
> 
> I guess I'm not clear on what the lifetime of a FxAccountsOAuthClient should
> be… ideally I think we should be using the same one and have the possibility
> to vary the tab opened after construction time e.g. arguments to
> launchWebFlow or modifying some/the parameters some other way.
> 
> ckarlof/vladikoff, what do you think? Do you understand what is going on or
> do you want to have a meeting about this with Fernando?
> 
> @@ +1692,5 @@
> > +      return MozLoopServiceInternal.promiseFxAOAuthToken(response.code, response.state);
> > +    }).then(tokenData => {
> > +      MozLoopServiceInternal.fxAOAuthTokenData = tokenData;
> > +      return tokenData;
> > +    }).then(tokenData => {
> 
> This method is almost identical to logInToFxA so I would rather we didn't
> duplicate it. How about factoring the two identical (IIUC) .then functions
> into one helper used for both logInToFxA and signupToFxA. e.g.
> _handleSignInSignUp? It may even be worth just keeping one method and adding
> an extra argument to logInToFxA to indicate whether to pass "signup" to
> promiseFxAOAuthAuthorization.
> 
> Please don't change the style if refactoring it so I can use reviewboard's
> code move detection to review. You can change the style in a follow-up.

Yeah Matt I remember this issue and we should try to keep the same behaviour as before where all tabs login properly. I agree with your concerns in your comment.

I also tried applying this patch (with gecko-dev) but it didn't work. Do you know which SHA or branch I should apply this patch on to test this properly? (I tried using the hg-patch-to-git-patch.sh tool but the changeset is getting rejected)

Comment 25

[Matthew N. [:MattN]]

(In reply to Vlad Filippov :vladikoff from comment #24)
> I also tried applying this patch (with gecko-dev) but it didn't work. Do you
> know which SHA or branch I should apply this patch on to test this properly?
> (I tried using the hg-patch-to-git-patch.sh tool but the changeset is
> getting rejected)

The Hello code was moved to a system add-on so that's why it doesn't apply. Try a rev from the day it was attached maybe.

Comment 26

[Chris Karlof [:ckarlof]]

Standard8, can we get this reviewed by someone on the Hello team with Vlad advising as necessary? I realize MattN originally landed this, but this patch seems well confined to the Hello codebase, and Hello should now be sufficiently provisioned to own it going forward.

Comment 27

[Fernando Campo (:fcampo)]

sorry guys I left this aside for a while, priority list. 
I'm updating the patch today so it applies to the current code.

Comment 28

[Mark Banner (:standard8)]

(In reply to Chris Karlof [:ckarlof] from comment #26)
> Standard8, can we get this reviewed by someone on the Hello team with Vlad
> advising as necessary? I realize MattN originally landed this, but this
> patch seems well confined to the Hello codebase, and Hello should now be
> sufficiently provisioned to own it going forward.

Yeah that's probably true. Mike, would you be willing to pick up the review for this? I won't have time for the next week or so to get my head into it.

Comment 29

[Mike de Boer [:mikedeboer]]

Yup.

Comment 30

[Matthew N. [:MattN]]

To be clear, I already explained problems with the current patch which can be addressed/discussed so this isn't really blocked on review at the moment.

Comment 31

[Fernando Campo (:fcampo)]

Attachment: [WIP] github changes

So I updated the WIP after changing for the add-on, and tried a new approach with Mike's help, but still a few problems with opening multiple tabs and the session/state:

- New approach creates different promises depending on the action {signup, signin}, but reusing the client if exists before, to not multiply states/sessions. This still leads to (as Matt mentioned) being able to login from all the tabs open (use case difficult to happen, but possible), and error occurring if doing so from a tab after another successfully login happened on a different one.

- Best option would be to manage just one tab, and change the content, but for that we'd need to modify the FxAOAuthClient.jsm (which is the one opening the URL) to receive parameters, which might be an overkill [https://github.com/mozilla/newtab-dev/blob/master/services/fxaccounts/FxAccountsOAuthClient.jsm]

- Tests - it seems that dangerouslySetInnerHTML (or renderToStaticMarkup) is not working very well with them, it's not creating the DOM properly and I can't test the link click
https://github.com/mozilla/loop/compare/master...fcampo:signin-up-1190978?expand=1#diff-705bc24cfa6f5c1421b6e20e28b62660R404
https://github.com/mozilla/loop/compare/master...fcampo:signin-up-1190978?expand=1#diff-e1cde674bbbf1d00cdae89683aae22daR231

...it's even more weird as the tests were working (or just passing?) before moving into the add-on. Maybe some other addition from a different bug, but I couldn't find anything.

Feel like needing a new pair of eyes, as I'm stuck :(

Comment 32

[Fernando Campo (:fcampo)]

Asking Mark as he advised me previously on the .dangerouslySetInnerHTML, and Mike for the states mixup. 

Not sure if I should ping Matt or Chris as per comment 26

Comment 33

[Mark Banner (:standard8)]

(In reply to Fernando Campo (:fcampo) from comment #32)
> Asking Mark as he advised me previously on the .dangerouslySetInnerHTML

There's a couple of examples here for how you can test this:

https://github.com/mozilla/loop/blob/master/standalone/test/standaloneRoomViews_test.js#L102

Comment 34

[Fernando Campo (:fcampo)]

priority cleaning

Comment 35

[Mark Banner (:standard8)]

Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.