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)
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•11 years ago
|
User Story: (updated)
Comment 1•11 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•11 years ago
|
backlog: Fx36+ → Fx35+
Priority: P1 → P2
Whiteboard: [rooms]
Comment 2•11 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•11 years ago
|
||
Agreed, let's create a new room link.
Sevaan do you agree?
Updated•11 years ago
|
Flags: needinfo?(dhenein)
Reporter | ||
Comment 4•11 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•11 years ago
|
Assignee: nobody → nperriault
Iteration: --- → 36.3
Points: --- → 2
Comment 5•11 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•11 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•11 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•11 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•11 years ago
|
Flags: needinfo?(standard8)
Assignee | ||
Comment 10•11 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•11 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•11 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•11 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•11 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•11 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•11 years ago
|
Attachment #8533744 -
Flags: review?(standard8) → review+
Assignee | ||
Comment 16•11 years ago
|
||
Target Milestone: --- → mozilla37
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 18•11 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•11 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•11 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•11 years ago
|
Updated•11 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+
Comment 21•11 years ago
|
||
Comment 22•11 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•