Issue #000000 Fix: Elasticsearch code improved #412
Conversation
WalkthroughIntroduces centralized Elasticsearch data-fetching and syncing services and integrates them across adapters, form submission, and Elasticsearch controllers/services. Adds LMS integration, course webhook endpoints, nested course/interfaces, and a shared Elasticsearch config; refactors ES update flows to use centralized sync and comprehensive user documents. Changes
Sequence Diagram(s)sequenceDiagram
participant Updater as Adapter / Service (User, CohortMembers, FormSubmission)
participant ES_Sync as ElasticsearchSyncService
participant DataFetcher as ElasticsearchDataFetcherService
participant ES as UserElasticsearchService / Elasticsearch
Updater->>ES_Sync: syncUserToElasticsearch(userId, {section})
ES_Sync->>ES: getUser(userId)
alt user exists
ES_Sync->>DataFetcher: comprehensiveUserSync(userId) [if needed by section or callback]
DataFetcher-->>ES_Sync: userDocument
ES_Sync->>ES: updateUser(userId, doc: userDocument)
else user missing
ES_Sync->>DataFetcher: comprehensiveUserSync(userId)
DataFetcher-->>ES_Sync: userDocument
ES_Sync->>ES: createUser(userDocument)
end
ES-->>ES_Sync: ack
ES_Sync-->>Updater: complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
🧹 Nitpick comments (9)
src/elasticsearch/elasticsearch.module.ts (2)
3-17: Fix quote style consistency for better formatting compliance.The imports are well-organized and support the centralized ElasticsearchDataFetcherService functionality. However, there are formatting inconsistencies with quote styles that should be addressed.
Apply this formatting fix:
-import { TypeOrmModule } from '@nestjs/typeorm'; -import { ElasticsearchConfig } from './elasticsearch.config'; -import { ElasticsearchService } from './elasticsearch.service'; -import { UserElasticsearchService } from './user-elasticsearch.service'; -import { UserElasticsearchController } from './user-elasticsearch.controller'; -import { ElasticsearchDataFetcherService } from './elasticsearch-data-fetcher.service'; -import { User } from '../user/entities/user-entity'; -import { CohortMembers } from '../cohortMembers/entities/cohort-member.entity'; -import { FormSubmission } from '../forms/entities/form-submission.entity'; -import { FieldValues } from '../fields/entities/fields-values.entity'; -import { Fields } from '../fields/entities/fields.entity'; -import { Cohort } from '../cohort/entities/cohort.entity'; -import { PostgresFieldsService } from '../adapters/postgres/fields-adapter'; -import { FormsService } from '../forms/forms.service'; -import { Form } from '../forms/entities/form.entity'; +import { TypeOrmModule } from "@nestjs/typeorm"; +import { ElasticsearchConfig } from "./elasticsearch.config"; +import { ElasticsearchService } from "./elasticsearch.service"; +import { UserElasticsearchService } from "./user-elasticsearch.service"; +import { UserElasticsearchController } from "./user-elasticsearch.controller"; +import { ElasticsearchDataFetcherService } from "./elasticsearch-data-fetcher.service"; +import { User } from "../user/entities/user-entity"; +import { CohortMembers } from "../cohortMembers/entities/cohort-member.entity"; +import { FormSubmission } from "../forms/entities/form-submission.entity"; +import { FieldValues } from "../fields/entities/fields-values.entity"; +import { Fields } from "../fields/entities/fields.entity"; +import { Cohort } from "../cohort/entities/cohort.entity"; +import { PostgresFieldsService } from "../adapters/postgres/fields-adapter"; +import { FormsService } from "../forms/forms.service"; +import { Form } from "../forms/entities/form.entity";
32-44: Fix missing comma in exports array.The providers and exports configuration correctly registers and exposes the ElasticsearchDataFetcherService, but there's a syntax issue that needs to be addressed.
Apply this syntax fix:
exports: [ ElasticsearchService, UserElasticsearchService, - ElasticsearchDataFetcherService + ElasticsearchDataFetcherService, ],src/adapters/postgres/user-adapter.ts (1)
51-51: Fix quote style consistency and approve service injection.The ElasticsearchDataFetcherService is properly injected following NestJS patterns, but there's a formatting inconsistency that should be addressed.
Apply this formatting fix for the import:
-import { ElasticsearchDataFetcherService } from '../../elasticsearch/elasticsearch-data-fetcher.service'; +import { ElasticsearchDataFetcherService } from "../../elasticsearch/elasticsearch-data-fetcher.service";Also applies to: 98-99
src/forms/services/form-submission.service.ts (2)
32-32: Use consistent quote style for imports.The import statement uses single quotes while the project appears to use double quotes for imports.
-import { ElasticsearchDataFetcherService } from '../../elasticsearch/elasticsearch-data-fetcher.service'; +import { ElasticsearchDataFetcherService } from "../../elasticsearch/elasticsearch-data-fetcher.service";
1177-1184: Good refactoring to use centralized data fetcher.The centralization of user document fetching is a good architectural improvement. The null check with appropriate logging is also a good practice.
Consider further refactoring this method as it's still quite long (300+ lines). The application update logic after line 1184 could potentially be extracted into separate methods for better maintainability.
src/elasticsearch/elasticsearch-data-fetcher.service.ts (4)
389-391: Consider implementing proper page mappingThe current implementation puts all fields in a 'default' page. While this works as a fallback, it loses the page structure information that might be important for the UI or analysis.
Would you like me to help implement a more sophisticated page mapping strategy that preserves the form's page structure? This could involve parsing the form schema to extract page information.
548-597: Robust schema extraction with good defensive programmingThe method handles various schema structures effectively. Consider adding TypeScript interfaces for the expected schema structures to improve type safety and maintainability.
Consider defining interfaces for the different schema structures:
interface FormSchema { properties?: Record<string, any>; schema?: { properties?: Record<string, any>; }; result?: Array<{ schema?: { properties?: Record<string, any>; }; }>; }
752-759: Document the array transformation behaviorWhile converting arrays to comma-separated strings works for Elasticsearch indexing, it makes it impossible to distinguish between actual string values containing commas and array values. Consider documenting this limitation.
/** * Process field value for Elasticsearch storage * Converts array values to comma-separated strings for multiselect fields + * Note: This transformation is lossy - original array structure cannot be recovered + * and string values containing commas cannot be distinguished from array values * * @param value - The field value to process * @returns Processed value (array becomes comma-separated string, other types unchanged) */
766-768: Consider removing redundant wrapper methodThis method simply delegates to
isElasticsearchEnabled()utility without adding any value. Consider either removing it and using the utility directly, or adding service-specific logic if needed.Either remove this method and use the utility directly, or add value:
- isElasticsearchEnabled(): boolean { - return isElasticsearchEnabled(); - } + // Option 1: Remove this method entirely and use isElasticsearchEnabled() directly + + // Option 2: Add service-specific logic + isElasticsearchEnabled(): boolean { + const enabled = isElasticsearchEnabled(); + if (enabled) { + this.logger.debug('Elasticsearch is enabled for data fetching'); + } + return enabled; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
src/adapters/postgres/cohortMembers-adapter.ts(3 hunks)src/adapters/postgres/user-adapter.ts(4 hunks)src/elasticsearch/elasticsearch-data-fetcher.service.ts(1 hunks)src/elasticsearch/elasticsearch.module.ts(1 hunks)src/forms/services/form-submission.service.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit Configuration File
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Files:
src/elasticsearch/elasticsearch.module.tssrc/adapters/postgres/cohortMembers-adapter.tssrc/forms/services/form-submission.service.tssrc/adapters/postgres/user-adapter.tssrc/elasticsearch/elasticsearch-data-fetcher.service.ts
🧬 Code Graph Analysis (2)
src/elasticsearch/elasticsearch.module.ts (5)
src/cohortMembers/cohortMembers.module.ts (1)
Module(62-102)src/cohort/cohort.module.ts (1)
Module(26-57)src/app.module.ts (1)
Module(54-93)src/forms/forms.module.ts (1)
Module(27-59)src/adapters/postgres/postgres-module.ts (1)
Module(28-69)
src/forms/services/form-submission.service.ts (1)
src/elasticsearch/interfaces/user.interface.ts (1)
IUser(81-88)
🪛 ESLint
src/elasticsearch/elasticsearch.module.ts
[error] 3-3: Replace '@nestjs/typeorm' with "@nestjs/typeorm"
(prettier/prettier)
[error] 4-4: Replace './elasticsearch.config' with "./elasticsearch.config"
(prettier/prettier)
[error] 5-5: Replace './elasticsearch.service' with "./elasticsearch.service"
(prettier/prettier)
[error] 6-6: Replace './user-elasticsearch.service' with "./user-elasticsearch.service"
(prettier/prettier)
[error] 7-7: Replace './user-elasticsearch.controller' with "./user-elasticsearch.controller"
(prettier/prettier)
[error] 8-8: Replace './elasticsearch-data-fetcher.service' with "./elasticsearch-data-fetcher.service"
(prettier/prettier)
[error] 9-9: Replace '../user/entities/user-entity' with "../user/entities/user-entity"
(prettier/prettier)
[error] 10-10: Replace '../cohortMembers/entities/cohort-member.entity' with "../cohortMembers/entities/cohort-member.entity"
(prettier/prettier)
[error] 11-11: Replace '../forms/entities/form-submission.entity' with "../forms/entities/form-submission.entity"
(prettier/prettier)
[error] 12-12: Replace '../fields/entities/fields-values.entity' with "../fields/entities/fields-values.entity"
(prettier/prettier)
[error] 13-13: Replace '../fields/entities/fields.entity' with "../fields/entities/fields.entity"
(prettier/prettier)
[error] 14-14: Replace '../cohort/entities/cohort.entity' with "../cohort/entities/cohort.entity"
(prettier/prettier)
[error] 15-15: Replace '../adapters/postgres/fields-adapter' with "../adapters/postgres/fields-adapter"
(prettier/prettier)
[error] 16-16: Replace '../forms/forms.service' with "../forms/forms.service"
(prettier/prettier)
[error] 17-17: Replace '../forms/entities/form.entity' with "../forms/entities/form.entity"
(prettier/prettier)
[error] 43-43: Insert ,
(prettier/prettier)
src/adapters/postgres/cohortMembers-adapter.ts
[error] 37-37: Replace 'src/elasticsearch/elasticsearch-data-fetcher.service' with "src/elasticsearch/elasticsearch-data-fetcher.service"
(prettier/prettier)
src/forms/services/form-submission.service.ts
[error] 32-32: Replace '../../elasticsearch/elasticsearch-data-fetcher.service' with "../../elasticsearch/elasticsearch-data-fetcher.service"
(prettier/prettier)
[error] 81-81: Delete ··
(prettier/prettier)
[error] 1178-1178: Replace ·await·this.elasticsearchDataFetcherService.fetchUserDocumentForElasticsearch(userId); with ⏎········await·this.elasticsearchDataFetcherService.fetchUserDocumentForElasticsearch(⏎··········userId
(prettier/prettier)
[error] 1179-1179: Insert ··);⏎
(prettier/prettier)
[error] 1181-1181: Replace User·document·not·found·for·${userId},·skipping·Elasticsearch·update with ⏎··········User·document·not·found·for·${userId},·skipping·Elasticsearch·update⏎········
(prettier/prettier)
[error] 1999-1999: Delete ·
(prettier/prettier)
[error] 2007-2007: Replace userId with ⏎······userId⏎····
(prettier/prettier)
src/adapters/postgres/user-adapter.ts
[error] 51-51: Replace '../../elasticsearch/elasticsearch-data-fetcher.service' with "../../elasticsearch/elasticsearch-data-fetcher.service"
(prettier/prettier)
src/elasticsearch/elasticsearch-data-fetcher.service.ts
[error] 1-1: Replace '@nestjs/common' with "@nestjs/common"
(prettier/prettier)
[error] 2-2: Replace '@nestjs/typeorm' with "@nestjs/typeorm"
(prettier/prettier)
[error] 3-3: Replace 'typeorm' with "typeorm"
(prettier/prettier)
[error] 4-4: Replace '../user/entities/user-entity' with "../user/entities/user-entity"
(prettier/prettier)
[error] 5-5: Replace '../cohortMembers/entities/cohort-member.entity' with "../cohortMembers/entities/cohort-member.entity"
(prettier/prettier)
[error] 6-6: Replace '../forms/entities/form-submission.entity' with "../forms/entities/form-submission.entity"
(prettier/prettier)
[error] 7-7: Replace '../fields/entities/fields-values.entity' with "../fields/entities/fields-values.entity"
(prettier/prettier)
[error] 8-8: Replace '../cohort/entities/cohort.entity' with "../cohort/entities/cohort.entity"
(prettier/prettier)
[error] 9-9: Replace './interfaces/user.interface' with "./interfaces/user.interface"
(prettier/prettier)
[error] 10-10: Replace '../common/utils/elasticsearch.util' with "../common/utils/elasticsearch.util"
(prettier/prettier)
[error] 11-11: Replace '../common/logger/LoggerUtil' with "../common/logger/LoggerUtil"
(prettier/prettier)
[error] 12-12: Replace '../adapters/postgres/fields-adapter' with "../adapters/postgres/fields-adapter"
(prettier/prettier)
[error] 13-13: Replace '../forms/forms.service' with "../forms/forms.service"
(prettier/prettier)
[error] 17-17: Delete ·
(prettier/prettier)
[error] 21-21: Delete ·
(prettier/prettier)
[error] 46-46: Delete ,
(prettier/prettier)
[error] 52-52: Delete ·
(prettier/prettier)
[error] 56-56: Replace userId:·string with ⏎····userId:·string⏎··
(prettier/prettier)
[error] 82-82: Replace ·?·user.createdAt.toISOString() with ⏎··········?·user.createdAt.toISOString()⏎·········
(prettier/prettier)
[error] 83-83: Replace ·?·user.updatedAt.toISOString() with ⏎··········?·user.updatedAt.toISOString()⏎·········
(prettier/prettier)
[error] 87-88: Delete ⏎
(prettier/prettier)
[error] 97-97: Delete ·
(prettier/prettier)
[error] 110-110: Replace 'string' with "string"
(prettier/prettier)
[error] 117-117: Replace '' with ""
(prettier/prettier)
[error] 118-118: Replace '' with ""
(prettier/prettier)
[error] 119-119: Replace '' with ""
(prettier/prettier)
[error] 120-120: Replace '' with ""
(prettier/prettier)
[error] 121-121: Replace '' with ""
(prettier/prettier)
[error] 122-122: Replace '' with ""
(prettier/prettier)
[error] 123-123: Replace '' with ""
(prettier/prettier)
[error] 124-124: Replace '' with ""
(prettier/prettier)
[error] 126-126: Replace '' with ""
(prettier/prettier)
[error] 127-127: Replace '' with ""
(prettier/prettier)
[error] 128-128: Replace '' with ""
(prettier/prettier)
[error] 129-129: Replace '' with ""
(prettier/prettier)
[error] 130-130: Replace '' with ""
(prettier/prettier)
[error] 131-131: Replace 'active' with "active"
(prettier/prettier)
[error] 136-137: Delete ⏎
(prettier/prettier)
[error] 138-138: Replace ``Failed·to·fetch·user·profile·for·${user.userId}:,·error with `⏎········`Failed·to·fetch·user·profile·for·${user.userId}:`,⏎········error⏎······`
(prettier/prettier)
[error] 146-146: Delete ·
(prettier/prettier)
[error] 153-153: Replace userId); with ⏎········userId
(prettier/prettier)
[error] 154-154: Insert );⏎
(prettier/prettier)
[error] 155-155: Replace Found·${customFields.length}·custom·fields·for·user·${userId} with ⏎········Found·${customFields.length}·custom·fields·for·user·${userId}⏎······
(prettier/prettier)
[error] 162-162: Replace Found·${submissions.length}·form·submissions·for·filtering·custom·fields with ⏎········Found·${submissions.length}·form·submissions·for·filtering·custom·fields⏎······
(prettier/prettier)
[error] 166-166: Delete ······
(prettier/prettier)
[error] 170-170: Replace submission.formId with ⏎············submission.formId⏎··········
(prettier/prettier)
[error] 171-171: Replace fieldId with (fieldId)
(prettier/prettier)
[error] 173-173: Replace ``Failed·to·extract·field·IDs·from·form·${submission.formId}:,·error with `⏎············`Failed·to·extract·field·IDs·from·form·${submission.formId}:`,⏎············error⏎··········`
(prettier/prettier)
[error] 177-177: Replace Form·field·IDs·to·exclude:·${Array.from(formFieldIds).join(',·')} with ⏎········Form·field·IDs·to·exclude:·${Array.from(formFieldIds).join(",·")}⏎······
(prettier/prettier)
[error] 180-180: Replace (field)·=>·!formFieldIds.has(field.fieldId)); with ⏎········(field)·=>·!formFieldIds.has(field.fieldId)
(prettier/prettier)
[error] 181-181: Insert );⏎
(prettier/prettier)
[error] 182-182: Replace After·filtering,·${filteredCustomFields.length}·custom·fields·remain with ⏎········After·filtering,·${filteredCustomFields.length}·custom·fields·remain⏎······
(prettier/prettier)
[error] 185-185: Replace field with (field)
(prettier/prettier)
[error] 187-187: Replace '' with ""
(prettier/prettier)
[error] 188-188: Replace '' with ""
(prettier/prettier)
[error] 189-189: Replace '' with ""
(prettier/prettier)
[error] 190-190: Replace '' with ""
(prettier/prettier)
[error] 191-191: Replace '' with ""
(prettier/prettier)
[error] 193-193: Replace '' with ""
(prettier/prettier)
[error] 194-194: Replace '' with ""
(prettier/prettier)
[error] 195-195: Replace '' with ""
(prettier/prettier)
[error] 199-199: Replace Returning·${transformedFields.length}·transformed·custom·fields with ⏎········Returning·${transformedFields.length}·transformed·custom·fields⏎······
(prettier/prettier)
[error] 200-201: Delete ⏎
(prettier/prettier)
[error] 210-210: Delete ·
(prettier/prettier)
[error] 221-221: Replace Found·${cohortMemberships.length}·cohort·memberships·for·user·${userId}`);` with `⏎········`Found·${cohortMemberships.length}·cohort·memberships·for·user·${userId}
(prettier/prettier)
[error] 222-222: Insert );⏎
(prettier/prettier)
[error] 225-225: Replace ``Cohort·membership·details:,·cohortMemberships.map(membership with `⏎··········`Cohort·membership·details:`,⏎··········cohortMemberships.map((membership)`
(prettier/prettier)
[error] 226-226: Insert ··
(prettier/prettier)
[error] 227-227: Insert ··
(prettier/prettier)
[error] 228-228: Replace ·········· with ············
(prettier/prettier)
[error] 229-229: Replace ··········cohortMembershipId:·membership.cohortMembershipId with ············cohortMembershipId:·membership.cohortMembershipId,
(prettier/prettier)
[error] 230-230: Replace ········})) with ··········}))⏎········
(prettier/prettier)
[error] 238-238: Replace Found·${submissions.length}·form·submissions·for·user·${userId}`);` with `⏎········`Found·${submissions.length}·form·submissions·for·user·${userId}
(prettier/prettier)
[error] 239-239: Insert );⏎
(prettier/prettier)
[error] 242-242: Replace ``Submission·details:,·submissions.map(sub with `⏎··········`Submission·details:`,⏎··········submissions.map((sub)`
(prettier/prettier)
[error] 243-243: Replace ·········· with ············
(prettier/prettier)
[error] 244-244: Insert ··
(prettier/prettier)
[error] 245-245: Insert ··
(prettier/prettier)
[error] 246-246: Replace updatedAt:·sub.updatedAt with ··updatedAt:·sub.updatedAt,
(prettier/prettier)
[error] 247-247: Replace ········})) with ··········}))⏎········
(prettier/prettier)
[error] 259-259: Delete ········
(prettier/prettier)
[error] 267-267: Replace Creating·default·application·for·user·${userId}·with·form·submissions`);` with `⏎··········`Creating·default·application·for·user·${userId}·with·form·submissions
(prettier/prettier)
[error] 268-268: Insert );⏎
(prettier/prettier)
[error] 271-271: Replace 'default' with "default"
(prettier/prettier)
[error] 272-272: Replace 'active' with "active"
(prettier/prettier)
[error] 273-273: Replace 'inactive' with "inactive"
(prettier/prettier)
[error] 282-282: Replace ·?·submission.updatedAt.toISOString() with ⏎··············?·submission.updatedAt.toISOString()⏎·············
(prettier/prettier)
[error] 286-286: Replace '' with ""
(prettier/prettier)
[error] 289-289: Replace 'active' with "active"
(prettier/prettier)
[error] 293-293: Delete ··········
(prettier/prettier)
[error] 298-298: Replace Returning·${applications.length}·applications·for·user·${userId} with ⏎········Returning·${applications.length}·applications·for·user·${userId}⏎······
(prettier/prettier)
[error] 299-300: Delete ⏎
(prettier/prettier)
[error] 309-309: Delete ·
(prettier/prettier)
[error] 323-323: Replace sub with (sub)
(prettier/prettier)
[error] 324-324: Delete ······
(prettier/prettier)
[error] 326-326: Replace No·form·submission·found·for·user·${userId}·in·cohort·${membership.cohortId} with ⏎··········No·form·submission·found·for·user·${userId}·in·cohort·${membership.cohortId}⏎········
(prettier/prettier)
[error] 334-334: Insert ⏎·······
(prettier/prettier)
[error] 345-345: Replace 'active' with "active"
(prettier/prettier)
[error] 346-346: Replace 'active' with "active"
(prettier/prettier)
[error] 349-349: Replace ·?·submission.updatedAt.toISOString() with ⏎··········?·submission.updatedAt.toISOString()⏎·········
(prettier/prettier)
[error] 350-350: Replace ·submission.status·===·'active'·?·submission.updatedAt?.toISOString() with ⏎··········submission.status·===·"active"⏎············?·submission.updatedAt?.toISOString()⏎···········
(prettier/prettier)
[error] 352-352: Replace 'Unknown·Cohort' with "Unknown·Cohort"
(prettier/prettier)
[error] 353-353: Replace '' with ""
(prettier/prettier)
[error] 356-356: Replace 'active' with "active"
(prettier/prettier)
[error] 360-361: Delete ⏎
(prettier/prettier)
[error] 362-362: Replace ``Failed·to·build·application·for·cohort·${membership.cohortId}:,·error with `⏎········`Failed·to·build·application·for·cohort·${membership.cohortId}:`,⏎········error⏎······`
(prettier/prettier)
[error] 369-369: Delete ·
(prettier/prettier)
[error] 373-373: Replace submission:·FormSubmission with ⏎····submission:·FormSubmission⏎··
(prettier/prettier)
[error] 378-378: Delete ·
(prettier/prettier)
[error] 381-381: Replace 'field' with "field"
(prettier/prettier)
[error] 386-386: Delete ······
(prettier/prettier)
[error] 390-390: Replace 'default' with "default"
(prettier/prettier)
[error] 391-391: Delete ········
(prettier/prettier)
[error] 395-395: Delete ········
(prettier/prettier)
[error] 396-396: Insert ⏎·········
(prettier/prettier)
[error] 400-401: Delete ⏎
(prettier/prettier)
[error] 402-402: Replace ``Failed·to·build·form·data·from·submission·${submission.submissionId}:,·error with `⏎········`Failed·to·build·form·data·from·submission·${submission.submissionId}:`,⏎········error⏎······`
(prettier/prettier)
[error] 409-409: Delete ·
(prettier/prettier)
[error] 413-413: Replace submission:·FormSubmission with ⏎····submission:·FormSubmission⏎··
(prettier/prettier)
[error] 417-417: Delete ·
(prettier/prettier)
[error] 420-420: Replace 'field' with "field"
(prettier/prettier)
[error] 433-433: Replace 'default'·?·'eligibilityCheck' with "default"·?·"eligibilityCheck"
(prettier/prettier)
[error] 442-442: Replace Field·${fieldValue.fieldId}·not·found·in·schema·mapping,·skipping with ⏎············Field·${fieldValue.fieldId}·not·found·in·schema·mapping,·skipping⏎··········
(prettier/prettier)
[error] 451-451: Replace fieldValue.value with ⏎··········fieldValue.value⏎········
(prettier/prettier)
[error] 460-461: Replace value·=>·⏎··········value·!==·null·&&·value·!==·undefined·&&·value·!==·'' with ⏎··········(value)·=>·value·!==·null·&&·value·!==·undefined·&&·value·!==·""
(prettier/prettier)
[error] 463-463: Delete ········
(prettier/prettier)
[error] 464-464: Insert ⏎·········
(prettier/prettier)
[error] 467-468: Delete ⏎
(prettier/prettier)
[error] 470-470: Replace ``Failed·to·build·form·data·with·pages·for·submission·${submission.submissionId}:,·error with `⏎········`Failed·to·build·form·data·with·pages·for·submission·${submission.submissionId}:`,⏎········error⏎······`
(prettier/prettier)
[error] 477-477: Delete ·
(prettier/prettier)
[error] 481-481: Replace ·percentage:·number;·progress:·any with ⏎····percentage:·number;⏎····progress:·any;⏎·
(prettier/prettier)
[error] 498-498: Replace '' with ""
(prettier/prettier)
[error] 516-516: Insert ⏎·····
(prettier/prettier)
[error] 532-532: Delete ·
(prettier/prettier)
[error] 544-544: Delete ·
(prettier/prettier)
[error] 551-551: Insert ⏎·······
(prettier/prettier)
[error] 569-569: Replace 'object' with "object"
(prettier/prettier)
[error] 579-579: Replace 'object' with "object"
(prettier/prettier)
[error] 591-591: Replace ``Extracted·schema·for·form·${formId}:,·Object.keys(schema) with `⏎········`Extracted·schema·for·form·${formId}:`,⏎········Object.keys(schema)⏎······`
(prettier/prettier)
[error] 601-601: Delete ·
(prettier/prettier)
[error] 605-605: Replace formId:·string with ⏎····formId:·string⏎··
(prettier/prettier)
[error] 613-613: Delete ········
(prettier/prettier)
[error] 625-625: Replace 'object' with "object"
(prettier/prettier)
[error] 628-628: Insert ⏎····················
(prettier/prettier)
[error] 629-629: Insert ··
(prettier/prettier)
[error] 632-632: Insert ⏎····················
(prettier/prettier)
[error] 633-633: Insert ··
(prettier/prettier)
[error] 636-636: Insert ⏎····················
(prettier/prettier)
[error] 637-637: Insert ··
(prettier/prettier)
[error] 650-650: Replace 'object' with "object"
(prettier/prettier)
[error] 653-653: Insert ⏎··············
(prettier/prettier)
[error] 654-654: Insert ··
(prettier/prettier)
[error] 657-657: Insert ⏎··············
(prettier/prettier)
[error] 658-658: Insert ··
(prettier/prettier)
[error] 661-661: Insert ⏎··············
(prettier/prettier)
[error] 662-662: Insert ··
(prettier/prettier)
[error] 668-668: Replace Extracted·${fieldIds.length}·field·IDs·from·form·${formId} with ⏎········Extracted·${fieldIds.length}·field·IDs·from·form·${formId}⏎······
(prettier/prettier)
[error] 669-670: Delete ⏎
(prettier/prettier)
[error] 672-672: Replace ``Failed·to·extract·field·IDs·from·form·${formId}:,·error with `⏎········`Failed·to·extract·field·IDs·from·form·${formId}:`,⏎········error⏎······`
(prettier/prettier)
[error] 679-679: Delete ·
(prettier/prettier)
[error] 687-687: Replace 'object' with "object"
(prettier/prettier)
[error] 689-689: Replace 'object' with "object"
(prettier/prettier)
[error] 695-695: Replace (fieldSchema·as·any).dependencies with ⏎············(fieldSchema·as·any).dependencies⏎··········
(prettier/prettier)
[error] 696-696: Replace 'object' with "object"
(prettier/prettier)
[error] 699-699: Insert ⏎················
(prettier/prettier)
[error] 700-700: Insert ··
(prettier/prettier)
[error] 703-703: Insert ⏎················
(prettier/prettier)
[error] 704-704: Insert ··
(prettier/prettier)
[error] 707-707: Insert ⏎················
(prettier/prettier)
[error] 708-708: Insert ··
(prettier/prettier)
[error] 717-717: Replace 'default'·?·'eligibilityCheck' with "default"·?·"eligibilityCheck"
(prettier/prettier)
[error] 722-722: Replace (pageSchema·as·any).dependencies with ⏎··········(pageSchema·as·any).dependencies⏎········
(prettier/prettier)
[error] 723-723: Replace 'object' with "object"
(prettier/prettier)
[error] 726-726: Insert ⏎··············
(prettier/prettier)
[error] 727-727: Replace ·············· with ················
(prettier/prettier)
[error] 730-730: Insert ⏎··············
(prettier/prettier)
[error] 731-731: Insert ··
(prettier/prettier)
[error] 734-734: Insert ⏎··············
(prettier/prettier)
[error] 735-735: Insert ··
(prettier/prettier)
[error] 748-748: Delete ·
(prettier/prettier)
[error] 755-755: Replace ',·' with ",·"
(prettier/prettier)
[error] 763-763: Delete ·
(prettier/prettier)
[error] 769-769: Replace · with ⏎
(prettier/prettier)
🔇 Additional comments (15)
src/adapters/postgres/cohortMembers-adapter.ts (2)
59-60: LGTM! Proper dependency injection setup.The constructor properly injects the new
ElasticsearchDataFetcherServicefollowing NestJS dependency injection patterns. The parameter naming and access modifier are consistent with the existing code style.
765-769: Excellent refactoring to centralized data fetching service.The change from
formSubmissionService.buildUserDocumentForElasticsearch(userId)toelasticsearchDataFetcherService.fetchUserDocumentForElasticsearch(userId)aligns with the PR objectives to centralize Elasticsearch data fetching logic. This improves:
- Separation of concerns: Form submission service no longer handles Elasticsearch document building
- Code reusability: Centralized service can be used across multiple adapters
- Maintainability: Single source of truth for user document construction
The change is consistent with similar updates mentioned in the AI summary for other files in the system.
src/elasticsearch/elasticsearch.module.ts (1)
20-30: LGTM! Comprehensive entity registration for centralized data fetching.The TypeOrmModule configuration properly registers all entities required by the ElasticsearchDataFetcherService, enabling comprehensive data fetching operations for Elasticsearch synchronization.
src/adapters/postgres/user-adapter.ts (2)
1036-1038: LGTM! Proper conditional Elasticsearch synchronization.The conditional check for Elasticsearch availability before syncing is a good defensive programming practice that prevents errors when Elasticsearch is not configured and keeps the feature optional.
2777-2799: LGTM! Excellent refactoring to centralized data fetching.The refactored
syncUserToElasticsearchmethod effectively delegates data fetching to the centralizedElasticsearchDataFetcherServicewhile maintaining proper error handling. The callback pattern allows clean separation of concerns between data fetching and document synchronization.The JSDoc documentation clearly explains the method's purpose and behavior, which aids maintainability.
src/forms/services/form-submission.service.ts (3)
95-95: LGTM! Proper dependency injection.The
ElasticsearchDataFetcherServiceis correctly injected following NestJS dependency injection patterns.
1964-1964: Verify the removal of customFields from cohortDetails.The comment indicates that
customFieldshas been removed fromcohortDetails. This is a breaking change that could affect consumers of this data structure.Please confirm:
- Is this removal intentional and aligned with the requirements?
- Have all consumers of
cohortDetailsbeen updated to handle this change?- Is there a migration plan for existing data that might expect this field?
1996-2008: Well-implemented delegation method.The new
buildUserDocumentForElasticsearchmethod properly delegates to the centralized service while maintaining a clean interface. The JSDoc documentation clearly explains its purpose.src/elasticsearch/elasticsearch-data-fetcher.service.ts (7)
1-47: Well-structured service with proper dependency injectionThe service follows NestJS best practices with clear documentation and proper dependency injection setup.
150-206: Good implementation of custom field filtering logicThe method properly handles the complex task of filtering out form fields from custom fields, preventing duplication in the Elasticsearch document. The error handling ensures graceful degradation.
413-473: Excellent implementation of page-structured form dataThis method properly handles the complex task of mapping field values to their respective pages and calculating completion status. The approach is well-thought-out and handles edge cases appropriately.
481-528: Well-implemented completion percentage calculationThe method provides accurate completion tracking at both page and overall levels. The handling of empty values and the progress data structure are appropriate for Elasticsearch indexing.
605-675: Comprehensive field ID extraction handling complex schemasExcellent implementation that properly handles JSON Schema's conditional structures (oneOf, allOf, anyOf) and nested dependencies. This ensures all field IDs are captured regardless of schema complexity.
683-743: Good implementation of field-to-page mappingThe method correctly builds the mapping while handling complex schema structures. The consistency with the field extraction logic ensures reliability.
122-122: Fix potential null reference error in mobile field conversionThe current code
user.mobile?.toString() || ''will throw an error ifmobileis explicitly set tonullbecause optional chaining doesn't preventtoString()from being called onnull.- mobile: user.mobile?.toString() || '', + mobile: user.mobile ? user.mobile.toString() : '',Likely an incorrect or invalid review comment.
|
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (2)
src/adapters/postgres/cohortMembers-adapter.ts (2)
779-791: Remove redundant Elasticsearch update logicThe comprehensive sync already handles all the Elasticsearch updates, making this manual update redundant. The code updates the user document and then immediately calls the comprehensive sync service, which will overwrite these changes.
Since you're already using the centralized sync service (lines 803-808), you can remove this redundant update logic:
-// Now update the user document in Elasticsearch with comprehensive data -const baseDoc = - typeof userDoc?._source === 'object' ? userDoc._source : {}; -await this.userElasticsearchService.updateUser( - cohortMembers.userId, - { doc: userDocument }, // Use comprehensive user document - async (userId: string) => { - // Use comprehensive sync to build the full user document for Elasticsearch - return await this.elasticsearchDataFetcherService.comprehensiveUserSync( - userId - ); - } -);
1211-1227: Inconsistent Elasticsearch update approachesThe code uses
comprehensiveUserSyncbut then passes the result toupdateUserwith a callback that callscomprehensiveUserSyncagain. This creates redundant API calls and potential inconsistencies.Simplify the logic to avoid redundant calls:
-// If application is missing, use comprehensive sync to build and upsert the full user document -const fullUserDoc = - await this.elasticsearchDataFetcherService.comprehensiveUserSync( - cohortMembershipToUpdate.userId - ); -if (fullUserDoc) { - await this.userElasticsearchService.updateUser( - cohortMembershipToUpdate.userId, - { doc: fullUserDoc }, - async (userId: string) => { - return await this.elasticsearchDataFetcherService.comprehensiveUserSync( - userId - ); - } - ); -} +// If application is missing, sync using centralized service +await this.elasticsearchSyncService.syncUserToElasticsearch( + cohortMembershipToUpdate.userId, + { section: SyncSection.APPLICATIONS } +);
♻️ Duplicate comments (2)
src/adapters/postgres/cohortMembers-adapter.ts (1)
37-42: Fix import statement formatting inconsistenciesThe import statements use inconsistent quote styles. Lines 37 and 42 use single quotes while the project's Prettier configuration expects double quotes.
Apply this diff to fix the quote style:
-import { ElasticsearchDataFetcherService } from 'src/elasticsearch/elasticsearch-data-fetcher.service'; +import { ElasticsearchDataFetcherService } from "src/elasticsearch/elasticsearch-data-fetcher.service"; import axios from 'axios'; import { ElasticsearchSyncService, SyncSection, -} from '../../elasticsearch/elasticsearch-sync.service'; +} from "../../elasticsearch/elasticsearch-sync.service";src/adapters/postgres/user-adapter.ts (1)
51-54: Fix import statement formatting inconsistenciesThe import statements use single quotes while the project's Prettier configuration expects double quotes.
Apply this diff to fix the quote style:
-import { ElasticsearchDataFetcherService } from '../../elasticsearch/elasticsearch-data-fetcher.service'; +import { ElasticsearchDataFetcherService } from "../../elasticsearch/elasticsearch-data-fetcher.service"; import { IUser } from '../../elasticsearch/interfaces/user.interface'; import { isElasticsearchEnabled } from 'src/common/utils/elasticsearch.util'; -import { ElasticsearchSyncService, SyncSection } from '../../elasticsearch/elasticsearch-sync.service'; +import { + ElasticsearchSyncService, + SyncSection, +} from "../../elasticsearch/elasticsearch-sync.service";
🧹 Nitpick comments (16)
src/app.controller.ts (1)
28-35: Consider adding response documentation for the health endpointThe health endpoint lacks API documentation decorators that are present on other endpoints. For consistency and better API documentation, consider adding Swagger decorators.
Apply this diff to add API documentation:
@Get("health") + @ApiOperation({ summary: "Health check endpoint" }) + @ApiResponse({ + status: 200, + description: "Service is healthy", + schema: { + example: { + status: "healthy", + timestamp: "2024-01-01T00:00:00.000Z", + service: "user-microservice" + } + } + }) getHealth(): object { return { status: "healthy", timestamp: new Date().toISOString(), service: "user-microservice", }; }src/shared/shared-elasticsearch.config.ts (1)
1-144: Fix formatting issues to comply with project standardsThe file has multiple formatting inconsistencies with single quotes instead of double quotes. These should be fixed to maintain consistency with the project's prettier configuration.
Run the following command to auto-fix all formatting issues:
#!/bin/bash # Auto-fix formatting issues in the shared elasticsearch config file npx prettier --write src/shared/shared-elasticsearch.config.tssrc/elasticsearch/controllers/course-webhook.controller.ts (2)
1-305: Fix extensive formatting issues throughout the fileThe file has numerous formatting issues including inconsistent quotes, spacing, and line breaks that violate the project's prettier configuration.
Run the following command to auto-fix all formatting issues:
#!/bin/bash # Auto-fix formatting issues in the course webhook controller npx prettier --write src/elasticsearch/controllers/course-webhook.controller.ts
282-292: Consider using Promise.allSettled for better bulk processingThe current implementation processes updates sequentially, which could be slow for large batches. Consider parallel processing with proper error handling.
Apply this optimization:
- // Process each update - for (const update of bulkData.updates) { - try { - await this.courseElasticsearchService.updateCourseProgress(update); - results.successful++; - } catch (error) { - results.failed++; - results.errors.push(`User ${update.userId}: ${error.message}`); - this.logger.error(`Failed to update progress for user ${update.userId}:`, error); - } - } + // Process updates in parallel with controlled concurrency + const updatePromises = bulkData.updates.map(async (update) => { + try { + await this.courseElasticsearchService.updateCourseProgress(update); + return { success: true, userId: update.userId }; + } catch (error) { + this.logger.error(`Failed to update progress for user ${update.userId}:`, error); + return { success: false, userId: update.userId, error: error.message }; + } + }); + + const results = await Promise.allSettled(updatePromises); + const processedResults = results.map(r => r.status === 'fulfilled' ? r.value : { success: false, error: r.reason }); + + return { + successful: processedResults.filter(r => r.success).length, + failed: processedResults.filter(r => !r.success).length, + errors: processedResults.filter(r => !r.success).map(r => `User ${r.userId}: ${r.error}`) + };src/elasticsearch/interfaces/user.interface.ts (1)
36-40: Fix formatting issues and consider documenting the nested structureThe new nested courses field needs formatting fixes to comply with the project's style guide. Also, the comment on line 36 could be more descriptive about the architectural change.
Apply this diff to fix the formatting:
- courses?: { //new change courses moved to applications + courses?: { // Courses now nested under applications for better data organization type: 'nested'; values: ICourseDetail[]; };src/elasticsearch/services/course-elasticsearch.service.ts (1)
156-185: Complex nested search logic may have performance implicationsThe fallback search through all applications and their nested course structures using multiple nested loops (O(n⁴) complexity) could be inefficient for users with many applications and courses. Consider indexing or caching strategies for better performance.
Consider implementing a more efficient lookup mechanism:
- Maintain a separate index mapping contentId to application index
- Use Elasticsearch's native search capabilities instead of in-memory iteration
- Implement caching for frequently accessed user-course mappings
Would you like me to provide an implementation using a more efficient approach?
src/adapters/postgres/cohortMembers-adapter.ts (2)
1252-1258: Duplicate centralized sync callThe centralized sync service is called again after the Elasticsearch update logic, which may be redundant since the update has already been performed.
Consider consolidating the Elasticsearch sync logic to avoid duplicate operations. You could either:
- Remove this sync call if the previous update logic is sufficient
- Or remove the previous update logic and rely solely on this centralized sync
The centralized sync approach is cleaner and more maintainable.
1048-1053: Fix line formatting issuesThe comment about numeric casting is useful, but the line has incorrect indentation.
- // Build completion percentage filter conditions with proper numeric casting + // Build completion percentage filter conditions with proper numeric castingsrc/adapters/postgres/user-adapter.ts (1)
100-101: Remove trailing comma in constructor parametersThere's an unnecessary trailing comma after the last parameter.
private readonly userElasticsearchService: UserElasticsearchService, private readonly elasticsearchDataFetcherService: ElasticsearchDataFetcherService, -private readonly elasticsearchSyncService: ElasticsearchSyncService, +private readonly elasticsearchSyncService: ElasticsearchSyncServicesrc/elasticsearch/controllers/elasticsearch.controller.ts (7)
1-14: Fix import formatting and remove trailing commaThe imports need formatting adjustments to match the project's style guide.
-import { Controller, Post, Body, Get, Param, Put, Delete, HttpCode, HttpStatus } from '@nestjs/common'; +import { + Controller, + Post, + Body, + Get, + Param, + Put, + Delete, + HttpCode, + HttpStatus, +} from "@nestjs/common"; import { UserElasticsearchService } from '../user-elasticsearch.service'; import { ElasticsearchDataFetcherService } from '../elasticsearch-data-fetcher.service'; import { Logger } from '@nestjs/common'; import { ApiOperation, ApiResponse } from '@nestjs/swagger'; @Controller('elasticsearch/users') export class ElasticsearchController { private readonly logger = new Logger(ElasticsearchController.name); constructor( private readonly userElasticsearchService: UserElasticsearchService, - private readonly dataFetcherService: ElasticsearchDataFetcherService, + private readonly dataFetcherService: ElasticsearchDataFetcherService ) {}
340-342: Use const instead of let for immutable variableThe
existingCoursevariable is never reassigned after initialization.-let existingCourse = application.courses.values.find((c: any) => c.courseId === courseData.courseId); +const existingCourse = application.courses.values.find((c: any) => c.courseId === courseData.courseId);
353-354: Use const instead of let for immutable variableThe
existingUnitvariable is never reassigned after initialization.-let existingUnit = existingCourse.units.values.find((u: any) => u.unitId === newUnit.unitId); +const existingUnit = existingCourse.units.values.find((u: any) => u.unitId === newUnit.unitId);
361-362: Use const instead of let for immutable variableThe
existingContentvariable is never reassigned after initialization.-let existingContent = existingUnit.contents.values.find((c: any) => c.contentId === newContent.contentId); +const existingContent = existingUnit.contents.values.find((c: any) => c.contentId === newContent.contentId);
98-229: Consider extracting assessment data processing into a separate methodThe assessment data processing logic (lines 98-229) is quite lengthy and could be extracted into a separate private method for better readability and maintainability.
private processAssessmentData(userData: any, assessmentData: any): void { // Extract lines 101-229 into this method const contentId = assessmentData.testId; const testId = assessmentData.testId; // ... rest of the logic } // Then in syncUserFromDatabase: if (webhookData && webhookData.assessmentData) { this.logger.log(`Enhancing user data with assessment data for userId: ${userId}`); this.processAssessmentData(userData, webhookData.assessmentData); }
232-477: Consider extracting course hierarchy processing into a separate methodThe course hierarchy processing logic is extensive (245+ lines) and would benefit from being extracted into a separate method for better code organization.
private processCourseHierarchy(userData: any, webhookData: any): void { // Extract the course hierarchy processing logic here } // Then in syncUserFromDatabase: if (webhookData && webhookData.courseHierarchy) { this.logger.log(`Enhancing user data with course hierarchy for userId: ${userId}`); this.processCourseHierarchy(userData, webhookData); }
394-397: Complex condition could be simplified for readabilityThe condition for updating lesson content is complex and spans multiple lines.
Consider extracting the condition into a descriptive variable:
+const shouldUpdateContent = content.contentId === webhookData.lessonAttemptData.lessonId && + (!content.lessonTrackId || content.lessonTrackId === webhookData.lessonAttemptData.attemptId); + -if (content.contentId === webhookData.lessonAttemptData.lessonId && - (!content.lessonTrackId || content.lessonTrackId === webhookData.lessonAttemptData.attemptId)) { +if (shouldUpdateContent) {
| // Use comprehensive sync to get complete user document including courses and assessment data | ||
| const userDocument = | ||
| await this.elasticsearchDataFetcherService.comprehensiveUserSync( | ||
| cohortMembers.userId | ||
| ); | ||
|
|
||
| if (!userDocument) { | ||
| LoggerUtil.warn( | ||
| `User document not found for ${cohortMembers.userId}, skipping Elasticsearch update` | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Critical: Fix early return issue breaking cohort member creation flow
The code returns early if the user document is not found, but this return statement doesn't provide a response object, which will break the API endpoint. Additionally, the early return prevents the cohort member from being saved in the database.
Apply this fix to handle the missing user document gracefully while allowing the database operation to continue:
if (!userDocument) {
LoggerUtil.warn(
`User document not found for ${cohortMembers.userId}, skipping Elasticsearch update`
);
- return;
+ // Continue with the database operation even if Elasticsearch sync fails
} else {
+ // Get the existing user document from Elasticsearch
+ const userDoc = await this.userElasticsearchService.getUser(
+ cohortMembers.userId
+ );
+ // ... rest of the Elasticsearch update logic
}
-
-// Get the existing user document from Elasticsearch
-const userDoc = await this.userElasticsearchService.getUser(
- cohortMembers.userId
-);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/adapters/postgres/cohortMembers-adapter.ts around lines 731 to 742, the
code currently does an early "return" when the Elasticsearch user document is
not found which aborts the API flow and prevents the cohort member from being
saved; remove that early return so the DB save continues, replace it with a
LoggerUtil.warn noting the missing document, and conditionally skip or no-op the
Elasticsearch update logic (or wrap the ES update in a try/catch) so missing
userDocument does not stop the method from completing and returning the proper
response object after the cohort member is persisted.
| import { AssignTenantModule } from './userTenantMapping/user-tenant-mapping.module'; | ||
| import { FormsModule } from './forms/forms.module'; | ||
| import { HttpService } from '@utils/http-service'; | ||
| import { LMSService } from './common/services/lms.service'; |
There was a problem hiding this comment.
Use double quotes for import path
Replace single quotes with double quotes to maintain consistency with the project's style guide.
Apply this diff:
-import { LMSService } from './common/services/lms.service';
+import { LMSService } from "./common/services/lms.service";📝 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.
| import { LMSService } from './common/services/lms.service'; | |
| import { LMSService } from "./common/services/lms.service"; |
🧰 Tools
🪛 ESLint
[error] 24-24: Replace './common/services/lms.service' with "./common/services/lms.service"
(prettier/prettier)
🤖 Prompt for AI Agents
In src/app.module.ts around line 24, the import statement uses single quotes for
the module path; update the import to use double quotes to match the project's
style guide (replace './common/services/lms.service' wrapped in single quotes
with the same path wrapped in double quotes) and ensure the rest of file imports
remain consistent with double-quote convention.
| class CourseProgressDto { | ||
| userId: string; | ||
| cohortId: string; | ||
| courseId: string; | ||
| courseTitle: string; | ||
| progress: number; | ||
| unitId?: string; | ||
| unitTitle?: string; | ||
| unitProgress?: number; | ||
| contentId?: string; | ||
| contentType?: string; | ||
| contentTitle?: string; | ||
| contentStatus?: string; | ||
| tracking?: { | ||
| percentComplete?: number; | ||
| lastPosition?: number; | ||
| currentPosition?: number; | ||
| timeSpent?: number; | ||
| visitedPages?: number[]; | ||
| totalPages?: number; | ||
| lastPage?: number; | ||
| currentPage?: number; | ||
| }; | ||
| } | ||
|
|
||
| class QuizAnswerDto { | ||
| userId: string; | ||
| cohortId: string; | ||
| courseId: string; | ||
| unitId: string; | ||
| contentId: string; | ||
| attemptId: string; | ||
| answers: Array<{ | ||
| questionId: string; | ||
| type: string; | ||
| submittedAnswer: string | string[]; | ||
| }>; | ||
| score?: number; | ||
| questionsAttempted: number; | ||
| totalQuestions: number; | ||
| percentComplete: number; | ||
| timeSpent: number; | ||
| } | ||
|
|
||
| class CourseInitializationDto { | ||
| userId: string; | ||
| cohortId: string; | ||
| courseData: any[]; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using proper NestJS DTOs with validation decorators
The DTOs are defined as plain classes without any validation decorators. For better request validation and API documentation, consider using class-validator decorators.
Apply this enhancement to add validation:
+import { IsString, IsNumber, IsOptional, IsArray, ValidateNested, IsUUID, Min, Max } from 'class-validator';
+import { Type } from 'class-transformer';
+import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger';
class CourseProgressDto {
+ @ApiProperty({ description: 'User ID' })
+ @IsUUID()
userId: string;
+
+ @ApiProperty({ description: 'Cohort ID' })
+ @IsUUID()
cohortId: string;
+
+ @ApiProperty({ description: 'Course ID' })
+ @IsString()
courseId: string;
+
+ @ApiProperty({ description: 'Course title' })
+ @IsString()
courseTitle: string;
+
+ @ApiProperty({ description: 'Course progress percentage', minimum: 0, maximum: 100 })
+ @IsNumber()
+ @Min(0)
+ @Max(100)
progress: number;
// Add similar decorators for other fields...
}📝 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.
| class CourseProgressDto { | |
| userId: string; | |
| cohortId: string; | |
| courseId: string; | |
| courseTitle: string; | |
| progress: number; | |
| unitId?: string; | |
| unitTitle?: string; | |
| unitProgress?: number; | |
| contentId?: string; | |
| contentType?: string; | |
| contentTitle?: string; | |
| contentStatus?: string; | |
| tracking?: { | |
| percentComplete?: number; | |
| lastPosition?: number; | |
| currentPosition?: number; | |
| timeSpent?: number; | |
| visitedPages?: number[]; | |
| totalPages?: number; | |
| lastPage?: number; | |
| currentPage?: number; | |
| }; | |
| } | |
| class QuizAnswerDto { | |
| userId: string; | |
| cohortId: string; | |
| courseId: string; | |
| unitId: string; | |
| contentId: string; | |
| attemptId: string; | |
| answers: Array<{ | |
| questionId: string; | |
| type: string; | |
| submittedAnswer: string | string[]; | |
| }>; | |
| score?: number; | |
| questionsAttempted: number; | |
| totalQuestions: number; | |
| percentComplete: number; | |
| timeSpent: number; | |
| } | |
| class CourseInitializationDto { | |
| userId: string; | |
| cohortId: string; | |
| courseData: any[]; | |
| } | |
| // Add at the top of your file, alongside your other imports | |
| import { IsString, IsNumber, IsOptional, IsArray, ValidateNested, IsUUID, Min, Max } from 'class-validator'; | |
| import { Type } from 'class-transformer'; | |
| import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; | |
| class CourseProgressDto { | |
| @ApiProperty({ description: 'User ID' }) | |
| @IsUUID() | |
| userId: string; | |
| @ApiProperty({ description: 'Cohort ID' }) | |
| @IsUUID() | |
| cohortId: string; | |
| @ApiProperty({ description: 'Course ID' }) | |
| @IsString() | |
| courseId: string; | |
| @ApiProperty({ description: 'Course title' }) | |
| @IsString() | |
| courseTitle: string; | |
| @ApiProperty({ description: 'Course progress percentage', minimum: 0, maximum: 100 }) | |
| @IsNumber() | |
| @Min(0) | |
| @Max(100) | |
| progress: number; | |
| // Add similar decorators for other fields... | |
| } |
🤖 Prompt for AI Agents
In src/elasticsearch/controllers/course-webhook.controller.ts around lines 19 to
68, the DTO classes are plain TypeScript without validation; convert them into
NestJS-ready DTOs by adding class-validator and class-transformer decorators:
annotate required string fields with @IsString, numeric fields with
@IsNumber/@IsInt and range checks where appropriate, mark optional fields with
@IsOptional, validate arrays with @IsArray and element types, use
@ValidateNested and @Type(() => ...) for nested objects/arrays, and export the
DTOs; update any places that instantiate/expect these DTOs to use the validated
classes and enable validation pipe on the controller if not already enabled.
| try { | ||
| this.logger.log(`[LMS SERVICE] Received course progress update for user ${progressData.userId}, course ${progressData.courseId}`); | ||
|
|
||
| // Validate required fields | ||
| if (!progressData.userId || !progressData.cohortId || !progressData.courseId) { | ||
| throw new Error('Missing required fields: userId, cohortId, courseId'); | ||
| } | ||
|
|
||
| // Update course progress in Elasticsearch | ||
| await this.courseElasticsearchService.updateCourseProgress(progressData); | ||
|
|
||
| return { | ||
| success: true, | ||
| message: 'LMS course progress updated successfully', | ||
| data: { | ||
| userId: progressData.userId, | ||
| courseId: progressData.courseId, | ||
| progress: progressData.progress, | ||
| source: 'lms-service' | ||
| } | ||
| }; | ||
|
|
||
| } catch (error) { | ||
| this.logger.error(`[LMS SERVICE] Failed to update course progress:`, error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error response handling consistency
The error handling throws raw errors which might expose internal details. Consider wrapping errors in proper HTTP exceptions for consistent API responses.
Apply this improvement:
+import { BadRequestException, InternalServerErrorException } from '@nestjs/common';
async updateLMSCourseProgress(@Body() progressData: CourseProgressDto): Promise<{
// ...
}> {
try {
// Validate required fields
if (!progressData.userId || !progressData.cohortId || !progressData.courseId) {
- throw new Error('Missing required fields: userId, cohortId, courseId');
+ throw new BadRequestException('Missing required fields: userId, cohortId, courseId');
}
// ...
} catch (error) {
this.logger.error(`[LMS SERVICE] Failed to update course progress:`, error);
- throw error;
+ if (error instanceof BadRequestException) {
+ throw error;
+ }
+ throw new InternalServerErrorException('Failed to update course progress');
}
}📝 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.
| try { | |
| this.logger.log(`[LMS SERVICE] Received course progress update for user ${progressData.userId}, course ${progressData.courseId}`); | |
| // Validate required fields | |
| if (!progressData.userId || !progressData.cohortId || !progressData.courseId) { | |
| throw new Error('Missing required fields: userId, cohortId, courseId'); | |
| } | |
| // Update course progress in Elasticsearch | |
| await this.courseElasticsearchService.updateCourseProgress(progressData); | |
| return { | |
| success: true, | |
| message: 'LMS course progress updated successfully', | |
| data: { | |
| userId: progressData.userId, | |
| courseId: progressData.courseId, | |
| progress: progressData.progress, | |
| source: 'lms-service' | |
| } | |
| }; | |
| } catch (error) { | |
| this.logger.error(`[LMS SERVICE] Failed to update course progress:`, error); | |
| throw error; | |
| } | |
| } | |
| import { BadRequestException, InternalServerErrorException } from '@nestjs/common'; | |
| async updateLMSCourseProgress(@Body() progressData: CourseProgressDto): Promise<{ | |
| success: boolean; | |
| message: string; | |
| data: { | |
| userId: string; | |
| courseId: string; | |
| progress: number; | |
| source: string; | |
| }; | |
| }> { | |
| try { | |
| this.logger.log( | |
| `[LMS SERVICE] Received course progress update for user ${progressData.userId}, course ${progressData.courseId}` | |
| ); | |
| // Validate required fields | |
| if (!progressData.userId || !progressData.cohortId || !progressData.courseId) { | |
| throw new BadRequestException('Missing required fields: userId, cohortId, courseId'); | |
| } | |
| // Update course progress in Elasticsearch | |
| await this.courseElasticsearchService.updateCourseProgress(progressData); | |
| return { | |
| success: true, | |
| message: 'LMS course progress updated successfully', | |
| data: { | |
| userId: progressData.userId, | |
| courseId: progressData.courseId, | |
| progress: progressData.progress, | |
| source: 'lms-service', | |
| }, | |
| }; | |
| } catch (error) { | |
| this.logger.error(`[LMS SERVICE] Failed to update course progress:`, error); | |
| if (error instanceof BadRequestException) { | |
| throw error; | |
| } | |
| throw new InternalServerErrorException('Failed to update course progress'); | |
| } | |
| } |
🧰 Tools
🪛 ESLint
[error] 108-108: Replace [LMS·SERVICE]·Received·course·progress·update·for·user·${progressData.userId},·course·${progressData.courseId} with ⏎········[LMS·SERVICE]·Received·course·progress·update·for·user·${progressData.userId},·course·${progressData.courseId}⏎······
(prettier/prettier)
[error] 111-111: Replace !progressData.userId·||·!progressData.cohortId·||·!progressData.courseId with ⏎········!progressData.userId·||⏎········!progressData.cohortId·||⏎········!progressData.courseId⏎······
(prettier/prettier)
[error] 112-112: Replace 'Missing·required·fields:·userId,·cohortId,·courseId' with "Missing·required·fields:·userId,·cohortId,·courseId"
(prettier/prettier)
[error] 120-120: Replace 'LMS·course·progress·updated·successfully' with "LMS·course·progress·updated·successfully"
(prettier/prettier)
[error] 125-125: Replace 'lms-service' with "lms-service",
(prettier/prettier)
[error] 126-126: Insert ,
(prettier/prettier)
[error] 127-128: Delete ⏎
(prettier/prettier)
[error] 130-130: Replace ``[LMS·SERVICE]·Failed·to·update·course·progress:,·error with `⏎········`[LMS·SERVICE]·Failed·to·update·course·progress:`,⏎········error⏎······`
(prettier/prettier)
🤖 Prompt for AI Agents
In src/elasticsearch/controllers/course-webhook.controller.ts around lines
107-133, the catch currently rethrows raw errors and the validation uses throw
new Error(...); change to throw and return consistent NestJS HTTP exceptions:
when required fields are missing, throw a BadRequestException with a concise
message; in the try/catch, log full error details with this.logger.error(...)
but replace throw error with throwing an InternalServerErrorException('Failed to
update course progress') for unexpected failures (or rethrow BadRequestException
for validation), so API responses are consistent and internal details are not
exposed.
| import { TypeOrmModule } from '@nestjs/typeorm'; | ||
| import { ConfigService } from '@nestjs/config'; | ||
| import { UserElasticsearchService } from './user-elasticsearch.service'; | ||
| import { ElasticsearchConfig } from './elasticsearch.config'; | ||
| import { ElasticsearchDataFetcherService } from './elasticsearch-data-fetcher.service'; | ||
| import { ElasticsearchSyncService } from './elasticsearch-sync.service'; | ||
| import { ElasticsearchService } from './elasticsearch.service'; | ||
| import { UserElasticsearchController } from './user-elasticsearch.controller'; | ||
| import { ElasticsearchController } from './controllers/elasticsearch.controller'; | ||
| import { User } from '../user/entities/user-entity'; | ||
| import { CohortMembers } from '../cohortMembers/entities/cohort-member.entity'; | ||
| import { FormSubmission } from '../forms/entities/form-submission.entity'; | ||
| import { Form } from '../forms/entities/form.entity'; | ||
| import { FieldValues } from '../fields/entities/fields-values.entity'; | ||
| import { Fields } from '../fields/entities/fields.entity'; | ||
| import { Cohort } from '../cohort/entities/cohort.entity'; | ||
| import { PostgresFieldsService } from '../adapters/postgres/fields-adapter'; | ||
| import { FormsService } from '../forms/forms.service'; | ||
| import { LMSService } from '../common/services/lms.service'; | ||
| import { HttpService } from '../common/utils/http-service'; |
There was a problem hiding this comment.
Use double quotes for all import paths
All import statements should use double quotes to maintain consistency with the project's style guide.
Apply this diff to fix all import statements:
-import { TypeOrmModule } from '@nestjs/typeorm';
-import { ConfigService } from '@nestjs/config';
-import { UserElasticsearchService } from './user-elasticsearch.service';
-import { ElasticsearchDataFetcherService } from './elasticsearch-data-fetcher.service';
-import { ElasticsearchSyncService } from './elasticsearch-sync.service';
-import { ElasticsearchService } from './elasticsearch.service';
-import { UserElasticsearchController } from './user-elasticsearch.controller';
-import { ElasticsearchController } from './controllers/elasticsearch.controller';
-import { User } from '../user/entities/user-entity';
-import { CohortMembers } from '../cohortMembers/entities/cohort-member.entity';
-import { FormSubmission } from '../forms/entities/form-submission.entity';
-import { Form } from '../forms/entities/form.entity';
-import { FieldValues } from '../fields/entities/fields-values.entity';
-import { Fields } from '../fields/entities/fields.entity';
-import { Cohort } from '../cohort/entities/cohort.entity';
-import { PostgresFieldsService } from '../adapters/postgres/fields-adapter';
-import { FormsService } from '../forms/forms.service';
-import { LMSService } from '../common/services/lms.service';
-import { HttpService } from '../common/utils/http-service';
+import { TypeOrmModule } from "@nestjs/typeorm";
+import { ConfigService } from "@nestjs/config";
+import { UserElasticsearchService } from "./user-elasticsearch.service";
+import { ElasticsearchDataFetcherService } from "./elasticsearch-data-fetcher.service";
+import { ElasticsearchSyncService } from "./elasticsearch-sync.service";
+import { ElasticsearchService } from "./elasticsearch.service";
+import { UserElasticsearchController } from "./user-elasticsearch.controller";
+import { ElasticsearchController } from "./controllers/elasticsearch.controller";
+import { User } from "../user/entities/user-entity";
+import { CohortMembers } from "../cohortMembers/entities/cohort-member.entity";
+import { FormSubmission } from "../forms/entities/form-submission.entity";
+import { Form } from "../forms/entities/form.entity";
+import { FieldValues } from "../fields/entities/fields-values.entity";
+import { Fields } from "../fields/entities/fields.entity";
+import { Cohort } from "../cohort/entities/cohort.entity";
+import { PostgresFieldsService } from "../adapters/postgres/fields-adapter";
+import { FormsService } from "../forms/forms.service";
+import { LMSService } from "../common/services/lms.service";
+import { HttpService } from "../common/utils/http-service";📝 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.
| import { TypeOrmModule } from '@nestjs/typeorm'; | |
| import { ConfigService } from '@nestjs/config'; | |
| import { UserElasticsearchService } from './user-elasticsearch.service'; | |
| import { ElasticsearchConfig } from './elasticsearch.config'; | |
| import { ElasticsearchDataFetcherService } from './elasticsearch-data-fetcher.service'; | |
| import { ElasticsearchSyncService } from './elasticsearch-sync.service'; | |
| import { ElasticsearchService } from './elasticsearch.service'; | |
| import { UserElasticsearchController } from './user-elasticsearch.controller'; | |
| import { ElasticsearchController } from './controllers/elasticsearch.controller'; | |
| import { User } from '../user/entities/user-entity'; | |
| import { CohortMembers } from '../cohortMembers/entities/cohort-member.entity'; | |
| import { FormSubmission } from '../forms/entities/form-submission.entity'; | |
| import { Form } from '../forms/entities/form.entity'; | |
| import { FieldValues } from '../fields/entities/fields-values.entity'; | |
| import { Fields } from '../fields/entities/fields.entity'; | |
| import { Cohort } from '../cohort/entities/cohort.entity'; | |
| import { PostgresFieldsService } from '../adapters/postgres/fields-adapter'; | |
| import { FormsService } from '../forms/forms.service'; | |
| import { LMSService } from '../common/services/lms.service'; | |
| import { HttpService } from '../common/utils/http-service'; | |
| import { TypeOrmModule } from "@nestjs/typeorm"; | |
| import { ConfigService } from "@nestjs/config"; | |
| import { UserElasticsearchService } from "./user-elasticsearch.service"; | |
| import { ElasticsearchDataFetcherService } from "./elasticsearch-data-fetcher.service"; | |
| import { ElasticsearchSyncService } from "./elasticsearch-sync.service"; | |
| import { ElasticsearchService } from "./elasticsearch.service"; | |
| import { UserElasticsearchController } from "./user-elasticsearch.controller"; | |
| import { ElasticsearchController } from "./controllers/elasticsearch.controller"; | |
| import { User } from "../user/entities/user-entity"; | |
| import { CohortMembers } from "../cohortMembers/entities/cohort-member.entity"; | |
| import { FormSubmission } from "../forms/entities/form-submission.entity"; | |
| import { Form } from "../forms/entities/form.entity"; | |
| import { FieldValues } from "../fields/entities/fields-values.entity"; | |
| import { Fields } from "../fields/entities/fields.entity"; | |
| import { Cohort } from "../cohort/entities/cohort.entity"; | |
| import { PostgresFieldsService } from "../adapters/postgres/fields-adapter"; | |
| import { FormsService } from "../forms/forms.service"; | |
| import { LMSService } from "../common/services/lms.service"; | |
| import { HttpService } from "../common/utils/http-service"; |
🧰 Tools
🪛 ESLint
[error] 3-3: Replace '@nestjs/typeorm' with "@nestjs/typeorm"
(prettier/prettier)
[error] 4-4: Replace '@nestjs/config' with "@nestjs/config"
(prettier/prettier)
[error] 5-5: Replace './user-elasticsearch.service' with "./user-elasticsearch.service"
(prettier/prettier)
[error] 6-6: Replace './elasticsearch-data-fetcher.service' with "./elasticsearch-data-fetcher.service"
(prettier/prettier)
[error] 7-7: Replace './elasticsearch-sync.service' with "./elasticsearch-sync.service"
(prettier/prettier)
[error] 8-8: Replace './elasticsearch.service' with "./elasticsearch.service"
(prettier/prettier)
[error] 9-9: Replace './user-elasticsearch.controller' with "./user-elasticsearch.controller"
(prettier/prettier)
[error] 10-10: Replace './controllers/elasticsearch.controller' with "./controllers/elasticsearch.controller"
(prettier/prettier)
[error] 11-11: Replace '../user/entities/user-entity' with "../user/entities/user-entity"
(prettier/prettier)
[error] 12-12: Replace '../cohortMembers/entities/cohort-member.entity' with "../cohortMembers/entities/cohort-member.entity"
(prettier/prettier)
[error] 13-13: Replace '../forms/entities/form-submission.entity' with "../forms/entities/form-submission.entity"
(prettier/prettier)
[error] 14-14: Replace '../forms/entities/form.entity' with "../forms/entities/form.entity"
(prettier/prettier)
[error] 15-15: Replace '../fields/entities/fields-values.entity' with "../fields/entities/fields-values.entity"
(prettier/prettier)
[error] 16-16: Replace '../fields/entities/fields.entity' with "../fields/entities/fields.entity"
(prettier/prettier)
[error] 17-17: Replace '../cohort/entities/cohort.entity' with "../cohort/entities/cohort.entity"
(prettier/prettier)
[error] 18-18: Replace '../adapters/postgres/fields-adapter' with "../adapters/postgres/fields-adapter"
(prettier/prettier)
[error] 19-19: Replace '../forms/forms.service' with "../forms/forms.service"
(prettier/prettier)
[error] 20-20: Replace '../common/services/lms.service' with "../common/services/lms.service"
(prettier/prettier)
[error] 21-21: Replace '../common/utils/http-service' with "../common/utils/http-service"
(prettier/prettier)
🤖 Prompt for AI Agents
In src/elasticsearch/elasticsearch.module.ts around lines 3 to 21, the import
statements use single quotes but the project requires double quotes; update
every import path to use double quotes (e.g. change '...' to "..." ) across all
listed imports, ensuring consistency and no other changes to the import
identifiers or ordering.
| const userDoc = await this.elasticsearchService.get('users', progressUpdate.userId); | ||
|
|
||
| if (!userDoc) { | ||
| this.logger.warn(`User document not found in Elasticsearch: ${progressUpdate.userId}`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Elasticsearch operations
The get operation could throw an error if Elasticsearch is unavailable. Consider wrapping it in a try-catch for better error handling.
Apply this diff:
- // Get the user's document from Elasticsearch
- const userDoc = await this.elasticsearchService.get('users', progressUpdate.userId);
-
- if (!userDoc) {
- this.logger.warn(`User document not found in Elasticsearch: ${progressUpdate.userId}`);
- return;
- }
+ // Get the user's document from Elasticsearch
+ let userDoc;
+ try {
+ userDoc = await this.elasticsearchService.get("users", progressUpdate.userId);
+ } catch (error) {
+ this.logger.error(`Failed to retrieve user document from Elasticsearch: ${progressUpdate.userId}`, error);
+ throw new Error(`Elasticsearch operation failed: ${error.message}`);
+ }
+
+ if (!userDoc) {
+ this.logger.warn(`User document not found in Elasticsearch: ${progressUpdate.userId}`);
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 83-83: Replace 'users',·progressUpdate.userId); with ⏎········"users",⏎········progressUpdate.userId
(prettier/prettier)
[error] 84-84: Insert );⏎
(prettier/prettier)
[error] 86-86: Replace User·document·not·found·in·Elasticsearch:·${progressUpdate.userId} with ⏎··········User·document·not·found·in·Elasticsearch:·${progressUpdate.userId}⏎········
(prettier/prettier)
🤖 Prompt for AI Agents
In src/elasticsearch/services/course-elasticsearch.service.ts around lines 83 to
88, the call to this.elasticsearchService.get('users', progressUpdate.userId)
can throw if ES is unavailable; wrap the get call in a try-catch, catch and log
the error (using this.logger.error with context including progressUpdate.userId
and the caught error), and return early on error to avoid unhandled exceptions;
ensure the existing not-found branch remains after the try so it only runs when
get succeeds.
| type: 'nested', | ||
| values: [] | ||
| }; | ||
| } |
There was a problem hiding this comment.
Use double quotes for nested type string
Replace single quotes with double quotes for consistency.
Apply this diff:
if (!applications[appIndex].courses) {
applications[appIndex].courses = {
- type: 'nested',
+ type: "nested",
values: []
};
}📝 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.
| type: 'nested', | |
| values: [] | |
| }; | |
| } | |
| type: "nested", | |
| values: [] | |
| }; | |
| } |
🧰 Tools
🪛 ESLint
[error] 106-106: Replace 'nested' with "nested"
(prettier/prettier)
[error] 107-107: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In src/elasticsearch/services/course-elasticsearch.service.ts around lines 106
to 109, the object sets type: 'nested' using single quotes; replace the single
quotes with double quotes so it reads type: "nested" to match project string
quoting conventions and maintain consistency; update any adjacent occurrences if
present.
| type: 'nested', | ||
| values: [] | ||
| } |
There was a problem hiding this comment.
Use double quotes for all nested type declarations
Multiple instances of single-quoted 'nested' strings need to be replaced with double quotes for consistency.
Apply these changes throughout the file:
units: {
- type: 'nested',
+ type: "nested",
values: []
}And similar changes for lines 268, 340, 356, 381, 398, 452, 486, and 492.
Also applies to: 268-270, 340-342, 356-358, 381-383, 398-400, 452-453, 486-486, 492-492
🧰 Tools
🪛 ESLint
[error] 233-233: Replace 'nested' with "nested"
(prettier/prettier)
[error] 234-234: Insert ,
(prettier/prettier)
[error] 235-235: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In src/elasticsearch/services/course-elasticsearch.service.ts around lines
233-235 (and also at the specified ranges 268-270, 340-342, 356-358, 381-383,
398-400, 452-453, 486, and 492), replace all occurrences of the nested type
string written with single quotes ('nested') with double quotes ("nested") for
consistency; update each object literal/type declaration accordingly and run a
quick project lint/format to ensure the file's quoting style is consistent
across these lines.
| cohortId: application.cohortDetails?.cohortId || cohortId, | ||
| name: application.cohortDetails?.name || '', | ||
| description: application.cohortDetails?.description ?? '', | ||
| startDate: application.cohortDetails?.startDate ?? '', | ||
| endDate: application.cohortDetails?.endDate ?? '', | ||
| status: application.cohortDetails?.status || '', | ||
| type: application.cohortDetails?.type || 'COHORT', | ||
| status: application.cohortDetails?.status || 'active', | ||
| }, |
There was a problem hiding this comment.
Use double quotes for string literals
Replace single quotes with double quotes for consistency with the project's style guide.
Apply this diff:
cohortDetails: {
cohortId: application.cohortDetails?.cohortId || cohortId,
- name: application.cohortDetails?.name || '',
- type: application.cohortDetails?.type || 'COHORT',
- status: application.cohortDetails?.status || 'active',
+ name: application.cohortDetails?.name || "",
+ type: application.cohortDetails?.type || "COHORT",
+ status: application.cohortDetails?.status || "active",
},📝 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.
| cohortId: application.cohortDetails?.cohortId || cohortId, | |
| name: application.cohortDetails?.name || '', | |
| description: application.cohortDetails?.description ?? '', | |
| startDate: application.cohortDetails?.startDate ?? '', | |
| endDate: application.cohortDetails?.endDate ?? '', | |
| status: application.cohortDetails?.status || '', | |
| type: application.cohortDetails?.type || 'COHORT', | |
| status: application.cohortDetails?.status || 'active', | |
| }, | |
| cohortDetails: { | |
| cohortId: application.cohortDetails?.cohortId || cohortId, | |
| name: application.cohortDetails?.name || "", | |
| type: application.cohortDetails?.type || "COHORT", | |
| status: application.cohortDetails?.status || "active", | |
| }, |
🧰 Tools
🪛 ESLint
[error] 45-45: Replace '' with ""
(prettier/prettier)
[error] 46-46: Replace 'COHORT' with "COHORT"
(prettier/prettier)
[error] 47-47: Replace 'active' with "active"
(prettier/prettier)
🤖 Prompt for AI Agents
In src/elasticsearch/user-elasticsearch.controller.ts around lines 44 to 48, the
object literal uses single-quoted string literals; replace them with
double-quoted string literals to match project style — change '' to "" and
'COHORT' to "COHORT" and 'active' to "active" (ensure no other single-quoted
strings remain on these lines).
| indexName: 'users', | ||
| node: process.env.ELASTICSEARCH_HOST || 'http://localhost:9200', | ||
| mappings: { | ||
| mappings: { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider extracting nested mappings structure
The mappings object contains a nested mappings property which might be confusing. Consider whether this double nesting is intentional or if it should be flattened.
🏁 Script executed:
#!/bin/bash
# Check how the SHARED_ELASTICSEARCH_CONFIG is used to verify if double nesting is needed
rg -A 5 "SHARED_ELASTICSEARCH_CONFIG" --type tsLength of output: 483
🏁 Script executed:
#!/bin/bash
# Search entire repo for usages of the shared config constant
rg -n "SHARED_ELASTICSEARCH_CONFIG"
# Look for index creation calls to see how mappings is passed
rg -n "indices.create" --type ts
# Check where body: mappings is used
rg -n "body:.*mappings" --type ts
# Find instantiation of the Elasticsearch client
rg -n "new Client" --type tsLength of output: 402
🏁 Script executed:
#!/bin/bash
# Inspect how SHARED_ELASTICSEARCH_CONFIG is imported
rg -C 5 "import.*shared-elasticsearch.config" -n src/elasticsearch/elasticsearch.service.ts
# Inspect the context around indices.create to see how mappings is passed
rg -C 5 "indices.create" -n src/elasticsearch/elasticsearch.service.tsLength of output: 610
Clarify nested mappings property
The mappings key in SHARED_ELASTICSEARCH_CONFIG currently holds an object with another mappings property (mappings.mappings.properties). That shape is required by the ES client (body: { mappings: { properties: … } }), but the double-nesting can be confusing. Consider one of the following to improve clarity:
• Rename the outer key (e.g. to body or indexOptions) so it reads body.mappings.properties.
• Extract the inner mapping into its own constant and reference it, for example:
// src/shared/shared-elasticsearch-mapping.ts
export const USER_PROPERTIES = {
properties: {
/* … */
}
}
// src/shared/shared-elasticsearch.config.ts
export const SHARED_ELASTICSEARCH_CONFIG = {
indexName: 'users',
node: process.env.ELASTICSEARCH_HOST || 'http://localhost:9200',
mapping: USER_PROPERTIES,
}Then in your service:
await this.client.indices.create({
index,
body: { mappings: config.mapping },
wait_for_active_shards: 1,
});This preserves the required API shape while removing the extra mappings.mappings nesting.
🤖 Prompt for AI Agents
In src/shared/shared-elasticsearch.config.ts around line 5 the
SHARED_ELASTICSEARCH_CONFIG uses a nested mappings.mappings shape which is
confusing; change the config to expose only the inner mapping (e.g. rename the
outer key to body or indexOptions so callers do body: { mappings: config.body }
or extract the inner properties into a separate constant
(shared-elasticsearch-mapping.ts) and reference it from the config as mapping or
properties; then update call sites to pass body: { mappings: config.mapping }
(or body: config.body) so the final request shape stays body: { mappings: {
properties: … } } but you remove the double-nesting in the config.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores