regression: Outbound message permissions not available in Community Edition #37107
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughReplaces scattered outbound-message permission/license checks with a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI Entrypoints\n(Create New / Sidebar / Contact Info)
participant Access as useOutboundMessageAccess
participant Flags as Hooks\n(useOmnichannelEnabled / useHasLicenseModule / usePermission)
User->>UI: open action menu
UI->>Access: canSendOutboundMessage?
Access->>Flags: isOmnichannelEnabled()
alt Omnichannel disabled
Access-->>UI: false
UI-->>User: hide action
else
Access->>Flags: has livechat-enterprise? / has outbound-messaging?
alt Any module missing
Access-->>UI: true (upsell path)
UI-->>User: show action (may open upsell)
else All modules present
Access->>Flags: has 'outbound.send-messages'?
Access-->>UI: true / false
UI-->>User: show / hide action
end
end
sequenceDiagram
autonumber
actor User
participant Wizard as OutboundMessageWizard
participant Env as useOmnichannelEnabled
participant Lic as useHasLicenseModule
participant Prov as Providers API
participant Upsell as Upsell Modal
User->>Wizard: open wizard
Wizard->>Env: isOmnichannelEnabled?
alt Not enabled
Wizard-->>User: render error state
else Enabled
Wizard->>Lic: check livechat-enterprise & outbound-messaging
Wizard->>Prov: fetch providers
alt Modules & providers present
Wizard-->>User: render wizard UI
else Missing module/provider
Wizard->>Upsell: open upsell modal
Upsell-->>User: show upsell
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧬 Code graph analysis (3)apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundProvidersList.ts (2)
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.spec.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (10)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37107 +/- ##
==================================================
+ Coverage 67.39% 67.43% +0.03%
==================================================
Files 3328 3329 +1
Lines 113353 113387 +34
Branches 20568 20576 +8
==================================================
+ Hits 76399 76462 +63
+ Misses 34348 34327 -21
+ Partials 2606 2598 -8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR moves outbound messaging permissions from the enterprise-only Livechat module to the common authorization system, making these permissions available in Community Edition workspaces to enable the feature upsell flow.
- Removes four outbound messaging permissions from the enterprise-specific
omnichannelEEPermissionsarray - Adds the same four permissions to the general
permissionsarray in the authorization module
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/meteor/ee/app/livechat-enterprise/server/permissions.ts | Removes outbound messaging permissions from enterprise-only permissions |
| apps/meteor/app/authorization/server/constant/permissions.ts | Adds outbound messaging permissions to common authorization system |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
aleksandernsilva
force-pushed
the
regression/outbound-comms-permissions
branch
from
ba32d56 to
5621ef8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.tsx (1)
79-87: Verify useLayoutEffect is necessary.Switching to
useLayoutEffectprevents the wizard from flashing before the upsell modal opens, but it also blocks the browser paint. Consider whetheruseEffectwould be acceptable here—most modal libraries handle rendering imperatively and the brief flash is typically unnoticeable.The dependency array correctly includes
hasOmnichannelModule, which is used in the condition on line 84.If the flash isn't a UX concern, consider reverting to
useEffectto avoid blocking paint:- useLayoutEffect(() => { + useEffect(() => { if (isLoadingModule || isLoadingProviders) { return; } if (!hasOmnichannelModule || !hasOutboundModule || !hasProviders) { upsellModal.open(); } }, [hasOmnichannelModule, hasOutboundModule, hasProviders, isLoadingModule, isLoadingProviders, upsellModal]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
apps/meteor/client/NavBarV2/NavBarPagesGroup/hooks/useCreateNewItems.tsx(2 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.spec.tsx(4 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.tsx(4 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageWizardSkeleton.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.spec.tsx(2 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/index.ts(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundProvidersList.ts(2 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/index.ts(1 hunks)apps/meteor/client/sidebar/header/actions/hooks/useCreateRoomItems.tsx(2 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/meteor/client/components/Omnichannel/OutboundMessage/modals/index.ts
🧰 Additional context used
🧬 Code graph analysis (7)
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.tsx (1)
useOutboundMessageAccess(6-21)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundProvidersList.ts (2)
packages/ui-contexts/src/index.ts (1)
usePermission(55-55)apps/meteor/client/lib/queryKeys.ts (1)
omnichannelQueryKeys(30-84)
apps/meteor/client/NavBarV2/NavBarPagesGroup/hooks/useCreateNewItems.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.tsx (1)
useOutboundMessageAccess(6-21)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.spec.tsx (2)
packages/ui-contexts/src/index.ts (1)
usePermission(55-55)apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.tsx (1)
useOutboundMessageAccess(6-21)
apps/meteor/client/sidebar/header/actions/hooks/useCreateRoomItems.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.tsx (1)
useOutboundMessageAccess(6-21)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.tsx (1)
packages/ui-contexts/src/index.ts (1)
usePermission(55-55)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.spec.tsx (3)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)packages/ui-client/src/components/Wizard/WizardContext.tsx (1)
WizardContext(17-17)apps/meteor/tests/mocks/data.ts (1)
createFakeLicenseInfo(213-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build - coverage
🔇 Additional comments (16)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.spec.tsx (1)
10-11: LGTM! Clean type-only import.The imports are properly structured with the
typekeyword for OmnichannelContextValue, ensuring it's only used at the type level.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/components/OutboundMessageWizardSkeleton.tsx (1)
5-5: LGTM: Good accessibility improvement.Adding
role='status'andaria-busy='true'properly signals the loading state to assistive technologies.apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/index.ts (1)
1-1: LGTM: Clean barrel export pattern.Re-exporting the new hook provides a cleaner public API for consumers.
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx (1)
4-4: LGTM: Centralized access logic simplifies maintenance.Replacing dual checks (license + permission) with the unified
useOutboundMessageAccesshook aligns with the PR objective to enable Community Edition upsell flows while maintaining proper authorization in licensed environments.Also applies to: 17-21
apps/meteor/client/sidebar/header/actions/hooks/useCreateRoomItems.tsx (1)
2-2: LGTM: Consistent migration to centralized access hook.Replacing the direct permission check with
useOutboundMessageAccessmaintains consistency across all outbound message entry points and enables Community Edition users to see the menu item and trigger the upsell flow.Also applies to: 5-6, 25-25, 78-78
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.tsx (2)
7-7: LGTM: Appropriate omnichannel enablement check.Adding
useOmnichannelEnabledensures the wizard validates omnichannel status before rendering, preventing access when the feature is disabled at the workspace level.Also applies to: 14-14, 41-41
151-153: LGTM: Defense-in-depth check for omnichannel enablement.Adding an early return when omnichannel is disabled prevents direct access to the wizard even if users bypass the UI entry points. The ordering (omnichannel enabled → permission → modules/providers) is correct.
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.tsx (1)
1-21: LGTM! Clean implementation of the access control logic.The hook correctly implements the upsell flow by returning
truewhen either required module is missing, allowing Community Edition users to see the option and trigger the upsell modal. The strict=== truecomparisons at lines 8-9 ensure that undefined/loading states are treated as missing modules, which is appropriate for the upsell scenario.apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.spec.tsx (1)
1-82: Excellent test coverage!The test suite comprehensively covers all logical branches of the
useOutboundMessageAccesshook:
- Omnichannel disabled case
- Individual module missing scenarios (both
livechat-enterpriseandoutbound-messaging)- Both modules missing
- Permission granted/denied when all modules present
The mock implementations correctly simulate the different module availability states.
apps/meteor/client/NavBarV2/NavBarPagesGroup/hooks/useCreateNewItems.tsx (3)
2-2: Clean refactor replacing direct permission check.The removal of
usePermissionis appropriate since the newuseOutboundMessageAccesshook encapsulates the permission check along with the omnichannel and module validation logic.
6-7: LGTM! Appropriate import of the new access hook.
25-25: Access control correctly delegated to the new hook.This change enables the upsell flow for Community Edition users by using the centralized access logic that returns
truewhen modules are missing.apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundProvidersList.ts (1)
29-29: Query gating aligns with upsell flow design.The query is appropriately disabled when modules are missing, preventing unnecessary API calls while still allowing the UI to show (via
useOutboundMessageAccessreturningtruefor upsell). However, verify the omnichannel check consistency flagged above.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.spec.tsx (3)
63-73: Consider aligningLivechat_enabledsetting withomnichannelEnabledparameter.The
appRootfunction always setsLivechat_enabledtotrue(line 66) regardless of theomnichannelEnabledparameter value passed to the context (line 70). This could cause confusion ifuseOmnichannelEnabledchecks the setting while the context uses a different value.Verify whether this inconsistency is intentional for testing purposes, or if the setting should mirror the parameter:
const appRoot = (omnichannelEnabled = true) => mockAppRoot() .withJohnDoe() - .withSetting('Livechat_enabled', true) + .withSetting('Livechat_enabled', omnichannelEnabled) .withEndpoint('GET', '/v1/omnichannel/outbound/providers', () => getProvidersMock())
80-100: Excellent test coverage for error states.The new test group thoroughly validates loading, authorization, and omnichannel-disabled scenarios. The use of
screen.findByRoleandscreen.findByTextfollows testing-library best practices.
122-140: Tests correctly validate module presence for upsell flow.The updated tests appropriately include both
livechat-enterpriseandoutbound-messagingmodules to verify the upsell modal behavior when modules are present versus absent.
.../OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.spec.tsx
Show resolved
Hide resolved
.../Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.spec.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundProvidersList.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.spec.ts (1)
29-81: Comprehensive test coverage for the access logic.The test suite effectively covers all key scenarios:
- Omnichannel disabled (guard condition)
- Missing modules triggering upsell flow (lines 38-63) - this is critical for the Community Edition fix
- Permission-based access when modules are present
The mock implementations correctly test the OR condition in the hook (either module missing → upsell).
Optional: Consider adding a test case for when
useHasLicenseModulereturnsundefined:it('should return true if license module status is undefined (upsell)', () => { useOmnichannelEnabledMock.mockReturnValue(true); useHasLicenseModuleMock.mockReturnValue(undefined); usePermissionMock.mockReturnValue(true); const { result } = renderHook(() => useOutboundMessageAccess()); expect(result.current).toBe(true); });This would verify the explicit
=== truechecks handle undefined correctly.apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.ts (1)
6-21: Logic correctly implements the Community Edition upsell flow.The hook properly handles three scenarios:
- Omnichannel disabled (lines 12-14): Returns
false- option hidden entirely.- Missing modules (lines 16-18): Returns
true- enables the upsell flow for Community Edition. This is the key fix for the regression.- Modules present (line 20): Returns permission status - standard access control for Enterprise users.
The explicit
=== truechecks on lines 8-9 correctly treatundefined/nullas falsy, ensuring missing modules trigger the upsell path.Minor naming consideration: The hook name
useOutboundMessageAccessreturnstruefor both "has actual access" and "should show upsell". Consider adding a JSDoc comment to clarify:/** * Determines if the outbound message option should be visible. * Returns true when: * - User has actual access (licensed + permission), OR * - Modules are missing (triggers upsell flow for Community Edition) * Returns false when omnichannel is disabled or user lacks permission in Enterprise. */ export const useOutboundMessageAccess = (): boolean => {This clarifies that
truedoesn't always mean "full access" but rather "show the option (with upsell if needed)".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.spec.ts(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.spec.ts (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.ts (1)
useOutboundMessageAccess(6-21)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.ts (1)
packages/ui-contexts/src/index.ts (1)
usePermission(55-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.spec.ts (2)
8-22: Mock setup is correct for isolated testing.The mock setup appropriately covers the three dependencies (usePermission, useOmnichannelEnabled, useHasLicenseModule) needed for the hook. The use of
jest.mockedfor type-safe mock access is a good practice.
25-27: LGTM!Proper cleanup between tests using
jest.clearAllMocks().apps/meteor/client/components/Omnichannel/OutboundMessage/hooks/useOutboundMessageAccess.ts (1)
1-4: LGTM!All necessary dependencies are imported with correct paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.spec.tsx (1)
63-73: Consider aligningLivechat_enabledsetting with theomnichannelEnabledparameter.The
appRoothelper always setsLivechat_enabledtotrueregardless of theomnichannelEnabledparameter value. This could create confusion when testing omnichannel-disabled scenarios, where the setting and context don't align.Consider applying this diff:
const appRoot = (omnichannelEnabled = true) => mockAppRoot() .withJohnDoe() - .withSetting('Livechat_enabled', true) + .withSetting('Livechat_enabled', omnichannelEnabled) .withEndpoint('GET', '/v1/omnichannel/outbound/providers', () => getProvidersMock()) .withEndpoint('GET', '/v1/licenses.info', () => getLicenseMock()) .wrap((children) => ( <OmnichannelContext.Provider value={{ enabled: omnichannelEnabled } as OmnichannelContextValue}> <WizardContext.Provider value={mockWizardApi}>{children}</WizardContext.Provider> </OmnichannelContext.Provider> ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.spec.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.spec.tsx (3)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)packages/ui-client/src/components/Wizard/WizardContext.tsx (1)
WizardContext(17-17)apps/meteor/tests/mocks/data.ts (1)
createFakeLicenseInfo(213-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/OutboundMessageWizard.spec.tsx (4)
3-3: LGTM! Appropriate test imports added.The
screenimport andOmnichannelContextimports are correctly added and used throughout the updated tests.Also applies to: 8-9
80-100: LGTM! Comprehensive error and loading state coverage.The new test suite properly covers:
- Loading state with
aria-busyverification- Unauthorized access when permission is missing
- Omnichannel disabled scenario
Note: The typo "unauthrized" from the previous review has been corrected to "unauthorized" on line 89.
122-124: LGTM! License mocks correctly updated with required modules.The
activeModulesarrays now properly include both'livechat-enterprise'and'outbound-messaging', which aligns with the PR's goal of ensuring outbound messaging permissions are available in Community Edition and matches the expected module dependencies.Also applies to: 138-140
107-107: No explicit permission needed for module/provider upsell tests. TheuseOutboundMessageAccesshook returnstruewhenever either license module is missing—before checking permissions—so omitting.withPermission('outbound.send-messages')in these tests is correct.
aleksandernsilva
force-pushed
the
regression/outbound-comms-permissions
branch
from
bec9718 to
41e9ad5
Compare
…dition (#37107) Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
…dition (#37107) Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
3 participants
Proposed changes (including videos or screenshots)
This PR adjusts the validations for accessing the outbound message feature, allowing community workspaces to access the upsell flow.
Issue(s)
CTZ-348
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Accessibility
Refactor
Tests