Closed Bug 1102170 Opened 11 years ago Closed 11 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.

Attachment

General

Created:
Updated:
Size: