1023507 - Need to expose/report SDK failures to the user

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

I've had a couple of failures from the SDK tonight.

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://anvil.opentok.com/session/1_MX40NDgzNTg5Mn5-VHVlIEp1biAxMCAxMjoyNDoxMyBQRFQgMjAxNH4wLjcwMzUxNjl-UH4?extended=true. This can be fixed by moving the resource to the same domain or enabling CORS
"getJSON said error:" error { target: XMLHttpRequest, lengthComputable: false, loaded: 0, total: 0, isTrusted: true, eventPhase: 0, bubbles: false, cancelable: false, defaultPrevented: false, timeStamp: 1402428253880141, originalTarget: XMLHttpRequest } opentok.min.js:348
"Could not connect to the OpenTok API Server." opentok.min.js:50
"OT.exception :: title: Connect Failed (1006) msg: Could not connect to the OpenTok API Server." opentok.min.js:50 

The other one was an invalid token reported from the sdk on the console.

Neither of these were reported to the user - the UI just stayed the same.

Setting as P1 as we should fix this sooner rather than later.

Comment 1

[:shell escalante]

https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#link-prompt
see console for details...

Comment 2

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Handle SDK failures when calling connect

Comment 3

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Handle SDK failures when calling connect

Comment 4

[Romain Testard [:RT]]

Marking as blocking both the standalone link clicker UI and client UI "Call failed" notifications as I assume this occurs during call set-up both on the client UI and standalone link clicker UI.

Can the user do anything about it apart from trying to call again? If not then we should use the UI already planned in 1000186 and 1000240

Comment 5

[Andrei Oprea [mention me on [e-mail]]]

Yes this occurs during setup (the .connect() call) and I agree that more improvements should be made to the 'call failed' experience, but that should be left for separate bugs. Right now having this patch in would catch several error issues that were so far overlooked (both in test cases and production)

This is the way it looks now [1] it would provide some form of feedback to the user explaining in plain terms that something went wrong. Also a dev might be interested in having a closer look at the error message that gets logged in the console.

[1] http://i.imgur.com/XyjAFFn.png

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8444615 [details] [diff] [review]
Handle SDK failures when calling connect

r=dmose for the most current version of the patch, which has all the issues addressed.

Comment 7

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Handle SDK failures when calling connect

Comment 8

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Screen Shot 2014-06-24 at 2.45.43 PM.png

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Romain Testard [:RT] from comment #4)
> 
> Can the user do anything about it apart from trying to call again? If not
> then we should use the UI already planned in 1000186 and 1000240

While you're right that we want the better UI, there's a lot to like about getting this into the hands of dogfooders ASAP, before that UI is implemented.  I'd propose we land this with the UI as currently up-for-review, and then do the better UI in those bugs.

Comment 10

[Maire Reavy [:mreavy]]

I agree with Dan.  We want better UI, but this is much better than the current state (no message at all).  I like also that it points dogfooders to check the console.

Comment 11

[Darrin Henein [:darrin]]

Comment on attachment 8445510 [details]
Screen Shot 2014-06-24 at 2.45.43 PM.png

Great for now, lets follow up UI in another bug as discussed.

Comment 12

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Handle SDK failures when calling connect, r=dmose, ui-r=darrin

Comment 13

[Romain Testard [:RT]]

(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #9)
> (In reply to Romain Testard [:RT] from comment #4)
> > 
> > Can the user do anything about it apart from trying to call again? If not
> > then we should use the UI already planned in 1000186 and 1000240
> 
> While you're right that we want the better UI, there's a lot to like about
> getting this into the hands of dogfooders ASAP, before that UI is
> implemented.  I'd propose we land this with the UI as currently
> up-for-review, and then do the better UI in those bugs.

Yes totally agree, let's go with this for now.
I captured the need to report failure to connect with the other peer at call set-up related to SDK issues on 1000186 (Desktop client call failed notification bucket) and 1000240 (standalone UI call failed notification bucket) so we can address this with a better UI later.

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b30d5443b70e

Comment 15

[Carsten Book [:Tomcat]]

sorry had to backout for marionette test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42432715&tree=Mozilla-Inbound

Comment 16

[Mark Banner (:standard8)]

Backed out due to test failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cbfe5dd1a1d

03:16:46     INFO -  AssertionError: 1 failure(s) encountered:
03:16:46     INFO -  should call connect
03:16:46     INFO -  expected connect to be called with exact arguments typeOf("string"), typeOf("string"), undefined
03:16:46     INFO -      connect(apiKey, sessionToken, function () {})
03:16:46     INFO -  fail@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/sinon-1.9.0.js:4687:25
03:16:46     INFO -  failAssertion@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/sinon-1.9.0.js:4648:9
03:16:46     INFO -  mirrorPropAsAssertion/assert[name]@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/sinon-1.9.0.js:4671:17
03:16:46     INFO -  @http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/models_test.js:174:1
03:16:46     INFO -  Runnable.prototype.run@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4336:24
03:16:46     INFO -  Runner.prototype.runTest@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4724:5
03:16:46     INFO -  next/<@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4802:7
03:16:46     INFO -  next@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4649:1
03:16:46     INFO -  next/<@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4659:7
03:16:46     INFO -  next@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4597:1
03:16:46     INFO -  next/<@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4621:7
03:16:46     INFO -  Runnable.prototype.run@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4338:5
03:16:46     INFO -  next@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4609:5
03:16:46     INFO -  Runner.prototype.hook/<@http://localhost:49677/build/tests/marionette/tests/browser/components/loop/test/shared/vendor/mocha-1.17.1.js:4626:5

Comment 17

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Handle SDK failures when calling connect

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Latest bits pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=3ea5bfd4467d

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5465fbff0676

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/5465fbff0676

Comment 21

[u279076]

Does this need manual testing or is it sufficiently covered by automation?

Comment 22

[u279076]

I missed this bug report in my triage but I'm calling this verified fixed based on prior testing. Sorry for missing this earlier.