Closed Bug 1102170 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Points:
2

Tracking

(firefox35+ fixed, firefox36+ fixed, firefox37 fixed)

RESOLVED FIXED
mozilla37
Iteration:
36.3
Tracking Status
firefox35 + fixed
firefox36 + fixed
firefox37 --- fixed
backlog Fx35+

People

(Reporter: RT, Unassigned)

Details

(Whiteboard: [rooms])

User Story

As a Hello user, I want to share a room URL as opposed to a call URL when a direct call fails. 

Rough implementation:
* When pressing the "Email link" on the screen informing the user that his direct call failed), rather than creating a new call URL on the server, create a new room URL on the server and provide that URL on the e-mail body
* The Room now appears in the list of rooms if the user opens the panel

Attachments

(1 file, 2 obsolete files)

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
User Story: (updated)
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.
backlog: --- → Fx36+
Flags: needinfo?(mreavy)
Priority: -- → P1
backlog: Fx36+ → Fx35+
Priority: P1 → P2
Whiteboard: [rooms]
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.
Flags: needinfo?(sfranks)
Flags: needinfo?(mreavy)
Flags: needinfo?(dhenein)
Agreed, let's create a new room link.
Sevaan do you agree?
Flags: needinfo?(dhenein)
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).
Flags: needinfo?(sescalante)
Assignee: nobody → nperriault
Iteration: --- → 36.3
Points: --- → 2
Niko's going to start on this bug now.  If there are no surprises, we should be able to get this uplifted into Fx35.
Flags: needinfo?(sescalante) → needinfo?(standard8)
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.
Attachment #8530378 - Flags: feedback?(standard8)
[Tracking Requested - why for this release]:
We need this change for "Rooms", which is already enabled in Fx35.
(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.
Flags: needinfo?(sfranks)
moved to P1 based on timing that we want to uplift
Priority: P2 → P1
Flags: needinfo?(standard8)
Rebased patch against latest fx-team.
Attachment #8530378 - Attachment is obsolete: true
Attachment #8530378 - Flags: feedback?(standard8)
Attachment #8532176 - Flags: review?(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?
Attachment #8532176 - Flags: review?(standard8)
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!
Flags: needinfo?(nperriault)
(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.
Flags: needinfo?(nperriault)
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.
Attachment #8532176 - Attachment is obsolete: true
Attachment #8533744 - Flags: review?(standard8)
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).
Flags: needinfo?(sescalante)
Flags: needinfo?(oteo)
Attachment #8533744 - Flags: review?(standard8) → review+
(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...
Flags: needinfo?(oteo)
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.
Flags: needinfo?(sescalante)
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
Attachment #8533744 - Flags: approval-mozilla-beta?
Attachment #8533744 - Flags: approval-mozilla-aurora?
Attachment #8533744 - Flags: approval-mozilla-beta?
Attachment #8533744 - Flags: approval-mozilla-beta+
Attachment #8533744 - Flags: approval-mozilla-aurora?
Attachment #8533744 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.