fix issue not show icon start call in chaneel - #1221
Conversation
…icense checker - Updated session tests to allow calls in DMs only for unlicensed cloud environments and added a new test for self-hosted deployments. - Modified the LicenseChecker to support group calls in self-hosted deployments without requiring a Professional license, based on environment variables.
📝 WalkthroughWalkthrough
ChangesGroup Calls: Self-Hosted Unrestricted
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/session_test.go (1)
171-185: ⚡ Quick winSubtest name overstates coverage (“all channels”).
This case only asserts
model.ChannelTypeOpen. Please either rename it to match what it verifies, or add private/group/direct cases so it actually validates “all channels” on self-hosted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/session_test.go` around lines 171 - 185, The test subtest "allow calls in all channels on self-hosted" is misleading because the addUserSession call at the end only tests with model.ChannelTypeOpen, not all channel types. Either rename the subtest to accurately describe that it only tests open channels, or expand the test to include additional assertions for private, group, and direct channel types (model.ChannelTypePrivate, model.ChannelTypeGroup, model.ChannelTypeDirect) to match the "all channels" claim in the test name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/session_test.go`:
- Around line 109-113: To stabilize the flaky tests, you need to pin the
MM_CALLS_GROUP_CALLS_ALLOWED environment variable in both test subtests to
ensure they don't depend on the runner's environment configuration. For the
"allow calls in DMs only on unlicensed cloud" subtest (at lines 109-113) and the
other affected subtest (at lines 171-175), use Go's testing utilities (such as
t.Setenv) to explicitly set the MM_CALLS_GROUP_CALLS_ALLOWED environment
variable to the expected value at the start of each subtest, ensuring the
GroupCallsAllowed() function behavior is consistent regardless of the host
environment's settings.
In `@webapp/src/components/channel_header_dropdown_button/index.ts`:
- Line 18: The code is using isDmGmChannel which includes both direct message
and group message channels, but the backend only permits direct message channels
to be callable when group calls are disallowed on unlicensed cloud. Replace the
isDmGmChannel import and its usage at line 26 with a function or check that only
verifies direct message channels (excluding group message channels) to align the
UI behavior with the backend policy that blocks group channels from being
callable.
---
Nitpick comments:
In `@server/session_test.go`:
- Around line 171-185: The test subtest "allow calls in all channels on
self-hosted" is misleading because the addUserSession call at the end only tests
with model.ChannelTypeOpen, not all channel types. Either rename the subtest to
accurately describe that it only tests open channels, or expand the test to
include additional assertions for private, group, and direct channel types
(model.ChannelTypePrivate, model.ChannelTypeGroup, model.ChannelTypeDirect) to
match the "all channels" claim in the test name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 696ccb76-9723-4346-a549-2a88f7973520
📒 Files selected for processing (3)
server/enterprise/license.goserver/session_test.gowebapp/src/components/channel_header_dropdown_button/index.ts
| t.Run("allow calls in DMs only on unlicensed cloud", func(t *testing.T) { | ||
| defer mockAPI.AssertExpectations(t) | ||
| defer mockMetrics.AssertExpectations(t) | ||
| defer ResetTestStore(t, p.store) | ||
|
|
There was a problem hiding this comment.
Stabilize these tests by pinning MM_CALLS_GROUP_CALLS_ALLOWED.
Both subtests rely on the default branch of GroupCallsAllowed(), but they never set/reset the env var. If the runner environment sets MM_CALLS_GROUP_CALLS_ALLOWED, these expectations can flip and become flaky.
Suggested fix
t.Run("allow calls in DMs only on unlicensed cloud", func(t *testing.T) {
+ t.Setenv("MM_CALLS_GROUP_CALLS_ALLOWED", "")
defer mockAPI.AssertExpectations(t)
defer mockMetrics.AssertExpectations(t)
defer ResetTestStore(t, p.store)
@@
t.Run("allow calls in all channels on self-hosted", func(t *testing.T) {
+ t.Setenv("MM_CALLS_GROUP_CALLS_ALLOWED", "")
defer mockAPI.AssertExpectations(t)
defer mockMetrics.AssertExpectations(t)
defer ResetTestStore(t, p.store)Also applies to: 171-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/session_test.go` around lines 109 - 113, To stabilize the flaky tests,
you need to pin the MM_CALLS_GROUP_CALLS_ALLOWED environment variable in both
test subtests to ensure they don't depend on the runner's environment
configuration. For the "allow calls in DMs only on unlicensed cloud" subtest (at
lines 109-113) and the other affected subtest (at lines 171-175), use Go's
testing utilities (such as t.Setenv) to explicitly set the
MM_CALLS_GROUP_CALLS_ALLOWED environment variable to the expected value at the
start of each subtest, ensuring the GroupCallsAllowed() function behavior is
consistent regardless of the host environment's settings.
| isLimitRestricted, | ||
| maxParticipants, | ||
| } from 'src/selectors'; | ||
| import {isDmGmChannel} from 'src/utils'; |
There was a problem hiding this comment.
GM channels are being shown as callable where backend still blocks them on unlicensed cloud.
Line 26 uses isDmGmChannel(channel), which includes GM. But backend policy still only exempts direct channels when group calls are disallowed, and the updated backend tests still expect group channels to fail. This creates a UI/backend contract mismatch.
Suggested fix
-import {isDmGmChannel} from 'src/utils';
@@
- show: callsShowButton(state, channel?.id) && (areGroupCallsAllowed(state) || isDmGmChannel(channel)),
+ show: callsShowButton(state, channel?.id) && (areGroupCallsAllowed(state) || channel?.type === 'D'),Also applies to: 26-26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webapp/src/components/channel_header_dropdown_button/index.ts` at line 18,
The code is using isDmGmChannel which includes both direct message and group
message channels, but the backend only permits direct message channels to be
callable when group calls are disallowed on unlicensed cloud. Replace the
isDmGmChannel import and its usage at line 26 with a function or check that only
verifies direct message channels (excluding group message channels) to align the
UI behavior with the backend policy that blocks group channels from being
callable.
|
Thanks for the contribution! A few concerns from review, most significant first: 1. 2. 3. |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
3 participants
…icense checker
Summary
Ticket Link
Screenshots
Release Note