Closed Bug 1172662 Opened 4 years ago Closed 4 years ago

ICE failures should be reported to the user

Categories

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

defect
Points:
5

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Iteration:
44.2 - Oct 19
Tracking Status
firefox44 --- fixed

People

(Reporter: dmose, Assigned: mancas)

References

Details

(Whiteboard: [display error][user quality])

User Story

As a user, when ICE fails under the hood, I should see a message explaining that the call has failed (ideally with suggestions about what I might do next) and offer a "Retry" button.

- If an exception of SDK type PUBLISHER_ICE_WORKFLOW_FAILED or SUBSCRIBER_ICE_WORKFLOW_FAILED occurs in OtSdkDriver#_onOTException, then we should:
-- Dispatch a ConnectionFailure action with a new FAILURE_DETAILS value.
-- Keep the existing exception _notifyMetricsEvent
- In the failure view(s), add a case for the new FAILURE_DETAILS value, which selects a new string (string: "Connection failed. Your firewall may be blocking calls.").
-- Make sure this is done for both desktop & standalone
- Ensure that on standalone & desktop, the failure views have a retry button for this case

Dispatching the ConnectionFailure action will hit the existing code which will leave the conversation or end the call attempt.

Attachments

(1 file, 4 obsolete files)

Before 1141296 landed, the sdk displayed a message and there was a failure, and then did nothing, leaving the user stranded.  We no longer display a message.
Flags: firefox-backlog+
Duplicate of this bug: 1172669
Adam: as we fix this - since we just added logging for ice failures - but it's an absolute number.  we should start logging ice successes as well so we have a percentage for visibility out of data.

Dan: create a new bug for the success part (separate user stories).... that should be P2 - 29.
Rank: 35
Flags: needinfo?(dmose)
Priority: -- → P3
Whiteboard: [display error]
See Also: → 1179513
Bug 1179513 spun off.
Flags: needinfo?(dmose)
Points: --- → 5
Rank: 35 → 28
User Story: (updated)
Priority: P3 → P2
Whiteboard: [display error] → [display error][user quality]
User Story: (updated)
Summary: log ICE failures and handle them gracefully → ICE failures should be reported to the user
Sevaan: Do we have any strings for this already, or can you come up with some.

The general failure mode is that due to some kind of issue we've not been able to connect the user - this is most likely network/firewalls being too aggressive and blocking too much. It could be either the user's or the peer's firewall or somewhere in-between.

So something like "Couldn't establish a connection. Is your firewall blocking calls?"
Flags: needinfo?(sfranks)
Is there a sumo article we could link to here (cursory searching doesn't seem to turn up anything)? I worry slightly that "Is your firewall blocking calls" won't mean much to our less tech-savvy users. If we're going to give a helpful message, it would be nice to give some instructions/more information.

As for string, what you suggested sounds reasonable. Matej, any way to make it pithier?

"Couldn't establish a connection. Is your firewall blocking calls?"
"Couldn't establish a connection. Your firewall may be blocking calls."
Flags: needinfo?(sfranks) → needinfo?(matej)
(In reply to Sevaan Franks [:sevaan] from comment #5)
> Is there a sumo article we could link to here (cursory searching doesn't
> seem to turn up anything)? I worry slightly that "Is your firewall blocking
> calls" won't mean much to our less tech-savvy users. If we're going to give
> a helpful message, it would be nice to give some instructions/more
> information.
> 
> As for string, what you suggested sounds reasonable. Matej, any way to make
> it pithier?
> 
> "Couldn't establish a connection. Is your firewall blocking calls?"
> "Couldn't establish a connection. Your firewall may be blocking calls."

We could shorten the first part to "Couldn't connect" or "Connection failed."

For the second part, I prefer the statement, since the question feels like it relies on the user knowing the answer.

"Couldn't connect. Your firewall may be blocking calls."
"Connection failed. Your firewall may be blocking calls."
Flags: needinfo?(matej)
Thanks, Matej! I like the last one: "Connection failed. Your firewall may be blocking calls."
User Story: (updated)
Assignee: nobody → b.mcb
Attachment #8666660 - Attachment is obsolete: true
Attachment #8667129 - Flags: review?(standard8)
Comment on attachment 8667129 [details] [diff] [review]
ICE failures are reported to the user

Let me add more unit tests to cover all the cases
Attachment #8667129 - Flags: review?(standard8)
Attachment #8667129 - Attachment is obsolete: true
Attachment #8667138 - Flags: review?(standard8)
Comment on attachment 8667138 [details] [diff] [review]
ICE failures are reported to the user

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

This looks good, unfortunately I couldn't test it as there seems to be a lot of bitrot. Can you try updating your tree, fixing the comments and then reattaching?

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +919,5 @@
>      _onOTException: function(event) {
> +      switch(event.code) {
> +        case OT.ExceptionCodes.PUBLISHER_ICE_WORKFLOW_FAILED:
> +        case OT.ExceptionCodes.SUBSCRIBER_ICE_WORKFLOW_FAILED:
> +          this.dispatcher.dispatch(new sharedActions.ConnectionFailure({

We should also log the metrics event here. Although we're handling it from a user perspective, we still need to know its happening.

(also please add a test for the metrics event for these cases).

::: browser/components/loop/content/shared/js/utils.js
@@ +80,5 @@
>      COULD_NOT_CONNECT: "reason-could-not-connect",
>      NETWORK_DISCONNECTED: "reason-network-disconnected",
>      EXPIRED_OR_INVALID: "reason-expired-or-invalid",
> +    UNKNOWN: "reason-unknown",
> +    ICE_FAILED: "ice-failed"

Can you make this reason-ice-failed please to be consistent with the rest. We wanted to make this distinct from some of the other failure strings.
Attachment #8667138 - Flags: review?(standard8)
Attachment #8667138 - Attachment is obsolete: true
Attachment #8669567 - Flags: review?(standard8)
Hey Mark! I took into account your comments, so the patch could be reviewed when you get a chance

Thanks!
Attachment #8669567 - Attachment is obsolete: true
Attachment #8669567 - Flags: review?(standard8)
Attachment #8669604 - Flags: review?(standard8)
Comment on attachment 8669604 [details] [diff] [review]
ICE failures messages are reported to the user

Looks good, r=Standard8
Attachment #8669604 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/9cb85a2c008e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Iteration: --- → 44.2 - Oct 19
You need to log in before you can comment on or make changes to this bug.