feat: Replace old voice extension assignment flow (#37245) #37312
Conversation
🦋 Changeset detectedLatest commit: 8d8c242 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR migrates VOIP extension assignment from a dedicated modal-based UI (separate Assign/Remove modals and button) to an integrated input field within the user edit form. It adds extension field support across REST APIs, server validation, and UI components while removing the legacy modal components and associated tests. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant Form as Admin Form
participant API as API Server
participant DB as Database
rect rgb(100, 150, 200)
Note over Admin,DB: Old Flow (Removed)
Admin->>Form: Click "Assign Extension"
Form->>Form: Open AssignExtensionModal
Admin->>API: POST /voip-freeswitch.extension.assign
end
rect rgb(150, 200, 150)
Note over Admin,DB: New Flow (Added)
Admin->>Form: Edit User → Enter Extension
Form->>Form: canManageVoipExtension check
Admin->>Form: Submit Form
Form->>API: POST /v1/users.create or /v1/users.update
API->>API: canEditExtension validation
API->>API: Check permission & settings
API->>API: Verify extension not in use
API->>DB: Save user with freeSwitchExtension
DB-->>API: Success
API-->>Form: User updated
Form-->>Admin: Display extension in UserInfo
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the old voice extension assignment flow by integrating extension management directly into the user creation and update forms, eliminating the need for separate modals and buttons.
Key Changes:
- Added
freeSwitchExtensionfield to user create and update API endpoints - Integrated voice extension input directly into the user admin form
- Deprecated the
/v1/voip-freeswitch.extension.assignendpoint (version 8.0.0)
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/rest-typings/src/v1/users/UsersUpdateParamsPOST.ts |
Added freeSwitchExtension parameter to user update API type definition and schema |
packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts |
Added freeSwitchExtension parameter to user create API type definition and schema |
apps/meteor/tests/end-to-end/api/users.ts |
Added comprehensive test coverage for voice extension management during user creation and updates |
apps/meteor/tests/data/users.helper.ts |
Added freeSwitchExtension parameter to test user creation helper |
apps/meteor/ee/app/api-enterprise/server/voip-freeswitch.ts |
Marked extension assignment endpoint as deprecated in favor of users.update |
apps/meteor/client/views/room/contextualBar/UserInfo/UserInfoWithData.tsx |
Added freeSwitchExtension field to user info display |
apps/meteor/client/views/admin/users/voip/hooks/useVoipExtensionAction.tsx |
Removed old action hook for modal-based extension management |
apps/meteor/client/views/admin/users/voip/RemoveExtensionModal.tsx |
Removed modal component for extension removal |
apps/meteor/client/views/admin/users/voip/AssignExtensionModal.tsx |
Removed modal component for extension assignment |
apps/meteor/client/views/admin/users/UsersTable/UsersTableRow.tsx |
Removed voip extension action from user table row actions |
apps/meteor/client/views/admin/users/UsersTable/UsersTable.tsx |
Updated import path for permission hook |
apps/meteor/client/views/admin/users/UsersPageHeaderContent.tsx |
Removed "Assign Extension" button from header |
apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx |
Added freeSwitchExtension to user info data retrieval |
apps/meteor/client/views/admin/users/AdminUserForm.tsx |
Added voice extension input field to user form |
apps/meteor/client/components/UserInfo/UserInfo.tsx |
Added voice extension field display to user info component |
apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts |
Added validation logic for extension editing permissions |
apps/meteor/app/lib/server/functions/saveUser/saveUser.ts |
Implemented extension field updates in user save logic |
apps/meteor/app/lib/server/functions/saveUser/saveNewUser.ts |
Added extension field handling for new user creation |
apps/meteor/app/api/server/v1/users.ts |
Added extension validation to user creation endpoint |
Comments suppressed due to low confidence (2)
apps/meteor/client/views/admin/users/voip/RemoveExtensionModal.spec.tsx:1
- Duplicate test case name. This test is named 'should call onClose when cancel button is clicked' but tests the Close button, not the Cancel button. The test on line 31 already tests the Cancel button with the same name.
apps/meteor/client/views/admin/users/voip/AssignExtensionModal.spec.tsx:1 - Duplicate test case name. This test is named 'should call onClose when cancel button is clicked' but tests the Close button, not the Cancel button. The test on line 104 already tests the Cancel button with the same name.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
.changeset/shy-bats-worry.md (1)
1-5: Changeset format and version bump look correct.The YAML frontmatter is properly formatted, and "minor" is an appropriate version bump for a feature addition with UI/API changes.
However, the description is very brief. The AI summary indicates this PR includes backend changes (REST API field support, server validation, test removals), but the changeset only mentions the UI migration. Consider whether the description should be expanded to reflect the scope of backend/API changes, depending on your team's changeset convention.
Example of a more comprehensive description:
-Replaces old `Assign Extension` button and modal by introducing a proper input in the user edit form. +Replaces old `Assign Extension` button and modal flow with a direct input field in the user edit form. Adds support for the `freeSwitchExtension` field across REST APIs, includes server-side validation, and removes legacy modal components.apps/meteor/app/lib/server/functions/saveUser/saveNewUser.ts (1)
50-52: Consider trimming whitespace from the extension value.The code correctly checks for non-empty strings, but extensions with only whitespace characters could still be stored. Consider trimming the value before the check.
Apply this diff to add trimming:
- if (typeof userData.freeSwitchExtension === 'string' && userData.freeSwitchExtension !== '') { - updater.set('freeSwitchExtension', userData.freeSwitchExtension); + if (typeof userData.freeSwitchExtension === 'string' && userData.freeSwitchExtension.trim() !== '') { + updater.set('freeSwitchExtension', userData.freeSwitchExtension.trim()); }apps/meteor/app/lib/server/functions/saveUser/saveUser.ts (1)
167-173: Consider consistent trimming of the extension value.The code trims the value for the empty-string check (line 168) but doesn't trim before setting (line 171). This could allow extensions with leading/trailing whitespace to be stored.
Apply this diff to ensure consistent trimming:
if (typeof userData.freeSwitchExtension === 'string' && userData.freeSwitchExtension !== (oldUserData?.freeSwitchExtension ?? '')) { - if (userData.freeSwitchExtension.trim() === '') { + const trimmedExtension = userData.freeSwitchExtension.trim(); + if (trimmedExtension === '') { updater.unset('freeSwitchExtension'); } else { - updater.set('freeSwitchExtension', userData.freeSwitchExtension); + updater.set('freeSwitchExtension', trimmedExtension); } }apps/meteor/client/views/admin/users/AdminUserForm.tsx (1)
345-356: Consider adding client-side validation for the extension field.The field currently has no validation rules. While the backend validates uniqueness and permissions, client-side validation for format or length constraints would improve the user experience.
Example validation:
<Controller control={control} name='freeSwitchExtension' rules={{ maxLength: { value: 20, message: t('Max_length_is', 20) }, pattern: { value: /^[0-9]*$/, message: t('Only_numbers_allowed') } }} render={({ field }) => ( <TextInput {...field} id={voiceExtensionId} flexGrow={1} error={errors.freeSwitchExtension?.message} aria-invalid={errors.freeSwitchExtension ? 'true' : 'false'} /> )} />Note: Adjust the validation pattern based on your extension format requirements.
apps/meteor/client/components/UserInfo/UserInfo.tsx (1)
185-190: Field rendering implemented correctly.The voice extension field follows the established pattern for optional user information fields. The conditional rendering, component structure, and placement are all appropriate.
Consider verifying that the translation key
'Voice_call_extension'exists in the i18n files:#!/bin/bash # Description: Verify the translation key exists in i18n files # Search for the Voice_call_extension translation key rg -n "Voice_call_extension" --type jsonpackages/rest-typings/src/v1/users/UsersUpdateParamsPOST.ts (1)
109-112: Consider adding format validation for extension values.The schema defines
freeSwitchExtensionas a nullable string with no format constraints. Depending on FreeSWitch extension requirements (e.g., numeric-only, length limits), consider adding validation rules likepattern,minLength, ormaxLengthto prevent invalid values at the API boundary.
📜 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)
apps/meteor/client/components/UserInfo/__snapshots__/UserInfo.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (26)
.changeset/shy-bats-worry.md(1 hunks)apps/meteor/app/api/server/v1/users.ts(2 hunks)apps/meteor/app/lib/server/functions/saveUser/saveNewUser.ts(1 hunks)apps/meteor/app/lib/server/functions/saveUser/saveUser.ts(2 hunks)apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts(3 hunks)apps/meteor/client/components/UserInfo/UserInfo.stories.tsx(1 hunks)apps/meteor/client/components/UserInfo/UserInfo.tsx(3 hunks)apps/meteor/client/views/admin/users/AdminUserForm.tsx(6 hunks)apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx(2 hunks)apps/meteor/client/views/admin/users/UsersPageHeaderContent.spec.tsx(0 hunks)apps/meteor/client/views/admin/users/UsersPageHeaderContent.tsx(0 hunks)apps/meteor/client/views/admin/users/UsersTable/UsersTable.spec.tsx(1 hunks)apps/meteor/client/views/admin/users/UsersTable/UsersTable.tsx(1 hunks)apps/meteor/client/views/admin/users/UsersTable/UsersTableRow.tsx(0 hunks)apps/meteor/client/views/admin/users/voip/AssignExtensionButton.tsx(0 hunks)apps/meteor/client/views/admin/users/voip/AssignExtensionModal.spec.tsx(0 hunks)apps/meteor/client/views/admin/users/voip/AssignExtensionModal.tsx(0 hunks)apps/meteor/client/views/admin/users/voip/RemoveExtensionModal.spec.tsx(0 hunks)apps/meteor/client/views/admin/users/voip/RemoveExtensionModal.tsx(0 hunks)apps/meteor/client/views/admin/users/voip/hooks/useVoipExtensionAction.tsx(0 hunks)apps/meteor/client/views/room/contextualBar/UserInfo/UserInfoWithData.tsx(2 hunks)apps/meteor/ee/app/api-enterprise/server/voip-freeswitch.ts(1 hunks)apps/meteor/tests/data/users.helper.ts(1 hunks)apps/meteor/tests/end-to-end/api/users.ts(2 hunks)packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts(2 hunks)packages/rest-typings/src/v1/users/UsersUpdateParamsPOST.ts(2 hunks)
💤 Files with no reviewable changes (9)
- apps/meteor/client/views/admin/users/UsersPageHeaderContent.spec.tsx
- apps/meteor/client/views/admin/users/voip/AssignExtensionModal.tsx
- apps/meteor/client/views/admin/users/voip/AssignExtensionModal.spec.tsx
- apps/meteor/client/views/admin/users/UsersTable/UsersTableRow.tsx
- apps/meteor/client/views/admin/users/voip/RemoveExtensionModal.tsx
- apps/meteor/client/views/admin/users/voip/AssignExtensionButton.tsx
- apps/meteor/client/views/admin/users/voip/RemoveExtensionModal.spec.tsx
- apps/meteor/client/views/admin/users/UsersPageHeaderContent.tsx
- apps/meteor/client/views/admin/users/voip/hooks/useVoipExtensionAction.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
apps/meteor/tests/end-to-end/api/users.ts (3)
apps/meteor/tests/data/users.helper.ts (3)
TestUser(8-8)deleteUser(55-62)createUser(10-37)packages/core-typings/src/IUser.ts (1)
IUser(186-255)apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)
apps/meteor/client/views/admin/users/AdminUserForm.tsx (3)
packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts (1)
UserCreateParamsPOST(8-28)packages/core-typings/src/IUser.ts (1)
AvatarObject(318-318)apps/meteor/client/views/admin/users/useVoipExtensionPermission.tsx (1)
useVoipExtensionPermission(3-8)
apps/meteor/client/components/UserInfo/UserInfo.tsx (1)
apps/meteor/app/utils/lib/i18n.ts (1)
t(6-6)
apps/meteor/app/api/server/v1/users.ts (1)
apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts (1)
canEditExtension(15-29)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/users.ts
[error] 586-589: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 2420-2422: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 2424-2430: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ 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). (1)
- GitHub Check: 📦 Build Packages
🔇 Additional comments (17)
apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx (1)
70-70: LGTM!The extraction and propagation of
freeSwitchExtensionis consistent with other user fields and correctly integrates with the existing data flow.Also applies to: 94-94
apps/meteor/client/components/UserInfo/UserInfo.stories.tsx (1)
38-41: LGTM!The new story variant appropriately demonstrates the voice call extension field in the UI.
apps/meteor/client/views/room/contextualBar/UserInfo/UserInfoWithData.tsx (1)
68-68: LGTM!Consistent with the AdminUserInfoWithData implementation. The field is properly extracted and propagated to the UI.
Also applies to: 91-91
apps/meteor/app/api/server/v1/users.ts (1)
329-331: LGTM!The permission and validation check is correctly positioned before user creation. The error message and code are clear and appropriate. Note that user updates are also protected via the
validateUserEditingfunction called fromsaveUser.apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts (2)
15-29: LGTM! Well-structured permission and validation helper.The
canEditExtensionfunction correctly validates VoIP settings, permissions, and extension uniqueness. The error message is clear and actionable.
118-126: LGTM! Proper integration of extension validation.The validation correctly uses
isEditingFieldto detect changes, which prevents false positives when a user already has an extension and no change is being made. The??operator appropriately handles the undefined case.apps/meteor/app/lib/server/functions/saveUser/saveUser.ts (1)
57-57: LGTM!The
freeSwitchExtensionfield is appropriately added to the publicSaveUserDatatype as an optional string.apps/meteor/client/views/admin/users/AdminUserForm.tsx (1)
127-127: LGTM!The permission check is properly computed and used to gate the UI field visibility.
apps/meteor/client/components/UserInfo/UserInfo.tsx (2)
43-43: Type definition correctly extended.The addition of
freeSwitchExtensiontoUserInfoDataPropsis properly typed and consistent with the existing pattern.
75-75: Component parameter correctly added.The
freeSwitchExtensionprop is properly destructured and matches the type definition.apps/meteor/client/views/admin/users/UsersTable/UsersTable.spec.tsx (2)
3-3: Import change is consistent with VoIP test removal.The removal of additional imports from this line (likely
screenand others) is consistent with the deletion of VoIP-specific test cases described in the AI summary.
1-21: Verify that test coverage for VoIP extension functionality exists elsewhere.The removal of VoIP extension tests from this file leaves only basic story rendering and accessibility checks. While this aligns with the PR's goal to replace the old voice extension assignment flow, ensure that:
- The new VoIP extension assignment flow has comprehensive test coverage in its own test files
- Integration tests cover the new flow's interaction with the users table
- No critical test scenarios were lost in the migration
Run the following script to locate test coverage for the new VoIP extension flow:
apps/meteor/client/views/admin/users/UsersTable/UsersTable.tsx (1)
24-24: Import path update verified successfully.The hook relocation is complete:
- New file exists at
apps/meteor/client/views/admin/users/useVoipExtensionPermission.tsx- Hook properly exports
useVoipExtensionPermission- No stale imports from the old path remain in the codebase
packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts (1)
25-25: LGTM - consistent with update params.The
freeSwitchExtensionfield addition mirrors the structure inUsersUpdateParamsPOST.ts, maintaining consistency across create and update endpoints. The same validation concerns regarding extension format and business logic checks apply here as noted in the update params review.Also applies to: 49-49
apps/meteor/tests/data/users.helper.ts (1)
21-21: LGTM - test helper properly extended.The addition of
freeSwitchExtension?: stringto theuserDataparameter correctly enables E2E tests to create users with voice extensions. The spread operator on line 30 ensures the field is passed through to the API request.packages/rest-typings/src/v1/users/UsersUpdateParamsPOST.ts (1)
28-28: Verify validation logic forfreeSwitchExtensionin the users.update endpoint.The type addition looks correct. However, the old
voip-freeswitch.extension.assignendpoint (lines 78-105 inapps/meteor/ee/app/api-enterprise/server/voip-freeswitch.ts) includes business logic to validate:
- VoIP is enabled (
settings.get('VoIP_TeamCollab_Enabled'))- Extension is not already assigned to another user
- Extension exists in the FreeSWitch system
Ensure these validations are implemented in the users.update flow, likely in the server-side save/validation logic mentioned in the relevant imports.
Run the following script to verify validation logic exists:
apps/meteor/ee/app/api-enterprise/server/voip-freeswitch.ts (1)
71-74: Verify deprecation version and document migration path.The deprecation notice correctly signals the transition to
/v1/users.updateas per the PR objective. However:
Version verification: Confirm that
8.0.0aligns with the project's planned deprecation timeline and semantic versioning policy.Migration documentation: API consumers using
voip-freeswitch.extension.assignwill need clear migration guidance:
- How to achieve the same result with
users.update(e.g.,POST /v1/users.updatewith{ userId, data: { freeSwitchExtension } })- Permission requirements (
manage-voip-extensionsvs. any different requirements forusers.update)- Behavioral differences, if any
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.12.0 #37312 +/- ##
==================================================
- Coverage 67.67% 67.59% -0.09%
==================================================
Files 3342 3338 -4
Lines 114027 113708 -319
Branches 20671 20646 -25
==================================================
- Hits 77170 76856 -314
+ Misses 34178 34169 -9
- Partials 2679 2683 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
New Features
Deprecated