feat: ✨member based limits controlled by org admins#23233
feat: ✨member based limits controlled by org admins#23233shaun-ak wants to merge 25 commits intocalcom:mainfrom
Conversation
|
@shaun-ak is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds per-member booking limits via a nullable Json? Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (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 |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/21/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (08/21/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/server/routers/viewer/organizations/getUser.handler.ts (1)
61-79: Ensure organization-level bookingLimits are returned explicitly (not per-team limits)• In
packages/trpc/server/routers/viewer/organizations/getUser.handler.ts:
- Update the existing
prisma.membership.findFirst({ …, select: { … } })call to also selectbookingLimitsfrom the organization membership.- In the
prisma.membership.findMany({ …, select: { … } })call for sub-teams, removebookingLimitsfrom the selection so callers can’t accidentally use per-team limits.- When constructing the
foundUserpayload:
• RemovebookingLimitsfrom each entry inteams.map(...).
• Add a new top-level propertyorgBookingLimits: membership.bookingLimits.• Follow-up: Update any UI or consumer code (e.g. EditUserSheet) to consume
orgBookingLimitsinstead ofteams?.[0]?.bookingLimits.Would you like me to also patch the EditUserSheet to use
orgBookingLimitsand drop its reliance on per-team limits?
🧹 Nitpick comments (18)
apps/web/public/static/locales/en/common.json (1)
3475-3476: String added in correct place; consider punctuation consistency.The key is added above the sentinel to avoid conflicts. Consider ending the sentence with a period to match many other description strings in this file.
Apply this minimal tweak if you want strict consistency:
- "booking_limits_description": "These limits apply to all bookings for this member across personal and team event types", + "booking_limits_description": "These limits apply to all bookings for this member across personal and team event types.",packages/trpc/server/routers/viewer/organizations/getUser.handler.ts (2)
43-47: Filter profiles by organization to avoid picking an arbitrary username.You return
requestedUser.profiles[0]?.username || requestedUser.username, butprofilesis not filtered byorganizationId. If a user belongs to multiple orgs, you may grab a profile for a different org.Apply this diff to scope profiles to the current org:
- profiles: { - select: { - username: true, - }, - }, + profiles: { + where: { organizationId: currentUser.organizationId }, + select: { username: true }, + },
100-100: Prefer named exports only for handlers.We already have a named export for
getUserHandler. Dropping the default export improves tree-shaking and refactors.-export default getUserHandler; +// Prefer named export onlypackages/features/users/components/UserTable/EditSheet/UserBookingLimitsDisplay.tsx (2)
11-16: Early return is fine; consider i18n’d empty-state instead of null (optional).Currently returns null when limits are absent. If UX wants explicit “No limits set” messaging, we can render a subtle label instead. Fine to keep as-is.
25-37: Stable ordering of intervals for predictable UI.
Object.entriesiteration order can vary by insertion. For consistent display (day → week → month → year), sort keys explicitly.Apply this tweak:
- {Object.entries(bookingLimits).map(([key, value]) => ( + {(["day","week","month","year"] as const) + .filter((k) => k in bookingLimits) + .map((key) => { + const value = bookingLimits[key]!; + return ( <div key={key} className="flex items-center justify-between"> <div className="flex items-center space-x-3"> <span className="text-default font-medium">{value}</span> <span className="text-subtle"> - {intervalLimitKeyToUnit(key as keyof typeof intervalLimitKeyToUnit)} + {intervalLimitKeyToUnit(key)} </span> </div> </div> - ))} + ); + })}packages/features/users/components/UserTable/EditSheet/utils.ts (1)
3-6: Type the return for safer interop and future refactorsExplicitly annotate the return type to the zod-inferred shape (or undefined) to help downstream callers and prevent accidental widening to any.
-import { intervalLimitsType } from "@calcom/lib/intervalLimits/intervalLimitSchema"; +import { intervalLimitsType } from "@calcom/lib/intervalLimits/intervalLimitSchema"; +import type { z } from "zod"; +type IntervalLimits = z.infer<typeof intervalLimitsType>; -export function validateBookingLimits(data: unknown) { +export function validateBookingLimits( + data: unknown +): IntervalLimits | undefined { const result = intervalLimitsType.safeParse(data); return result.success ? result.data : undefined; }packages/features/users/components/UserTable/EditSheet/EditUserSheet.tsx (3)
83-83: Typo in className: likely meant border-subtleThe class "border-sublte" looks like a typo and won’t apply the intended style.
- <div className="border-sublte bg-default w-full rounded-xl border p-4"> + <div className="border-subtle bg-default w-full rounded-xl border p-4">
89-93: Localize embedded strings per guidelinesDirect strings should use t(). Convert "Nameless User", "This user does not have a bio...", and "Cal" to i18n keys.
- <h2 className="text-emphasis font-sans text-2xl font-semibold"> - {loadedUser?.name || "Nameless User"} - </h2> + <h2 className="text-emphasis font-sans text-2xl font-semibold"> + {loadedUser?.name || t("nameless_user")} + </h2> - <p className="text-subtle max-h-[3em] overflow-hidden text-ellipsis text-sm font-normal"> - {loadedUser?.bio || "This user does not have a bio..."} - </p> + <p className="text-subtle max-h-[3em] overflow-hidden text-ellipsis text-sm font-normal"> + {loadedUser?.bio || t("no_bio")} + </p> ... - <DisplayInfo - label="Cal" + <DisplayInfo + label={t("cal")} value={removeProtocol( `${orgBranding?.fullDomain ?? WEBAPP_URL}/${loadedUser?.username}` )}Follow-up: please ensure the new keys exist in apps/web/public/static/locales/*/common.json. If not, I can add them.
Also applies to: 100-101
145-145: Redundant fallbackvalidateBookingLimits already returns undefined on failure; the trailing “|| undefined” is redundant.
packages/trpc/server/routers/viewer/organizations/_router.tsx (1)
102-107: Harden authorization at the router: use authedOrgAdminProcedure and named handler importYou already recheck in the handler, but gating here reduces attack surface and avoids running handler code for ineligible users. Also, prefer named exports per repo guideline to avoid default exports.
-import { ZUpdateMembershipBookingLimitsInputSchema } from "./updateMembershipBookingLimits.schema"; +import { ZUpdateMembershipBookingLimitsInputSchema } from "./updateMembershipBookingLimits.schema"; ... - updateMembershipBookingLimits: authedProcedure + updateMembershipBookingLimits: authedOrgAdminProcedure .input(ZUpdateMembershipBookingLimitsInputSchema) .mutation(async (opts) => { - const { default: handler } = await import("./updateMembershipBookingLimits.handler"); - return handler(opts); + const { updateMembershipBookingLimitsHandler } = await import("./updateMembershipBookingLimits.handler"); + return updateMembershipBookingLimitsHandler(opts); }),packages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.schema.ts (1)
5-9: Allow clearing limits and restrict unknown keysThe column is Json?; support clearing by accepting null/undefined, and prevent stray keys via strict(). If intervalLimitsType isn’t strict, wrap it.
-import { intervalLimitsType } from "@calcom/lib/intervalLimits/intervalLimitSchema"; +import { intervalLimitsType } from "@calcom/lib/intervalLimits/intervalLimitSchema"; export const ZUpdateMembershipBookingLimitsInputSchema = z.object({ userId: z.number(), teamId: z.number(), - bookingLimits: intervalLimitsType, + bookingLimits: intervalLimitsType.strict().nullish(), });Follow-up: update the handler to set bookingLimits to null when input.bookingLimits is null/undefined.
packages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.handler.ts (5)
31-37: Select only needed fields in Prisma queriesPer guidelines, avoid fetching entire rows. Here you only need role to authorize.
- const userMembership = await prisma.membership.findFirst({ - where: { - userId: user.id, - teamId: organizationId, - accepted: true, - }, - }); + const userMembership = await prisma.membership.findFirst({ + where: { userId: user.id, teamId: organizationId, accepted: true }, + select: { role: true }, + });
58-65: Select only the membership id for the updateYou only need id here; fetch minimal data.
- const membership = await prisma.membership.findFirst({ - where: { - userId: input.userId, - teamId: input.teamId, - accepted: true, - }, - }); + const membership = await prisma.membership.findFirst({ + where: { userId: input.userId, teamId: input.teamId, accepted: true }, + select: { id: true }, + });
71-79: Handle clearing limits and keep types preciseIf the schema accepts nullish, set bookingLimits to null when absent. Also, avoid casting; Prisma accepts JsonValue directly.
- await prisma.membership.update({ + await prisma.membership.update({ where: { id: membership.id, }, data: { - bookingLimits: input.bookingLimits as Prisma.InputJsonValue, + bookingLimits: input.bookingLimits ?? null, }, });
86-86: Prefer named export only; drop default exportRepository guidelines recommend named exports for clearer imports and tree-shaking. The router can import the named handler.
-export default updateMembershipBookingLimitsHandler; +// no default export
25-44: Authorization is good; consider early exit with authedOrgAdminProcedureYou correctly enforce admin/owner here. For defense-in-depth and faster failures, also gate at the router with authedOrgAdminProcedure. See router comment for diff.
packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx (2)
9-14: Good reuse, but consider de-coupling from EventTypes domainReusing IntervalLimitsManager and the intervalLimitsType is pragmatic. To reduce cross-domain coupling and path churn, consider re-exporting these in a shared, domain-agnostic module (for example, features/limits) and consuming from there.
174-184: Mutation wiring and cache invalidations look fine; minor nit on consolidationThe success and error handlers are correct, and invalidations target the right queries. Optionally, consolidate invalidations with utils.invalidate().then(...) or batch where available to reduce network chatter.
📜 Review details
Configuration used: .coderabbit.yaml
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.
📒 Files selected for processing (11)
apps/web/public/static/locales/en/common.json(1 hunks)packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx(7 hunks)packages/features/users/components/UserTable/EditSheet/EditUserSheet.tsx(3 hunks)packages/features/users/components/UserTable/EditSheet/UserBookingLimitsDisplay.tsx(1 hunks)packages/features/users/components/UserTable/EditSheet/utils.ts(1 hunks)packages/prisma/migrations/20250816142522_add_booking_limits_to_membership/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)packages/trpc/server/routers/viewer/organizations/_router.tsx(2 hunks)packages/trpc/server/routers/viewer/organizations/getUser.handler.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.handler.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.schema.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/users/components/UserTable/EditSheet/utils.tspackages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.schema.tspackages/trpc/server/routers/viewer/organizations/getUser.handler.tspackages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.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/features/users/components/UserTable/EditSheet/utils.tspackages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.schema.tspackages/features/users/components/UserTable/EditSheet/UserBookingLimitsDisplay.tsxpackages/trpc/server/routers/viewer/organizations/_router.tsxpackages/trpc/server/routers/viewer/organizations/getUser.handler.tspackages/features/users/components/UserTable/EditSheet/EditUserSheet.tsxpackages/features/users/components/UserTable/EditSheet/EditUserForm.tsxpackages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.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/features/users/components/UserTable/EditSheet/utils.tspackages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.schema.tspackages/features/users/components/UserTable/EditSheet/UserBookingLimitsDisplay.tsxpackages/trpc/server/routers/viewer/organizations/_router.tsxpackages/trpc/server/routers/viewer/organizations/getUser.handler.tspackages/features/users/components/UserTable/EditSheet/EditUserSheet.tsxpackages/features/users/components/UserTable/EditSheet/EditUserForm.tsxpackages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.handler.ts
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/users/components/UserTable/EditSheet/UserBookingLimitsDisplay.tsxpackages/trpc/server/routers/viewer/organizations/_router.tsxpackages/features/users/components/UserTable/EditSheet/EditUserSheet.tsxpackages/features/users/components/UserTable/EditSheet/EditUserForm.tsx
🧬 Code Graph Analysis (5)
packages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.schema.ts (1)
packages/prisma/zod-utils.ts (1)
intervalLimitsType(229-229)
packages/trpc/server/routers/viewer/organizations/_router.tsx (1)
packages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.schema.ts (1)
ZUpdateMembershipBookingLimitsInputSchema(5-9)
packages/features/users/components/UserTable/EditSheet/EditUserSheet.tsx (3)
apps/web/app/_trpc/trpc.ts (1)
trpc(7-7)packages/features/users/components/UserTable/EditSheet/UserBookingLimitsDisplay.tsx (1)
UserBookingLimitsDisplay(11-41)packages/features/users/components/UserTable/EditSheet/utils.ts (1)
validateBookingLimits(3-6)
packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx (2)
packages/features/users/components/UserTable/EditSheet/utils.ts (1)
validateBookingLimits(3-6)packages/features/eventtypes/components/tabs/limits/EventLimitsTab.tsx (1)
IntervalLimitsManager(909-1014)
packages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.handler.ts (1)
packages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.schema.ts (1)
TUpdateMembershipBookingLimitsInputSchema(11-13)
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
packages/prisma/schema.prisma (1)
661-663: Modeling looks right; aligns with existing pattern for Team/EventType.Adding
bookingLimits Json?onMembershipwith the zod annotation keeps parity withTeam.bookingLimits/EventType.bookingLimits. Optional + no default is a safe migration and avoids heavy table locking.Please confirm that the zod import
intervalLimitsTypealready supports the same shape you expect for membership-level limits (e.g., { day, week, month, year } ascending validation). If not, I can help update the schema and validator.packages/prisma/migrations/20250816142522_add_booking_limits_to_membership/migration.sql (1)
1-2: Safe, minimal migration.Nullable JSONB without default avoids long rewrites. No further changes needed.
packages/features/users/components/UserTable/EditSheet/UserBookingLimitsDisplay.tsx (1)
21-23: Good localization usage.Uses
t()for title and description; string key added to en locale. LGTM.packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx (3)
104-109: RBAC-derived UI gating: LGTMThe belongsToOrg and isOrgAdminOrOwner checks are clear and match the intended visibility constraints.
290-292: Good i18n coverage for the new UI copyUsing t("booking_limits") and t("booking_limits_description") aligns with our localization guidelines.
68-69: Empty object is valid for intervalLimitsTypeThe
intervalLimitsTypeis defined asz.object({ PER_DAY: z.number().optional(), PER_WEEK: z.number().optional(), PER_MONTH: z.number().optional(), PER_YEAR: z.number().optional(), }).nullable()so all interval keys are optional and the object itself may be null. In Zod, an object schema with only optional properties accepts
{}without error (e.g.z.object({ age: z.number().optional() }).parse({})succeeds) (v4.zod.dev). Chaining.optional()on that schema also allowsundefinedas a valid value, so leavingbookingLimitsundefined (or even defaulting it to{}) will never cause the resolver to fail.No change needed to the
bookingLimits: intervalLimitsType.optional()usage.Likely an incorrect or invalid review comment.
packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx
Outdated
Show resolved
Hide resolved
packages/features/users/components/UserTable/EditSheet/EditUserSheet.tsx
Show resolved
Hide resolved
packages/features/users/components/UserTable/EditSheet/EditUserSheet.tsx
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/organizations/updateMembershipBookingLimits.handler.ts
Outdated
Show resolved
Hide resolved
…mits - Import validateIntervalLimitOrder from intervalLimits library - Add validation logic similar to team booking limits - Ensure booking limits are in ascending order (day ≤ week ≤ month ≤ year) - Throw BAD_REQUEST error with descriptive message for invalid limits This incorporates the key improvement from PR #23233 to maintain consistency with existing team booking limits validation patterns. Co-Authored-By: Devanshu Sharma <devanshusharma658@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/v1/pages/api/memberships/[id]/_patch.ts (1)
61-81: Critical: Missing permission check forbookingLimitsfield.The
checkPermissionsfunction only validates admin/owner status when modifying therolefield (lines 74-80). However, according to the PR objectives, only organization admins/owners should be able to set booking limits for members. A regular member could potentially modify their ownbookingLimitsby sending a PATCH request that doesn't includerole.Compare with the POST handler in
_post.ts(lines 54-57), which requires admin/owner status for all membership creation, properly protecting thebookingLimitsfield.Add a permission check for
bookingLimits:// Only team OWNERS and ADMINS can modify `role` if ("role" in data) { const membership = await prisma.membership.findFirst({ where: { userId, teamId, role: { in: ["ADMIN", "OWNER"] } }, }); if (!membership || (membership.role !== "OWNER" && req.body.role === "OWNER")) throw new HttpError({ statusCode: 403, message: "Forbidden" }); } + // Only team OWNERS and ADMINS can modify `bookingLimits` + if ("bookingLimits" in data) { + const membership = await prisma.membership.findFirst({ + where: { userId, teamId, role: { in: ["ADMIN", "OWNER"] } }, + }); + if (!membership) + throw new HttpError({ statusCode: 403, message: "Only admins and owners can set booking limits" }); + } }
🧹 Nitpick comments (1)
apps/api/v1/pages/api/memberships/_post.ts (1)
27-31: Consider inlining the normalization for consistency.The normalization logic is correct and handles Prisma's preference for
undefinedovernull. However, the PATCH handler (in_patch.tsline 50) performs this normalization inline within the args construction, while here you create an intermediatenormalizedDatavariable. Consider inlining for consistency:- // Ensure bookingLimits is undefined instead of null - const normalizedData = { - ...data, - bookingLimits: data.bookingLimits === null ? undefined : data.bookingLimits, - }; const args: Prisma.MembershipCreateArgs = { - data: normalizedData, + data: { + ...data, + bookingLimits: data.bookingLimits === null ? undefined : data.bookingLimits, + }, };
📜 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.
📒 Files selected for processing (2)
apps/api/v1/pages/api/memberships/[id]/_patch.ts(1 hunks)apps/api/v1/pages/api/memberships/_post.ts(1 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 useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/api/v1/pages/api/memberships/_post.tsapps/api/v1/pages/api/memberships/[id]/_patch.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:
apps/api/v1/pages/api/memberships/_post.tsapps/api/v1/pages/api/memberships/[id]/_patch.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:
apps/api/v1/pages/api/memberships/_post.tsapps/api/v1/pages/api/memberships/[id]/_patch.ts
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
apps/api/v1/pages/api/memberships/_post.ts (1)
32-38: LGTM!The args construction correctly uses the normalized data, and the permission check appropriately ensures only team admins/owners can create memberships with booking limits.
apps/api/v1/pages/api/memberships/[id]/_patch.ts (1)
46-53: LGTM with a note on style consistency.The inline normalization correctly converts
nulltoundefinedfor Prisma compatibility. Note that the POST handler uses a separatenormalizedDatavariable (lines 27-31 in_post.ts), while here you normalize inline. Both approaches work, but consistency would improve maintainability.
| where: { userId_teamId }, | ||
| data: { | ||
| ...data, | ||
| bookingLimits: data.bookingLimits === null ? undefined : data.bookingLimits, |
There was a problem hiding this comment.
For null values, we set it to undefined for type matching.
| // Ensure bookingLimits is undefined instead of null | ||
| const normalizedData = { | ||
| ...data, | ||
| bookingLimits: data.bookingLimits === null ? undefined : data.bookingLimits, |
video.proof.1.mp4 |
pallava-joshi
left a comment
There was a problem hiding this comment.
hey can you please fix the merge conflicts.
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)