1102170 - [User story] As a Hello user, I want to share a room URL as opposed to a call URL when a direct call fails.

Status: RESOLVED FIXED

Description

[Romain Testard [:RT]]

Currently when initiating a direct call to a contact, the fall-back solution consists in sharing a call-back URL as opposed to a room URL.
This bug is about replacing the shared call URL with a room URL.

The benefits would be:
* Link clicker consistency (all links clicked are rooms)
* Desktop client consistency (all URLs are rooms)
* FxOS app simplicity (current scope for next version requires support for call URLs in case you sign into an FxA where previously you generated a call URL on desktop)
* Ability to retire the call URL server implementation and link clicker UI code as the next version of Hello on FxOS gets released

Comment 1

[:shell escalante]

Maire will update with info from conversation with Mark and then need info sevaan and darrin on UX.  If no string changes - uplift to Fx35.

Comment 2

[Maire Reavy [:mreavy] Plz needinfo]

Darrin, Sevaan -- To make this change, we need to know: do we share a new Rooms link, or do we give the user the option to share an existing Room?

I'm thinking we should use a new Rooms link for pre-populating the Email message.  (The user can always modify it.) And we can look to give the user an option in a future Firefox revisions.

I'm not sure we need this for Fx35, but it would be good to make the switchover so that we don't need to continue maintaining that link-clicker calling code past Fx34.

Comment 3

[Romain Testard [:RT]]

Agreed, let's create a new room link.
Sevaan do you agree?

Comment 4

[Romain Testard [:RT]]

Shell can you confirm if it will make it for Fx35? The reporting requirements we discuss in Fx35 depend on it (easier if only room URLs are shared).

Comment 5

[Maire Reavy [:mreavy] Plz needinfo]

Niko's going to start on this bug now.  If there are no surprises, we should be able to get this uplifted into Fx35.

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Created attachment 8530378 [details] [diff] [review]
Share a room url by email when Loop direct call fails.

This works, though I'm using the contact email to name the new room… Using the incremental name generation feature would involve a bit more refactoring, as that feature is tightly coupled with the RoomStore for now - and we don't want direct calling stuff to know about it.

Feedback and thoughts welcome.

Comment 7

[Maire Reavy [:mreavy] Plz needinfo]

[Tracking Requested - why for this release]:
We need this change for "Rooms", which is already enabled in Fx35.

Comment 8

[Sevaan Franks [:sevaan]]

(In reply to Romain Testard [:RT] from comment #3)
> Agreed, let's create a new room link.
> Sevaan do you agree?

New Room link sounds right to me.

Comment 9

[:shell escalante]

moved to P1 based on timing that we want to uplift

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Created attachment 8532176 [details] [diff] [review]
Share a room url by email when Loop direct call fails.

Rebased patch against latest fx-team.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8532176 [details] [diff] [review]
Share a room url by email when Loop direct call fails.

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

I don't think this is far off, but there's a couple of improvements to be had.

::: browser/components/loop/content/js/conversationViews.jsx
@@ +259,5 @@
>        });
>  
> +      this.props.dispatcher.dispatch(new sharedActions.FetchRoomEmailLink({
> +        roomOwner: navigator.mozLoop.userProfile.email,
> +        roomName: _getPreferredEmail(this.props.contact).value

I think it might be nicer to use the contact name as primary, and fallback to email.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +336,2 @@
>          if (err) {
> +          console.error("fetchRoomEmailLink error", err);

Can you make sure we've got a bug on handling the failure nicely (I think we might have one already, but would be good to reference here).

@@ +338,1 @@
>            this.trigger("error:emailLink");

We're not doing anything with the trigger, do we really need it here?

Comment 12

[Maire Reavy [:mreavy] Plz needinfo]

Hey Niko -- Just FYI: I feel this bug is more important priority-wise than bug 1104051.  

Do you think you could finish this bug off today (or tomorrow)?  Thanks!

Comment 13

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #12)
> Do you think you could finish this bug off today (or tomorrow)?  Thanks!

Yes.

Comment 14

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Created attachment 8533744 [details] [diff] [review]
Share a room url by email when Loop direct call fails.

Addressed comments.

(In reply to Mark Banner (:standard8) from comment #11)
> > +      this.props.dispatcher.dispatch(new sharedActions.FetchRoomEmailLink({
> > +        roomOwner: navigator.mozLoop.userProfile.email,
> > +        roomName: _getPreferredEmail(this.props.contact).value
> 
> I think it might be nicer to use the contact name as primary, and fallback
> to email.

Fixed.

> ::: browser/components/loop/content/shared/js/conversationStore.js
> @@ +336,2 @@
> >          if (err) {
> > +          console.error("fetchRoomEmailLink error", err);
> 
> Can you make sure we've got a bug on handling the failure nicely (I think we
> might have one already, but would be good to reference here).

We do display an error message already, in CallFailedView#render and CallFailedView#_renderError. I removed the console.error call, which was there for debugging purpose.

> @@ +338,1 @@
> >            this.trigger("error:emailLink");
> 
> We're not doing anything with the trigger, do we really need it here?

We subscribe to this event in CallFailedView#componentDidMount, that's how we can render the error message.

Comment 15

[Romain Testard [:RT]]

Shell, is this confirmed for uplift in 35?
Maria, if we get this in Fx35 I think we need the same approach in the 1.1.1 mobile app (fallback to room URLs when direct call fails).

Comment 16

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

https://hg.mozilla.org/integration/fx-team/rev/46bf7d2c4ca5

Comment 17

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/46bf7d2c4ca5

Comment 18

[Maria Angeles Oteo (:oteo)]

(In reply to Romain Testard [:RT] from comment #15)
> Shell, is this confirmed for uplift in 35?
> Maria, if we get this in Fx35 I think we need the same approach in the 1.1.1
> mobile app (fallback to room URLs when direct call fails).

Clearing my ni, I'm also waiting for Shell's answer...

Comment 19

[Maire Reavy [:mreavy] Plz needinfo]

I plan to ask for uplift to Fx35 today.  We were just waiting for it to land on m-c first and verify it works.  I hope to have this in Beta before the next Beta build.

Comment 20

[Randell Jesup [:jesup]]

Comment on attachment 8533744 [details] [diff] [review]
Share a room url by email when Loop direct call fails.

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: UI update for rooms - default share option when a call fails should be email a room URL, not a direct call url.

[Describe test coverage new/current, TBPL]: On m-c.  Includes test updates to make sure the right thing is presented for email.

[Risks and why]: Mostly refactoring to centralize the code for selecting what to share.  Low risk; no significant logic flow changes.

[String/UUID change made/needed]: none

Comment 21

[Randell Jesup [:jesup]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9618c0682604

Comment 22

[Randell Jesup [:jesup]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0692e20fefef