Page MenuHomekolab.org

Meet: Improved error handling
ClosedPublic

Authored by machniak on Feb 2 2021, 11:49 AM.

Details

Reviewers
mollekopf
Group Reviewers
Restricted Project
Commits
rKe4a2e7060d30: Meet: Improved error handling
Summary
  • Display error message or any error
  • Handler errors when connecting to the OpenVidu session
Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

machniak requested review of this revision.Feb 2 2021, 11:49 AM
machniak created this revision.
mollekopf requested changes to this revision.Feb 3 2021, 1:52 PM
mollekopf added a subscriber: mollekopf.
mollekopf added inline comments.
src/resources/vue/Meet/Room.vue
371

Do we need a room state label for 422 as well above?

This revision now requires changes to proceed.Feb 3 2021, 1:52 PM
mollekopf added inline comments.Feb 3 2021, 1:59 PM
src/resources/vue/Meet/Room.vue
452

A session.token seems to be implied by a 'ready' state. Can we somehow loose the token?

machniak added inline comments.Feb 3 2021, 2:04 PM
src/resources/vue/Meet/Room.vue
371

I don't think so. A 422 response should contain the 3xx code response in the json data. In normal circumstances there will be no 422 response without the 3xx code in json response.

452

No, you can't loose the (main) token. Here we're preventing from calling initSession() again if the token has been already acquired. It's needed because after openvidu connection error we might end up in joinSession() again.

mollekopf accepted this revision.Feb 3 2021, 2:23 PM

See comment why I'm not entirely clear why we don't require the roomStateLabel but nevertheless require a special case for 422, but looks good enough.

src/resources/vue/Meet/Room.vue
371

I don't follow. It seems to me this.roomState can be equal to 422, and in this case it seems like there should be a corresponding label, unless the label is somehow not displayed, in which case I don't get why we need to make an exception for 422.

If you are certain it's correct, that's fine by me.

This revision is now accepted and ready to land.Feb 3 2021, 2:23 PM
machniak planned changes to this revision.Feb 3 2021, 2:42 PM

I'll review this again regarding 422 handling.

machniak updated this revision to Diff 6217.Feb 3 2021, 3:21 PM
  • Remove redundant condition
This revision is now accepted and ready to land.Feb 3 2021, 3:21 PM
mollekopf accepted this revision.Feb 3 2021, 4:12 PM
This revision was automatically updated to reflect the committed changes.