"Something went wrong" isn't displayed when the call fails in the connection phase

VERIFIED FIXED in Firefox 34

Status

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla36
Points:
---
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox33 unaffected, firefox34+ verified, firefox35+ verified, firefox36+ verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
STR:

1) Generate a link
2) Click the link in a browser
3) At the generation end, select Accept
4) Wait (do not accept the camera/microphone prompt)

Actual Results

=> "Your conversation has ended" and feedback form is displayed at both ends.

Expected Results

=> "Something went wrong" is displayed at both ends.
(Assignee)

Comment 1

4 years ago
We're receiving a termination reason of "timeout" from the server, but my guess is that we're not handling it correctly.

Updated

4 years ago
backlog: --- → Fx34?
TOkeshu - Do you think you could take this bug as soon as you finish with bug 1079811?  Thanks.
Flags: needinfo?(rgauthier)
Assignee: nobody → rgauthier
Flags: needinfo?(rgauthier)
[Tracking Requested - why for this release]:
Tracking fix for uplift.  We should have a patch for review today.
tracking-firefox34: --- → ?
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
tracking-firefox34: ? → +
tracking-firefox35: --- → +
tracking-firefox36: --- → +
(Assignee)

Comment 4

4 years ago
Created attachment 8507225 [details] [diff] [review]
"Something went wrong" isn't displayed when the call fails in the connection phase

This fixes the issues here:

- On the standalone side, the issue was that we'd get the failure message in, and set the state to failure, but then the call would get a hangup from the sdk, which would override this and set the state to end, hence not showing the failure message.

There's still issues with the retry - I'm puting a patch up in bug 1074517.

- On the desktop side, we weren't handling terminate/timeout from the websocket and hence we weren't setting the call failed state before ending the call.

Note that as we haven't yet got a "call failed" view for incoming calls for desktop, its just the title that shows "Something went wrong".

This needs the unit tests updating before its ready for review.
(Assignee)

Comment 5

4 years ago
Created attachment 8507254 [details] [diff] [review]
"Something went wrong" isn't displayed when the call fails in the connection phase

Now with tests.
Attachment #8507225 - Attachment is obsolete: true
Attachment #8507254 - Flags: review?(dmose)
Hi Dan -- Can you prioritize this to the top of your review queue?  I need a review this morning so I can land this today -- assuming the patch is good.  If you're not available to review, can you recommend a reviewer? Thanks.
Flags: needinfo?(dmose)
@mreavy: looking at it now
Flags: needinfo?(dmose)
Comment on attachment 8507254 [details] [diff] [review]
"Something went wrong" isn't displayed when the call fails in the connection phase

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

r- because of the unit test problem on the webapp side; otherwise looks good.

The IncomingConversation and tests require a LOT of different bits of mental context to be held at once to really convince oneself that the code and tests are going to do the right thing.  I'm looking forward to the fluxification, which I imagine will help ease this some.  However, I do wonder if we wouldn't also benefit by using an FSA library like we did with Talkilla to simplify state and transition management around the call itself.  Thoughts?

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +418,5 @@
>  
>          describe("WebSocket Events", function() {
>            describe("Call cancelled or timed out before acceptance", function() {
>              beforeEach(function() {
> +              // Mounting the test component automatically calls the required setup functions

Nit: let's wrap this at 80 columns.

@@ +549,5 @@
>                  });
>                });
>              });
> +
> +            describe("progress - terminated - timeout (previousState not init nor alerting)",

80 columns would be good here too.

::: browser/components/loop/test/standalone/webapp_test.js
@@ +343,5 @@
> +
> +            expect(function() {
> +              TestUtils.findRenderedComponentWithType(ocView,
> +                loop.webapp.EndedConversationView); }).to.Throw(/one match/);
> +          });

This does not appear to be testing the thing it claims to be testing. Furthermore, it looks to me like we don't want to be testing either of those things: I think we want to be testing that the FailedConversationView is displayed in that case.
Attachment #8507254 - Flags: review?(dmose) → review-
Created attachment 8508055 [details] [diff] [review]
(landed fx-team) "Something went wrong" isn't displayed when the call fails in the connection phase
Attachment #8507254 - Attachment is obsolete: true
Comment on attachment 8508055 [details] [diff] [review]
(landed fx-team) "Something went wrong" isn't displayed when the call fails in the connection phase

With review comments fixed.
Attachment #8508055 - Flags: review+
Before closing this bug, please file another bug for handling the case where somebody gets more than a few of these in a row.  We may well want to show the feedback form in that case, since a bunch of these in a row could be enough to frustrate a user significantly.
Assignee: rgauthier → standard8
Keywords: leave-open
Attachment #8508055 - Attachment description: "Something went wrong" isn't displayed when the call fails in the connection phase → (landed fx-team) "Something went wrong" isn't displayed when the call fails in the connection phase
Filed bug 1085519.  So removing leave open.
Keywords: leave-open
Whiteboard: [landed in fx-team]
https://hg.mozilla.org/mozilla-central/rev/3b21becf8c2a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox36: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Pauly -- This should be in today's Nightly.  Will you have a chance to verify it today?
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #15)
> Pauly -- This should be in today's Nightly.  Will you have a chance to
> verify it today?

Meant to do a needinfo to Pauly with the last comment  ^^
Flags: needinfo?(paul.silaghi)
Here is what I see in 36.0a1 (2014-10-21) using the STR in comment 0 and the Loop dev-server:
On the link clicker side - call failed and 'something went wrong' in the tab title
On the desktop client side - feedback and 'something went wrong' in the title bar
http://i.imgur.com/ctMgB9E.png
Is this ok and expected ?
Flags: needinfo?(paul.silaghi) → needinfo?(dmose)
It's what's described in comment 4:

'Note that as we haven't yet got a "call failed" view for incoming calls for desktop, its just the title that shows "Something went wrong".'

So we're not where things need to be in order to ship, but this does implement what this bug intended to implement and is forward progress, so it can be verified.

That said, we need to be sure there is another bug on file (which I suspect, but don't know, that there is) which does the rest of the job: implements a call failed view for the incoming desktop call, and that that is nominated to block 34 and 35.
Flags: needinfo?(dmose)
Flags: qe-verify+
The bug Dan is referring to is Bug 1047410, which only impacts users who see call failures with link clickers.  (They would see "Something went wrong" and the feedback form.)  Link clicking is being replaced by Rooms in Fx35.  Direct callers already see the proper call failure notification.  Currently Bug 1047410 is blocking Fx35, not Fx34.
Verified fixed 36.0a1 (2014-10-21) based on comment 18.
Status: RESOLVED → VERIFIED
status-firefox36: fixed → verified
This bug has been verified on m-c. Can you please request uplift with the intention of getting this onto Aurora this week for verification and landing the fix in time for Beta4 (land before Mon)?
Flags: needinfo?(standard8)
Comment on attachment 8508055 [details] [diff] [review]
(landed fx-team) "Something went wrong" isn't displayed when the call fails in the connection phase

Approval Request Comment
[Feature/regressing bug #]: Communicate to the user that something went wrong
[User impact if declined]: User will see "conversation ended" even though the conversation never started
[Describe test coverage new/current, TBPL]: unit tests and manually tested
[Risks and why]: No risk outside of Hello and minimal risk to Hello
[String/UUID change made/needed]: No strings
Flags: needinfo?(standard8)
Attachment #8508055 - Flags: approval-mozilla-beta?
Attachment #8508055 - Flags: approval-mozilla-aurora?
Comment on attachment 8508055 [details] [diff] [review]
(landed fx-team) "Something went wrong" isn't displayed when the call fails in the connection phase

Aurora+
Attachment #8508055 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/29109842cec2
status-firefox35: affected → fixed
Whiteboard: [landed in fx-team]
Verified fixed 35.0a2 (2014-10-23) Win 7
status-firefox35: fixed → verified
Comment on attachment 8508055 [details] [diff] [review]
(landed fx-team) "Something went wrong" isn't displayed when the call fails in the connection phase

Now that this has been verified on Aurora, up we go to Beta. Beta+
Attachment #8508055 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/8cf65ccdce3d
status-firefox34: affected → fixed
Flags: in-testsuite+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> https://hg.mozilla.org/releases/mozilla-beta/rev/8cf65ccdce3d

Paul, can you please verify this is fixed in the next beta?
Flags: needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes → paul.silaghi
Verified fixed FF 34b4 Win 7, OS X 10.9.5
status-firefox34: fixed → verified
Flags: needinfo?(paul.silaghi)

Updated

4 years ago
Iteration: --- → 36.1
You need to log in before you can comment on or make changes to this bug.