fix: Ignore userIds form filter segment if no permission#24194
fix: Ignore userIds form filter segment if no permission#24194anikdhabal merged 15 commits intomainfrom
Conversation
WalkthroughThe bookings GET handler now fetches user IDs and emails for which the requester is an admin/owner, filters incoming Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
dosubot
bot
added
bookings
![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
📜 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 (1)
packages/trpc/server/routers/viewer/bookings/get.handler.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:
packages/trpc/server/routers/viewer/bookings/get.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/bookings/get.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/bookings/get.handler.ts
🧠 Learnings (1)
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
Applied to files:
packages/trpc/server/routers/viewer/bookings/get.handler.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). (3)
- GitHub Check: Type check / check-types
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/playwright/booking-filters.e2e.ts (2)
123-135: Consider waiting for the API response after reload to avoid flakiness.After
page.reload(), the test immediately checks for UI elements but doesn't explicitly wait for the/api/trpc/bookings/getresponse like earlier in the test (lines 86-92). This could lead to flaky assertions if the UI renders before data loads.Apply this diff to add the response wait:
await test.step("Verify filter segment still works and UI is not stuck", async () => { + const bookingsGetResponseReload = page.waitForResponse((response) => + /\/api\/trpc\/bookings\/get.*/.test(response.url()) + ); await page.reload(); await page.waitForLoadState("domcontentloaded"); + await bookingsGetResponseReload; await expect(dataTable).toBeVisible();
132-132: Clarify the purpose of the Escape key press.The
Escapekey press at line 132 appears disconnected from the surrounding assertions. If it's intended to close a dialog or dropdown that might interfere with the permission message check, consider adding a comment to explain its purpose.
📜 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 (1)
apps/web/playwright/booking-filters.e2e.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 useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/playwright/booking-filters.e2e.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/web/playwright/booking-filters.e2e.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/web/playwright/booking-filters.e2e.ts
🧬 Code graph analysis (1)
apps/web/playwright/booking-filters.e2e.ts (1)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
⏰ 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 (3)
apps/web/playwright/booking-filters.e2e.ts (3)
3-3: LGTM!The import is necessary for role-based test setup and follows the project's naming conventions.
59-82: Well-structured test setup.The test properly creates a team owner and two members with explicit roles. Using
createManyfor memberships is efficient and theaccepted: trueflag ensures the memberships are active for the test scenario.
54-136: Excellent E2E test coverage for the filter permission fix.This test effectively validates the PR objective by ensuring that when a filter segment contains userIds that are no longer valid (due to membership removal), the UI remains functional and no permission errors are displayed. The test structure with explicit
test.stepcalls makes the scenario clear and easy to follow.
E2E results are ready! |
|
This PR is being marked as stale due to inactivity. |
Refactor booking filters tests to improve clarity and structure. Update user creation and filter application logic.
Refactor user ID and email retrieval logic for clarity and efficiency.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/web/playwright/booking-filters.e2e.ts">
<violation number="1" location="apps/web/playwright/booking-filters.e2e.ts:269">
Rule violated: **E2E Tests Best Practices**
Missing expect(page).toHaveURL after both the initial goto and subsequent reload violates the E2E Tests Best Practices requirement to catch unexpected redirects. Please assert the URL after each navigation so the test fails fast if a redirect occurs.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="apps/web/playwright/booking-filters.e2e.ts">
<violation number="1" location="apps/web/playwright/booking-filters.e2e.ts:269">
Rule violated: **E2E Tests Best Practices**
After navigating with page.goto we should assert expect(page).toHaveURL(...) per the E2E Tests Best Practices so the test fails fast on unexpected redirects. Please add the missing URL assertion right after this navigation.</violation>
<violation number="2" location="apps/web/playwright/booking-filters.e2e.ts:285">
Rule violated: **E2E Tests Best Practices**
The new assertions rely on page.getByText, but the E2E Tests Best Practices require using stable data-testid (or role-based) locators to keep selectors resilient. Please switch these checks to test ids; the same adjustment is needed for the later "You do not have permissions" assertion in this block.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
3 participants
Fixes #24172
Fixes #24167