chore: new ABAC upsell modal #37124
chore: new ABAC upsell modal #37124
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 |
|
WalkthroughAdds a new ABACUpsellModal React component with Storybook story, unit tests, and three English i18n strings; provides translations, mocked AppRoot decorator for stories/tests, and wiring for close/cancel/confirm interactions to upsell and modal context. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as ABACUpsellModal
participant License as useHasLicenseModule
participant Upsell as useUpsellActions
participant ModalCtx as useSetModal
participant Generic as GenericUpsellModal
User->>UI: open modal
UI->>License: check('abac')
UI->>Generic: render (i18n, image via getURL)
note right of Generic: shows Upgrade / Cancel / Close
User->>Generic: click Cancel or Close
Generic->>UI: onCancel / onClose
UI->>ModalCtx: setModal(null)
User->>Generic: click Upgrade (Confirm)
Generic->>UI: onConfirm
UI->>Upsell: handleManageSubscription()
Upsell->>User: redirect/open billing (external)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37124 +/- ##
===========================================
- Coverage 67.38% 67.38% -0.01%
===========================================
Files 3330 3331 +1
Lines 113490 113519 +29
Branches 20598 20598
===========================================
+ Hits 76480 76498 +18
- Misses 34403 34408 +5
- Partials 2607 2613 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
MartinSchoeler
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (1)
38-49: Consider adding interaction tests.While snapshot and accessibility tests provide good coverage, consider adding tests that verify user interaction behavior, such as:
- Clicking the close/cancel button calls
setModal(null)- Clicking the confirm button calls
handleManageSubscription- Modal renders different content based on
hasABACvalueExample interaction test:
it('should call handleManageSubscription when confirm is clicked', async () => { const mockHandleManageSubscription = jest.fn(); jest.mock('../../GenericUpsellModal/hooks', () => ({ useUpsellActions: jest.fn(() => ({ shouldShowUpsell: true, cloudWorkspaceHadTrial: false, handleManageSubscription: mockHandleManageSubscription, handleTalkToSales: jest.fn(), })), })); const { getByRole } = render(<ABACUpsellModal />, { wrapper: appRoot }); const confirmButton = getByRole('button', { name: /upgrade/i }); await userEvent.click(confirmButton); expect(mockHandleManageSubscription).toHaveBeenCalledTimes(1); });apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.stories.tsx (2)
7-41: Remove duplicateonCloseaction definition.The
onCloseaction is defined in both themeta.args(line 12-14) andDefault.args(line 38-40). The definition inDefault.argsis redundant since it will inherit frommeta.args.Apply this diff to remove the duplicate:
export const Default = { - args: { - onClose: action('onClose'), - }, };
17-24: Add missing translation keys to Storybook decorator.The decorator is missing
Premium_capability,Upgrade, andCanceltranslation keys that the component uses. While Storybook may still render the component, including all keys improves the story's accuracy and completeness.Apply this diff to add the missing keys:
const AppRoot = mockAppRoot() .withTranslations('en', 'core', { + Premium_capability: 'Premium capability', Attribute_based_access_control: 'Attribute-Based Access Control', Attribute_based_access_control_title: 'Automate complex access management across your entire organization', Attribute_based_access_control_description: 'ABAC automates room access, granting or revoking access based on dynamic user attributes rather than fixed roles.', + Upgrade: 'Upgrade', + Cancel: 'Cancel', }) .build();apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.tsx (1)
12-13: Create and link a tracking issue for the ABAC license module
The// @ts-expect-errorshows'abac'support isn’t implemented—add a project issue (with timeline) to schedule and track its completion.
📜 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 (2)
apps/meteor/client/components/ABAC/ABACUpsellModal/__snapshots__/ABACUpsellModal.spec.tsx.snapis excluded by!**/*.snapapps/meteor/public/images/abac-upsell-modal.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx(1 hunks)apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.stories.tsx(1 hunks)apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.stories.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useSetModal(68-68)
🔇 Additional comments (4)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (2)
1-24: LGTM! Appropriate mock setup for component isolation.The mocks correctly isolate the component's external dependencies (license detection, upsell actions, URL utilities) for testing, ensuring consistent test behavior.
26-36: Missing translation keys from the mock.The
appRootmock is missing thePremium_capability,Upgrade, andCanceltranslation keys that are used by the component (as seen in the component file). While the tests may still pass if these translations have fallback behavior, it's better to include all expected keys for completeness.Apply this diff to add the missing translation keys:
const appRoot = mockAppRoot() .withTranslations('en', 'core', { + Premium_capability: 'Premium capability', Attribute_based_access_control: 'Attribute-Based Access Control', Attribute_based_access_control_title: 'Automate complex access management across your entire organization', Attribute_based_access_control_description: 'ABAC automates room access, granting or revoking access based on dynamic user attributes rather than fixed roles.', + Upgrade: 'Upgrade', + Cancel: 'Cancel', }) .build();Likely an incorrect or invalid review comment.
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.tsx (2)
25-27: LGTM! Correct handler wiring.The event handlers correctly wire up the modal control:
onCloseandonCancelboth dismiss the modal viasetModal(null)onConfirmtriggers the subscription management flow
24-24: Image asset exists. The fileapps/meteor/public/images/abac-upsell-modal.svgis present.
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (2)
9-11: Remove unused useHasLicenseModule mock
Thejest.mock('../../../hooks/useHasLicenseModule', …)in ABACUpsellModal.spec.tsx is not needed—neither ABACUpsellModal nor GenericUpsellModal references this hook.
92-110: Consider the value of the multiple-clicks test.This test verifies that clicking multiple buttons in sequence works correctly, which is already implicitly covered by the individual interaction tests. While it adds confidence that handlers don't interfere with each other, it may be redundant.
Consider whether this test adds sufficient value to justify the maintenance cost, or if the individual interaction tests provide adequate coverage.
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.tsx (2)
6-9: Consider adding optionalonCancelprop for flexibility.While using
onClosefor both close and cancel actions (line 23) is reasonable for simplicity, exposing an optionalonCancelprop would provide more flexibility if different cancel behavior is needed in the future.Apply this diff to add the optional prop:
type ABACUpsellModalProps = { onClose: () => void; onConfirm: () => void; + onCancel?: () => void; };Then update the component:
-const ABACUpsellModal = ({ onClose, onConfirm }: ABACUpsellModalProps) => { +const ABACUpsellModal = ({ onClose, onConfirm, onCancel }: ABACUpsellModalProps) => { const { t } = useTranslation(); return ( <GenericUpsellModal tagline={t('Premium_capability')} title={t('Attribute_based_access_control')} subtitle={t('Attribute_based_access_control_title')} description={t('Attribute_based_access_control_description')} img={getURL('images/abac-upsell-modal.svg')} onClose={onClose} onConfirm={onConfirm} - onCancel={onClose} + onCancel={onCancel ?? onClose} imgHeight={256} /> ); };
24-24: Consider documenting the image height choice.The fixed height of 256 is hardcoded. If this value is specific to the ABAC upsell visual design, consider adding a brief comment explaining the choice, or if multiple upsell modals share similar dimensions, consider extracting it as a constant.
📜 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 (3)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx(1 hunks)apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.stories.tsx(1 hunks)apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.stories.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
⏰ 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 (30)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.tsx (11)
1-4: LGTM!The imports are clean and appropriate for the component's needs.
6-9: LGTM!The type definition is appropriate for the component's needs. Note that
onCancel(line 23) reuses theonClosehandler, which is a reasonable design choice.
11-27: LGTM!The component implementation is clean and follows React best practices:
- Proper hook usage for translations
- Clear prop forwarding to
GenericUpsellModal- Intentional design to forward
onCanceltoonClosehandler
29-29: LGTM!Standard default export pattern.
1-4: LGTM!The imports are clean and necessary for the component functionality.
29-29: LGTM!Standard default export following React component conventions.
11-27: Approve ABACUpsellModal implementation
All required translation keys are defined in packages/i18n/src/locales/en.i18n.json.
1-4: LGTM!The imports are appropriate for the component's needs: i18n translation hook, URL utility for image sources, and the generic upsell modal wrapper.
6-9: LGTM!The props interface is clean and minimal, defining only the essential callbacks needed by the consumer.
29-29: LGTM!Standard default export pattern for the component.
16-16: Translation key defined in i18n resources
ThePremium_capabilitykey is present in all locale files.apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (19)
1-6: LGTM!Modern testing setup with appropriate imports for accessibility testing and user interactions.
13-20: LGTM!The
useUpsellActionsmock provides appropriate test values for the upsell flow.
23-25: LGTM!Simple pass-through mock for the URL utility is appropriate for unit testing.
27-37: LGTM!Comprehensive translation setup for the test environment, covering all strings used in the modal.
39-45: LGTM!Clean test suite setup with proper mock cleanup between tests.
47-90: LGTM!Comprehensive test coverage including:
- Snapshot testing for visual regression detection
- Accessibility validation with jest-axe
- User interaction flows for all primary actions
The tests follow modern testing-library best practices using
userEventfor realistic interactions.
92-110: Ensure the modal is still mounted before clicking “Cancel,” or split confirm and cancel into separate tests. If “Upgrade” closes the modal, you won’t be able to click “Cancel” afterward—either add an assertion on modal visibility between clicks or test each action in isolation.
1-6: LGTM!Excellent choice of testing libraries, including jest-axe for accessibility testing.
8-25: LGTM!The mocks are appropriately configured to test the component in isolation. The setup effectively simulates the component's dependencies.
27-37: LGTM!The appRoot configuration properly provides all necessary translations for testing. The builder pattern usage is clean and the translation keys match those used in the component.
47-56: LGTM!Excellent test coverage including both snapshot testing and accessibility validation with jest-axe.
58-90: LGTM!The interaction tests properly verify that the correct callbacks are invoked for upgrade, cancel, and close actions. The use of
userEventprovides realistic user interaction simulation.
92-110: Note: Multiple clicks test doesn't reflect real-world modal behavior.While the test correctly verifies that both handlers can be called, in real-world usage the modal would typically close/unmount after the first button click (either upgrade or cancel), preventing subsequent clicks. This test is still valuable for verifying handler implementation but doesn't fully reflect the actual user experience.
Consider this test as a unit-level handler verification rather than an integration test of modal lifecycle behavior.
1-6: LGTM!The imports cover all necessary testing utilities: mock providers for app context, testing-library for rendering and queries, user-event for interactions, and axe for accessibility checks.
8-25: LGTM!The mocks appropriately isolate the component under test by providing controlled return values for external dependencies. The hook mocks align with expected behavior for an unlicensed state where the upsell should display.
27-37: LGTM!The test fixture properly sets up a mocked app root with all necessary translations. The translation keys match those used by the component and include button labels for interaction testing.
39-45: LGTM!Standard test suite structure with proper setup. Mock functions are declared at the appropriate scope, and
beforeEachensures test isolation by clearing mock state.
47-56: LGTM!The snapshot and accessibility tests provide baseline coverage. Using
baseElementfor snapshots captures the full rendered output, and the axe accessibility check is a valuable addition.
58-90: LGTM!The interaction tests thoroughly cover all user-triggered callbacks. Each test properly uses
userEvent.setup()for realistic interactions and verifies that the correct handler is called while ensuring the other is not.
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
removed
the
stat: QA assured
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Adds an upsell modal to be used in the ABAC admin screens (yet to be implemented, so a
chore)Issue(s)
ABAC-45
Summary by CodeRabbit
New Features
Tests
Documentation