Skip to content
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

⚠️ [MM-57781] Limit group calls to DMs only on unlicensed servers #809

Merged
merged 11 commits into from
Aug 13, 2024

Conversation

streamer45
Copy link
Collaborator

Summary

PR implements limits to restrict calls to DMs only in unlicensed servers. This means no functional changes for either Professional or Enterprise installations.

Note
This is a breaking change which is planned to be merged to make into v1 (MM v10 Sept release)

@cpoile Originally I wanted to make this fully backward compatible with older mobile clients by overriding the Enabled field in the channel response, which ultimately controls whether we show the join/start button, but looking at the mobile side, I don't think that's possible since it mostly relies on the handleGetAllCallChannelStates API handler which only returns explicitly set channels. Let me know if you have any ideas.

Spec

https://mattermost.atlassian.net/wiki/spaces/Calls/pages/2780495913/Calls+Limits#Limits-based-on-license

Related PRs

mattermost/calls-common#36

Ticket Link

https://mattermost.atlassian.net/browse/MM-57781

@streamer45 streamer45 added 2: Dev Review Requires review by a core committer 2: QA Review Requires review by a QA tester labels Jul 8, 2024
@streamer45 streamer45 added this to the v1.0.0 🚀 / MM 10.0 milestone Jul 8, 2024
@streamer45 streamer45 self-assigned this Jul 8, 2024
@streamer45 streamer45 requested a review from cpoile July 8, 2024 14:49
@streamer45 streamer45 added the Do Not Merge Should not be merged until this label is removed label Jul 8, 2024
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I wanted to make this fully backward compatible with older mobile clients by overriding the Enabled field in the channel response, which ultimately controls whether we show the join/start button, but looking at the mobile side, I don't think that's possible since it mostly relies on the handleGetAllCallChannelStates API handler which only returns explicitly set channels. Let me know if you have any ideas.

Right... We wanted to have a single call for channel states, and not a call for every switch (bc mobile), so we only take into account "explicitly enabled" and "explicitly disabled", and only on connect/reconnect...

I guess the backwards incompatibility aspect is that we'll get an error when trying to start a call, pre mobile PR, right? That seems acceptable (not that we have a choice, but it's not horrible). Considering that we explicitly chose to have the Start Call button always visible on mobile, even when calls was globally disabled (which is probably worse than this UX, tbh).

server/enterprise/license.go Show resolved Hide resolved
@cpoile cpoile removed the 2: Dev Review Requires review by a core committer label Jul 9, 2024
@streamer45
Copy link
Collaborator Author

Right... We wanted to have a single call for channel states, and not a call for every switch (bc mobile), so we only take into account "explicitly enabled" and "explicitly disabled", and only on connect/reconnect...

I guess the backwards incompatibility aspect is that we'll get an error when trying to start a call, pre mobile PR, right? That seems acceptable (not that we have a choice, but it's not horrible). Considering that we explicitly chose to have the Start Call button always visible on mobile, even when calls was globally disabled (which is probably worse than this UX, tbh).

@cpoile Yep, it will error out after the usual timeout, but I don't think it's avoidable. I am actually sending a ws error in this PR but it doesn't look like the mobile client is picking that up to fail fast.

I decided to implement a simple ephemeral message to cover this case (unsupported client) which makes this marginally easier on the user. It will still fail to connect but at least they get a reason. Let me know what you think.

image

@streamer45
Copy link
Collaborator Author

@DHaussermann We'd like to check the following:

  • Enterprise or Professional: no changes.
  • Everything else: calls available in DMs only. The channel header button to start/join the call should be hidden in any other channel type. Slash commands should also fail with an error.

Base automatically changed from MM-57782 to main July 17, 2024 14:11
@streamer45
Copy link
Collaborator Author

@DHaussermann Let me know if you need help with the above.

@DHaussermann
Copy link

Hi @streamer45,

  1. I need help testing this. I have this Calls build deployed locally and the mobile build on my device from the corresponding PR. However, I do not see the change where calls cannot be started or joined in outside DMs when there is no license. 0/5 maybe there is also a check for the server version being 10.0.0 or greater that I am missing? If so, not sure how I can put this in place locally.

  2. Is there another related PR somewhere for these restrictions on webapp? If I understand, the cope here seems to be only for mobile.

@streamer45
Copy link
Collaborator Author

@DHaussermann No version checks that I am aware of. Could you confirm the plugin build you are testing against is 8cb9c1f9e520754d56a382ff2bb1033d95561bf7? You can check from the /plugins/com.mattermost.calls/version endpoint.

@DHaussermann
Copy link

@streamer45
Yes, according to the end point I have the right build.
image

I also removed the plugin webapp node node dependencies and re-installed them as a precaution incase something odd was happening there but I still see the start functionality.

If it matters my local is running master branch for the server build. Maybe ping me if you have other suggestions here.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and passed

  • Tested that Call are only available in DMs when restricted by license with v1.0.0.
  • In this case Calls options in channel menu and slash command are not available in Public channels, Private Channels or GM channels
  • Tested the following license cases with the new Calls build:
    • E0
    • Team Edition
    • Legacy E20 license
    • Cloud Enterprise
    • Cloud Professional
    • Self hosted Enterprise
  • Tested the following clients
    • Webapp
    • Desktop App
    • Mobile Build for v2.20 on Android and iOS
    • Mobile Builds prior tov2.20 on Android and iOS
    • For all clients the Calls start slash command has also been restricted.
  • In cases above, no restrictions observed when a license was in place and the new Calls build for v1.0.0 is running
  • For Cases with E0 OR Team Eddition I can see the restrictions
  • Ensured no change when running a an existing Calls build of 0.29.0

I notice that if all that you are changing is adding or removing the license, the webapp needs to be refreshed to reflect the change in functionality. However I feel this is minor because restarting the plugin also serves to bring the state of the webapp current and the plugin restart will happen during the Upgrade to Calls v1.0.0 so this only affects cases where the license file is modified after the upgrade without a server restart or a plugin restart and is easily solved by restarting the plugin.

LGTM! Thanks @streamer45.

@DHaussermann DHaussermann added 3: Reviews Complete All reviewers have approved the pull request and removed 2: QA Review Requires review by a QA tester labels Aug 12, 2024
@streamer45 streamer45 removed the Do Not Merge Should not be merged until this label is removed label Aug 12, 2024
@streamer45 streamer45 merged commit 5490d7c into main Aug 13, 2024
19 checks passed
@streamer45 streamer45 deleted the MM-57781 branch August 13, 2024 06:24
@thereisnotime
Copy link

This is really crappy!

@ThiefMaster
Copy link

Yeah, REMOVING existing features and putting them behind a paywall after they've been available for evreyone feels rather evil...

@thereisnotime
Copy link

Yeah, REMOVING existing features and putting them behind a paywall after they've been available for evreyone feels rather evil...

This feels like Rocket chat all over again...

@the-black-wolf
Copy link

Wow, ok. Didn't know things are going so bad for you that you had to put a feature so ubiquitous in 2024, as a team voice call is, behind a paywall. Which is really weird, imho, you should be looking at adding video calls to self hosted servers, not removing stuff.
Yea, its free and I have no grounds to complain, fine. Thank you for the free product, honestly, and we do have a workaround for meetings. I am just worried about something else, is this the sign of times to come? Are you going down the Slack route and should we expect a 90 days message limit in the future? This does affect us operationally, please advise in time so that we do not read it in the main Mattermost changelog when people start complaining.

@streamer45
Copy link
Collaborator Author

@the-black-wolf I am sorry to hear this is impacting your team. I'd encourage you to reach us directly in-channel (e.g. https://community.mattermost.com/core/pl/xka6s5sfx7fopp61axw914rcbc) since this PR is likely going to be only seen by me and a few other developers who had not much to do with the product decision behind this.

@githubkoma
Copy link

oh no is this really happening or just a bad dream?
i hoped for Mattermost to get a Discord Alternative step by step, but okay, dreams and teams shattered

@codingPotato21
Copy link

This should be rolled back at once! Calls are a basic feature and enough features are being offered for the premium like call recording and even video calls can be added to that list. But normal voice calls with screen sharing are not something special. Calls in DM's are useless and surely have very rare use-cases.

Find other ways to make money without forcing people to pay. I would rather and will use the zoom plugin (Or any other third-party video plugin) and pay for a better priced API rather than being forced to pay to use this plugin!

@ThiefMaster
Copy link

Calls in DM's are useless and surely have very rare use-cases.

Not sure how true THAT is tbh. While I'm very much not a fan of paywalling calls outside DMs, the vast majority of calls on my pretty large instance is in DMs... YMMV though.

@chumbalum
Copy link

Very disappointing change. Any reason for this?

@roest01
Copy link

roest01 commented Sep 26, 2024

Bruhh ! This is a bad move. Mattermost is going evil.

@Judx
Copy link

Judx commented Oct 23, 2024

What a bummer! This should be reverted ASAP❗

@zehDonut
Copy link

Even as a paying customer, putting core features behind a paywall is a reason to drop your product entirely.

@mz-aimcom
Copy link

Maybe this is working for some of you: #864 (comment)

services:
  mattermost:
    environment:
      # additional settings
      - MM_CALLS_GROUP_CALLS_ALLOWED=true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.