Closed Bug 1118061 Opened 6 years ago Closed 6 years ago

direct calls sometimes tell the user "something went wrong" when they should say "caller unavailable", part 1

Categories

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

x86
macOS
defect

Tracking

(firefox36-, relnote-firefox 37+)

RESOLVED FIXED
mozilla37
Iteration:
37.3 - 12 Jan
Tracking Status
firefox36 - ---
relnote-firefox --- 37+
Blocking Flags:
backlog Fx37+

People

(Reporter: dmose, Assigned: dmose)

References

Details

(Whiteboard: [strings])

Attachments

(1 file, 1 obsolete file)

My first impression on this, until I looked into it a bit more, was that something was going wrong in the system (i.e. it was broken).

Most telephony systems I've run across in no way suggest to their users that the system is broken when someone is busy (landline: busy signal; mobile: voice mail) or doesn't answer (landline: ringing forever; mobile: voice mail or ringing followed by voicemail). Skype and FaceTime also don't seem to do that.

Given this amount of user training, I'm concerned that a substantial number of users may interpret every non-answer or busy as "the system is broken right now", which would effectively make the product seem extremely broken to those people, even when it's not.

Sevanne, do have any sense of how likely this is to be a significant issue?  Or any way to quickly figure that out?
Flags: needinfo?
Flags: needinfo?
I'd say it's a significant issue. We should definitely be displaying some other message when it comes to timing out rather than a generic error message.

Maybe "The user you are trying to call is unavailable. Please try again later."?

Needinfo'ing Matej for wordsmithing.

Also, as an aside, wouldn't it be great if when a user receives a busy signal, they could leave a video message?
Flags: needinfo?(matej)
[Tracking Requested - why for this release]:
 
I'm going to speculate that this is worth uplifting to 36, because I'm concerned that if enough people just interpret Hello as being super buggy, they'll just find some other tool and not bother trying it again.

(In reply to Sevaan Franks [:sevaan] from comment #1)
>
> Maybe "The user you are trying to call is unavailable. Please try again
> later."?

Given that below the link, we're displaying (cancel) (try again) (email a link) buttons, I suspect we'll want different verbiage.  

The thing, of course, is that this requires strings, it'd be extremely hard/impossible to get into 35.  If (and how high) we should uplift this depends on what you, RT, and others think of the severity.

> Needinfo'ing Matej for wordsmithing.
> 
> Also, as an aside, wouldn't it be great if when a user receives a busy
> signal, they could leave a video message?

That would be awesome!  However, given the unlikelihood of implementing voicemail in the near term, how about we use the same string for the busy case as well?
backlog: --- → Fx36?
Hey Dan and Sevaan -- I would push hard to get this into Fx37 -- at least the strings.  Once we have this secured for Fx37, then we can talk about how feasible this is for Fx36.  

Dan -- Do you want to take this bug?  The goal is to fix this for Fx37, but short of that, at least get the strings in so that an uplift to Fx37 next week is a no-brainer.  I've just added it to our trello board under "to do - higher priority".
Flags: needinfo?(dmose)
Sure, I'll take this.
Assignee: nobody → dmose
Flags: needinfo?(dmose)
For Matej: just talked to Sevaan over IRC, and he said that an "unavailable" style string should be used for both "busy" and "no answer" states, so that's probably worth keeping in mind as you consider the options. :-)
It turns out that Matej is out until Monday.  In the interest of getting a string in before uplift so that it will be possible to uplift further to 36, Sevaan and I chatted in IRC, and his suggestion is simply to change the "Something went wrong." to "John Doe is unavailable."
putting in 37 and once landed with fix - will evaluate uplift to 36 for this fix if logical.
Blocks: 1100969
backlog: Fx36? → Fx37+
Priority: -- → P2
Whiteboard: [strings]
Attachment #8546849 - Flags: review?(jaws)
The above patch adds a caller unavailable message for the following cases:

XX rejected (reason=reject)
XX blocked (reason=busy)
XX busy (reason=busy)
XX user doesn’t exist: (reason=setup*)

It falls back to the message "That person is unavailable." if the contact doesn't have a display name or email address.

Solving the case for when the user doesn't answer involves reason=timeout, and that turns out to be used for multiple things, so that will be spun out to a different bug.

* reason=setup often means that the user doesn't exist, but sometimes means other things.  Cleaning that up will also be spun out into another bug.
Comment on attachment 8546849 [details] [diff] [review]
add "caller unavailable" messages to Hello

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

Do you think we'll be able to get the reason="timeout" fixed for 37 (or 36)?

::: browser/components/loop/content/shared/css/conversation.css
@@ +279,5 @@
>  .call-window h2 {
>    font-size: 1.5em;
>    font-weight: normal;
>  
> +  text-align: center;

nit, please remove the blank lines surrounding this.

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +428,5 @@
> +
> +        view = mountTestComponent({contact: contact});
> +
> +        sinon.assert.calledWith(document.mozL10n.get,
> +          "contact_unavailable_title");

Shouldn't this also check that the second parameter to .get is the contact? Here and the next two.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +225,5 @@
>  conversation_has_ended=Your conversation has ended.
>  restart_call=Rejoin
>  
> +## LOCALIZATION NOTE (contact_unavailable_title): The title displayed
> +## when a contact is unavailable. Don't translate the part between {{..}} 

nit, trailing space

@@ +228,5 @@
> +## LOCALIZATION NOTE (contact_unavailable_title): The title displayed
> +## when a contact is unavailable. Don't translate the part between {{..}} 
> +## because this will be replaced by the contact's name.
> +contact_unavailable_title={{contactName}} is unavailable.
> +generic_contact_unavailable_title=That person is unavailable.

I think "This person is unavailable" is more grammatically correct.
Attachment #8546849 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Comment on attachment 8546849 [details] [diff] [review]
> add "caller unavailable" messages to Hello
> 
> Review of attachment 8546849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you think we'll be able to get the reason="timeout" fixed for 37 (or 36)?

Only if we uplift, which I suspect we're going to want to do.  A big problem is that callStateReason is actually overloaded.  Sometimes it comes from the reason field of the websocket API (as documented at <https://docs.services.mozilla.com/loop/apis.html#termination-reasons>), other times it actually is populated from the server REST API via client.js (eg the "setup" case).  Furthermore, there are other things coming into play here both client bugs and various protocol-level overloading of error code.  Some of this stuff will become clear in the bugs I'm going to spin off after I land this one.  I think Standard8 is going to unpack more in bug 1119574 next week.

> ::: browser/components/loop/test/desktop-local/conversationViews_test.js
> @@ +428,5 @@
> > +
> > +        view = mountTestComponent({contact: contact});
> > +
> > +        sinon.assert.calledWith(document.mozL10n.get,
> > +          "contact_unavailable_title");
> 
> Shouldn't this also check that the second parameter to .get is the contact?
> Here and the next two.

It won't actually be the contact itself, it'll be the result of _getContactDisplayName.  I'll expose that outside the module and use it to ensure that we're invoking the API correctly, at least.  I've added an XXX comment about how we need to unit test that code in addition to centralizing it somewhere.

> > +generic_contact_unavailable_title=That person is unavailable.
> 
> I think "This person is unavailable" is more grammatically correct.

Fixed.
Comment on attachment 8546881 [details] [diff] [review]
add "caller unavailable" messages to Hello

Carrying forward r=jaws.
Attachment #8546881 - Flags: review+
Summary: every Hello direct call that fails due to timeout (ie no answer) or busy displays the phrase "Something went wrong" → every Hello direct call that fails because the caller is unavailable displays the phrase "Something went wrong" (part 1)
Summary: every Hello direct call that fails because the caller is unavailable displays the phrase "Something went wrong" (part 1) → make Hello direct calls expose "caller unavailable" when appropriate, part 1
Summary: make Hello direct calls expose "caller unavailable" when appropriate, part 1 → direct calls sometimes tell the user "something went wrong" when they should say "caller unavailable", part 1
Blocks: 1119992
Blocks: 1120001
Blocks: 1120003
https://hg.mozilla.org/mozilla-central/rev/1d30c40f6e7a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Hi all,

Let me know if there's anything you still need from me.

Thanks.
Flags: needinfo?(matej)
Iteration: --- → 37.3 - 12 Jan
Will be relnoted once 37 hits beta as "Hello: Improved notification about user availability".
Blocks: 1124365
Maire, Dan, what do you want to do about this bug in 36? Thanks
Flags: needinfo?(mreavy)
Flags: needinfo?(dmose)
(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> Maire, Dan, what do you want to do about this bug in 36? Thanks

I feel this is an important change that would help the user and would love to see it Fx36, but I defer to Gavin (who is now managing the front-end Hello development) to weigh the pros/cons of uplifting this to Fx36 versus waiting until Fx37 to ship this change.  So I'm moving my needinfo to him.
Flags: needinfo?(mreavy) → needinfo?(gavin.sharp)
I think this is not critical enough to merit breaking string freeze and can wait for 37.
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(dmose)
You need to log in before you can comment on or make changes to this bug.