Closed Bug 1023507 Opened 10 years ago Closed 10 years ago

Need to expose/report SDK failures to the user

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla33

People

(Reporter: standard8, Assigned: aoprea)

References

Details

(Whiteboard: [p=1])

User Story

This needs to happen at both ends of the conversation. Use of the existing notification infrastructure should help.

The SDK has an Exception event that appears we need to listen for it in the ConversationModel, and then take appropriate action:

http://tokbox.com/opentok/libraries/client/js/reference/ExceptionEvent.html

Attachments

(2 files, 4 obsolete files)

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.
User Story: (updated)
Whiteboard: p=1 → [p=1]
Assignee: nobody → aoprea
Attachment #8444611 - Attachment is obsolete: true
Attachment #8444615 - Flags: review?(dmose)
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
Blocks: 1000186, 1000240
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 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.
Attachment #8444615 - Flags: review?(dmose) → review+
Attachment #8445509 - Flags: review?(dmose)
Attachment #8444615 - Attachment is obsolete: true
Attachment #8445509 - Flags: review?(dmose) → review+
(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.
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 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.
Attachment #8445510 - Flags: ui-review?(dhenein) → ui-review+
Attachment #8445509 - Attachment is obsolete: true
(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.
sorry had to backout for marionette test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42432715&tree=Mozilla-Inbound
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
Attachment #8445562 - Attachment is obsolete: true
Attachment #8445987 - Flags: ui-review+
Attachment #8445987 - Flags: review+
Does this need manual testing or is it sufficiently covered by automation?
Whiteboard: [p=1] → [p=1][qa?]
I missed this bug report in my triage but I'm calling this verified fixed based on prior testing. Sorry for missing this earlier.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
Whiteboard: [p=1][qa?] → [p=1]
See Also: → 1169456
You need to log in before you can comment on or make changes to this bug.