Conversation
keithwillcode
added
core
…-mixing-managed-users-with-non
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (03/21/25)1 reviewer was added to this PR based on Keith Williams's automation. |
E2E results are ready! |
|
This PR is being marked as stale due to inactivity. |
|
This PR is being marked as stale due to inactivity. |
…-mixing-managed-users-with-non
|
✅ No security or compliance issues detected. Reviewed everything up to 87017a8. Security Overview
Detected Code Changes
Reply to this PR with |
apps/api/v2/src/modules/memberships/services/memberships.service.ts
Outdated
Show resolved
Hide resolved
…-mixing-managed-users-with-non
|
Warning Rate limit exceeded@supalarry has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 58 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 selected for processing (1)
WalkthroughAdds OAuth-client–aware validation to organization and team membership creation. TeamsMembershipsService and OrganizationsMembershipService now inject OAuthClientRepository, expose new error constants, and implement canUserBeAddedToTeam / canUserBeAddedToOrg which enforce that user and team/org are either both non-platform or created by the same OAuth client (throwing BadRequest errors otherwise). OrganizationsTeamsMembershipsService now depends on TeamsMembershipsService and calls its pre-check before creating a team membership. OAuthClientRepository gains getByOrgId. TeamsMembershipsModule exports TeamsMembershipsService. An e2e suite exercises allowed and disallowed combinations. Possibly related PRs
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
![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
♻️ Duplicate comments (1)
apps/api/v2/src/modules/memberships/services/memberships.service.ts (1)
29-44: Platform/non-platform and same-OAuth-client logic is sound.Covers all cases; early-returns keep it simple. Nice.
🧹 Nitpick comments (6)
apps/api/v2/src/modules/memberships/services/memberships.service.ts (5)
46-54: Tighten error messages (single-line, consistent phrasing).Current template literals introduce newlines/indentation in responses. Prefer single-line messages for cleaner API errors.
Apply:
- if (userOAuthClient && !teamOAuthClient) { - throw new BadRequestException(`Can't add user to team - the user is platform managed user but team is not because team probably - was not created using OAuth credentials.`); - } + if (userOAuthClient && !teamOAuthClient) { + throw new BadRequestException( + "Can't add user to team — user is platform-managed but team is not (team likely not created via OAuth)." + ); + } @@ - if (!userOAuthClient && teamOAuthClient) { - throw new BadRequestException(`Can't add user to team - the user is not platform managed user but team is platform managed. Both have to be created - using OAuth credentials.`); - } + if (!userOAuthClient && teamOAuthClient) { + throw new BadRequestException( + "Can't add user to team — user is not platform-managed but team is platform-managed. Both must be created using OAuth." + ); + }
56-60: Minor: unify final message style.Consider the same dash style and no trailing period for consistency with messages above.
- throw new BadRequestException( - `Can't add user to team - managed user and team were created using different OAuth clients.` - ); + throw new BadRequestException( + "Can't add user to team — user and team were created using different OAuth clients" + );
62-62: Unreachable return.After prior returns/throws, this
return trueis redundant.- return true; + // no-op: all valid paths already returned; invalid ones threw
29-63: Optional: make this an assertion method returning void.Call sites don’t use the boolean. Renaming clarifies intent and reduces misleading return types.
- async canUserBeAddedToTeam(userId: number, teamId: number) { + async assertUserCanBeAddedToTeam(userId: number, teamId: number): Promise<void> { @@ - return true; + return; @@ - return true; + return; @@ - // no-op: all valid paths already returned; invalid ones threw + // no-opFollow-up usage (outside this hunk) in teams-memberships.service.ts:
await this.membershipsService.assertUserCanBeAddedToTeam(data.userId, teamId);
29-63: Tests missing for mismatch cases.Add unit/integration tests covering: both unmanaged; both managed same client; user-only managed; team-only managed; both managed different clients.
I can scaffold tests with fixtures for user/team OAuth-client mappings if helpful.
apps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.ts (1)
17-17: Pre-check before create is correct; consider atomicityGating creation via canUserBeAddedToTeam is correct—there remains a TOCTOU window between the check and the repository call; consolidate them into a single repository‐level method or Prisma transaction (e.g., createOrgTeamMembershipIfAllowed). canUserBeAddedToTeam is already async and contains no Prisma include.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/api/v2/src/modules/memberships/services/memberships.service.ts(2 hunks)apps/api/v2/src/modules/organizations/organizations.module.ts(2 hunks)apps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.ts(2 hunks)apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{service,repository}.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.tsapps/api/v2/src/modules/memberships/services/memberships.service.tsapps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.ts
**/*.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/teams/memberships/services/teams-memberships.service.tsapps/api/v2/src/modules/memberships/services/memberships.service.tsapps/api/v2/src/modules/organizations/organizations.module.tsapps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.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/teams/memberships/services/teams-memberships.service.tsapps/api/v2/src/modules/memberships/services/memberships.service.tsapps/api/v2/src/modules/organizations/organizations.module.tsapps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.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/teams/memberships/services/teams-memberships.service.tsapps/api/v2/src/modules/memberships/services/memberships.service.tsapps/api/v2/src/modules/organizations/organizations.module.tsapps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.ts
🧬 Code graph analysis (2)
apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts (1)
apps/api/v2/src/modules/teams/memberships/inputs/create-team-membership.input.ts (1)
CreateTeamMembershipInput(6-25)
apps/api/v2/src/modules/memberships/services/memberships.service.ts (2)
apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts (1)
Injectable(88-1100)apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts (1)
Injectable(9-62)
⏰ 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). (4)
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
- GitHub Check: Tests / Unit
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
apps/api/v2/src/modules/memberships/services/memberships.service.ts (1)
29-33: Confirm select-only id in OAuthClientRepository methods
EnsuregetByUserIdandgetByTeamIduse a Prismaselect: { id: true }clause to fetch only theidfield (oauth-client.repository.ts:98–110).apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts (2)
16-19: Pre-check placement is correct.Guarding createTeamMembership with the assertion ensures invariant before write. Good.
11-14: DI configuration verified
TeamsMembershipsModule already imports MembershipsModule (which exports MembershipsService), so the new DI dependency is correctly wired and no changes are needed.apps/api/v2/src/modules/organizations/organizations.module.ts (1)
16-16: MembershipsModule exports MembershipsService and MembershipsRepository; import is appropriate.apps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.ts (2)
1-1: LGTM: importing MembershipsService to enforce platform/OAuth policyImport looks correct and aligns with the PR objective to gate membership creation.
12-14: DI wiring verified; no circular dependency
No reverse import from MembershipsService to OrganizationsTeamsMembershipsService, and MembershipsModule is included in the organizations module imports.
apps/api/v2/src/modules/memberships/services/memberships.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (16)
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (2)
11-13: Tighten wording and consistency in user-facing error messages.Minor grammar/hyphenation for clarity and consistency with “platform-managed” phrasing used elsewhere.
-export const PLATFORM_USER_BEING_ADDED_TO_REGULAR_ORG_ERROR = `Can't add user to organization - the user is platform managed user but organization is not because organization probably was not created using OAuth credentials.`; -export const REGULAR_USER_BEING_ADDED_TO_PLATFORM_ORG_ERROR = `Can't add user to organization - the user is not platform managed user but organization is platform managed. Both have to be created using OAuth credentials.`; -export const MANAGED_USER_AND_MANAGED_ORG_CREATED_WITH_DIFFERENT_OAUTH_CLIENTS_ERROR = `Can't add user to organization - managed user and organization were created using different OAuth clients.`; +export const PLATFORM_USER_BEING_ADDED_TO_REGULAR_ORG_ERROR = `Can't add user to organization — the user is platform-managed but the organization is not (likely not created via OAuth).`; +export const REGULAR_USER_BEING_ADDED_TO_PLATFORM_ORG_ERROR = `Can't add user to organization — the user is not platform-managed but the organization is. Both must be created via OAuth.`; +export const MANAGED_USER_AND_MANAGED_ORG_CREATED_WITH_DIFFERENT_OAUTH_CLIENTS_ERROR = `Can't add user to organization — the managed user and organization were created using different OAuth clients.`;
109-146: Reduce data fetched, add explicit return type, and simplify logic.
- Add
: Promise<boolean>to the signature.- Since
getByOrgIdcan return only{ id }, rely on IDs and avoid carrying full rows.- Slightly simplify boolean checks to reduce branches.
- async canUserBeAddedToOrg(userId: number, orgId: number) { - const [userOAuthClient, orgOAuthClients] = await Promise.all([ + async canUserBeAddedToOrg(userId: number, orgId: number): Promise<boolean> { + const [userOAuthClient, orgOAuthClients] = await Promise.all([ this.oAuthClientsRepository.getByUserId(userId), this.oAuthClientsRepository.getByOrgId(orgId), ]); - const userAndOrgAreNotPlatform = !userOAuthClient && (!orgOAuthClients || orgOAuthClients.length === 0); + const userAndOrgAreNotPlatform = !userOAuthClient && (!orgOAuthClients || orgOAuthClients.length === 0); if (userAndOrgAreNotPlatform) { return true; } - const userAndOrgAreCreatedBySameOAuthClient = - userOAuthClient && - orgOAuthClients && - orgOAuthClients.some((orgClient) => orgClient.id === userOAuthClient.id); + const userAndOrgAreCreatedBySameOAuthClient = + !!userOAuthClient && !!orgOAuthClients?.some((orgClient) => orgClient.id === userOAuthClient.id); if (userAndOrgAreCreatedBySameOAuthClient) { return true; } if (userOAuthClient && (!orgOAuthClients || orgOAuthClients.length === 0)) { throw new BadRequestException(PLATFORM_USER_BEING_ADDED_TO_REGULAR_ORG_ERROR); } if (!userOAuthClient && orgOAuthClients && orgOAuthClients.length > 0) { throw new BadRequestException(REGULAR_USER_BEING_ADDED_TO_PLATFORM_ORG_ERROR); } - if ( - userOAuthClient && - orgOAuthClients && - orgOAuthClients.length > 0 && - !orgOAuthClients.some((orgClient) => orgClient.id === userOAuthClient.id) - ) { + if (userOAuthClient && orgOAuthClients?.length && !orgOAuthClients.some((c) => c.id === userOAuthClient.id)) { throw new BadRequestException(MANAGED_USER_AND_MANAGED_ORG_CREATED_WITH_DIFFERENT_OAUTH_CLIENTS_ERROR); } return true; }Note:
getByUserIdcurrently returns a full row; if feasible, add a narrow variant (e.g.,getIdByUserId) or update it toselect: { id: true }to fully adhere to the “select-only” guideline.Would you like me to raise a follow-up PR to introduce
getIdByUserIdand migrate the two membership services to use it?apps/api/v2/src/modules/organizations/organizations.module.ts (2)
69-71: Prefer importing TeamsMembershipsModule over hand-wiring service/repositoryWiring TeamsMembershipsService and TeamsMembershipsRepository directly couples modules. If TeamsMembershipsModule already exports the service, import that module instead for cleaner DI boundaries.
Apply:
+import { TeamsMembershipsModule } from "@/modules/teams/memberships/teams-memberships.module";And see diff below (providers section).
153-156: Import TeamsMembershipsModule and drop duplicate providers
TeamsMembershipsModule exports TeamsMembershipsService, so add it to imports and remove TeamsMembershipsService and TeamsMembershipsRepository from providers to avoid multiple instances.apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts (3)
9-12: Tighten error copy (optional)Messages are clear but a bit verbose. Consider shorter, action-focused phrasing to reduce log noise (keeping exact strings if tests assert them).
67-99: Return void from guard and drop unused booleansThe guard’s return value isn’t used. Returning void simplifies the contract and avoids misleading “true” returns.
- async canUserBeAddedToTeam(userId: number, teamId: number) { + async canUserBeAddedToTeam(userId: number, teamId: number): Promise<void> { @@ - if (userAndTeamAreNotPlatform) { - return true; - } + if (userAndTeamAreNotPlatform) return; @@ - if (userAndTeamAreCreatedBySameOAuthClient) { - return true; - } + if (userAndTeamAreCreatedBySameOAuthClient) return; @@ - return true; + return;
67-71: Fetch only what you need from PrismaIf OAuthClientRepository returns full rows, add repo methods that select only { id: true } to reduce payload and surface area.
I can send a follow-up PR adding getIdByUserId/getIdByTeamId with select and wire them here.
apps/api/v2/src/modules/organizations/teams/memberships/e2e/organizations-teams-memberships-guard.controller.e2e-spec.ts (9)
109-111: Remove duplicate ApiKeysRepositoryFixture instantiation.It’s already constructed on Line 79.
- apiKeysRepositoryFixture = new ApiKeysRepositoryFixture(moduleRef); const { keyString } = await apiKeysRepositoryFixture.createApiKey(orgOwner.id, null); orgApiKey = `cal_test_${keyString}`;
309-319: Disambiguate test title.Two tests share the same name; make the scope explicit.
- it("should not add managed user to organization team", async () => { + it("should not add managed user to organization team (org route)", async () => {
321-333: Disambiguate test title (teams route).- it("should not add managed user to organization team", async () => { + it("should not add managed user to organization team (teams route)", async () => {
378-389: Disambiguate platform test title (org route).- it("should not add user to platform organization team", async () => { + it("should not add user to platform organization team (org route)", async () => {
390-403: Disambiguate platform test title (teams route).- it("should not add user to platform organization team", async () => { + it("should not add user to platform organization team (teams route)", async () => {
9-11: Import org-level OAuth-client mismatch error constant to cover missing negative case.import { PLATFORM_USER_BEING_ADDED_TO_REGULAR_ORG_ERROR, REGULAR_USER_BEING_ADDED_TO_PLATFORM_ORG_ERROR, + MANAGED_USER_AND_MANAGED_ORG_CREATED_WITH_DIFFERENT_OAUTH_CLIENTS_ERROR, } from "@/modules/organizations/memberships/services/organizations-membership.service";
418-419: Add missing negative: platform org membership with user from a different OAuth client should fail.}); }); + it("should not add user to platform organization because user is created using different oAuth client", async () => { + const response = await request(app.getHttpServer()) + .post(`/v2/organizations/${platformOrg.id}/memberships`) + .set("x-cal-client-id", platformOAuthClient.id) + .set("x-cal-secret-key", platformOAuthClient.secret) + .send({ + userId: secondPlatformOrgUser.user.id, + accepted: true, + role: "MEMBER", + } satisfies CreateOrgTeamMembershipDto) + .expect(400); + expect(response.body.error.message).toEqual( + MANAGED_USER_AND_MANAGED_ORG_CREATED_WITH_DIFFERENT_OAUTH_CLIENTS_ERROR + ); + });
25-27: Prefer concrete NestExpressApplication typing to avoid casting.-import { INestApplication } from "@nestjs/common"; import { NestExpressApplication } from "@nestjs/platform-express"; @@ - let app: INestApplication; + let app: NestExpressApplication; @@ - app = moduleRef.createNestApplication(); - bootstrap(app as NestExpressApplication); + app = moduleRef.createNestApplication<NestExpressApplication>(); + bootstrap(app);Also applies to: 43-47, 150-154
450-456: Avoid Prismaincludein fixtures; select only needed fields.
oauthClientRepositoryFixture.getUsers()usesinclude: { users: true }. Per guidelines, switch toselectto fetch only the needed user fields (email).Apply this change in apps/api/v2/test/fixtures/repository/oauth-client.repository.fixture.ts:
- const response = await this.prismaReadClient.platformOAuthClient.findFirst({ - where: { id: clientId }, - include: { - users: true, - }, - }); + const response = await this.prismaReadClient.platformOAuthClient.findFirst({ + where: { id: clientId }, + select: { + users: { select: { email: true } }, + }, + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/api/v2/src/modules/oauth-clients/oauth-client.repository.ts(1 hunks)apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts(2 hunks)apps/api/v2/src/modules/organizations/organizations.module.ts(4 hunks)apps/api/v2/src/modules/organizations/teams/memberships/e2e/organizations-teams-memberships-guard.controller.e2e-spec.ts(1 hunks)apps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.ts(1 hunks)apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts(2 hunks)apps/api/v2/src/modules/teams/memberships/teams-memberships.module.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:
apps/api/v2/src/modules/teams/memberships/teams-memberships.module.tsapps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.tsapps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.tsapps/api/v2/src/modules/oauth-clients/oauth-client.repository.tsapps/api/v2/src/modules/organizations/teams/memberships/e2e/organizations-teams-memberships-guard.controller.e2e-spec.tsapps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.tsapps/api/v2/src/modules/organizations/organizations.module.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/teams/memberships/teams-memberships.module.tsapps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.tsapps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.tsapps/api/v2/src/modules/oauth-clients/oauth-client.repository.tsapps/api/v2/src/modules/organizations/teams/memberships/e2e/organizations-teams-memberships-guard.controller.e2e-spec.tsapps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.tsapps/api/v2/src/modules/organizations/organizations.module.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/teams/memberships/teams-memberships.module.tsapps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.tsapps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.tsapps/api/v2/src/modules/oauth-clients/oauth-client.repository.tsapps/api/v2/src/modules/organizations/teams/memberships/e2e/organizations-teams-memberships-guard.controller.e2e-spec.tsapps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.tsapps/api/v2/src/modules/organizations/organizations.module.ts
**/*.{service,repository}.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.tsapps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.tsapps/api/v2/src/modules/oauth-clients/oauth-client.repository.tsapps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts
🧬 Code graph analysis (4)
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (4)
apps/api/v2/src/modules/oauth-clients/oauth-client.repository.ts (1)
Injectable(6-144)apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts (1)
Injectable(13-100)apps/api/v2/src/modules/organizations/memberships/organizations-membership.repository.ts (1)
Injectable(32-122)apps/api/v2/src/modules/organizations/memberships/services/organizations-membership-output.service.ts (1)
Injectable(16-89)
apps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.ts (2)
apps/api/v2/src/modules/oauth-clients/oauth-client.repository.ts (1)
Injectable(6-144)apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts (1)
Injectable(13-100)
apps/api/v2/src/modules/organizations/teams/memberships/e2e/organizations-teams-memberships-guard.controller.e2e-spec.ts (13)
apps/api/v2/test/fixtures/repository/membership.repository.fixture.ts (1)
MembershipRepositoryFixture(8-44)apps/api/v2/test/fixtures/repository/oauth-client.repository.fixture.ts (1)
OAuthClientRepositoryFixture(8-64)apps/api/v2/test/fixtures/repository/profiles.repository.fixture.ts (1)
ProfileRepositoryFixture(6-30)apps/api/v2/test/fixtures/repository/api-keys.repository.fixture.ts (1)
ApiKeysRepositoryFixture(6-37)apps/api/v2/src/modules/oauth-clients/controllers/oauth-client-users/outputs/create-managed-user.output.ts (2)
CreateManagedUserData(9-16)CreateManagedUserOutput(18-31)apps/api/v2/src/app.ts (1)
bootstrap(27-89)apps/api/v2/src/modules/users/inputs/create-user.input.ts (1)
CreateUserInput(24-165)packages/platform/constants/api.ts (1)
SUCCESS_STATUS(9-9)apps/api/v2/src/modules/users/inputs/create-managed-user.input.ts (1)
CreateManagedUserInput(10-79)packages/platform/types/api.ts (1)
ApiSuccessResponse(8-8)apps/api/v2/src/modules/organizations/teams/memberships/inputs/create-organization-team-membership.input.ts (1)
CreateOrgTeamMembershipDto(6-24)apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (2)
PLATFORM_USER_BEING_ADDED_TO_REGULAR_ORG_ERROR(11-11)REGULAR_USER_BEING_ADDED_TO_PLATFORM_ORG_ERROR(12-12)apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts (3)
PLATFORM_USER_BEING_ADDED_TO_REGULAR_TEAM_ERROR(9-9)REGULAR_USER_BEING_ADDED_TO_PLATFORM_TEAM_ERROR(10-10)PLATFORM_USER_AND_PLATFORM_TEAM_CREATED_WITH_DIFFERENT_OAUTH_CLIENTS_ERROR(11-11)
apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts (3)
apps/api/v2/src/modules/oauth-clients/oauth-client.repository.ts (1)
Injectable(6-144)apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (1)
Injectable(15-147)apps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.ts (1)
Injectable(9-83)
⏰ 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: Install dependencies / Yarn install & cache
🔇 Additional comments (14)
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (3)
1-1: DI wiring and imports look correct.Importing the repository and exceptions is appropriate for the new pre-checks.
Also applies to: 4-4
19-21: Constructor changes LGTM.New dependency is clearly injected and used only for pre-checks.
103-107: Good guard: pre-validate before creating membership.Early eligibility check matches the PR objective and mirrors the teams flow.
apps/api/v2/src/modules/teams/memberships/teams-memberships.module.ts (1)
15-15: No circular dependency detected between TeamsMembershipsModule and OrganizationsModule; no forwardRef needed.apps/api/v2/src/modules/organizations/organizations.module.ts (1)
99-99: Import of MembershipsModule looks goodNo issues spotted with the module import.
apps/api/v2/src/modules/organizations/teams/memberships/services/organizations-teams-memberships.service.ts (3)
4-4: LGTM: Using TeamsMembershipsService for pre-checksImport is appropriate and aligns org-team membership creation with team membership rules.
17-18: Pre-check before creation is correct; mirror on updates if userId can changeIf UpdateOrgTeamMembershipDto allows changing userId, add the same canUserBeAddedToTeam guard in update to keep invariants intact.
Do we allow userId changes? If yes, I can draft the guarded update flow.
12-14: Confirm potential circular dependency between TeamsMembershipsService and OrganizationsModuleTeamsMembershipsModule and OrganizationsTeamsMembershipsModule imports arrays don’t reference each other; manually verify whether TeamsMembershipsService (or its module) imports OrganizationsModule transitively and, if so, inject via @Inject(forwardRef(() => TeamsMembershipsService)).
apps/api/v2/src/modules/teams/memberships/services/teams-memberships.service.ts (5)
1-1: LGTM: Injecting OAuthClientRepositoryNecessary for platform/OAuth consistency checks.
5-5: LGTM: Exception types addedBadRequestException is appropriate for mismatch cases.
15-19: LGTM: DI for repo + OAuth client repoConstructor wiring looks good.
21-23: LGTM: Pre-check integrated into create pathThis enforces invariants on creation.
68-71: Assumption: one OAuth client per user/teamUsing findFirst implies singular ownership. Verify schema guarantees at most one PlatformOAuthClient per user and per team; otherwise, compare against all linked client ids.
I can script a quick check against the Prisma schema or adjust logic to handle many-to-many if needed.
apps/api/v2/src/modules/organizations/teams/memberships/e2e/organizations-teams-memberships-guard.controller.e2e-spec.ts (1)
179-185: Confirm intentional route prefix mix (/v2vs/api/v2).Platform endpoints use
/api/v2/...while org endpoints use/v2/.... Validate this aligns with the router setup and won’t regress if global prefixes/versioning change.Also applies to: 207-212, 238-243
...nizations/teams/memberships/e2e/organizations-teams-memberships-guard.controller.e2e-spec.ts
Show resolved
Hide resolved
supalarry
deleted the
lauris/cal-5334-platform-refactor-dont-allow-mixing-managed-users-with-non
branch
3 participants
Linear CAL-5334