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)
Hello (Loop)
Client
Tracking
(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)
20.28 KB,
patch
|
standard8
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•9 years ago
|
User Story: (updated)
Comment 1•9 years ago
|
||
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
Updated•9 years ago
|
backlog: Fx36+ → Fx35+
Priority: P1 → P2
Whiteboard: [rooms]
Comment 2•9 years ago
|
||
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)
Reporter | ||
Comment 3•9 years ago
|
||
Agreed, let's create a new room link. Sevaan do you agree?
Updated•9 years ago
|
Flags: needinfo?(dhenein)
Reporter | ||
Comment 4•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → nperriault
Iteration: --- → 36.3
Points: --- → 2
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
[Tracking Requested - why for this release]: We need this change for "Rooms", which is already enabled in Fx35.
tracking-firefox35:
--- → ?
tracking-firefox36:
--- → ?
Comment 8•9 years ago
|
||
(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)
Updated•9 years ago
|
Flags: needinfo?(standard8)
Assignee | ||
Comment 10•9 years ago
|
||
Rebased patch against latest fx-team.
Attachment #8530378 -
Attachment is obsolete: true
Attachment #8530378 -
Flags: feedback?(standard8)
Attachment #8532176 -
Flags: review?(standard8)
Comment 11•9 years ago
|
||
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)
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
(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)
Assignee | ||
Comment 14•9 years ago
|
||
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)
Reporter | ||
Comment 15•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8533744 -
Flags: review?(standard8) → review+
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/46bf7d2c4ca5
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/46bf7d2c4ca5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 18•9 years ago
|
||
(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)
Comment 19•9 years ago
|
||
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 20•9 years ago
|
||
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?
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8533744 -
Flags: approval-mozilla-beta?
Attachment #8533744 -
Flags: approval-mozilla-beta+
Attachment #8533744 -
Flags: approval-mozilla-aurora?
Attachment #8533744 -
Flags: approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•