refactor:add pagination for required API's and fixes#1558
Conversation
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds pagination types and threads paginated requests/responses through controllers, services, and repositories across ecosystem and organization modules; adjusts DTOs and enums for invitations and org relations; updates Prisma schema/migrations to link invitations to organisations; removes a seed JSON file and tweaks Swagger and JWT error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant APIGW as API-Gateway Controller
participant GWService as API-Gateway Service
participant EcosystemSvc as Ecosystem Service
participant Repo as Repository
participant DB as Database
Client->>APIGW: GET /ecosystems?pageNumber=1&pageSize=20
APIGW->>GWService: parse pagination & user
GWService->>EcosystemSvc: getEcosystems(userId, pageDetail)
EcosystemSvc->>Repo: fetch paged data (take/skip) + count
Repo->>DB: SELECT ... LIMIT ... OFFSET ...
DB-->>Repo: rows + totalCount
Repo-->>EcosystemSvc: { totalPages, data }
EcosystemSvc-->>GWService: PaginatedResponse
GWService-->>APIGW: 200 { totalPages, data }
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api-gateway/src/ecosystem/dtos/ecosystem.ts (1)
26-31:⚠️ Potential issue | 🟡 Minor
GetEcosystemInvitationsQueryDtowas simplified to only exposeecosystemIdat the API level.The DTO no longer exposes
roleandroleis still extracted separately via@Query('role')anduserIdfrom the authenticated user context, both passed to the service for filtering. Email filtering is no longer available—if the UI relied on server-side filtering by email, it will need to filter client-side or email support must be re-added to the query parameters.apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
348-374:⚠️ Potential issue | 🟡 MinorReturning HTTP 404 for an empty paginated page discards pagination metadata.
When
ecosystemData.data.length === 0, the response is a 404 with nototalPages. A page with zero results (e.g., last page + 1, or an ecosystem with no orgs yet) is a valid paginated response, not "not found." Return 200 with{ totalPages: 0, data: [] }instead.♻️ Suggested fix
const ecosystemData = await this.ecosystemService.getAllEcosystemOrgsByEcosystemId(ecosystemId, pageDto); - if (ecosystemData.data && 0 < ecosystemData.data.length) { - return res.status(HttpStatus.OK).json({ - statusCode: HttpStatus.OK, - message: ResponseMessages.ecosystem.success.fetchOrgs, - data: ecosystemData - }); - } - - return res.status(HttpStatus.NOT_FOUND).json({ - statusCode: HttpStatus.NOT_FOUND, - message: ResponseMessages.ecosystem.error.ecosystemOrgsFetchFailed + return res.status(HttpStatus.OK).json({ + statusCode: HttpStatus.OK, + message: ResponseMessages.ecosystem.success.fetchOrgs, + data: ecosystemData });apps/ecosystem/repositories/ecosystem.repository.ts (1)
299-345:⚠️ Potential issue | 🔴 CriticalDivision by zero when
pageSizeis0, and negativeskipwhenpageNumber < 1across all paginated methods.The repository methods receive
IPageDetailwhich has no validation constraints. WhilePaginationDto(with@Min(1)validators) exists in the api-gateway, the ecosystem service controller acceptsIPageDetaildirectly via@MessagePatternhandlers, bypassing validation entirely. IfpageSize === 0,Math.ceil(totalCount / 0)producesInfinity. IfpageNumber < 1,(pageNumber - 1) * pageSizeproduces a negativeskip, which Prisma rejects. This pattern repeats across at least 6 methods:getAllEcosystems,getAllEcosystemOrgsByEcosystemId,getEcosystemInvitations,getIntents,getTemplatesByOrgId, andgetEcosystemsForEcosystemLead.Add validation to
IPageDetailinterface or validate at the start of each repository method. Alternatively, enforce validation in the ecosystem service layer before passing to repository methods.
🤖 Fix all issues with AI agents
In `@apps/api-gateway/src/ecosystem/ecosystem.controller.ts`:
- Around line 140-142: The route handler currently defaults the query param to
Invitation.ACCEPTED which silently accepts invitations when clients omit
?status; remove the default so the signature becomes `@Query`('status') status:
Invitation (or better: `@Query`('status', new ParseEnumPipe(Invitation)) status:
Invitation) and import ParseEnumPipe from `@nestjs/common` to validate/require the
enum value; update the handler (the method taking `@Res`() res: Response and
`@Query`('status') status: Invitation) to return a 400 or throw
BadRequestException when status is missing/invalid so state-changing actions
must be explicit.
In `@apps/api-gateway/src/organization/organization.controller.ts`:
- Around line 829-832: getAllOrganisations currently returns the raw service
result via res.send and bypasses the controller's standard IResponse envelope;
change getAllOrganisations to build an IResponse object (with statusCode:
HttpStatus.OK, message: a short success string, and data: the value from
this.organizationService.getAllOrganizations(paginationDto)) and return it using
res.status(HttpStatus.OK).json(finalResponse) (matching other endpoints),
ensuring you still accept PaginationDto and keep the service call to
this.organizationService.getAllOrganizations.
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 334-339: Several paginated repository methods are inconsistent:
when pageDetail.pageNumber > totalPages (and totalPages !== 0) some functions
return { totalPages, data: [] } while others let Prisma return stale/partial
results. For each paginated method (e.g., getEcosystemInvitations, getIntents,
getTemplatesByOrgId, getAllEcosystemOrgsByEcosystemId and the method around
lines 1339-1344), add the same out-of-range guard used in
getAllEcosystems/getEcosystemsForEcosystemLead — check if pageDetail.pageNumber
> totalPages && totalPages !== 0 and if so return { totalPages, data: [] }
immediately before calling Prisma — ensuring consistent behavior and using the
same result shape and variable names (totalPages, data) as the existing
implementations.
- Around line 1290-1301: In getEcosystemsForEcosystemLead update the role enum
used in the where clause: replace OrgRoles.ECOSYSTEM_LEAD with
EcosystemRoles.ECOSYSTEM_LEAD so the repository matches the service layer and
other methods; locate the usage inside the ecosystemOrgs.some.ecosystemRole.name
check in getEcosystemsForEcosystemLead and swap the enum reference accordingly.
In `@apps/organization/repositories/organization.repository.ts`:
- Around line 1227-1260: The repository method getAllOrganizations must guard
against invalid pagination inputs: ensure pageSize and pageNumber are normalized
before use (e.g., if pageSize < 1 set a sensible default like 10 or throw a
BadRequest, and set pageNumber = Math.max(1, pageNumber)) so skip = (pageNumber
- 1) * pageSize is never negative and totalPages = Math.ceil(totalCount /
pageSize) never divides by zero; implement this normalization/check at the start
of getAllOrganizations, then use the normalized values for prisma.findMany
skip/take and totalPages calculation.
In `@apps/organization/src/organization.service.ts`:
- Around line 2103-2110: The error log in getAllOrganisation incorrectly
references "In fetchUserInvitation"; update the logger call inside the catch of
getAllOrganisation to use the correct method name and context (e.g., "In
getAllOrganisation") and keep logging the serialized error; ensure the
RpcException throw remains unchanged and still uses error.response if present.
Reference: getAllOrganisation method and this.logger.error usage.
In `@libs/enum/src/enum.ts`:
- Around line 73-76: The string value for InvitationViewRole.ECOSYSTEM_LEAD
contains a typo ("Ecosysetm Lead"); update the enum in enum.ts so
InvitationViewRole.ECOSYSTEM_LEAD is set to "Ecosystem Lead" to match the
existing EcosystemRoles.ECOSYSTEM_LEAD value and avoid mismatches when comparing
these enums.
🧹 Nitpick comments (12)
libs/common/src/interfaces/organization.interface.ts (1)
111-120:IAllOrgsNameIdlackstotalCount;IGetAllOrgsPayload.searchshould be optional.
- Other paginated responses in this codebase (e.g.,
IGetOrganization) return bothtotalCountandtotalPages.IAllOrgsNameIdonly hastotalPages, which may limit the consumer's ability to display "showing X of Y" style UI.searchis declared as required inIGetAllOrgsPayload, butPaginationDto(used at the gateway) defaults it to''. Consider marking it optional (search?: string) for consistency.Suggested change
export interface IAllOrgsNameId { + totalCount: number; totalPages: number; orgs: { id: string; name: string }[]; } export interface IGetAllOrgsPayload { pageSize: number; pageNumber: number; - search: string; + search?: string; }apps/organization/repositories/organization.repository.ts (1)
1229-1249: Duplicatedwhereclause in transaction queries.The
where: { name: { contains: search, mode: 'insensitive' } }filter is repeated in bothfindManyandcount. Extract it to a local variable to keep them in sync and reduce duplication, consistent with howgetOrgInvitationsPagination(line 360) uses a sharedqueryObject.Suggested refactor
async getAllOrganizations(search: string, pageNumber: number, pageSize: number): Promise<IAllOrgsNameId> { try { + const where = { name: { contains: search, mode: 'insensitive' as const } }; const result = await this.prisma.$transaction([ this.prisma.organisation.findMany({ - where: { - name: { contains: search, mode: 'insensitive' } - }, + where, take: pageSize, select: { id: true, name: true }, skip: (pageNumber - 1) * pageSize, orderBy: { createDateTime: 'desc' } }), this.prisma.organisation.count({ - where: { - name: { contains: search, mode: 'insensitive' } - } + where }) ]);apps/api-gateway/common/interface.ts (1)
30-38: Consider addingtotalCounttoPaginatedResponse.Most paginated APIs return both
totalPagesandtotalCount(total number of records) so clients can display accurate pagination info (e.g., "showing 1–10 of 47 results"). Currently, clients can only derive an approximate total fromtotalPages * pageSize.Proposed enhancement
export interface PaginatedResponse<T> { + totalCount: number; totalPages: number; data: T[]; }apps/ecosystem/interfaces/ecosystem.interfaces.ts (1)
212-212: Remove unused type aliasEcosystemInvitationRoles.This type is defined but never referenced anywhere in the codebase. It can be safely removed.
apps/ecosystem/src/ecosystem.service.ts (3)
165-185: Remove commented-out code.This large block of dead code clutters the method. The
returnon Line 164 makes this unreachable anyway. If this code is no longer needed, delete it; if it's kept for reference, track it in a separate issue or commit message instead.🧹 Proposed cleanup
try { return await this.ecosystemRepository.getInvitationsByUserId(userId); - // for (const val of invitationData) { - // if (val.invitedOrg && val.ecosystemId) { - // const orgDetails = await this.ecosystemRepository.getEcosystemOrg(val.ecosystemId, val.invitedOrg); - // if (orgDetails) { - // val.organization = orgDetails; - // } - // } - // } - - // return invitationData; - // const includeOrg = invitationData.map(async (val) => { - // if (val.invitedOrg && val.ecosystemId) { - // const orgDetails = await this.ecosystemRepository.getEcosystemOrg(val.ecosystemId, val.invitedOrg); - // if (orgDetails) { - // val.organization = orgDetails; - // } - // } else { - // return val; - // } - // }); - // return includeOrg; } catch (error) {
304-320: Potentialnulldereference whengetEcosystemByRolereturnsnull— and the role check is redundant.
getEcosystemByRoleusesfindFirst, which resolves tonullwhen no match exists. The&&guard on Line 310 protects against that, so there's no crash. However, the role-name comparison is redundant because the repository already filters byecosystemRole.name === EcosystemRoles.ECOSYSTEM_LEAD— if a result is returned, it will always match. Theelsebranch also silently returns all ecosystems for any non-lead user (including members), which may be unintended from an access-control perspective.♻️ Simplify the condition
- const ecosystem = await this.ecosystemRepository.getEcosystemByRole(userId, EcosystemRoles.ECOSYSTEM_LEAD); - if (ecosystem && ecosystem.ecosystemRole.name === EcosystemRoles.ECOSYSTEM_LEAD) { - const leadEcosystems = await this.ecosystemRepository.getEcosystemsForEcosystemLead(userId, pageDetail); - return leadEcosystems; - } else { - return this.ecosystemRepository.getAllEcosystems(pageDetail); - } + const isLead = await this.ecosystemRepository.getEcosystemByRole(userId, EcosystemRoles.ECOSYSTEM_LEAD); + if (isLead) { + return this.ecosystemRepository.getEcosystemsForEcosystemLead(userId, pageDetail); + } + return this.ecosystemRepository.getAllEcosystems(pageDetail);
44-44: Cross-layer import: microservice imports from API gateway.The ecosystem microservice (
apps/ecosystem) importsIPageDetailandPaginatedResponsefromapps/api-gateway/common/interface. This creates a dependency from the microservice layer back into the API gateway layer, which inverts the expected dependency direction. Consider moving these shared interfaces to a common library (e.g.,@credebl/commonorlibs/common) so both layers can depend on a shared module.apps/ecosystem/src/ecosystem.controller.ts (2)
162-167: Confusing nestedpayload.payloadnaming.The parameter name
payloadshadows the convention already used in every other handler. Havingpayload.payload(inner field also namedpayload) is easy to misread and a maintenance hazard. Consider renaming the inner field (e.g.,paramsorinvitationParams).♻️ Suggested rename
async getEcosystemMemberInvitations(payload: { - payload: IEcosystemMemberInvitations; + params: IEcosystemMemberInvitations; pageDetail: IPageDetail; }): Promise<PaginatedResponse<IEcosystemInvitation>> { - return this.ecosystemService.getEcosystemMemberInvitations(payload.payload, payload.pageDetail); + return this.ecosystemService.getEcosystemMemberInvitations(payload.params, payload.pageDetail); }The same rename would need to be applied in the API gateway service where the NATS message is constructed.
279-285: Method namegetTemplatesByIntentIdis misleading — it fetches byorgId.The handler accepts
payload.orgIdand delegates togetTemplatesByOrgId, yet the method is namedgetTemplatesByIntentId. This will confuse future readers.♻️ Rename to match the actual behaviour
- async getTemplatesByIntentId(payload: { + async getVerificationTemplatesByOrgId(payload: { orgId: string; pageDetail: IPageDetail; }): Promise<PaginatedResponse<object>> {apps/ecosystem/repositories/ecosystem.repository.ts (1)
326-327: Duplicated pagination boilerplate across 6 methods.The
$transaction([findMany(..., take, skip), count()])+Math.ceil+ return{ totalPages, data }pattern is copy-pasted ingetAllEcosystems,getAllEcosystemOrgsByEcosystemId,getEcosystemInvitations,getIntents,getTemplatesByOrgId, andgetEcosystemsForEcosystemLead. Consider extracting a generic helper, e.g.:private async paginate<T>( model: { findMany: Function; count: Function }, args: { where: any; orderBy?: any; select?: any; include?: any }, pageDetail: IPageDetail ): Promise<PaginatedResponse<T>> { ... }This would centralize the
take/skip/totalPageslogic, the division-by-zero guard, and the out-of-range page behavior in one place.Also applies to: 880-881, 1123-1124, 1152-1153, 1191-1192, 1329-1330
apps/api-gateway/src/ecosystem/ecosystem.service.ts (1)
124-127: Parameterusershadows the imported Prismausertype.Line 13 imports
user(the Prisma type), and Line 124 declares a parameter also nameduser. While TypeScript allows this, it prevents using theusertype elsewhere in this method if needed. A rename (e.g.,currentUserorreqUser) would improve clarity.apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
302-313: Defaultstatus = EcosystemOrgStatus.INACTIVEforupdateEcosystemOrgStatusmay be surprising.Similar to the invitation status default, this silently deactivates orgs when the
statusquery param is omitted. Given therequired: truein the@ApiQuerydecorator (Line 305), Swagger will enforce it, but NestJS itself won't reject the request if the param is missing — it'll use the default. Consider usingParseEnumPipewithout a default for consistency with therequired: truedeclaration.
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
386-415:⚠️ Potential issue | 🟡 MinorSwagger
required: truecontradicted by default value onroleparameter.Line 389 declares
required: truein@ApiQuery, but line 409 provides a default (InvitationViewRole.ECOSYSTEM_LEAD). This means:
- Swagger UI shows
roleas mandatory, yet the endpoint works without it (defaulting toECOSYSTEM_LEAD).- A client omitting both
roleandecosystemIdgets a 400 — which is confusing since the API appears to accept the call.Either remove the default to truly require the parameter (add
ParseEnumPipefor validation), or setrequired: falsein@ApiQueryto match the runtime behavior.apps/ecosystem/src/ecosystem.service.ts (2)
283-299:⚠️ Potential issue | 🔴 CriticalNull-pointer crash when user has no ecosystem role.
getEcosystemByRole(line 288) returnsnullwhen no matchingecosystem_orgsrecord is found (Prisma'sfindFirstreturnsnull). Line 289 then dereferencesecosystem.ecosystemRole.name, which throws aTypeError: Cannot read properties of null.This means any non-lead user hitting this endpoint will crash the service instead of falling through to
getAllEcosystems.Proposed fix
- const ecosystem = await this.ecosystemRepository.getEcosystemByRole(userId, EcosystemRoles.ECOSYSTEM_LEAD); - if (ecosystem && ecosystem.ecosystemRole.name === EcosystemRoles.ECOSYSTEM_LEAD) { - const leadEcosystems = await this.ecosystemRepository.getEcosystemsForEcosystemLead(userId, pageDetail); + const ecosystemOrg = await this.ecosystemRepository.getEcosystemByRole(userId, EcosystemRoles.ECOSYSTEM_LEAD); + if (ecosystemOrg?.ecosystemRole?.name === EcosystemRoles.ECOSYSTEM_LEAD) { + const leadEcosystems = await this.ecosystemRepository.getEcosystemsForEcosystemLead(userId, pageDetail); return leadEcosystems; } else { - return this.ecosystemRepository.getAllEcosystems(pageDetail); + return this.ecosystemRepository.getAllEcosystems(pageDetail); }Additionally, since
getEcosystemByRolealready filters byecosystemRole.name = EcosystemRoles.ECOSYSTEM_LEAD, theifcheck on line 289 is redundant — a truthy result already guarantees the role matches. A simpleif (ecosystemOrg)would suffice.
381-408:⚠️ Potential issue | 🟠 MajorEmpty
ORarray when bothuserIdare undefined may return all pending invitations.Lines 404–405 construct
OR: [email ? {email} : undefined, userId ? {userId} : undefined].filter(Boolean). If bothuserIdare falsy, the resultingORis[]. Prisma treats an emptyORarray as vacuously true, so the query matches all pending member invitations across all users — a potential data leak.Proposed fix — guard against empty OR
} else { + const orConditions = [ + email ? { email } : undefined, + userId ? { userId } : undefined + ].filter(Boolean); + + if (0 === orConditions.length) { + throw new BadRequestException('Either email or userId is required for member invitations'); + } + where = { ...baseWhere, status: Invitation.PENDING, - OR: [email ? { email } : undefined, userId ? { userId } : undefined].filter(Boolean) + OR: orConditions }; }
🧹 Nitpick comments (2)
apps/ecosystem/repositories/ecosystem.repository.ts (2)
1077-1124: Missingtry/catchingetEcosystemInvitationsunlike other paginated methods.Other repository methods (e.g.,
getAllEcosystems,getAllEcosystemOrgsByEcosystemId) wrap their Prisma calls intry/catchwith error logging. This method lets exceptions propagate without logging, making debugging harder.
42-42: Cross-application import: repository imports pagination types from the API gateway layer.
IPageDetailandPaginatedResponseare defined inapps/api-gateway/common/interface.tsbut imported here in the ecosystem microservice's repository, service, and controller. This inverts the dependency direction — inner service layers should not depend on the gateway/presentation layer.Move
IPageDetailandPaginatedResponseto a shared library (e.g.,@credebl/common) so both the gateway and ecosystem service can consume them without introducing cross-application coupling. This also affectsapps/ecosystem/src/ecosystem.service.tsandapps/ecosystem/src/ecosystem.controller.ts.
sujitaw
changed the title
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 8
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)
394-408:⚠️ Potential issue | 🟡 Minor
OR: []returns an empty result set when bothuserIdare falsy.When
getEcosystemMemberInvitationsis called with a non-lead role and bothuserIdare undefined, the.filter(Boolean)on line 405 yieldsOR: []. In Prisma, this is deterministic behavior that returns an empty result set (no rows). If this function is ever called without at least one of these identifiers, returning zero results may be unexpected. Consider guarding against this case for clarity.apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
386-409:⚠️ Potential issue | 🟡 Minor
roleparam: noParseEnumPipe, has a default, and@ApiQuerysaysrequired: true— all inconsistent.Three issues with the
rolequery parameter:
@ApiQuery({ required: true })(Line 389) vs. a default value on Line 409 — Swagger says "must provide" but the runtime silently falls back toECOSYSTEM_LEAD.- No
ParseEnumPipe— an invalid value (e.g.?role=foo) won't fail with 400; it'll use the default.GetEcosystemInvitationsQueryDtoalready declares arolefield (validated with@IsEnum), so the separate@Query('role')extraction on Line 409 is redundant and potentially confusing.Consider either relying solely on the DTO's
rolefield (which already has@IsEnumvalidation) or usingParseEnumPipeon the standalone@Query('role')and dropping the DTO field.
🤖 Fix all issues with AI agents
In `@apps/api-gateway/src/ecosystem/ecosystem.controller.ts`:
- Around line 153-158: The JSON body (finalResponse.statusCode set to
HttpStatus.OK) and the HTTP response status (res.status(HttpStatus.CREATED)) are
inconsistent; update them to the same status—e.g., change
res.status(HttpStatus.CREATED) to res.status(HttpStatus.OK) or set
finalResponse.statusCode to HttpStatus.CREATED—so finalResponse and the call to
res.status(...) use the same HttpStatus constant (refer to finalResponse,
HttpStatus.OK/CREATED and res.status).
- Around line 303-314: updateEcosystemOrgStatus currently assigns a default
EcosystemOrgStatus.INACTIVE to the `@Query`('status') parameter which can silently
mutate state; remove the default value and validate the incoming value using
ParseEnumPipe instead (e.g., change the parameter signature for
updateEcosystemOrgStatus to accept `@Query`('status', new
ParseEnumPipe(EcosystemOrgStatus)) status: EcosystemOrgStatus) so the request
must supply an explicit enum value, and ensure the `@ApiQuery` metadata remains
consistent with required: true.
In `@apps/api-gateway/src/ecosystem/ecosystem.service.ts`:
- Around line 90-96: The return type of getAllEcosystemOrgsByEcosystemId is
wrong: change the method signature to return PaginatedResponse<IGetAllOrgs>
instead of PaginatedResponse<ecosystem_orgs>, import or reference the
IGetAllOrgs DTO/interface used by the ecosystem microservice, and update any
local typings/usages that assumed ecosystem_orgs; ensure the NATS call
this.natsClient.sendNatsMessage(this.serviceProxy, 'get-ecosystem-orgs', {
ecosystemId, pageDetail }) is kept as-is but typed to the new
PaginatedResponse<IGetAllOrgs> return type so the gateway proxy matches the
downstream shape.
In `@apps/ecosystem/dtos/create-ecosystem-dto.ts`:
- Around line 51-69: PaginationGetAllEcosystem currently inherits PaginationDto
(which defines search) but the service/repository interface
IPaginationSortingDto expects searchByText, so the controller passes a DTO with
the wrong field name and search is ignored; update PaginationGetAllEcosystem to
expose the expected field name (searchByText) instead of/alongside search so it
matches IPaginationSortingDto when sent to the service layer, e.g., rename or
add the property in PaginationGetAllEcosystem to searchByText (keeping
transforms/validators like `@Transform/`@IsOptional/@IsEnum as appropriate) so
consumers (controller -> service -> repository) receive the correct key.
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 304-309: The current orderBy uses a computed key
[pageDetail.sortField] which can be undefined; change the
prisma.ecosystem.findMany call (and the analogous call in
getEcosystemsForEcosystemLead) to guard against undefined by deriving a
safeSortField (e.g., const safeSortField = pageDetail.sortField ?? 'createdAt')
and use that (orderBy: { [safeSortField]: SortValue.ASC === pageDetail.sortBy ?
'asc' : 'desc' }) or alternatively only include orderBy when
pageDetail.sortField is truthy; update both occurrences: pageDetail.sortField in
the shown transaction block and the one in getEcosystemsForEcosystemLead.
In `@apps/ecosystem/src/ecosystem.controller.ts`:
- Around line 282-288: Rename the misnamed controller method
getTemplatesByIntentId to match its behavior and message pattern (e.g.,
getTemplatesByOrgId or getTemplatesByOrgIdHandler) so it reflects that it
handles { cmd: 'get-verification-templates-by-org-id' }, accepts payload.orgId,
and delegates to this.ecosystemService.getTemplatesByOrgId; update any internal
references and tests to call the new method name to avoid confusion.
In `@libs/common/src/interfaces/organization.interface.ts`:
- Around line 79-94: Remove sensitive credential fields (clientSecret, clientId,
idpId) from the IDeleteOrganization interface so delete-response objects no
longer carry secrets; create a separate internal interface (e.g.,
IOrganizationCredentials or IOrgAuth) to hold those three fields for use only in
contexts that require them, and update any types that currently extend
IDeleteOrganization (notably IOrgData) to either extend the credential interface
explicitly where needed or reference it only in internal/non-response APIs to
avoid leaking credentials via delete-related types.
- Around line 116-121: Change the IGetAllOrgsPayload interface so that search is
optional while leaving orgId required: update the property declaration from
search: string to search?: string in the IGetAllOrgsPayload interface; ensure
any code that consumes this interface (e.g., PaginationDto-related logic or
service methods that default search to '') continues to handle undefined by
falling back to an empty string or the existing default behavior.
🧹 Nitpick comments (13)
apps/api-gateway/src/organization/organization.controller.ts (2)
807-807: Non-RESTful route path with mixed spelling convention.The path
get-all-platform-organisationsuses a verb prefix (get-all-) which is redundant for a GET endpoint, and uses British spelling ("organisations") while the rest of the controller uses American spelling ("organizations"). Consider renaming to something likeplatform-organizationsfor consistency and RESTful convention.
813-814: Consider using theEcosystemRolesenum instead ofOrgRolesfor ecosystem-specific authorization.While
OrgRoles.ECOSYSTEM_LEADexists andEcosystemRolesGuardcorrectly reads theROLES_KEYmetadata, using theOrgRolesenum here is architecturally inconsistent. The codebase defines a separateEcosystemRolesenum specifically for ecosystem authorization. For clarity and maintainability, use@Roles(EcosystemRoles.ECOSYSTEM_LEAD)or ensure consistency across all ecosystem-guarded endpoints.libs/common/src/interfaces/organization.interface.ts (2)
25-32: GenericUserexport name risks collisions in a shared library.Exporting a bare
Userinterface from a sharedlibs/commonmodule is likely to clash with otherUsertypes across the codebase (e.g., Prisma models, auth modules). Consider a more scoped name likeIOrgUserorIOrganizationUserto stay consistent with theI-prefix convention used by the other interfaces in this file.
56-59: Pagination interfaces only carrytotalPages— consider addingtotalCountfor consistency.Both
IOrganizationInvitationsandIAllOrgsNameIdincludetotalPagesbut omittotalCount(total number of records). Consumers often need the total item count for UI display (e.g., "Showing 1–10 of 47 results"). If the backend already computes the count to derivetotalPages, surfacing it here costs nothing and improves the pagination contract.Also applies to: 111-114
libs/common/src/interfaces/interface.ts (1)
74-77: Consider addingtotalCounttoPaginatedResponse<T>.Clients often need the total number of items (not just total pages) for rendering pagination controls (e.g., "showing 1–10 of 57 results"). With only
totalPages, the exact item count is lost due toMath.ceil. This is a minor API ergonomics gap.Also, the naming convention in this file uses
I-prefixed interfaces (IPaginationSortingDto,IAccessTokenData, etc.), butPaginatedResponseandCommonTableColumnsbreak that convention.Proposed change
-export interface PaginatedResponse<T> { +export interface IPaginatedResponse<T> { totalPages: number; + totalCount: number; data: T[]; }apps/ecosystem/dtos/create-ecosystem-dto.ts (1)
46-49:SortFieldsenum is co-located with the DTO — verify it isn't needed elsewhere.If
SortFieldsis used by other modules (e.g., organization pagination), consider moving it to a shared location. If it's truly ecosystem-specific, the current placement is fine.apps/ecosystem/src/ecosystem.controller.ts (1)
165-170: Confusingpayload.payloadnaming.The outer parameter is named
payloadand the inner member is alsopayload, resulting inpayload.payloadaccess at line 169. Consider renaming either the parameter or the member for clarity (e.g.,msg: { payload: ...; pageDetail: ... }orpayload: { memberInvitations: ...; pageDetail: ... }).apps/ecosystem/src/ecosystem.service.ts (1)
283-299: Redundant role name check after role-filtered query.Line 288 calls
getEcosystemByRole(userId, EcosystemRoles.ECOSYSTEM_LEAD)which already filters by role name. The subsequent check on line 289 (ecosystem.ecosystemRole.name === EcosystemRoles.ECOSYSTEM_LEAD) is always true ifecosystemis non-null, making it redundant.Simplified version
- const ecosystem = await this.ecosystemRepository.getEcosystemByRole(userId, EcosystemRoles.ECOSYSTEM_LEAD); - if (ecosystem && ecosystem.ecosystemRole.name === EcosystemRoles.ECOSYSTEM_LEAD) { - const leadEcosystems = await this.ecosystemRepository.getEcosystemsForEcosystemLead(userId, pageDetail); - return leadEcosystems; - } else { + const ecosystem = await this.ecosystemRepository.getEcosystemByRole(userId, EcosystemRoles.ECOSYSTEM_LEAD); + if (ecosystem) { + return this.ecosystemRepository.getEcosystemsForEcosystemLead(userId, pageDetail); + } else { return this.ecosystemRepository.getAllEcosystems(pageDetail); }apps/ecosystem/repositories/ecosystem.repository.ts (2)
1077-1124:getEcosystemInvitationsignoressortField/sortByfrompageDetail.Sort is hardcoded to
createDateTime: 'desc'while other paginated methods respect the caller's sort preferences. This may be intentional for invitations, but it's inconsistent.
1128-1153: Minor:wheretyped asany— usePrisma.intentsWhereInputfor type safety.Proposed fix
- const where: any = { + const where: Prisma.intentsWhereInput = { ecosystemId };apps/api-gateway/src/ecosystem/ecosystem.controller.ts (3)
362-374: Returning 404 for an empty page may confuse paginated clients.With pagination in place, an empty
dataarray on a valid page (e.g. page 2 with no more results) is a normal condition, not a "not found" scenario. Paginated endpoints conventionally return200 OKwith an empty array and total-count metadata, so consumers can distinguish "no more pages" from "resource doesn't exist."Consider always returning 200 and letting the pagination metadata (e.g.
totalItems: 0) convey emptiness instead of a 404.
411-415: Good:userIdis sourced from the authenticated user, not the query string.Overriding
query.userIdwithreqUser.idis the right security pattern. TheecosystemIdvalidation for theECOSYSTEM_LEADrole is also appropriate.One minor note: since
userIdinGetEcosystemInvitationsQueryDtois always overridden here, consider removing it from the DTO to avoid giving clients the impression they can control it.
757-762: Method namegetTemplateByIntentIdis misleading — it fetches templates byorgId.The route is
GET /:orgId/templates, the JSDoc says "Get template details by org ID", and the implementation callsgetVerificationTemplates(orgId, pageDto). The method name should reflect this, e.g.getTemplatesByOrgId.Suggested rename
- async getTemplateByIntentId( + async getTemplatesByOrgId(
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 1
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)
386-409:⚠️ Potential issue | 🟡 Minor
roleparameter:@ApiQuery({ required: true })contradicts the default value.Line 389 declares
required: true, but line 409 provides a defaultInvitationViewRole.ECOSYSTEM_LEAD. This means the endpoint never actually requires the client to sendrole— the default silently kicks in. This is the same pattern that was deliberately fixed forstatusparameters withParseEnumPipe.If the default is intentional, change
required: truetorequired: falsein the@ApiQuerydecorator. If it should be required, useParseEnumPipeand remove the default, consistent with the other enum query params in this file.Option A: Make it truly required (consistent with other enum params)
`@ApiQuery`({ name: 'role', enum: InvitationViewRole, required: true }) ... async getEcosystemMemberInvitations( ... - `@Query`('role') role: InvitationViewRole = InvitationViewRole.ECOSYSTEM_LEAD + `@Query`('role', new ParseEnumPipe(InvitationViewRole)) role: InvitationViewRole ): Promise<Response> {Option B: Keep the default, fix the docs
`@ApiQuery`({ name: 'role', enum: InvitationViewRole, - required: true + required: false })
🤖 Fix all issues with AI agents
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 1077-1124: The getEcosystemInvitations method currently calls
this.prisma.$transaction and computes totalPages without error handling; wrap
the method body in a try/catch, catch any error thrown by
this.prisma.$transaction or Math.ceil, log the error using the repository logger
(e.g., this.logger.error or the same logger used by other paginated methods)
with context (method name and relevant identifiers like ecosystemId/pageDetail),
and re-throw the error so callers still receive it; ensure you reference the
existing symbols getEcosystemInvitations, this.prisma.$transaction,
this.prisma.ecosystem_invitations.count and pageDetail.pageSize when adding the
log message for consistency with other methods.
🧹 Nitpick comments (4)
libs/common/src/interfaces/organization.interface.ts (1)
1-42: New interface decomposition looks clean.The reorganization of
IOrganizationinto discrete top-level interfaces (UserOrgRole,User,IOrgRoles) improves clarity. One minor note: theUserinterface name (line 25) is very generic and could shadow or be confused with the Prismausertype that's used elsewhere in this codebase (e.g.,import { user } from '@prisma/client'). Consider a more specific name likeIOrgUserto avoid ambiguity.apps/ecosystem/repositories/ecosystem.repository.ts (1)
1128-1153:getIntentspagination —orderByis hardcoded, nosortFieldsupport.Unlike
getAllEcosystemswhich respectspageDetail.sortField,getIntentshardcodesorderBy: { createDateTime: 'desc' }and ignoressortField/sortByfrompageDetail. This is fine if intentional, but may confuse consumers who expect the pagination DTO's sort fields to take effect.apps/ecosystem/src/ecosystem.controller.ts (1)
165-170: Confusingpayload.payloadnesting ingetEcosystemMemberInvitations.The method parameter is
payloadwhich contains a property also namedpayload(line 166, 169). Thispayload.payloadpattern is hard to read and error-prone. Consider renaming the outer parameter or the inner property for clarity.Proposed fix
async getEcosystemMemberInvitations(payload: { - payload: IEcosystemMemberInvitations; + invitationFilter: IEcosystemMemberInvitations; pageDetail: IPaginationSortingDto; }): Promise<PaginatedResponse<IEcosystemInvitation>> { - return this.ecosystemService.getEcosystemMemberInvitations(payload.payload, payload.pageDetail); + return this.ecosystemService.getEcosystemMemberInvitations(payload.invitationFilter, payload.pageDetail); }This would require a matching change in the API gateway service that sends this NATS message.
apps/api-gateway/src/ecosystem/ecosystem.service.ts (1)
125-128:createIntentTemplatesignature change: parameterusershadows the imported Prismausertype.The parameter name
user(line 125) is the same as the imported typeuserfrom@prisma/client(line 14). While TypeScript distinguishes between type and value positions, this can be confusing to readers. Consider renaming the parameter tocurrentUserorreqUserfor clarity.
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
…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>
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/ecosystem/repositories/ecosystem.repository.ts (1)
830-900:⚠️ Potential issue | 🟡 Minor
getAllEcosystemOrgsByEcosystemIdhas noorderBy— non-deterministic pagination.Paginated queries without a stable sort order can return duplicated or missing records across pages. Consider adding an
orderByclause (e.g.,createDateTime: 'desc').🐛 Proposed fix
this.prisma.ecosystem_orgs.findMany({ where: whereClause, select: { ... }, + orderBy: { + createDateTime: 'desc' + }, take: pageDetail.pageSize, skip: (pageDetail.pageNumber - 1) * pageDetail.pageSize }),apps/api-gateway/src/ecosystem/ecosystem.controller.ts (2)
114-120:⚠️ Potential issue | 🟠 MajorHTTP status mismatch: body says
INTERNAL_SERVER_ERRORbut response is sent asOK(200).Line 115 sets
statusCode: HttpStatus.INTERNAL_SERVER_ERRORin the JSON body, but line 119 sends the response withHttpStatus.OK(200). The client receives a 200 status code for what is actually a server error, making error detection impossible for HTTP-aware consumers.🐛 Proposed fix
const finalResponse: IResponse = { statusCode: HttpStatus.INTERNAL_SERVER_ERROR, message: ResponseMessages.errorMessages.serverError }; - return res.status(HttpStatus.OK).json(finalResponse); + return res.status(HttpStatus.INTERNAL_SERVER_ERROR).json(finalResponse);
362-376:⚠️ Potential issue | 🟡 MinorReturning 404 for empty paginated results is unconventional.
For paginated endpoints, an empty page (e.g., page 5 when only 3 pages exist) is a normal condition — returning
{ totalPages, data: [] }with HTTP 200 is standard. Returning 404 forces clients to special-case empty results vs. actual not-found scenarios.🐛 Proposed fix
async getEcosystemOrgs( ... ): Promise<Response> { const ecosystemData = await this.ecosystemService.getAllEcosystemOrgsByEcosystemId(ecosystemId, pageDto); - if (ecosystemData.data && 0 < ecosystemData.data.length) { - return res.status(HttpStatus.OK).json({ - statusCode: HttpStatus.OK, - message: ResponseMessages.ecosystem.success.fetchOrgs, - data: ecosystemData - }); - } - - return res.status(HttpStatus.NOT_FOUND).json({ - statusCode: HttpStatus.NOT_FOUND, - message: ResponseMessages.ecosystem.error.ecosystemOrgsFetchFailed + return res.status(HttpStatus.OK).json({ + statusCode: HttpStatus.OK, + message: ResponseMessages.ecosystem.success.fetchOrgs, + data: ecosystemData }); }apps/ecosystem/src/ecosystem.service.ts (1)
344-366:⚠️ Potential issue | 🟠 MajorCorrect the second parameter in the
getEcosystemOrgcall on line 361.The method signature at line 527 of the repository expects
getEcosystemOrg(ecosystemId, orgId), but line 361 passesresult.userIdas the second parameter. Sinceecosystem_invitations.userIdis a user ID andecosystem_invitations.invitedOrgis the organization ID, the query will search for a record with the wrong field value and never find existing entries. This can result in duplicateecosystem_orgsrecords.🐛 Proposed fix
- const existingOrg = await this.ecosystemRepository.getEcosystemOrg(result.ecosystemId, result.userId); + const existingOrg = await this.ecosystemRepository.getEcosystemOrg(result.ecosystemId, result.invitedOrg);
🤖 Fix all issues with AI agents
In `@libs/supabase/src/supabase.service.ts`:
- Line 21: Validate that process.env.SUPABASE_URL and process.env.SUPABASE_KEY
are present before calling createClient in the code that assigns
this.clientInstance; if either is missing, throw a clear Error (or call
process.exit with a message) describing the missing variable(s) so the service
fails fast instead of passing undefined into createClient (referencing
this.clientInstance and createClient).
🧹 Nitpick comments (7)
libs/supabase/src/supabase.service.ts (2)
12-23: Noisy and misleading log messages.
- Line 12/15/19: Logging at
loglevel on everygetClient()call is excessive for a routine accessor —debugis more appropriate.- Line 23:
"auth has been set!"is misleading — no authentication is configured here; the client is simply instantiated with a URL and key.Suggested adjustments
- this.logger.log('getting supabase client... '); + this.logger.debug('getting supabase client...'); if (this.clientInstance) { - this.logger.log('client exists - returning for current Scope.REQUEST'); + this.logger.debug('returning existing client for current Scope.REQUEST'); return this.clientInstance; } - this.logger.log('initialising new supabase client for new Scope.REQUEST'); + this.logger.debug('initialising new supabase client for Scope.REQUEST'); ... - this.logger.log('auth has been set!'); + this.logger.debug('supabase client initialised');
8-8: Remove empty constructor.An empty constructor with no parameters or initialization logic is unnecessary boilerplate.
Proposed fix
- constructor() {}libs/prisma-service/prisma/schema.prisma (1)
805-808: Relations and constraint look correct; consider explicitonDeletefor clarity.The relations and unique constraint are consistent with the migration. One minor note: the
organisationrelation on Line 805 has no explicitonDeletedirective. The corresponding migration usesON DELETE SET NULL, which matches Prisma's default for optional relations — so behavior is correct. However, adding an explicitonDelete: SetNullwould make the intent self-documenting, especially since this model's deletion behavior is partly application-managed.Optional: make onDelete explicit
- organisation organisation? `@relation`(fields: [invitedOrg], references: [id]) + organisation organisation? `@relation`(fields: [invitedOrg], references: [id], onDelete: SetNull)Based on learnings, deletions involving
invitedOrgare handled at the application level with conditions matching bothecosystemIdandorgId.apps/ecosystem/src/ecosystem.controller.ts (1)
171-176: Confusing nestedpayload.payloadnaming.The parameter is named
payloadand it contains a property also namedpayload, leading topayload.payloadon line 175. This hurts readability. Consider renaming the inner field (e.g.,invitationParamsorparams).♻️ Proposed fix
async getEcosystemMemberInvitations(payload: { - payload: IEcosystemMemberInvitations; + params: IEcosystemMemberInvitations; pageDetail: IPaginationSortingDto; }): Promise<PaginatedResponse<IEcosystemInvitation>> { - return this.ecosystemService.getEcosystemMemberInvitations(payload.payload, payload.pageDetail); + return this.ecosystemService.getEcosystemMemberInvitations(payload.params, payload.pageDetail); }This would require a corresponding change in the gateway service where the NATS message payload is constructed.
apps/api-gateway/src/ecosystem/ecosystem.service.ts (1)
104-112: Root of thepayload.payloadnaming — consider renaming the key here.This is where the confusing
{ payload, pageDetail }is constructed, which becomespayload.payloadin the microservice controller. Renaming the key (e.g., toparams) here would fix the naming confusion end-to-end.♻️ Proposed rename
async getEcosystemMemberInvitations( payload: IEcosystemMemberInvitations, pageDetail: IPaginationSortingDto ): Promise<PaginatedResponse<IEcosystemInvitation>> { return this.natsClient.sendNatsMessage(this.serviceProxy, 'get-ecosystem-member-invitations', { - payload, + params: payload, pageDetail }); }apps/api-gateway/src/ecosystem/ecosystem.controller.ts (2)
758-763: Misleading method namegetTemplateByIntentId— it operates onorgId.The route is
/:orgId/templates, the parameter isorgId, and it callsgetVerificationTemplates(orgId, ...). The method namegetTemplateByIntentIdis a leftover misnomer.♻️ Proposed fix
- async getTemplateByIntentId( + async getTemplatesByOrgId( `@Param`('orgId') orgId: string, `@Res`() res: Response, `@Query`() pageDto: PaginationDto ): Promise<Response> {
406-416:rolequery parameter defaults toECOSYSTEM_LEADwithout enum validation.Unlike
statusinupdateEcosystemInvitationStatus(which usesParseEnumPipe), theroleparameter accepts any string and falls back toECOSYSTEM_LEAD. Since this is a read endpoint the risk is lower, but an invalid role value would silently produce incorrect query results. Consider addingParseEnumPipe(InvitationViewRole).🛡️ Proposed fix
- `@Query`('role') role: InvitationViewRole = InvitationViewRole.ECOSYSTEM_LEAD + `@Query`('role', new ParseEnumPipe(InvitationViewRole)) role: InvitationViewRole
| process.env.SUPABASE_URL, | ||
| process.env.SUPABASE_KEY | ||
| ); | ||
| this.clientInstance = createClient(process.env.SUPABASE_URL, process.env.SUPABASE_KEY); |
There was a problem hiding this comment.
Missing validation for required environment variables.
If SUPABASE_URL or SUPABASE_KEY is undefined, createClient will receive undefined and either throw a cryptic error or silently create a non-functional client. Fail fast with a clear message.
Proposed fix
+ const supabaseUrl = process.env.SUPABASE_URL;
+ const supabaseKey = process.env.SUPABASE_KEY;
+
+ if (!supabaseUrl || !supabaseKey) {
+ throw new Error('SUPABASE_URL and SUPABASE_KEY environment variables must be set');
+ }
+
- this.clientInstance = createClient(process.env.SUPABASE_URL, process.env.SUPABASE_KEY);
+ this.clientInstance = createClient(supabaseUrl, supabaseKey);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.clientInstance = createClient(process.env.SUPABASE_URL, process.env.SUPABASE_KEY); | |
| const supabaseUrl = process.env.SUPABASE_URL; | |
| const supabaseKey = process.env.SUPABASE_KEY; | |
| if (!supabaseUrl || !supabaseKey) { | |
| throw new Error('SUPABASE_URL and SUPABASE_KEY environment variables must be set'); | |
| } | |
| this.clientInstance = createClient(supabaseUrl, supabaseKey); |
🤖 Prompt for AI Agents
In `@libs/supabase/src/supabase.service.ts` at line 21, Validate that
process.env.SUPABASE_URL and process.env.SUPABASE_KEY are present before calling
createClient in the code that assigns this.clientInstance; if either is missing,
throw a clear Error (or call process.exit with a message) describing the missing
variable(s) so the service fails fast instead of passing undefined into
createClient (referencing this.clientInstance and createClient).
3 participants
What
Summary by CodeRabbit
New Features
API Changes
Documentation