test: Display upsell modal for voice calls on community edition. #37230
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 E2E tests and supporting page-object fragments for the Team voice-calls upsell: new CE test suite, new upsell modal fragment, modal close helper, added HomeContent locators, and an accessibility id wired into TeamsVoipConfigModal. Changes
Sequence Diagram(s)sequenceDiagram
actor Tester as Test Runner
participant Play as Playwright
participant UI as App UI
participant Upsell as "Team voice calls" Modal
rect rgb(245,250,255)
note right of Tester: Voice Calls CE upsell flows
Tester->>Play: start test, open /home and prepare session
Play->>UI: open DM with user2 / open user menu / open contact info
end
alt DM toolbar
Play->>UI: click `btnVoiceCall` (room toolbar)
UI-->>Upsell: render upsell modal
else Contact Info
Play->>UI: click `btnContactInfoVoiceCall` (contact sidebar)
UI-->>Upsell: render upsell modal
else User Menu
Play->>UI: select "New voice call" from user menu
UI-->>Upsell: render upsell modal
end
Play->>Upsell: assert visible
Play->>Upsell: optionally call `close()` → modal dismissed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)apps/meteor/tests/e2e/page-objects/**/*.ts📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
apps/meteor/tests/e2e/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
⏰ 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)
🔇 Additional comments (1)
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: 0
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts (1)
302-304: Scope the selector to avoid ambiguity.The
btnVoiceCalllocator searches the entire page for any button with the name "Voice call". This could match multiple buttons if both the toolbar button and contact information button are visible simultaneously, potentially causing test flakiness.Consider scoping the selector to the primary room actions toolbar:
get btnVoiceCall(): Locator { - return this.page.getByRole('button', { name: 'Voice call' }); + return this.primaryRoomActionsToolbar.getByRole('button', { name: 'Voice call' }); }This pattern is consistent with
btnVideoCallat line 492, which already uses this scoping approach.
📜 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 (2)
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts(2 hunks)apps/meteor/tests/e2e/voice-calls-ce.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/voice-calls-ce.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/voice-calls-ce.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/voice-calls-ce.spec.ts
🧬 Code graph analysis (1)
apps/meteor/tests/e2e/voice-calls-ce.spec.ts (1)
apps/meteor/tests/e2e/page-objects/home-channel.ts (1)
HomeChannel(5-123)
⏰ 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). (6)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
apps/meteor/tests/e2e/voice-calls-ce.spec.ts (5)
1-6: LGTM!The imports and storage state configuration are correct and follow the established patterns for E2E tests.
20-30: LGTM!The test properly verifies the upsell modal appears when clicking the voice call button from a DM. The use of
test.stepfor organization and web-first assertions (toBeVisible) follows best practices.
32-43: LGTM!The test correctly validates the upsell modal appears when clicking the voice call button from the contact information sidebar. Well-structured with clear test steps.
45-54: LGTM!The test properly verifies the upsell modal appears when initiating a new voice call from the user menu. Clear test organization and appropriate use of page object methods.
11-13: No changes needed:createDirectMessagealready creates a DM betweenuser1anduser2.apps/meteor/tests/e2e/page-objects/fragments/home-content.ts (2)
306-308: LGTM!The modal locator uses a semantic selector targeting a unique heading text. This is an appropriate way to locate the upsell modal.
330-332: LGTM!The locator is well-scoped by first finding the group container before locating the button. This prevents ambiguity with other voice call buttons on the page.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37230 +/- ##
===========================================
+ Coverage 67.59% 67.61% +0.02%
===========================================
Files 3338 3338
Lines 113720 113721 +1
Branches 20657 20660 +3
===========================================
+ Hits 76868 76896 +28
+ Misses 34171 34146 -25
+ Partials 2681 2679 -2
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: 1
📜 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 (4)
apps/meteor/client/views/room/contextualBar/TeamsVoipConfigModal.tsx(3 hunks)apps/meteor/tests/e2e/page-objects/fragments/home-content.ts(2 hunks)apps/meteor/tests/e2e/page-objects/fragments/upsell-modal.ts(1 hunks)apps/meteor/tests/e2e/voice-calls-ce.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/tests/e2e/voice-calls-ce.spec.ts
- apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/fragments/upsell-modal.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/page-objects/fragments/upsell-modal.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/page-objects/fragments/upsell-modal.ts
🧬 Code graph analysis (2)
apps/meteor/tests/e2e/page-objects/fragments/upsell-modal.ts (1)
apps/meteor/tests/e2e/page-objects/modal.ts (1)
Modal(3-25)
apps/meteor/client/views/room/contextualBar/TeamsVoipConfigModal.tsx (1)
apps/meteor/tests/e2e/page-objects/modal.ts (1)
Modal(3-25)
🔇 Additional comments (2)
apps/meteor/client/views/room/contextualBar/TeamsVoipConfigModal.tsx (1)
19-19: LGTM! Excellent accessibility improvement.The use of React's
useIdhook to generate a unique identifier and wire it to both the modal'saria-labelledbyand the title'sidattribute follows ARIA best practices for modal dialogs. This ensures proper screen reader announcement of the modal's purpose.Also applies to: 35-35, 50-50, 54-54
apps/meteor/tests/e2e/page-objects/fragments/upsell-modal.ts (1)
10-21: Implementation follows E2E best practices.The code properly uses semantic locators (
getByRole), stores commonly used locators as getters for reuse, and follows the Page Object Model pattern as required by the coding guidelines. Assuming the Modal base class supports the constructor pattern and provides therootproperty andwaitForDismissal()method, this implementation is sound.
There was a problem hiding this comment.
I think this is ok for now, but our CI is overloaded with naive system level tests. We have to to keep in mind that system level tests are very costly for us. We have to question how we can guarantee test coverage without counting on simulating a real user so frequently
Harmeet221
added
the
stat: QA assured
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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/tests/e2e/page-objects/fragments/modal.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/fragments/modal.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/page-objects/fragments/modal.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/page-objects/fragments/modal.ts
⏰ 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 (1)
apps/meteor/tests/e2e/page-objects/fragments/modal.ts (1)
16-18: LGTM!The implementation correctly mirrors the existing
save()pattern, uses semantic locators per guidelines, and provides a reusable close utility for modal interactions.Also applies to: 20-21, 23-23
dionisio-bot
bot
added
the
stat: ready to merge
) Co-authored-by: Douglas Fabris <[email protected]>
) Co-authored-by: Douglas Fabris <[email protected]>
JIRA Task: https://rocketchat.atlassian.net/browse/QA-104
This PR adds automated test coverage to verify that the upsell modal is displayed for voice call actions in the Community Edition.
Scenarios
DM Action: Should display the upsell modal when the user clicks on the Voice Call button inside a Direct Message.
User Info Action: Should display the upsell modal when the user clicks the Voice Call button from the User Info sidebar.
User Menu Action: Should display the upsell modal when the user initiates a New Voice Call from the User Menu.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Tests
Tests / Test helpers
Accessibility