Skip to content

Comments

fix: Consistently remove a member from organization/sub-team/team - through all the APIs and webapp actions#22806

Merged
hariombalhara merged 28 commits intomainfrom
fix-api-v2-org-user-membership-deletion
Aug 18, 2025
Merged

fix: Consistently remove a member from organization/sub-team/team - through all the APIs and webapp actions#22806
hariombalhara merged 28 commits intomainfrom
fix-api-v2-org-user-membership-deletion

Conversation

@hariombalhara
Copy link
Member

@hariombalhara hariombalhara commented Jul 30, 2025

What does this PR do?

This PR fixes the organization membership deletion functionality in API V2 by properly calling the TeamService.removeMembers method instead of directly deleting the membership record. This ensures that all necessary cleanup operations are performed when removing a user from an organization.

Fixes PRI-297

Bug Fixes

  • When setting organizationId=null, we explicitly make username unique so that slug constraint DB error is avoided
  • ChildrenManaged Events weren't deleted in case of team membership deletion, that is now fixed
  • Even though there are multiple tables need to be cleaned up when deleting a membership from org, that is being done in an atomic(mostly) manner now.

Technical Details:

  • The previous implementation was directly deleting memberships which could leave orphaned data
  • Now uses TeamService.removeMembers({ teamIds: [organizationId], userIds: [membership.userId], isOrg: true })
  • Maintains backward compatibility while ensuring proper cleanup

Visual Demo (For contributors especially)

N/A - This is a backend API change with no visual components.

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. N/A - This is an internal API fix that doesn't change the public API interface.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

Prerequisites:

  • Set up a local Cal.com development environment
  • Ensure you have API V2 enabled
  • Create a test organization with multiple members

Testing Steps:

  • Membership deletion should return 200 with the deleted membership data
  • All related data (hosts, permissions, etc.) should be properly cleaned up
  • Attempting to delete a non-existent membership returns 404
  • Non-admin users should receive 403 when attempting to delete memberships

Checklist

  • I have read the contributing guide
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked if my changes generate no new warnings

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jul 30, 2025

Walkthrough

The PR centralizes member-removal into TeamService.removeMembers, refactors deletion flows to fetch memberships and throw NotFound when absent, and replaces direct repository deletes with calls to TeamService (isOrg flag supported). It removes the legacy removeMember utility and a TeamsEventTypesService method, updates API v2 services/controllers to call TeamService, adds integration tests for TeamService.removeMembers, adds small test fixtures, and exports TeamService from platform libraries. Non-functional tweaks (imports/formatting) and a deleted skipped TRPC test were also included.

Assessment against linked issues

Objective Addressed Explanation
Remove sub-team memberships on org member removal (PRI-297)
Remove sub-team event hosts on org member removal (PRI-297)
Remove sub-team managed events for the user (PRI-297)
Delete user’s organization profile on org member removal (PRI-297)

Out-of-scope changes

Code Change Explanation
Removed method deleteUserTeamEventTypesAndHosts (apps/api/v2/src/modules/teams/event-types/services/teams-event-types.service.ts) This deletes a controller/service method unrelated to the specific PRI-297 objectives; it's a refactor choice rather than a direct requirement of the linked issue.
Deleted TRPC removeMember test file (packages/trpc/server/routers/viewer/teams/removeMember.test.ts) Test removal is not part of the functional objectives in PRI-297 and may reduce test coverage for TRPC handler behavior.

Possibly related PRs

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 729861b and 07bd81c.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (1)
  • apps/api/v2/package.json (1 hunks)
⏰ 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: Install dependencies / Yarn install & cache
  • GitHub Check: Atoms E2E Tests
🔇 Additional comments (1)
apps/api/v2/package.json (1)

41-41: Validated TeamService integration with pinned @calcom/platform-libraries

All imports of TeamService and calls to TeamService.removeMembers({ … }) across the codebase align with the object signature introduced in version 0.0.314. No mismatches or API-breaks were found.

• Verified imports in apps/api/v2/src/modules/**/services/*service.ts
• Confirmed removeMembers calls in both API and server packages
• Integration tests in packages/lib/server/service/__tests__/teamService.integration-test.ts pass against the new signature

If you intentionally want to bypass local workspace sources and always consume the published package, you can leave the alias as-is. Otherwise, switch back to workspace resolution to reflect local changes during development:

-    "@calcom/platform-libraries": "npm:@calcom/platform-libraries@0.0.314",
+    "@calcom/platform-libraries": "workspace:*",
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-api-v2-org-user-membership-deletion

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 30, 2025

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Fix membership deletion handling and add tests". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@hariombalhara hariombalhara self-assigned this Jul 30, 2025
@keithwillcode keithwillcode added core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO labels Jul 30, 2025
@linear
Copy link

linear bot commented Jul 30, 2025

PRI-297

@vercel
Copy link

vercel bot commented Jul 30, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
cal Ignored Ignored Aug 18, 2025 10:16am
cal-eu Ignored Ignored Aug 18, 2025 10:16am

@github-actions
Copy link
Contributor

github-actions bot commented Jul 30, 2025

E2E results are ready!

@hariombalhara hariombalhara force-pushed the fix-api-v2-org-user-membership-deletion branch from 8069a94 to 262ffe4 Compare July 30, 2025 09:19
@socket-security
Copy link

socket-security bot commented Jul 30, 2025

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

…ic methods

- Moved removeMember and all related helper functions from separate file into TeamService class
- Made all functions private static methods instead of exporting them
- Deleted the original removeMember.ts file since it's no longer needed
- Updated imports to use the new location
- All integration tests pass successfully
- Renamed memberId parameter to userId in removeMember private method
- Renamed memberIds parameter to userIds in removeMembers public method
- Updated all calls to removeMembers to use userIds instead of memberIds
- Parameter names now accurately reflect that they are user IDs, not membership IDs
…letion

- Add unit tests for membership deletion services
- Remove redundant deletion behavior tests from controllers
- Keep only happy path tests in e2e controller tests
- Fix unused imports and variables
@hariombalhara hariombalhara force-pushed the fix-api-v2-org-user-membership-deletion branch from baf51ea to 3fef285 Compare July 31, 2025 07:52
@hariombalhara
Copy link
Member Author

hariombalhara commented Aug 14, 2025

@ThyMinimalDev could you publish a new version with the changes, I took the latest merge of main which had bumped platform libraries and the API V2 build started failing again

@alishaz-polymath replied to your feedback.

ThyMinimalDev
ThyMinimalDev previously approved these changes Aug 14, 2025
ThyMinimalDev
ThyMinimalDev previously approved these changes Aug 14, 2025
Copy link
Contributor

@keithwillcode keithwillcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving since previously approved and libraries were bumped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api area: API, enterprise API, access token, OAuth core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO organizations area: organizations, orgs platform Anything related to our platform plan ready-for-e2e teams area: teams, round robin, collective, managed event-types

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants