fix(outbound): Upsell modal conditions #36930
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 |
|
WalkthroughExtends OutboundMessageUpsellModal API with optional isCommunity and refines admin gating. Hook now derives isCommunity from license state and passes it to the modal. Tests are reorganized to cover admin vs non-admin and community vs non-community scenarios, updating expected buttons, annotations, and external-link behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant VC as View/Component
participant H as useOutboundMessageUpsellModal
participant L as useLicense
participant M as OutboundMessageUpsellModal
participant N as Navigation
U->>VC: Triggers open()
VC->>H: open()
H->>L: license = useLicense()
H->>M: setModal(<OutboundMessageUpsellModal isAdmin, hasModule, isCommunity = !license>)
Note right of M: Renders buttons based on isAdmin/hasModule/isCommunity
alt Admin && !hasModule
alt isCommunity
U->>M: Click "Upgrade"
M->>N: Open upgrade flow
else Non-community
U->>M: Click "Contact sales"
M->>N: Open sales URL
end
else Non-admin or hasModule
U->>M: Click "Learn more"
M->>N: Open docs URL
end
U->>M: Close modal
M-->>H: onClose()
H-->>VC: setModal(null)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Comment |
aleksandernsilva
changed the base branch from
develop
to
feat/outbound-msg-ui
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/useOutboundMessageUpsellModal.tsx (2)
16-18: Avoid assuming “community” while license is still loadingIf
license.datais undefined at click time,!license.data?.licenseevaluates totrueand the modal will show an “Upgrade” CTA for licensed workspaces until the hook resolves. Consider deferring modal open until license is known, or defaulting to a neutral/“Contact sales” state and/or moving the license read into the modal so it re-renders when license resolves. Please verify howuseLicense()behaves here.
7-7: Small readability win: deriveisCommunityonceComputing once makes the JSX cleaner and avoids double-negation in render sites.
import { useHasLicenseModule } from '../../../../../hooks/useHasLicenseModule'; import { useLicense } from '../../../../../hooks/useLicense'; export const useOutboundMessageUpsellModal = () => { const setModal = useSetModal(); const isAdmin = useRole('admin'); - const license = useLicense(); + const license = useLicense(); + const isCommunity = !license.data?.license; const hasModule = useHasLicenseModule('outbound-messaging') === true; const close = useEffectEvent(() => setModal(null)); const open = useEffectEvent(() => - setModal(<OutboundMessageUpsellModal isCommunity={!license.data?.license} isAdmin={isAdmin} hasModule={hasModule} onClose={close} />), + setModal(<OutboundMessageUpsellModal isCommunity={isCommunity} isAdmin={isAdmin} hasModule={hasModule} onClose={close} />), );Also applies to: 12-12, 16-18
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.tsx (2)
18-18: Default optional props to avoid relying on “undefined is falsy”This makes intent explicit and prevents accidental upsell path if a caller forgets to pass
hasModule.-const OutboundMessageUpsellModal = ({ isCommunity, hasModule, isAdmin, onClose }: OutboundMessageUpsellModalProps) => { +const OutboundMessageUpsellModal = ({ isCommunity, hasModule = false, isAdmin = false, onClose }: OutboundMessageUpsellModalProps) => {
28-30: “Upgrade” label still opens the sales link—confirm intended behaviorWhen
isCommunityis true, the confirm label is “Upgrade” butonConfirmnavigates to the Contact Sales URL. If “Upgrade” should route to a self-serve upgrade/marketplace/billing page, update the link or rename the CTA to avoid mismatched expectations. Also consider adding a test for the “Upgrade” click target.apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.spec.tsx (1)
56-61: Add click assertion for the “Upgrade” CTAWe render the button but don’t verify its action. Add a test to assert it opens the expected link.
it('should render "Upgrade" button when isCommunity is true', () => { render(<OutboundMessageUpsellModal isAdmin isCommunity onClose={onClose} />, { wrapper: appRoot.build() }); expect(screen.getByRole('button', { name: 'Upgrade' })).toBeInTheDocument(); expect(screen.queryByRole('button', { name: 'Contact sales' })).not.toBeInTheDocument(); }); + +it('should call openExternalLink with sales link when "Upgrade" is clicked', async () => { + render(<OutboundMessageUpsellModal isAdmin isCommunity onClose={onClose} />, { wrapper: appRoot.build() }); + await userEvent.click(screen.getByRole('button', { name: 'Upgrade' })); + expect(openExternalLink).toHaveBeenCalledWith('https://go.rocket.chat/i/contact-sales'); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.tsx(2 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/useOutboundMessageUpsellModal.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/useOutboundMessageUpsellModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useSetModal(68-68)
⏰ 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). (5)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
🔇 Additional comments (4)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.tsx (2)
24-32: Admin gating logic looks correctShowing the upsell CTA only for
isAdmin && !hasModulealigns with the PR goal.
40-40: Good dependency coverage
useMemoincludesisCommunityand handlers; props won’t go stale.apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.spec.tsx (2)
50-55: Admin no-module path assertions look goodBoth buttons rendered as expected for admins.
62-66: Link/annotation expectations are preciseDocs and sales link checks plus annotation visibility look correct for admin scenarios.
Also applies to: 68-72, 74-77
...hannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.spec.tsx
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/outbound-msg-ui #36930 +/- ##
========================================================
- Coverage 66.92% 66.80% -0.13%
========================================================
Files 3398 3382 -16
Lines 117272 114577 -2695
Branches 21534 21597 +63
========================================================
- Hits 78486 76542 -1944
+ Misses 36093 35333 -760
- Partials 2693 2702 +9
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.spec.tsx (1)
31-46: Resolved: “hasModule is false” tests no longer passhasModuleThis addresses the earlier mismatch noted in a past review. Nice cleanup.
🧹 Nitpick comments (4)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/useOutboundMessageUpsellModal.tsx (2)
16-18: Prevent “community” misclassification while license is loadingWhen
license.datais undefined,!license.data?.licenseevaluates totrue, showing “Upgrade” prematurely. Passundefinedwhile loading to default to the non-community path in the modal.- const open = useEffectEvent(() => - setModal(<OutboundMessageUpsellModal isCommunity={!license.data?.license} isAdmin={isAdmin} hasModule={hasModule} onClose={close} />), - ); + const open = useEffectEvent(() => + setModal( + <OutboundMessageUpsellModal + isCommunity={license.data ? !license.data.license : undefined} + isAdmin={isAdmin} + hasModule={hasModule} + onClose={close} + />, + ), + );Note: Confirm that
useEffectEventguarantees the latestlicensevalue at call time to avoid stale closures. If not, we should memoize the element or recompute on license changes.
13-13: Prefer boolean coercion over=== trueMore idiomatic and resilient to non-boolean returns from the hook.
- const hasModule = useHasLicenseModule('outbound-messaging') === true; + const hasModule = Boolean(useHasLicenseModule('outbound-messaging'));apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.tsx (1)
28-29: “Upgrade” label should route to an upgrade path, not salesCurrently the “Upgrade” button still opens the sales link. Suggest routing to a dedicated upgrade URL.
const OMNICHANNEL_DOCS_LINK = 'https://go.rocket.chat/i/omnichannel-docs'; const CONTACT_SALES_LINK = 'https://go.rocket.chat/i/contact-sales'; +const UPGRADE_LINK = 'https://go.rocket.chat/i/upgrade'; @@ - confirmText: isCommunity ? t('Upgrade') : t('Contact_sales'), - onConfirm: () => openExternalLink(CONTACT_SALES_LINK), + confirmText: isCommunity ? t('Upgrade') : t('Contact_sales'), + onConfirm: () => openExternalLink(isCommunity ? UPGRADE_LINK : CONTACT_SALES_LINK),If a different deep link is desired (e.g., cloud console or in-app admin route), replace
UPGRADE_LINKaccordingly.apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.spec.tsx (1)
56-60: Add a click test for the “Upgrade” CTAEnsure we assert the outbound link for the “Upgrade” path as well.
it('should render "Upgrade" button when isCommunity is true', () => { render(<OutboundMessageUpsellModal isAdmin isCommunity onClose={onClose} />, { wrapper: appRoot.build() }); expect(screen.getByRole('button', { name: 'Upgrade' })).toBeInTheDocument(); expect(screen.queryByRole('button', { name: 'Contact sales' })).not.toBeInTheDocument(); }); + + it('should call openExternalLink with upgrade link when "Upgrade" is clicked', async () => { + render(<OutboundMessageUpsellModal isAdmin isCommunity onClose={onClose} />, { wrapper: appRoot.build() }); + await userEvent.click(screen.getByRole('button', { name: 'Upgrade' })); + expect(openExternalLink).toHaveBeenCalledWith('https://go.rocket.chat/i/upgrade'); + });If product decides to keep “Upgrade” -> sales for now, change the expected URL accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.tsx(2 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/useOutboundMessageUpsellModal.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.spec.tsx (1)
apps/meteor/client/lib/appLayout.tsx (1)
render(26-28)
⏰ 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 (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageUpsellModal/OutboundMessageUpsellModal.tsx (2)
11-13: Prop addition looks good
isCommunity?: booleanis a clean extension of the props surface.
24-24: Confirm gating rule changeSwitching to
isAdmin && !hasModulechanges who sees the CTA. Please confirm this matches product expectations for admins with and without the module.
aleksandernsilva
merged commit aaa3705
into
feat/outbound-msg-ui
Proposed changes (including videos or screenshots)
Issue(s)
CTZ-334
CTZ-333
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests