-
Notifications
You must be signed in to change notification settings - Fork 509
Fix leaving conversation when fetching the conversation fails #4338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix leaving conversation when fetching the conversation fails #4338
Conversation
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
| defaultPageTitle: false, | ||
| loading: false, | ||
| isRefreshingCurrentConversation: false, | ||
| errorUpdatingConversationToast: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: s/Toast/Message to not make this name implementation dependent ?
| this.$store.dispatch('updateToken', to.params.token) | ||
| } else if (to.name === 'notfound') { | ||
| this.setPageTitle('') | ||
| this.$store.dispatch('updateToken', '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as someone not knowing what effect this code will have, would be nice to add a comment :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updating the token should automatically change the pagetitle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow I think we should still keep the current URL, page title and token in place and only render a "not found" content. The switching will be done anyway by the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't work reliably with guests. They would re initialise the conversation immediately and continue chatting without being annoyed for a second, until we can really block people.
|
Master is 22 now |
|
I will close the PR for now (as there was no active development since 6 months). Anyone can pick it up whenever they want and reopen the PR. |
Fixes #4273
Errors other than 404 no longer cause a redirection to
spreed/not-found. An error notification is shown, though, similar to what happens if fetching the participant list fails.How to test
getSingleRoomendpoint randomly return errors by prepending:Result with this pull request
An error occurred while updating the conversation notification is shown, but the chat is still open.
Result without this pull request
The chat is replaced by The conversation does not exist. The title page still shows the conversation name.
In the browser console sometimes NavigationDuplicated: Avoided redundant navigation to current location: "/apps/spreed/not-found" appears (it seems to be caused by two
participantListChangedevents emitted quickly in a row, so the second one triggers another fetch of the conversation before the first one returned, and if both fail the second one causes a redundant redirection).