fix: allow multiple orgs of lead to be member of his own ecosystem#1562
Conversation
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds pagination types and pages through ecosystem and organization APIs, propagates page details across gateway, microservices, and repositories, introduces org-scoped invitation handling (invitedOrg/orgId), adds InvitationViewRole enum, restructures organization interfaces, and updates Prisma schema and migrations to link invitations to organisations. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant APIGW as API-Gateway
participant EcosystemMS as Ecosystem-Service
participant Repo as Ecosystem-Repository
participant DB as Database
Client->>APIGW: GET /ecosystems?pageNumber=1&pageSize=20
APIGW->>EcosystemMS: request { userId, pageDetail }
EcosystemMS->>Repo: getAllEcosystems(pageDetail, userId)
Repo->>DB: SELECT ... LIMIT/OFFSET, COUNT(...)
DB-->>Repo: rows + totalCount
Repo-->>EcosystemMS: { totalPages, data[] }
EcosystemMS-->>APIGW: PaginatedResponse
APIGW-->>Client: 200 { totalPages, data[] }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ecosystem/src/ecosystem.service.ts (1)
351-365:⚠️ Potential issue | 🔴 CriticalCritical bug:
getEcosystemOrgcalled withuserIdinstead oforgId— prevents creating ecosystem_orgs entries for a lead's additional organizations.At line 361, the method is called as
getEcosystemOrg(result.ecosystemId, result.userId), but the repository method signature expects(ecosystemId: string, orgId: string). The second parameter should beresult.invitedOrgto match theorgIdin theecosystemOrgPayload(line 351).When a lead's second organization accepts an invitation, the existing check will incorrectly find the first org's entry (since it searches by userId instead of orgId), causing
createEcosystemOrgto be skipped. This directly breaks the PR's goal of supporting multiple orgs per lead in an ecosystem.Proposed fix
- const existingOrg = await this.ecosystemRepository.getEcosystemOrg(result.ecosystemId, result.userId); + const existingOrg = await this.ecosystemRepository.getEcosystemOrg(result.ecosystemId, result.invitedOrg);apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
114-119:⚠️ Potential issue | 🔴 CriticalBug: Returns HTTP 200 for internal server errors.
The catch block sets
statusCode: HttpStatus.INTERNAL_SERVER_ERRORin the response body but sends it withres.status(HttpStatus.OK). Clients relying on HTTP status codes will treat server errors as successful responses.🐛 Proposed fix
- return res.status(HttpStatus.OK).json(finalResponse); + return res.status(HttpStatus.INTERNAL_SERVER_ERROR).json(finalResponse);
🤖 Fix all issues with AI agents
In `@apps/api-gateway/src/ecosystem/ecosystem.controller.ts`:
- Around line 304-315: The status query parameter for updateEcosystemOrgStatus
is not validated against the EcosystemOrgStatus enum; add validation by using
Nest's ParseEnumPipe with EcosystemOrgStatus on the `@Query`('status') parameter
so the framework rejects invalid values before reaching updateEcosystemOrgStatus
and downstream services (i.e., apply ParseEnumPipe(EcosystemOrgStatus) to the
status query param definition and keep the parameter type as
EcosystemOrgStatus).
In `@apps/api-gateway/src/organization/organization.controller.ts`:
- Around line 806-837: The endpoint currently allows JWT auth only and uses
British spelling inconsistently; update the controller method
getAllOrganisations to use the same authorization stack as the root GET by
adding UserAccessGuard alongside AuthGuard('jwt') (i.e.,
`@UseGuards`(AuthGuard('jwt'), UserAccessGuard)) so service accounts and 'holder'
role checks are enforced, and rename the route/metadata and method identifier to
American spelling to match organizationService.getAllOrganizations and
ResponseMessages.organisation.success.getOrganizations (e.g., change
'search/get-all-organisations' to 'search/get-all-organizations', ApiOperation
summary/description to "Get all organizations", and rename the controller method
to getAllOrganizations) to keep API naming consistent.
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 1351-1372: The method getEcosystemByRole uses
prisma.ecosystem_orgs.findFirst which can return null, but the signature
currently declares a non-nullable Promise<{ ecosystemRole: { name: string } }>,
so update the implementation to handle the null case: either change the return
type to Promise<{ ecosystemRole: { name: string } } | null> to reflect
findFirst’s possible null return, or keep the non-nullable signature and
explicitly throw a descriptive error when the result is null (and adjust callers
accordingly); locate this logic around getEcosystemByRole and the
prisma.ecosystem_orgs.findFirst call to apply the change.
- Around line 830-895: The whereClause in getAllEcosystemOrgsByEcosystemId only
filters by ecosystemId and may return soft-deleted rows; update the whereClause
used by prisma.ecosystem_orgs.findMany and prisma.ecosystem_orgs.count to
include deletedAt: null so both the data and count exclude soft-deleted records
(i.e., add deletedAt: null alongside ecosystemId in the existing whereClause).
- Around line 299-334: getAllEcosystems (and the other paginated methods
getAllEcosystemOrgsByEcosystemId, getIntents, getEcosystemsForEcosystemLead)
ignore the optional IPageDetail.search field; update each method to incorporate
pageDetail.search into the Prisma whereClause when present by adding a
case-insensitive "contains"/OR filter on the relevant text fields (e.g.,
ecosystem.name, ecosystem.description, related organisation.name or intent.name)
so results are filtered by the search term while preserving deletedAt/null and
existing pagination; implement this by conditionally building the whereClause
(merge the search OR filter only if pageDetail.search is truthy) and keep the
same transaction/count logic.
🧹 Nitpick comments (7)
libs/common/src/interfaces/organization.interface.ts (1)
111-119:IGetAllOrgsPayload.searchis non-optional but may receive empty strings.The
searchfield is typed asstring(not optional), which is fine sincePaginationDtodefaults it to''. However, consider marking it as optional (search?: string) to be consistent with the optional nature of search across the codebase and to avoid confusion for future callers.apps/organization/repositories/organization.repository.ts (1)
1227-1260: No guard againstpageSize = 0— division by zero and Prisma errors.
Math.ceil(totalCount / pageSize)yieldsInfinitywhenpageSizeis0, and(pageNumber - 1) * pageSizewithpageNumber = 0yields a negative skip. This is the same pattern used elsewhere in this file (e.g.,getOrgInvitationsPagination), but it's worth noting for robustness. Consider adding a guard or ensuring callers always validate.apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
107-107: Fragile error type check.
error instanceof ConflictException || HttpStatus.CONFLICT === error.statusCode— the second branch accesses.statusCodeon an untypederror. If the caught error is a plainError, this readsundefinedand silently falls through. This works today but is fragile. Consider narrowing the type or using optional chaining (error?.statusCode).apps/ecosystem/src/ecosystem.controller.ts (2)
22-22: Cross-layer import: microservice imports from the API gateway module.
IPageDetailandPaginatedResponseare imported fromapps/api-gateway/common/interface. The ecosystem microservice should not depend on the API gateway — this inverts the dependency direction. These shared types belong in a common library (e.g.,@credebl/common/interfaces).
163-167: Confusingpayload.payloadnesting.The message handler receives
payload: { payload: IEcosystemMemberInvitations; pageDetail: IPageDetail }, leading topayload.payloadon Line 167. Consider renaming the inner field (e.g.,invitationQueryorfilter) to improve clarity.♻️ Suggested improvement
async getEcosystemMemberInvitations(payload: { - payload: IEcosystemMemberInvitations; + filter: IEcosystemMemberInvitations; pageDetail: IPageDetail; }): Promise<PaginatedResponse<IEcosystemInvitation>> { - return this.ecosystemService.getEcosystemMemberInvitations(payload.payload, payload.pageDetail); + return this.ecosystemService.getEcosystemMemberInvitations(payload.filter, payload.pageDetail); }apps/ecosystem/repositories/ecosystem.repository.ts (2)
42-42: Cross-layer import: repository imports from the API gateway module.Same issue as in the ecosystem microservice controller —
IPageDetailandPaginatedResponseare imported fromapps/api-gateway/common/interface. The repository layer should never depend on the gateway. Move these interfaces to a shared library like@credebl/common.
388-392: Throwing plainErrorinstead of a domain-specific exception.Line 391 throws
new Error('Invitation not found for this specific organization'). Since this repository is called from a microservice context, this should probably be aNotFoundExceptionorRpcExceptionfor consistent error handling up the call stack. The plainErrorwill be caught by the outer catch and re-thrown, but upstream handlers may not map it to the correct HTTP status.♻️ Suggested improvement
if (!record) { - throw new Error('Invitation not found for this specific organization'); + throw new NotFoundException('Invitation not found for this specific organization'); }
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
114-119:⚠️ Potential issue | 🟠 MajorBug: HTTP 200 response for internal server error.
The response body reports
statusCode: HttpStatus.INTERNAL_SERVER_ERRORbut the actual HTTP status sent isHttpStatus.OK(200). This misleads clients and monitoring systems into treating server errors as successful responses.🐛 Proposed fix
- return res.status(HttpStatus.OK).json(finalResponse); + return res.status(HttpStatus.INTERNAL_SERVER_ERROR).json(finalResponse);
🤖 Fix all issues with AI agents
In `@apps/api-gateway/src/ecosystem/ecosystem.controller.ts`:
- Around line 154-159: The response body and actual HTTP status are
inconsistent: finalResponse.statusCode is HttpStatus.OK (200) but the controller
uses res.status(HttpStatus.CREATED) (201); update the controller to use the same
status for both (change res.status(HttpStatus.CREATED) to
res.status(HttpStatus.OK)) so finalResponse, HttpStatus.OK, res.status(...), and
the message built from ResponseMessages.ecosystem.success.updateInvitation and
status are aligned.
- Line 410: Add enum validation to the role query param by using Nest's
ParseEnumPipe: update the decorator for the role parameter (the one currently
declared as "@Query('role') role: InvitationViewRole =
InvitationViewRole.ECOSYSTEM_LEAD") to include "new
ParseEnumPipe(InvitationViewRole)". Also consider removing the default
assignment so the parameter is required to match the `@ApiQuery`(required: true)
declaration; if you want a default, validate first with ParseEnumPipe and then
apply the default in the method body. Ensure you reference the
InvitationViewRole enum and the role parameter when making the change.
sujitaw
changed the base branch from
main
to
refactor/ecosystem_apis_for_ui_requirements
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
|
sujitaw
merged commit 5d6b814
into
refactor/ecosystem_apis_for_ui_requirements
* refactor/update api with suggested fixes Signed-off-by: sujitaw <sujit.sutar@ayanworks.com> * fix/code rabbit issue Signed-off-by: sujitaw <sujit.sutar@ayanworks.com> * fix/pr comments Signed-off-by: sujitaw <sujit.sutar@ayanworks.com> * fix/code rabbit comments Signed-off-by: sujitaw <sujit.sutar@ayanworks.com> * fix: allow multiple orgs of lead to be member of his own ecosystem (#1562) * wip Signed-off-by: sujitaw <sujit.sutar@ayanworks.com> * fix/update to allow multiple orgs of lead as member in ecosystem Signed-off-by: sujitaw <sujit.sutar@ayanworks.com> * fix/code rabbit issues Signed-off-by: sujitaw <sujit.sutar@ayanworks.com> --------- Signed-off-by: sujitaw <sujit.sutar@ayanworks.com> --------- Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
2 participants
What
Summary by CodeRabbit
New Features
Improvements
API Changes