raghavaggarwal2308 commented 2 years ago

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-commenter commented 2 years ago

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

```diff @@ 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](https://codecov.io/gh/mattermost/mattermost-plugin-jitsi/pull/219?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost) | Coverage Δ | | |---|---|---| | [server/api.go](https://codecov.io/gh/mattermost/mattermost-plugin-jitsi/pull/219/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL2FwaS5nbw==) | `0.00% <0.00%> (ø)` | | | [server/configuration.go](https://codecov.io/gh/mattermost/mattermost-plugin-jitsi/pull/219/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL2NvbmZpZ3VyYXRpb24uZ28=) | `13.23% <0.00%> (-3.44%)` | :arrow_down: | | [server/manifest.go](https://codecov.io/gh/mattermost/mattermost-plugin-jitsi/pull/219/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL21hbmlmZXN0Lmdv) | `100.00% <ø> (ø)` | | | [server/plugin.go](https://codecov.io/gh/mattermost/mattermost-plugin-jitsi/pull/219/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL3BsdWdpbi5nbw==) | `44.57% <11.73%> (-17.21%)` | :arrow_down: | | [server/command.go](https://codecov.io/gh/mattermost/mattermost-plugin-jitsi/pull/219/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL2NvbW1hbmQuZ28=) | `60.49% <20.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Kshitij-Katiyar commented 1 year ago

@mickmister review fixes fixed

raghavaggarwal2308 commented 1 year ago

@mickmister Gentle reminder for re-review

raghavaggarwal2308 commented 1 year ago

@mickmister Gentle reminder for re-review

raghavaggarwal2308 commented 1 year ago

@mickmister gentle reminder for re-review

raghavaggarwal2308 commented 1 year ago

@mickmister Gentle reminder for re-review

mickmister commented 1 year ago

@raghavaggarwal2308 @Kshitij-Katiyar I added some more comments on some existing threads. Please let me know your thoughts :+1:

Kshitij-Katiyar commented 1 year ago

@mickmister replied to some existing threads.

VincentSC commented 1 year ago

I continued the conversation on some existing threads

Which treads? Can you link them here?

Kshitij-Katiyar commented 1 year ago

@mickmister This new commit mainly has 2 changes:- 1) I removed the store and reducers which we have created for Jaas individually. 2) 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.

VincentSC commented 1 year ago

Small remark: README.md needs an update too. But preferably as a follow-up PR, since this PR has been delayed for years already...

VincentSC commented 1 year ago

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

VincentSC commented 1 year ago

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.

raghavaggarwal2308 commented 7 months ago

@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.

VincentSC commented 7 months ago

@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)

VincentSC commented 6 months ago

Can this be rebased against https://github.com/mattermost/mattermost-plugin-jitsi/pull/237 ? Simply because that PR builds, and this one doesn't.

mickmister commented 6 months ago

@raghavaggarwal2308 Can you please update your branch with master to so it includes https://github.com/mattermost/mattermost-plugin-jitsi/pull/237?

VincentSC commented 6 months ago

It has been merged into master, I just saw.

raghavaggarwal2308 commented 6 months ago

@mickmister @VincentSC Updated

Snektron commented 6 months ago

It seems to be working for me (after fixing above issues)

VincentSC commented 5 months ago

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.

mickmister commented 5 months ago

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

mattermost-community / mattermost-plugin-jitsi

[GH-183] Add jaas support #219

Summary

Screenshots

Ticket

Codecov Report