Skip to content

[CORRUPTED] Synthetic Benchmark PR #25434 - fix: get bookings handler for pbac and fallback roles#662

Open
tomerqodo wants to merge 30 commits intobase_pr_25434_20260121_6310from
corrupted_pr_25434_20260121_6310
Open

[CORRUPTED] Synthetic Benchmark PR #25434 - fix: get bookings handler for pbac and fallback roles#662
tomerqodo wants to merge 30 commits intobase_pr_25434_20260121_6310from
corrupted_pr_25434_20260121_6310

Conversation

@tomerqodo
Copy link

Benchmark PR calcom#25434

Type: Corrupted (contains bugs)

Original PR Title: fix: get bookings handler for pbac and fallback roles
Original PR Description: ## What does this PR do?

This PR is stacked upon calcom#25387

Fixes the issue where PBAC/fallback roles were not taken into consideration correctly when calling get booking. The changes include:

  • Refactored get.handler.ts to use PermissionCheckService instead of direct membership queries
  • Added orgId parameter to getTeamIdsWithPermission and getTeamIdsWithPermissions to properly scope results to the user's organization
  • Updated SQL queries in PermissionRepository to filter teams by organization scope
  • Added comprehensive unit tests for PBAC permission checks in the bookings handler
  • Added integration tests for the orgId filtering functionality

Updates since last revision

  • Merged latest main to resolve conflicts
  • Fixed failing unit tests by updating the PermissionCheckService mock to use function() instead of arrow function (required for proper constructor mocking in Vitest)
  • Renamed scopedOrgId parameter to orgId for consistency
  • Changed fallback roles to use MembershipRole enum instead of hardcoded strings

Visual Demo (For contributors especially)

N/A - Backend logic changes only

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
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  1. Run the unit tests: TZ=UTC yarn vitest run packages/trpc/server/routers/viewer/bookings/get.handler.test.ts
  2. Run the integration tests: VITEST_MODE=integration yarn test packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts
  3. Verify that users with PBAC permissions or ADMIN/OWNER fallback roles can view bookings for their team members
  4. Verify that users cannot view bookings for users outside their permission scope

Human Review Checklist

  • Verify the SQL queries in PermissionRepository.ts correctly handle the orgId filtering (including null/undefined cases)
  • Verify the permission checks work correctly for both PBAC-enabled teams and fallback role scenarios
  • Confirm the test mock fix using function() instead of arrow function is the correct pattern

Link to Devin run: https://app.devin.ai/sessions/8454efaba8ea4ecdb672f6ec9bde2876
Requested by: sean@cal.com (@sean-brydon)
Original PR URL: calcom#25434

sean-brydon and others added 30 commits November 25, 2025 09:48
Co-Authored-By: sean@cal.com <Sean@brydon.io>
…ermission-org' into fix/getTeamIdsWithPermission-org
Co-Authored-By: sean@cal.com <Sean@brydon.io>
…row function

Co-Authored-By: sean@cal.com <Sean@brydon.io>
Co-Authored-By: sean@cal.com <Sean@brydon.io>
Co-Authored-By: sean@cal.com <Sean@brydon.io>
@github-actions
Copy link

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 "[CORRUPTED] Synthetic Benchmark PR #25434 - fix: get bookings handler for pbac and fallback roles". 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

@github-actions
Copy link

This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active.

@github-actions github-actions bot added the Stale label Feb 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants