Skip to content

Conversation

@PVince81
Copy link
Member

@PVince81 PVince81 commented Feb 4, 2021

When the current conversation was deleted while in a call, the redirect
now targets the not-found page instead of the one about duplicate
session.

Added event bus handler for the not found page which now also removes
the session store's conversation token.

Fixes #4552

@PVince81 PVince81 added this to the 💖 Next Major (22) milestone Feb 4, 2021
@PVince81 PVince81 self-assigned this Feb 4, 2021
@PVince81
Copy link
Member Author

PVince81 commented Feb 4, 2021

/backport to stable21

@PVince81
Copy link
Member Author

@nickvergessen I've pushed a fix to prevent leaving or deleting a conversation while in a call. This will prevent the can of worms of trying to handle warning dialogs, etc.

As a bonus I also added a "close that freaky popup already when I click".

Regarding router push, see my comment above. It's also a can of worms.

@PVince81 PVince81 force-pushed the bugfix/4552/fix-duplicate-on-delete branch from a68ac9a to 67c21a2 Compare February 10, 2021 16:35
@PVince81
Copy link
Member Author

PVince81 commented Feb 10, 2021

I've changed the approach. The new approach doesn't refactor the old code.

Since we'll need this kind of handling outside of App.vue, I've put the code into the mixin duplicateSessionHandler which I renamed to sessionIssueHandler.

Cases to cover and test all while in a call:

  • deleting conversation from outside
    • regular call
    • sidebar call => redirects from files app to talk app not found
    • video verification => redirects to login page
    • guest call => brutally kicked out, redirects to login page...
  • being removed from conversation from outside
    • regular call
    • sidebar call => not kickable
    • video verification => redirects to login page
    • guest call => redirects to login page
  • leaving current conversation from left sidebar while in the same call: todo, see Fix redirect when deleting current conversation #5082 (comment)
  • deleting current conversation from left sidebar while in the same call: todo, see Fix redirect when deleting current conversation #5082 (comment)

@PVince81
Copy link
Member Author

PVince81 commented Feb 10, 2021

looks like we were redirecting to talk's "duplicate session" page already in a sidebar call, despite being in the files app :-/
I'll make it the same now for "not found", we can try and find a cleaner solution for that later... maybe need to embed a "not found" in the sidebar tab and make sure it's recoverable

@PVince81
Copy link
Member Author

PVince81 commented Feb 12, 2021

it seems that when deleting the current conversation there's already another handler that sets the route before it reaches the sessionIssueHandler, this is what triggers that dialog.
I might need a second mechanism to inhibit the dialog in this code path.

@PVince81
Copy link
Member Author

Aha, finally found something: when setting the route to not-found it doesn't automatically clear the token, this is why I've seen a lot of JS errors because the right sidebar is still present with its chat tab.

When the current conversation was deleted while in a call, or
whenever someone has been removed from a room, the redirect
now targets the not-found page instead of the one about duplicate
session.

Signed-off-by: Vincent Petry <[email protected]>
@PVince81 PVince81 force-pushed the bugfix/4552/fix-duplicate-on-delete branch from 67c21a2 to 2999b5d Compare February 12, 2021 14:18
@PVince81
Copy link
Member Author

PVince81 commented Feb 12, 2021

After some research and tests I realized the following:

  • we can push the "notfound" route instead of doing a full page reload:
    • the JS errors I saw before were only related to the fact that we forgot to clear the current token in some places
    • I've applied this now to this PR and it looks smooth when getting kicked out...
    • instead of a semi-global attribute for saying "please don't warn about leaving", we can use a route param when using named route, see https://router.vuejs.org/guide/essentials/navigation.html
  • duplicate session handling must trigger a page reload to avoid side effects
    • in my local tests if I simply push the duplicate session route, it interferes with the other tab where I click "Join here" and that other tab gets a "not found"
    • there's probably something not cleant up properly. Considering that we'll get rid of dupe sessions soonish I think it's not worth the effort fixing this.

@nickvergessen please re-review

@PVince81
Copy link
Member Author

@nickvergessen any concerns against merging this ?

I've noticed this might tie in with my discovery in #4670 (comment) where we could also make the "go to not found" event only work if the token we're leaving is also the one on the page.
This could be achieved by passing the token from the 404 error to the event. Then the event handler compares the passed token with the one in store. If it's the same, redirect. Else: ignore.

@nickvergessen

This comment has been minimized.

@PVince81
Copy link
Member Author

PVince81 commented Mar 1, 2021

When I tested in chromium back then I didn't see the browser's native dialog, because now we're using the router, it should switch you to the "not found" page without a page refresh. Strange that it didn't work.

I'll recheck and test with Firefox then.

@PVince81
Copy link
Member Author

PVince81 commented Mar 1, 2021

With the following I did not see any browser dialog:

  1. Login as admin in Chromium
  2. Create a conversation and invite user "alice"
  3. Start a call
  4. In Firefox login as "alice"
  5. Join the call
  6. Back in Chromium, open the left sidebar and delete the conversation

=> in the admin browser I land on the welcome page
=> in alice's browser I land on the "not found" page

Not sure if you used different steps or if it's a cache issue ?

@nickvergessen nickvergessen merged commit c48008e into master Mar 2, 2021
@nickvergessen nickvergessen deleted the bugfix/4552/fix-duplicate-on-delete branch March 2, 2021 09:12
@PVince81
Copy link
Member Author

PVince81 commented Mar 4, 2021

/backport to stable21.1

@nickvergessen
Copy link
Member

So I'm currently on the "Not found" page with Ivan in a call. We still hear each others as the webrtc stuff all didn't get canceled, but we can't change mic/camera/screenshare status anymore.

So I propose to go back to the redirect at least while being in a call.

@PVince81
Copy link
Member Author

So I'm currently on the "Not found" page with Ivan in a call. We still hear each others as the webrtc stuff all didn't get canceled, but we can't change mic/camera/screenshare status anymore.

So I propose to go back to the redirect at least while being in a call.

PR here: #5595

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error "Duplicate session" after deleting room when joined call

3 participants