mattermost-plugin-jitsi
mattermost-plugin-jitsi copied to clipboard
mattermost-community
[GH-183] Add jaas support
Summary
Most of the code here is from #185 . The person who created this PR was not responding to the review comments for a while. So, we recreated the same PR with the review comments fixed.
The review comments fixed are-
- Added i18n translation to texts in Jitsi plugin system console settings.
- Updated UI of system console to clarify the type of Jitsi server user will be using
- An error occurred when we were using the locally-generated private key to sign the JWT token for JaaS. That error is also fixed in this PR.
Screenshots
-
Updated UI
-
Updated description to clarify how to generate a private key
Ticket
Fixes #183 (JWT Configuration with JaaS)
Codecov Report
Base: 45.47% // Head: 36.69% // Decreases project coverage by -8.78% :warning:
Coverage data is based on head (
9966780) compared to base (f52f77e). Patch coverage: 7.33% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
- Coverage 45.47% 36.69% -8.79%
==========================================
Files 7 7
Lines 873 1101 +228
==========================================
+ Hits 397 404 +7
- Misses 451 669 +218
- Partials 25 28 +3
| Impacted Files | Coverage Δ | |
|---|---|---|
| server/api.go | 0.00% <0.00%> (ø) |
|
| server/configuration.go | 13.23% <0.00%> (-3.44%) |
:arrow_down: |
| server/manifest.go | 100.00% <ø> (ø) |
|
| server/plugin.go | 44.57% <11.73%> (-17.21%) |
:arrow_down: |
| server/command.go | 60.49% <20.00%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
@raghavaggarwal2308 @Kshitij-Katiyar I added some more comments on some existing threads. Please let me know your thoughts :+1:
I continued the conversation on some existing threads
Which treads? Can you link them here?
@mickmister This new commit mainly has 2 changes:-
- I removed the store and reducers which we have created for Jaas individually.
- The load config API was getting called when the mattermost page gets rendered but that API has a check auth which checks whether the user is logged in or not and if the user is not logged it will return the "Unauthorised" error and the redux state in the frontend will not get updated. So I created a dummy component that will get mounted on the user log in and call the API and update the states.
Small remark: README.md needs an update too. But preferably as a follow-up PR, since this PR has been delayed for years already...
For who wants to test, see https://github.com/mattermost/mattermost-plugin-jitsi/issues/206 how to build. Use the gh pr checkout 219 to checkout this PR.
I'll be testing it out the coming weeks. I think it needs some more documentation, but that should not be obstructing for this PR
Problems we found till now (everybody in EU):
- doesn't handle high DPI very well. In some moments some UI elements were off-screen
- for some users the video would freeze
- audio and video were slightly out-of-sync
- video would turn blurry once in a while
I think all of these problems can be seen as different problem from this PR, except maybe the first. We have not tested with Jitsi's public server.
@mickmister @VincentSC This PR has been on hold for a while. Should we assign this for QA review?
doesn't handle high DPI very well. In some moments some UI elements were off-screen
I am not sure if this is something we can handle as we are just sending a payload to an external Jitsi API. Can we create a separate issue for this if this is not concerning? So, this PR can be merged as it has been delayed a lot already.
@mickmister @VincentSC This PR has been on hold for a while. Should we assign this for QA review?
As the plugin has many problems (it does not build, as the dependencies are very outdated), I have not managed to test it yet. Even my comment of https://github.com/mattermost/mattermost-plugin-jitsi/issues/206#issuecomment-1428737754 is now outdated...
Suggestion for order of fixes:
- update dependencies (so we can close #206)
- 8x8 support (this PR, rebased)
- UI glitches (of later concern, needs new issue as suggested)
Can this be rebased against https://github.com/mattermost/mattermost-plugin-jitsi/pull/237 ? Simply because that PR builds, and this one doesn't.
@raghavaggarwal2308 Can you please update your branch with master to so it includes https://github.com/mattermost/mattermost-plugin-jitsi/pull/237?
We tested for a few weeks. One big problem: every reconnect adds another MAU (Monthly Active User). So it becomes more a pay-per-use or a pay-per-connection instead of a pay-per-user. :(
Got this as a reply from 8x8:
Regarding the MAU count, you can check our docs here, so in short we store a deviceId on the local storage of the browser when a user joins a meeting, if the user clears the cookies, switches browser or device it will join with a different deviceId thus it will be counted as an extra MAU.
Hi @VincentSC, thanks for reporting this.
every reconnect adds another MAU (Monthly Active User).
Do you happen to have any suggestions on how we might solve this on our end? I'm not 100% sure what is meant by "reconnect" here