Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Propagate room join errors to the UI #1007

Merged
merged 3 commits into from
Jun 2, 2017
Merged

Propagate room join errors to the UI #1007

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03f4f26
Propagate room join errors to the UI
dbkr Jun 2, 2017
f52035f
Set state from dispatch payload unconditionally
dbkr Jun 2, 2017
ac0f2f7
Remove redundant room_id
dbkr Jun 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/components/structures/RoomView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ module.exports = React.createClass({
roomLoading: RoomViewStore.isRoomLoading(),
roomLoadError: RoomViewStore.getRoomLoadError(),
joining: RoomViewStore.isJoining(),
joinError: RoomViewStore.getJoinError(),
}, () => {
this._onHaveRoom();
this.onRoom(MatrixClientPeg.get().getRoom(this.state.roomId));
Expand Down
43 changes: 36 additions & 7 deletions src/stores/RoomViewStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ limitations under the License.
import dis from '../dispatcher';
import {Store} from 'flux/utils';
import MatrixClientPeg from '../MatrixClientPeg';
import sdk from '../index';
import Modal from '../Modal';
import { _t } from '../languageHandler';

const INITIAL_STATE = {
// Whether we're joining the currently viewed room
Expand Down Expand Up @@ -76,6 +79,12 @@ class RoomViewStore extends Store {
case 'join_room':
this._joinRoom(payload);
break;
case 'joined_room':
this._joinedRoom(payload);
break;
case 'join_room_error':
this._joinRoomError(payload);
break;
case 'on_logged_out':
this.reset();
break;
Expand Down Expand Up @@ -128,16 +137,36 @@ class RoomViewStore extends Store {
this._setState({
joining: true,
});
MatrixClientPeg.get().joinRoom(this._state.roomId, payload.opts).then(
() => {
this._setState({
joining: false,
MatrixClientPeg.get().joinRoom(this._state.roomId, payload.opts).done(() => {
dis.dispatch({
action: 'joined_room',
room_id: this._state.roomId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used anymore

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't, is it? Since we need to update joining to false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we don't need the room_id to do that

});
}, (err) => {
this._setState({
joining: false,
joinError: err,
dis.dispatch({
action: 'join_room_error',
room_id: this._state.roomId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that nothing reads this error, but if it's there then it should probably be kept up to date. We could remove the join error though (unless we need it in RoomView by moving the Modal there)

Copy link
Contributor

@lukebarnard1 lukebarnard1 Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to the room_id

err: err,
});
const msg = err.message ? err.message : JSON.stringify(err);
const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog");
Modal.createDialog(ErrorDialog, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels quite wrong to create modals in the store. I would at least move this to _onRoomViewStoreUpdate of RoomView, but I am surprised that we weren't already doing this somewhere else...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were doing it in RoomView but it was removed - I wasn't sure whether the intention was to have the thing doing the joining display these errors or the view that caused the join: if the former, you'd get dialogs pop up even if you navigated away from the room view, if the latter, you could get multiple dialogs if multiple things were listening to the dispatch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IRL we decided that the value added by moving the location of the dialog creation is probably quite low.

The store should probably not be doing API calls and it should probably be dumb in that it should just accept state changes via dispatches that it trusts fully but that have originated elsewhere.

title: _t("Failed to join room"),
description: msg,
});
});
}

_joinedRoom(payload) {
this._setState({
joining: false,
});
}

_joinRoomError(payload) {
this._setState({
joining: false,
joinError: payload.err,
});
}

Expand Down