Conversation
|
Warning Rate limit exceeded@ThyMinimalDev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughReplaces a local OmitType-based update payload with a shared UpdatePrivateLinkBody imported from @calcom/platform-types in the event-types private-links controller. Adds a new exported UpdatePrivateLinkBody input type (optional expiresAt and maxUsageCount with validation and Swagger metadata). Adds E2E tests for organizations bookings (seeding manager and managed orgs and validating bookings) and a fixture method createManagedOrganization. Updates TRPC viewer bookings membership filter to include teamId: user.orgId. Bumps @calcom/platform-libraries in apps/api/v2/package.json. Estimated code review effort🎯 4 (Complex) | ⏱️ ~30 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
keithwillcode
added
core
dosubot
bot
added
bookings
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/13/25)1 reviewer was added to this PR based on Keith Williams's automation. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/api/v2/test/fixtures/repository/managed-organizations.repository.fixture.ts (1)
38-45: Helper looks good; consider idempotent creation to prevent unique violations in parallel testsCreating the relation directly is fine for deterministic setups. If test retries or parallel runs are possible, consider upsert/createMany with skipDuplicates or catching unique violations to keep this helper idempotent.
Example update:
- async createManagedOrganization(managerOrganizationId: number, managedOrganizationId: number) { - return this.prismaWriteClient.managedOrganization.create({ - data: { - managerOrganizationId, - managedOrganizationId, - }, - }); - } + async createManagedOrganization(managerOrganizationId: number, managedOrganizationId: number) { + try { + return await this.prismaWriteClient.managedOrganization.create({ + data: { managerOrganizationId, managedOrganizationId }, + }); + } catch (e: any) { + // Optionally ignore unique constraint if already present + if (e.code === "P2002") { + return this.getManagedOrganization(managerOrganizationId, managedOrganizationId); + } + throw e; + } + }packages/platform/types/event-types/inputs/private-link.input.ts (1)
62-83: Align OpenAPI types for expiresAt with “string date-time” for consistencyIn CreatePrivateLinkInput, expiresAt is documented as type: String with format: date-time, but here it’s type: Date. For consistent API docs and client generation, prefer String + date-time everywhere (validation still uses @isdate + @type(() => Date)).
export class UpdatePrivateLinkBody { @IsOptional() @IsDate() @Type(() => Date) @ApiPropertyOptional({ - description: "New expiration date for time-based links", - type: Date, - example: "2024-12-31T23:59:59.000Z", + description: "New expiration date for time-based links", + type: String, + format: "date-time", + example: "2024-12-31T23:59:59.000Z", }) expiresAt?: Date; @IsOptional() @IsInt() @Min(1) @ApiPropertyOptional({ description: "New maximum number of times the link can be used", type: Number, example: 10, minimum: 1, }) maxUsageCount?: number; }Optional: If helpful for consumers migrating from legacy links (defaulted to single-use), you can clarify in the description that omitting maxUsageCount here will leave it unchanged (defaults apply to create, not update).
apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts (1)
541-549: Tighten teardown to avoid data leakage across test runsCurrently only OAuth client, one team (managerOrganization), and two users plus some bookings are deleted. Consider deleting:
- managerOrgTeam1, managedOrgTeam1
- managedOrganization
- created event types
- memberships and profiles created in setup
- the manager↔managed relation
If fixture delete helpers don’t exist, we can add symmetric delete methods.
Would you like me to add delete helpers to the fixtures and update afterAll accordingly?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/v2/src/ee/event-types-private-links/controllers/event-types-private-links.controller.ts(1 hunks)apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts(1 hunks)apps/api/v2/test/fixtures/repository/managed-organizations.repository.fixture.ts(1 hunks)packages/platform/types/event-types/inputs/private-link.input.ts(2 hunks)packages/trpc/server/routers/viewer/bookings/get.handler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/v2/test/fixtures/repository/managed-organizations.repository.fixture.tspackages/trpc/server/routers/viewer/bookings/get.handler.tsapps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.tspackages/platform/types/event-types/inputs/private-link.input.tsapps/api/v2/src/ee/event-types-private-links/controllers/event-types-private-links.controller.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/v2/test/fixtures/repository/managed-organizations.repository.fixture.tspackages/trpc/server/routers/viewer/bookings/get.handler.tsapps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.tspackages/platform/types/event-types/inputs/private-link.input.tsapps/api/v2/src/ee/event-types-private-links/controllers/event-types-private-links.controller.ts
🧠 Learnings (2)
📚 Learning: 2025-07-16T05:10:22.891Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/prisma/schema.prisma:1068-1071
Timestamp: 2025-07-16T05:10:22.891Z
Learning: In PR #22304 for Cal.com private link expiration features, the `maxUsageCount` field was intentionally set to default to 1 (non-nullable) as a breaking change, making all existing private links single-use after migration. This was a deliberate design decision by alishaz-polymath.
Applied to files:
packages/platform/types/event-types/inputs/private-link.input.ts
📚 Learning: 2025-07-16T06:42:27.024Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.024Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the `currentLink.maxUsageCount ?? 1` fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
Applied to files:
packages/platform/types/event-types/inputs/private-link.input.ts
🧬 Code Graph Analysis (1)
apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts (11)
apps/api/v2/test/fixtures/repository/bookings.repository.fixture.ts (1)
BookingsRepositoryFixture(8-44)apps/api/v2/test/fixtures/repository/event-types.repository.fixture.ts (1)
EventTypesRepositoryFixture(9-62)apps/api/v2/test/fixtures/repository/oauth-client.repository.fixture.ts (1)
OAuthClientRepositoryFixture(8-64)apps/api/v2/test/fixtures/repository/membership.repository.fixture.ts (1)
MembershipRepositoryFixture(6-38)apps/api/v2/test/fixtures/repository/hosts.repository.fixture.ts (1)
HostsRepositoryFixture(7-27)apps/api/v2/test/fixtures/repository/profiles.repository.fixture.ts (1)
ProfileRepositoryFixture(6-30)apps/api/v2/test/fixtures/repository/managed-organizations.repository.fixture.ts (1)
ManagedOrganizationsRepositoryFixture(5-46)apps/api/v2/src/app.ts (1)
bootstrap(27-89)apps/api/v2/test/utils/randomString.ts (1)
randomString(3-6)packages/platform/constants/api.ts (5)
CAL_API_VERSION_HEADER(72-72)VERSION_2024_08_13(59-59)X_CAL_CLIENT_ID(50-50)X_CAL_SECRET_KEY(49-49)SUCCESS_STATUS(9-9)packages/platform/types/bookings/2024-08-13/outputs/booking.output.ts (3)
BookingOutput_2024_08_13(280-306)RecurringBookingOutput_2024_08_13(308-323)GetSeatedBookingOutput_2024_08_13(325-331)
⏰ 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: Atoms E2E Tests
🔇 Additional comments (9)
packages/platform/types/event-types/inputs/private-link.input.ts (1)
21-22: Doc reflow only—no functional changeThe description rewrap in CreatePrivateLinkInput is fine and keeps the default behavior clear.
apps/api/v2/src/ee/event-types-private-links/controllers/event-types-private-links.controller.ts (1)
7-11: Good consolidation to platform-types for UpdatePrivateLinkBodyRemoving the local OmitType-based declaration and sourcing UpdatePrivateLinkBody from platform-types reduces duplication and keeps API shape centralized. No issues spotted.
Also applies to: 16-16
apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts (7)
75-112: Bootstrap and guard overrides look fineUsing withApiAuth plus permissive guard overrides for this suite is appropriate to isolate booking behavior. No issues found.
317-455: Managed org setup aligns with test intentEstablishing the manager↔managed relation and assigning OWNER membership to the manager in the managed org is coherent with the scenarios under test.
458-479: Manager org bookings assertions are precise and relevantValidating both the org event type and the non-org booking (with an org member attendee) makes the scope explicit. Good coverage.
481-500: Mixed auth flows—confirm headers are intentionally optional hereThis test omits OAuth headers, unlike the previous one. Given the guard overrides, it’ll pass, but confirm this reflects intended API behavior (i.e., server-side auth acceptable for this path) and not an accidental omission.
503-524: Managed org bookings assertion captures the core requirementEnsuring only the managed org’s event type surfaces here closely matches the PR objective. Looks good.
25-25: No action needed:@calcom/lib/randomimport is valid and already in use
randomStringis exported inpackages/lib/random.ts.- Multiple e2e specs (including this one) and other modules (e.g. auth options) import from
@calcom/lib/randomwithout issue.- The path alias is correctly resolved in tests, so no changes are required.
113-315: Profile username uniqueness is scoped by organization—no change neededThe Prisma schema enforces
@@unique([username, organizationId])so usernames only must be unique within the same organization. In this test you create two profiles in the same org with distinct emails as usernames, so there’s no collision and no further action is required.
Likely an incorrect or invalid review comment.
| ...(user.orgId && { | ||
| OR: [ | ||
| // note(Lauris): in case orgId provided refers to a manager (parent) org | ||
| { | ||
| teamId: user.orgId, | ||
| }, | ||
| // note(Lauris): in case orgId provided refers to a managed (child) org | ||
| { | ||
| team: { | ||
| parentId: user.orgId, | ||
| }, | ||
| }, | ||
| ], | ||
| }), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Org-admin membership scoping expanded to parent org and its child teams—verify managed-org semantics
This narrows ADMIN/OWNER membership IDs to those within the current org context: either the org itself (teamId = orgId) or its child teams (team.parentId = orgId). That aligns with the stated goal, but note:
- This does not pick up memberships in a separate “managed organization” unless those are represented as child teams via Team.parentId. If your data model uses a separate managedOrganizations relation (not parentId), this won’t include those unless there’s also a membership within the managed org or its child teams.
If that’s intended, LGTM. Otherwise, consider augmenting the filter to include teams belonging to organizations that are managed by orgId via the managedOrganizations relation.
To double-check the data model paths used by tRPC for “managed orgs,” run:
🏁 Script executed:
#!/bin/bash
# Inspect usage of managedOrganization relations and where Team.parentId is used for scoping
rg -n "managedOrganization" -A 4
rg -n "team.parentId" packages/trpc --iglob "**/*.ts" -A 2
# Confirm tRPC viewer booking handlers don't require managedOrganizations join-based scoping
rg -n "getAllUserBookings|getBookings" packages/trpc --iglob "**/*.ts" -A 20Length of output: 79408
Augment org‐admin booking scope to include managed organizations
The current filter only picks up memberships where
membership.teamId = user.orgId(the org itself)membership.team.parentId = user.orgId(its direct child teams)
However, your Prisma schema also models “managed organizations” via a separate ManagedOrganization relation (not via parentId). To ensure bookings in those managed orgs are included, extend the OR clause. For example, in packages/trpc/server/routers/viewer/bookings/get.handler.ts (lines 102–115):
...(user.orgId && {
OR: [
// direct org
{ teamId: user.orgId },
// child teams
{ team: { parentId: user.orgId } },
+ // managed organizations
+ { team: { managedOrganization: { managerOrganizationId: user.orgId } } },
],
}),• This adds memberships in any organization whose ManagedOrganization.managerOrganizationId matches user.orgId.
• If you also need child teams under those managed orgs, you may need further nesting (e.g. { team: { managedOrganization: { managerOrganization: { … } } } }).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ...(user.orgId && { | |
| OR: [ | |
| // note(Lauris): in case orgId provided refers to a manager (parent) org | |
| { | |
| teamId: user.orgId, | |
| }, | |
| // note(Lauris): in case orgId provided refers to a managed (child) org | |
| { | |
| team: { | |
| parentId: user.orgId, | |
| }, | |
| }, | |
| ], | |
| }), | |
| ...(user.orgId && { | |
| OR: [ | |
| // note(Lauris): in case orgId provided refers to a manager (parent) org | |
| { | |
| teamId: user.orgId, | |
| }, | |
| // note(Lauris): in case orgId provided refers to a managed (child) org | |
| { | |
| team: { | |
| parentId: user.orgId, | |
| }, | |
| }, | |
| // managed organizations | |
| { | |
| team: { | |
| managedOrganization: { | |
| managerOrganizationId: user.orgId, | |
| }, | |
| }, | |
| }, | |
| ], | |
| }), |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/api/v2/test/fixtures/repository/managed-organizations.repository.fixture.ts (1)
20-23: Use select instead of include in Prisma queries (tests too) per repository guidelinesThe coding guidelines require using select (and only the fields you need) instead of include. Even though this is test code, please align to avoid accidental data shape regressions and to keep fixtures lean.
Apply this diff to switch to select:
return this.prismaReadClient.team.findUnique({ where: { id: organizationId, isOrganization: true, }, - include: { - managedOrganization: true, - managedOrganizations: true, - }, + select: { + id: true, + isOrganization: true, + managedOrganization: true, + managedOrganizations: true, + }, });
🧹 Nitpick comments (3)
apps/api/v2/test/fixtures/repository/managed-organizations.repository.fixture.ts (1)
38-45: Fixture method LGTM; consider upsert to avoid test flakiness with existing relationsThe create wrapper is fine for fixtures. If this gets reused across multiple tests, an upsert (or guarded create) could prevent unique-constraint errors when tests re-run without full teardown.
Apply this diff if you prefer an upsert-style operation:
- async createManagedOrganization(managerOrganizationId: number, managedOrganizationId: number) { - return this.prismaWriteClient.managedOrganization.create({ - data: { - managerOrganizationId, - managedOrganizationId, - }, - }); - } + async createManagedOrganization(managerOrganizationId: number, managedOrganizationId: number) { + return this.prismaWriteClient.managedOrganization.upsert({ + where: { + managerOrganizationId_managedOrganizationId: { + managerOrganizationId, + managedOrganizationId, + }, + }, + create: { managerOrganizationId, managedOrganizationId }, + update: {}, + }); + }packages/platform/types/event-types/inputs/private-link.input.ts (1)
62-83: Exported UpdatePrivateLinkBody looks good; align OpenAPI type for dates with Create inputCreatePrivateLinkInput documents expiresAt as type String with format date-time, while UpdatePrivateLinkBody uses type Date. For consistency and accurate OpenAPI generation, prefer type: String with format: "date-time" here too.
Apply this diff to align the OpenAPI metadata:
@ApiPropertyOptional({ - description: "New expiration date for time-based links", - type: Date, - example: "2024-12-31T23:59:59.000Z", + description: "New expiration date for time-based links", + type: String, + format: "date-time", + example: "2024-12-31T23:59:59.000Z", })Also confirm product semantics: with Min(1) on maxUsageCount, there is no way to “unset” the cap via the API (i.e., unlimited). If “unlimited” is needed, we’d need a separate flag or allow null. Otherwise, current validation matches the learnings/default of single-use where omitted. Do you want to keep it strictly >= 1?
apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts (1)
481-489: Add OAuth client headers here for consistency with other testsThe first manager-org test sets X_CAL_CLIENT_ID and X_CAL_SECRET_KEY, this one doesn’t. If ApiAuthGuard is ever re-enabled in tests, this will start failing. Better to keep it consistent now.
Apply this diff:
return request(app.getHttpServer()) .get(`/v2/organizations/${managerOrganization.id}/bookings?userIds=${managerOrgUser2.id}`) .set(CAL_API_VERSION_HEADER, VERSION_2024_08_13) + .set(X_CAL_CLIENT_ID, oAuthClient.id) + .set(X_CAL_SECRET_KEY, oAuthClient.secret) .expect(200)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/v2/src/ee/event-types-private-links/controllers/event-types-private-links.controller.ts(1 hunks)apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts(1 hunks)apps/api/v2/test/fixtures/repository/managed-organizations.repository.fixture.ts(1 hunks)packages/platform/types/event-types/inputs/private-link.input.ts(2 hunks)packages/trpc/server/routers/viewer/bookings/get.handler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/v2/test/fixtures/repository/managed-organizations.repository.fixture.tspackages/trpc/server/routers/viewer/bookings/get.handler.tsapps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.tspackages/platform/types/event-types/inputs/private-link.input.tsapps/api/v2/src/ee/event-types-private-links/controllers/event-types-private-links.controller.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/v2/test/fixtures/repository/managed-organizations.repository.fixture.tspackages/trpc/server/routers/viewer/bookings/get.handler.tsapps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.tspackages/platform/types/event-types/inputs/private-link.input.tsapps/api/v2/src/ee/event-types-private-links/controllers/event-types-private-links.controller.ts
🧠 Learnings (2)
📚 Learning: 2025-07-16T06:42:27.024Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.024Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the `currentLink.maxUsageCount ?? 1` fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
Applied to files:
packages/platform/types/event-types/inputs/private-link.input.ts
📚 Learning: 2025-07-16T05:10:22.891Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/prisma/schema.prisma:1068-1071
Timestamp: 2025-07-16T05:10:22.891Z
Learning: In PR #22304 for Cal.com private link expiration features, the `maxUsageCount` field was intentionally set to default to 1 (non-nullable) as a breaking change, making all existing private links single-use after migration. This was a deliberate design decision by alishaz-polymath.
Applied to files:
packages/platform/types/event-types/inputs/private-link.input.ts
🧬 Code Graph Analysis (2)
packages/trpc/server/routers/viewer/bookings/get.handler.ts (1)
scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(15-17)
apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts (11)
apps/api/v2/test/fixtures/repository/bookings.repository.fixture.ts (1)
BookingsRepositoryFixture(8-44)apps/api/v2/test/fixtures/repository/event-types.repository.fixture.ts (1)
EventTypesRepositoryFixture(9-62)apps/api/v2/test/fixtures/repository/oauth-client.repository.fixture.ts (1)
OAuthClientRepositoryFixture(8-64)apps/api/v2/test/fixtures/repository/membership.repository.fixture.ts (1)
MembershipRepositoryFixture(6-38)apps/api/v2/test/fixtures/repository/hosts.repository.fixture.ts (1)
HostsRepositoryFixture(7-27)apps/api/v2/test/fixtures/repository/profiles.repository.fixture.ts (1)
ProfileRepositoryFixture(6-30)apps/api/v2/test/fixtures/repository/managed-organizations.repository.fixture.ts (1)
ManagedOrganizationsRepositoryFixture(5-46)apps/api/v2/src/app.ts (1)
bootstrap(27-89)apps/api/v2/test/utils/randomString.ts (1)
randomString(3-6)packages/platform/constants/api.ts (5)
CAL_API_VERSION_HEADER(72-72)VERSION_2024_08_13(59-59)X_CAL_CLIENT_ID(50-50)X_CAL_SECRET_KEY(49-49)SUCCESS_STATUS(9-9)packages/platform/types/bookings/2024-08-13/outputs/booking.output.ts (3)
BookingOutput_2024_08_13(280-306)RecurringBookingOutput_2024_08_13(308-323)GetSeatedBookingOutput_2024_08_13(325-331)
⏰ 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: Detect changes
- GitHub Check: Atoms E2E Tests
🔇 Additional comments (4)
packages/trpc/server/routers/viewer/bookings/get.handler.ts (2)
102-115: Scopes admin/owner memberships to both manager org and its managed orgs — aligns with PR intentAdding the OR constraint with teamId = orgId OR team.parentId = orgId correctly captures both parent (manager) org and its managed (child) org teams when user.orgId is present. This is consistent with the requirement to scope bookings at the managed org level and avoids leaking memberships outside the manager/managed hierarchy.
102-115: Consider indexing Team.parentId to keep this query performantThe added relation filter
team: { parentId: user.orgId }will translate into a join on Team. If not already indexed, consider adding an index on Team.parentId to prevent regressions for large org graphs.Would you like me to generate a migration to add an index on Team.parentId?
packages/platform/types/event-types/inputs/private-link.input.ts (1)
21-22: Non-functional doc wrap change — OKapps/api/v2/src/ee/event-types-private-links/controllers/event-types-private-links.controller.ts (1)
7-11: Switch to centralized UpdatePrivateLinkBody type — good consolidationImporting UpdatePrivateLinkBody from platform-types and composing updateInput with linkId in the controller keeps DTOs centralized and avoids local OmitType duplication. This matches our validation pipe setup and should maintain transformation semantics for Date fields.
Also applies to: 16-16, 70-77
.../v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts (1)
541-550: Incomplete test teardown - missing user cleanup.The teardown is missing cleanup for
managerOrgUser2andmanagedOrgUser. While Prisma cascades will handle some related data when teams are deleted, explicitly cleaning up users and their bookings is a best practice to prevent test pollution.Apply this diff to ensure complete cleanup:
afterAll(async () => { await oauthClientRepositoryFixture.delete(oAuthClient.id); await teamRepositoryFixture.delete(managerOrganization.id); await teamRepositoryFixture.delete(managedOrganization.id); await userRepositoryFixture.deleteByEmail(managerOrgUser1.email); + await userRepositoryFixture.deleteByEmail(managerOrgUser2.email); + await userRepositoryFixture.deleteByEmail(managedOrgUser.email); await userRepositoryFixture.deleteByEmail(nonOrgUser1.email); await bookingsRepositoryFixture.deleteAllBookings(managerOrgUser1.id, managerOrgUser1.email); + await bookingsRepositoryFixture.deleteAllBookings(managerOrgUser2.id, managerOrgUser2.email); + await bookingsRepositoryFixture.deleteAllBookings(managedOrgUser.id, managedOrgUser.email); await bookingsRepositoryFixture.deleteAllBookings(nonOrgUser1.id, nonOrgUser1.email); await app.close(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
apps/api/v2/package.json(1 hunks)apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/api/v2/package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.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/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts
🧬 Code Graph Analysis (1)
apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts (10)
apps/api/v2/test/fixtures/repository/bookings.repository.fixture.ts (1)
BookingsRepositoryFixture(8-44)apps/api/v2/test/fixtures/repository/event-types.repository.fixture.ts (1)
EventTypesRepositoryFixture(9-62)apps/api/v2/test/fixtures/repository/oauth-client.repository.fixture.ts (1)
OAuthClientRepositoryFixture(8-64)apps/api/v2/test/fixtures/repository/membership.repository.fixture.ts (1)
MembershipRepositoryFixture(6-38)apps/api/v2/test/fixtures/repository/hosts.repository.fixture.ts (1)
HostsRepositoryFixture(7-27)apps/api/v2/test/fixtures/repository/managed-organizations.repository.fixture.ts (1)
ManagedOrganizationsRepositoryFixture(5-46)apps/api/v2/src/app.ts (1)
bootstrap(27-89)apps/api/v2/test/utils/randomString.ts (1)
randomString(3-6)packages/platform/constants/api.ts (5)
CAL_API_VERSION_HEADER(72-72)VERSION_2024_08_13(59-59)X_CAL_CLIENT_ID(50-50)X_CAL_SECRET_KEY(49-49)SUCCESS_STATUS(9-9)packages/platform/types/bookings/2024-08-13/outputs/booking.output.ts (3)
BookingOutput_2024_08_13(280-306)RecurringBookingOutput_2024_08_13(308-323)GetSeatedBookingOutput_2024_08_13(325-331)
🔇 Additional comments (5)
apps/api/v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts (5)
1-40: LGTM! Well-organized imports and test structure.The imports are properly organized and the test suite is correctly set up for testing the Organizations Bookings endpoints with version 2024-08-13.
41-111: Good test setup with proper guard overrides.The test setup correctly:
- Overrides permission and billing guards for testing
- Initializes all necessary repository fixtures
- Bootstraps the application properly
113-315: Comprehensive manager organization setup.The setup creates a thorough test environment with all necessary entities including users, teams, event types, schedules, profiles, memberships, hosts, and bookings. The relationships are properly established.
317-455: Well-structured managed organization setup with proper relationships.The managed organization is correctly linked to the manager organization and includes all necessary test data.
503-524: Test correctly validates managed organization bookings scope.The test properly verifies that managed organization bookings are scoped correctly and only return the managed organization's booking.
.../v2/src/modules/organizations/bookings/managed-organizations-bookings.controller.e2e-spec.ts
Show resolved
Hide resolved
| ...(user.orgId && { | ||
| teamId: user.orgId, | ||
| }), |
There was a problem hiding this comment.
this might actually be a breaking change, membershipIdsWhereUserIsAdminOwner is suppose to return both memberships from the teams and orgs if I recall correctly
This reverts commit 13ba680.
E2E results are ready! |
3 participants
When fetching managed org bookings as admin we need to scope bookings to managed org level which we now do in bookings/get.handler.ts.
Added tests in managed-organizations-bookings.controller.e2e-spec.ts that show that a bookings are done in parent org and then managed org and when fetching bookings for parent and managed org correct bookings are fetched.