Skip to content

Comments

fix: api-v2 tests#23896

Merged
anikdhabal merged 1 commit intomainfrom
fix-v2-tests
Sep 17, 2025
Merged

fix: api-v2 tests#23896
anikdhabal merged 1 commit intomainfrom
fix-v2-tests

Conversation

@hariombalhara
Copy link
Member

What does this PR do?

  • Fixes #XXXX (GitHub issue number)
  • Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Visual Demo (For contributors especially)

A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).

Video Demo (if applicable):

  • Show screen recordings of the issue or feature.
  • Demonstrate how to reproduce the issue, the behavior before and after the change.

Image Demo (if applicable):

  • Add side-by-side screenshots of the original and updated change.
  • Highlight any significant change(s).

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. 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?

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

Walkthrough

The end-to-end test for organizations event types was updated. The setup now creates an ADMIN membership for userAdmin on the team (accepted: true). In the test "should assign all members to managed event-type," the expected count of returned event types increased from 3 to 4, accounting for the additional member’s event type after assigning all team members. No exported or public entity signatures were changed. Estimated code review effort noted as Medium.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: api-v2 tests" is concise and directly reflects the main change in the changeset, which updates apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts and adjusts test expectations to resolve failing API v2 tests. It is a short, single-sentence summary that focuses on the primary intent without extraneous details. Therefore it meets the project's title guidance for clarity and relevance.
Description Check ✅ Passed The PR description is related to the changeset because it declares the intent to fix API v2 tests and includes checklists and testing guidance placeholders, so it is not off-topic. However, it contains placeholders (Fixes #XXXX, CAL-XXXX), leaves mandatory checklist items unchecked, and lacks concrete reproduction steps, environment variables, and test details. Under the lenient criteria this remains related and therefore passes the description check.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-v2-tests

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 17, 2025

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 "fix api-v2 tests". 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

Copy link
Member Author

hariombalhara commented Sep 17, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@hariombalhara hariombalhara changed the base branch from hariom/pri-305-bookingcreateservice to graphite-base/23896 September 17, 2025 12:44
@hariombalhara hariombalhara changed the base branch from graphite-base/23896 to main September 17, 2025 12:44
@vercel
Copy link

vercel bot commented Sep 17, 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 17, 2025 0:46am
cal-eu Ignored Ignored Sep 17, 2025 0:46am

@anikdhabal anikdhabal changed the title fix api-v2 tests fix: api-v2 tests Sep 17, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 17, 2025

E2E results are ready!

@anikdhabal anikdhabal marked this pull request as ready for review September 17, 2025 13:28
@anikdhabal anikdhabal requested a review from a team September 17, 2025 13:28
@anikdhabal anikdhabal requested a review from a team as a code owner September 17, 2025 13:28
@graphite-app graphite-app bot requested a review from a team September 17, 2025 13:28
Copy link
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

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

Looks good

@anikdhabal anikdhabal enabled auto-merge (squash) September 17, 2025 13:29
@dosubot dosubot bot added api area: API, enterprise API, access token, OAuth automated-tests area: unit tests, e2e tests, playwright labels Sep 17, 2025
Copy link
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

Thanks man! 🙏

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 (2)
apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts (2)

910-947: Strengthen the assertion by explicitly validating the admin’s user-level managed event

Right now we only assert count=4. Let’s assert the additional item belongs to userAdmin so the test fails for the right reason.

Apply this diff (additions only):

           expect(responseTeamEvent?.assignAllTeamMembers).toEqual(true);
+          const adminEvent = responseBody.data.find(
+            (eventType) => eventType.ownerId === userAdmin.id
+          );
+          expect(adminEvent).toBeDefined();
+          expect(adminEvent?.parentEventTypeId).toEqual(responseTeamEvent?.id);

939-941: Fix variable used in expectation (copy/paste typo)

You compute responseTeammate2Event but assert responseTeammate1Event again.

-          expect(responseTeammate1Event).toBeDefined();
+          expect(responseTeammate2Event).toBeDefined();
📜 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 39917eb and 6d047eb.

📒 Files selected for processing (1)
  • apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.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:

  • apps/api/v2/src/modules/organizations/event-types/organizations-event-types.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/event-types/organizations-event-types.e2e-spec.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/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally set to 0, as confirmed by sean-brydon (PR author). This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior that maintains consistency with the previous implementation.
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally hard-coded to 0, as confirmed by sean-brydon. This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior.
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.

Applied to files:

  • apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.

Applied to files:

  • apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally set to 0, as confirmed by sean-brydon (PR author). This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior that maintains consistency with the previous implementation.

Applied to files:

  • apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts
🔇 Additional comments (1)
apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts (1)

181-187: Adding team-level ADMIN membership for userAdmin makes sense

This looks correct to satisfy team-admin auth and to ensure assignAllTeamMembers includes the admin in the set.

Please confirm that assignAllTeamMembers is intended to include team admins; otherwise the later assertion on total items would regress.

@anikdhabal anikdhabal merged commit c2a44e9 into main Sep 17, 2025
134 of 145 checks passed
@anikdhabal anikdhabal deleted the fix-v2-tests branch September 17, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api area: API, enterprise API, access token, OAuth automated-tests area: unit tests, e2e tests, playwright core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO ready-for-e2e size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants