Feat/Endaoment project update feature #1892
Conversation
WalkthroughThis pull request introduces a migration script that adds an Changes
Sequence DiagramsequenceDiagram
participant CronJob as Monthly Cron Job
participant ProjectService as Project Service
participant EndaomentAPI as Endaoment API
CronJob->>ProjectService: Retrieve Endaoment Projects
ProjectService->>EndaomentAPI: Fetch Organization Details
alt Organization Active
EndaomentAPI-->>ProjectService: Return Project Data
ProjectService->>ProjectService: Update Project Details
else Organization Offboarded
EndaomentAPI-->>ProjectService: Return 404
ProjectService->>ProjectService: Mark Project as Cancelled
end
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts (1)
42-79: Consider refactoringEndaomentServiceto use simple functionsThe
EndaomentServiceclass contains only static methods and does not maintain any internal state. It's preferable to use simple functions instead of a class with only static members to reduce complexity and improve readability.You could refactor the code by exporting the functions directly:
export const fetchOrgDetails = async (orgId: string): Promise<any> => { // function body }; export const updateProjectDetails = async (project: Project, orgData: any): Promise<void> => { // function body };🧰 Tools
🪛 Biome (1.9.4)
[error] 42-79: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
migration/1735909243926-insertEndaomentProjectsIds.ts (1)
16-19: Use TypeORM methods instead of raw SQL for schema changesUsing TypeORM's schema builder methods, such as
queryRunner.addColumn(), is recommended over executing raw SQL queries. This ensures better portability and consistency across different database systems supported by TypeORM.Here's how you can modify the code:
await queryRunner.addColumn('project', new TableColumn({ name: 'endaomentId', type: 'uuid', isNullable: true, isUnique: true, }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
migration/1735909243926-insertEndaomentProjectsIds.ts(1 hunks)src/entities/project.ts(2 hunks)src/services/cronJobs/checkAndUpdateEndaomentProject.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts
[error] 42-79: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts (2)
61-61:⚠️ Potential issueReview logging of potentially sensitive data
The line
logger.debug('Fetched org data:', orgData);logs the entireorgDataobject. Ensure that this data does not contain any sensitive information before logging it to prevent potential PII leakage.Consider logging only necessary information or sanitizing the data before logging.
8-8:⚠️ Potential issueEnsure
runCheckAndUpdateEndaomentProjectis invoked to schedule the cron jobThe
runCheckAndUpdateEndaomentProjectfunction schedules the cron job when it is called. Please ensure that this function is invoked during the application initialization; otherwise, the cron job will not be scheduled, and the task will not run.You can run the following script to verify if
runCheckAndUpdateEndaomentProjectis being called in the codebase:
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts (2)
45-45:⚠️ Potential issueValidate
API_URLbefore making requests
API_URLis retrieved from the environment variables without validation. If it's undefined, making requests will cause errors.-const API_URL = process.env.ENDAOMENT_API_URL; +const API_URL = process.env.ENDAOMENT_API_URL; +if (!API_URL) { + throw new Error('ENDAOMENT_API_URL environment variable is not defined'); +}
82-82: 🛠️ Refactor suggestionRemove redundant
await project.save();The
await project.save();call is redundant as the project is already saved in both the if and else blocks above.- await project.save();
🧹 Nitpick comments (5)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts (5)
23-23: Consider making organizationId configurableThe organizationId is hardcoded to 5. Consider making this configurable through environment variables for better maintainability.
- const projects = await Project.find({ where: { organizationId: 5 } }); + const ENDAOMENT_ORG_ID = config.get('ENDAOMENT_ORGANIZATION_ID') || 5; + const projects = await Project.find({ where: { organizationId: ENDAOMENT_ORG_ID } });
34-36: Enhance error handling specificityThe catch block could be more specific about the type of error and provide better error context for debugging.
- } catch (error) { - logger.error(`Failed to update project ID ${project.id}`, error); + } catch (error: unknown) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + logger.error( + `Failed to update project ID ${project.id}: ${errorMessage}`, + { projectId: project.id, error } + );
23-23: Add logging for empty project resultsConsider adding a log when no projects are found to help with monitoring and debugging.
const projects = await Project.find({ where: { organizationId: 5 } }); + if (projects.length === 0) { + logger.info('No Endaoment projects found to update'); + return; + }
47-84: Convert class to module with standalone functionsThe
EndaomentServiceclass contains only static methods. Consider converting it to a module with standalone functions for better maintainability.-export class EndaomentService { - static async fetchOrgDetails(orgId: string): Promise<any> { +export const fetchOrgDetails = async (orgId: string): Promise<OrgDetails> => { // ... rest of the code - } +}; - static async updateProjectDetails( +export const updateProjectDetails = async ( // ... rest of the code - } +}; -} +interface OrgDetails { + name?: string; + description?: string; + // Add other relevant fields +}🧰 Tools
🪛 Biome (1.9.4)
[error] 47-84: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
48-48: Replace 'any' type with proper interfaceUsing 'any' type reduces type safety. Define a proper interface for the organization details.
- static async fetchOrgDetails(orgId: string): Promise<any> { + static async fetchOrgDetails(orgId: string): Promise<OrgDetails> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
migration/1735909243926-insertEndaomentProjectsIds.ts(1 hunks)src/entities/project.ts(2 hunks)src/services/cronJobs/checkAndUpdateEndaomentProject.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/entities/project.ts
- migration/1735909243926-insertEndaomentProjectsIds.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts
[error] 47-84: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (1)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts (1)
8-11: LGTM! Well-structured configuration with fallback.The cron job configuration is properly implemented with an environment variable override and a sensible default value that runs monthly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts (1)
25-43: Implement batch processing and timeout handlingFor better scalability and reliability:
- Process projects in batches to avoid memory issues with large datasets
- Add timeout handling for API calls
- Consider implementing retry logic for failed API calls
Example implementation:
const BATCH_SIZE = 100; const API_TIMEOUT = 5000; // 5 seconds async function processBatch(projects: Project[]) { for (const project of projects) { try { if (!project.endaomentId) { logger.warn(`Project ID ${project.id} does not have an endaomentId.`); continue; } const orgData = await Promise.race([ EndaomentService.fetchOrgDetails(project.endaomentId), new Promise((_, reject) => setTimeout(() => reject(new Error('API Timeout')), API_TIMEOUT) ) ]); await EndaomentService.updateProjectDetails(project, orgData); } catch (error) { if (error.message === 'API Timeout') { logger.error(`API timeout for project ID ${project.id}`); } else { logger.error(`Failed to update project ID ${project.id}`, error); } } } } // In the main function: const allProjects = await Project.find({ where: { organizationId: ENDAOMENT_ORGANIZATION_ID } }); for (let i = 0; i < allProjects.length; i += BATCH_SIZE) { const batch = allProjects.slice(i, i + BATCH_SIZE); await processBatch(batch); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts
[error] 53-88: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (1)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts (1)
1-11: LGTM! Well-structured configuration setup.The configuration is properly implemented with environment variable support and a sensible default cron schedule.
@CarlosQ96, to bi noticed here, this will use cronJob to fetch all Endaoments projects inside our database and check using Endaoment API is there any changes in title or description and script will update that attributes. Also if project has been deprecated on Endaoment on our side it will be marked as canceled
Summary by CodeRabbit
New Features
endaomentIdfield to projects to link with the Endaoment platform.Chores