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

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

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

Comment 2

[Maire Reavy [:mreavy]]

TOkeshu - Do you think you could take this bug as soon as you finish with bug 1079811?  Thanks.

Comment 3

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
Tracking fix for uplift.  We should have a patch for review today.

Comment 4

[Mark Banner (:standard8)]

Attachment: "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.

Comment 5

[Mark Banner (:standard8)]

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

Now with tests.

Comment 6

[Maire Reavy [:mreavy]]

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.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

@mreavy: looking at it now

Comment 8

[Dan Mosedale (:dmosedale, :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.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: (landed fx-team) "Something went wrong" isn't displayed when the call fails in the connection phase

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/3b21becf8c2a

Comment 13

[Maire Reavy [:mreavy]]

Filed bug 1085519.  So removing leave open.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3b21becf8c2a

Comment 15

[Maire Reavy [:mreavy]]

Pauly -- This should be in today's Nightly.  Will you have a chance to verify it today?

Comment 16

[Maire Reavy [:mreavy]]

(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  ^^

Comment 17

[Paul Silaghi, QA [:pauly]]

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 ?

Comment 18

[Dan Mosedale (:dmosedale, :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.

Comment 19

[Maire Reavy [:mreavy]]

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.

Comment 20

[Paul Silaghi, QA [:pauly]]

Verified fixed 36.0a1 (2014-10-21) based on comment 18.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

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)?

Comment 22

[Maire Reavy [:mreavy]]

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

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/29109842cec2

Comment 25

[Paul Silaghi, QA [:pauly]]

Verified fixed 35.0a2 (2014-10-23) Win 7

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8cf65ccdce3d

Comment 28

[u279076]

(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?

Comment 29

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b4 Win 7, OS X 10.9.5