matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

End jitsi call when member is banned

Open maheichyk opened this issue 2 years ago • 11 comments

Jitsi call needs to be ended when member is banned.

Currently if user A bans user B, then A don't see B in the call anymore, but B stays in the call (call window dialog is minimized) and B can hear/see what is going on in the call.

This PR invokes Jitsi call ending logic when user is banned.


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • End jitsi call when member is banned (#8879). Contributed by @maheichyk.

maheichyk avatar Jun 22 '22 12:06 maheichyk

Signed-off-by: Mikhail Aheichyk [email protected]

maheichyk avatar Jun 22 '22 12:06 maheichyk

It only partially solves the "Jitsi" problem, but at least it will fix that any always-on-screen widget will be closed when the user is banned from a room. It is not more inconsistent than the current flow where the leave state closes the call but also don't actually block the user from joining it again. Both with leave or ban, the home server will not accept any requests from a widget anymore.

dhenneke avatar Jun 23 '22 13:06 dhenneke

It would still improve the sitation for some use-cases, we have examples of closed environments where this change would be very helpful.

@turt2live what changes could be applied to this PR to make it acceptable?

benparsons avatar Jun 28 '22 12:06 benparsons

Hi @turt2live could you approve this MR or provide some more feedback?

maheichyk avatar Jul 08 '22 06:07 maheichyk

@turt2live thanks for feedback. Since the change is fine, we would like it to be approved and merged to be used. I currently don't have enough experience and time to create cypress test for this issue unfortunately, but an internal ticket has been created and will be addressed later. Hope this approach works.

maheichyk avatar Jul 11 '22 09:07 maheichyk

The test should be relatively easy as it won't need to test Jitsi. If you run into issues, visiting #element-dev:matrix.org on Matrix would be best so the team can help out.

At the moment, we require tests when the change can reasonably have tests - this PR should be on the easier side to test.

turt2live avatar Jul 11 '22 13:07 turt2live

@turt2live e2e tests are added, please review

maheichyk avatar Aug 02 '22 08:08 maheichyk

Hi @turt2live, could you please approve?

maheichyk avatar Aug 08 '22 06:08 maheichyk

Hi @turt2live branch was updated to fix an issue in the spec connected to matrix-js-sdk version, could you please rerun workflows?

maheichyk avatar Aug 10 '22 07:08 maheichyk

widgets/widget-pip-close.spec.ts fails. Maybe somebody (@turt2live) could help how to fix that? Error:

Oops...we found an error preparing this test file:

  > cypress/e2e/widgets/widget-pip-close.spec.ts

The error was:

Error: Webpack Compilation Error
./node_modules/matrix-js-sdk/src/matrix.ts 53:9
Module parse failed: Unexpected token (53:9)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
| export * from './@types/search';
| export * from './models/room-summary';
> export * as ContentHelpers from "./content-helpers";
| export {
|     createNewMatrixCall,
 @ ./cypress/e2e/widgets/widget-pip-close.spec.ts 28:17-52
    at handle (/home/runner/.cache/Cypress/10.4.0/Cypress/resources/app/node_modules/@packages/server/node_modules/@cypress/webpack-preprocessor/dist/index.js:180:23)

....

This occurred while Cypress was compiling and bundling your test code. This is usually caused by:

- A missing file or dependency
- A syntax error in the file or one of its dependencies

maheichyk avatar Aug 11 '22 16:08 maheichyk

That looks like something went wrong in the js-sdk - if you have a js-sdk branch name which matches this PR's branch name then it might need updating/fixing, or possibly just merge latest react-sdk develop into this PR (again).

turt2live avatar Aug 11 '22 16:08 turt2live

Hi @t3chguy, @kerryarchibald could you please merge these changes?

This PR was approved by Travis but he cannot merge it right now as far as I know.

I did additional changes to the widget-pip-close.spec.ts spec that were necessary due to webpack compilation error and updates to 'matrix-js-sdk'.

maheichyk avatar Aug 16 '22 09:08 maheichyk

@t3chguy thanks for review, please approve :) Could you please trigger workflows to see that all fine? (since I am first-time contributor the workflows are not started automatically with my commits)

maheichyk avatar Aug 16 '22 10:08 maheichyk

timeline.spec.ts failed twice in a row, looks like an issue to me, but not related to this PR

maheichyk avatar Aug 16 '22 13:08 maheichyk

Looks sane to me otherwise, and once the CI passes, thanks for fixing!!

@t3chguy Thanks a lot for checking that!

I fixed the comment issue and with the next update of develop branch could restart the pipelines. If my test is fine but some other one not, could you merge PR then?

maheichyk avatar Aug 18 '22 13:08 maheichyk