Skip to content

Comments

refactor: use permission service for team delete trpc endpoint#24077

Merged
sean-brydon merged 5 commits intomainfrom
refactor/delete-handler
Sep 26, 2025
Merged

refactor: use permission service for team delete trpc endpoint#24077
sean-brydon merged 5 commits intomainfrom
refactor/delete-handler

Conversation

@hbjORbj
Copy link
Contributor

@hbjORbj hbjORbj commented Sep 25, 2025

What does this PR do?

  • Use Permission Check Service for team delete trpc endpoint

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A - I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Please use the latest Vercel preview and test please 🙏.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 25, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Replaces a direct owner check with PermissionCheckService-based checks: the handler reads the team via prisma.team.findUnique (handles NOT_FOUND and isOrganization), then checks organization.delete if isOrganization is true, otherwise team.delete, falling back to MembershipRole.OWNER. If the permission check fails it returns FORBIDDEN; if it passes it calls TeamService.delete. Adds imports for PermissionCheckService, prisma, and MembershipRole, and adds a default export for deleteHandler.

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly and concisely summarizes the primary change by stating that the team delete TRPC endpoint is being refactored to use the permission service, directly reflecting the main purpose of the pull request.
Description Check ✅ Passed The pull request description accurately describes using the Permission Check Service for the team delete TRPC endpoint and includes relevant checklist and testing instructions, making it clearly related to the changeset.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@hbjORbj hbjORbj changed the title refactor: delete handler refactor: use permission service for team delete trpc endpoint Sep 25, 2025
@graphite-app graphite-app bot requested review from a team September 25, 2025 13:15
@keithwillcode keithwillcode added core area: core, team members only foundation labels Sep 25, 2025
eunjae-lee
eunjae-lee previously approved these changes Sep 25, 2025
@dosubot dosubot bot added teams area: teams, round robin, collective, managed event-types 💻 refactor labels Sep 25, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
packages/trpc/server/routers/viewer/teams/delete.handler.ts (2)

31-31: Avoid default export; prefer named export only

Default exports hinder tree‑shaking and refactors. Keep the named export only.

-export default deleteHandler;

18-24: Confirm fallbackRoles and consider DI for PermissionCheckService

  • “team.delete” is registered in PERMISSION_REGISTRY.
  • Decide if deletion should be limited to owners only; if admins need delete rights, include MembershipRole.ADMIN in fallbackRoles.
  • Optional: hoist new PermissionCheckService to module scope or inject via ctx for reuse and testability.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • 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 8ed0fb6 and abbe6f7.

📒 Files selected for processing (1)
  • packages/trpc/server/routers/viewer/teams/delete.handler.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/trpc/server/routers/viewer/teams/delete.handler.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • packages/trpc/server/routers/viewer/teams/delete.handler.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/trpc/server/routers/viewer/teams/delete.handler.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts:13-24
Timestamp: 2025-08-21T16:34:10.839Z
Learning: In calcom/cal.com PR #22995, the deletePhoneNumber function in packages/trpc/server/routers/viewer/phoneNumber/delete.handler.ts is only used for imported phone numbers that don't have active Stripe subscriptions. Purchased phone numbers with subscriptions use a separate cancellation flow first (per maintainer Udit-takkar).
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/teams/delete.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (2)
  • PermissionCheckService (19-306)
  • hasPermission (183-201)
🔇 Additional comments (2)
packages/trpc/server/routers/viewer/teams/delete.handler.ts (2)

1-1: LGTM: Importing PermissionCheckService

Import path and usage look correct.


3-3: LGTM: Importing MembershipRole

Enums import is appropriate for fallback role checks.

@hbjORbj hbjORbj marked this pull request as draft September 25, 2025 13:27
@vercel
Copy link

vercel bot commented Sep 25, 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 Sep 26, 2025 1:29am
cal-eu Ignored Ignored Sep 26, 2025 1:29am

@hbjORbj hbjORbj marked this pull request as ready for review September 25, 2025 13:54
@hbjORbj hbjORbj marked this pull request as draft September 25, 2025 13:54
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/teams/delete.handler.ts (1)

43-43: Prefer named exports over default exports.

Handlers in this codebase are typically consumed via named exports; introducing a default export makes refactors harder and goes against our TS guideline to avoid defaults. Please drop the default export and keep the existing named export.

Apply this diff:

-export default deleteHandler;
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • 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 abbe6f7 and 310ab04.

📒 Files selected for processing (1)
  • packages/trpc/server/routers/viewer/teams/delete.handler.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/trpc/server/routers/viewer/teams/delete.handler.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • packages/trpc/server/routers/viewer/teams/delete.handler.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/trpc/server/routers/viewer/teams/delete.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/teams/delete.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (2)
  • PermissionCheckService (19-306)
  • hasPermission (183-201)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Tests / E2E API v2
  • GitHub Check: Tests / E2E Atoms
  • GitHub Check: Tests / E2E Embed React
  • GitHub Check: Tests / E2E Embed Core
  • GitHub Check: Tests / Integration
  • GitHub Check: Tests / E2E (1/4)
  • GitHub Check: Tests / E2E (3/4)
  • GitHub Check: Tests / E2E (4/4)

@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2025

E2E results are ready!

@sean-brydon sean-brydon merged commit fba096f into main Sep 26, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core area: core, team members only foundation ready-for-e2e 💻 refactor size/S teams area: teams, round robin, collective, managed event-types

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants