Implement entity persistence for project creation #1326
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces comprehensive changes to the project-factory service, focusing on enhancing campaign project management. The modifications include creating new database tables for tracking campaign projects and process statuses, adding utility functions for handling campaign-related operations, and updating various API and service components to support more robust project creation and tracking mechanisms. The changes span across database migrations, API endpoints, Kafka message handling, and utility functions, providing a more structured approach to managing campaign projects. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MainProcessController
participant CampaignService
participant ProjectCampaignUtils
participant KafkaProducer
participant Database
Client->>MainProcessController: Start Project Creation
MainProcessController->>CampaignService: Process Project Creation
CampaignService->>ProjectCampaignUtils: Persist Campaign Project
ProjectCampaignUtils->>KafkaProducer: Produce Save Campaign Project Message
ProjectCampaignUtils->>Database: Save Campaign Project
Database-->>ProjectCampaignUtils: Project Saved
ProjectCampaignUtils-->>CampaignService: Project Creation Confirmed
CampaignService-->>MainProcessController: Process Completed
MainProcessController-->>Client: Project Creation Response
Possibly related PRs
Suggested reviewers
Poem
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
Documentation and Community
|
…ps://github.com/egovernments/health-campaign-services into HCMPRE-1788-project-creation-entity-persistence
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 45
🔭 Outside diff range comments (3)
health-services/project-factory/src/server/utils/transforms/projectTypeUtils.ts (1)
Line range hint
1-118: Consider splitting utilities into separate modules.The file currently handles multiple concerns:
- Project type configuration
- Data transformation
- Validation logic
- Type definitions
Consider splitting these into separate modules for better maintainability and separation of concerns:
types.tsfor type definitionsconfig.tsfor configurationtransformers.tsfor transformation logicvalidators.tsfor validation logicAlso, clean up the commented-out code as it adds noise to the codebase.
Would you like me to help restructure the code into separate modules?
🧰 Tools
🪛 Biome (1.9.4)
[error] 99-99: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 98-98: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.(lint/style/noVar)
health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts (1)
Line range hint
1769-1770: Replace var declarations with let/const.Using
varcan lead to scoping issues. Useletorconstinstead.Apply this diff:
- var projectFound = false; - var retry = 20; + let projectFound = false; + let retry = 20;health-services/project-factory/src/server/utils/generateUtils.ts (1)
Line range hint
32-67: Improve type safety and error handling.
- Consider defining an interface for the request object structure to improve type safety.
- Extract the magic number timeout (2000ms) to a named constant.
- Consider breaking down the complex generate logic into smaller, focused functions.
Example refactor:
interface CampaignRequest { body: { RequestInfo: any; CampaignDetails: { boundaries: any[]; tenantId: string; hierarchyType: string; id: string; }; ExistingCampaignDetails?: { boundaries: any[]; projectType: string; }; }; query: any; } const RESOURCE_GENERATION_DELAY_MS = 2000; async function generateResources(request: CampaignRequest, boundaries: any[]) { const baseParams = { tenantId: request.body.CampaignDetails.tenantId, forceUpdate: 'true', hierarchyType: request.body.CampaignDetails.hierarchyType, campaignId: request.body.CampaignDetails.id }; await Promise.all([ generateBoundaryResources(request, boundaries, baseParams), generateFacilityResources(request, boundaries, baseParams), generateUserResources(request, boundaries, baseParams) ]); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (27)
health-services/project-factory/migration/main/V20250106120000__create_campaign_project.sql(1 hunks)health-services/project-factory/migration/main/V20250113154600__create_campaign_process_table.sql(1 hunks)health-services/project-factory/migration/main/V20250113163400__create_idx_campaignnumber.sql(1 hunks)health-services/project-factory/src/server/api/boundaryApis.ts(1 hunks)health-services/project-factory/src/server/api/campaignApis.ts(6 hunks)health-services/project-factory/src/server/api/genericApis.ts(4 hunks)health-services/project-factory/src/server/api/projectApis.ts(1 hunks)health-services/project-factory/src/server/config/constants.ts(1 hunks)health-services/project-factory/src/server/config/index.ts(2 hunks)health-services/project-factory/src/server/controllers/index.ts(1 hunks)health-services/project-factory/src/server/controllers/mainProcessController/mainProcess.controller.ts(1 hunks)health-services/project-factory/src/server/kafka/Listener.ts(3 hunks)health-services/project-factory/src/server/service/mainProcessService.ts(1 hunks)health-services/project-factory/src/server/utils/boundaryUtils.ts(2 hunks)health-services/project-factory/src/server/utils/campaignMappingUtils.ts(1 hunks)health-services/project-factory/src/server/utils/campaignUtils.ts(20 hunks)health-services/project-factory/src/server/utils/excelUtils.ts(2 hunks)health-services/project-factory/src/server/utils/generateUtils.ts(1 hunks)health-services/project-factory/src/server/utils/genericUtils.ts(2 hunks)health-services/project-factory/src/server/utils/logger/index.ts(1 hunks)health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts(5 hunks)health-services/project-factory/src/server/utils/processTrackUtils.ts(2 hunks)health-services/project-factory/src/server/utils/projectCampaignUtils.ts(1 hunks)health-services/project-factory/src/server/utils/targetUtils.ts(3 hunks)health-services/project-factory/src/server/utils/transforms/projectTypeUtils.ts(2 hunks)health-services/project-factory/src/server/validators/campaignValidators.ts(2 hunks)health-services/project-factory/src/server/validators/genericValidator.ts(2 hunks)
🧰 Additional context used
📓 Learnings (2)
health-services/project-factory/src/server/utils/campaignMappingUtils.ts (1)
Learnt from: nitish-egov
PR: egovernments/health-campaign-services#1082
File: health-services/project-factory/src/server/utils/campaignMappingUtils.ts:640-646
Timestamp: 2024-12-04T12:30:22.889Z
Learning: In `health-services/project-factory/src/server/utils/campaignMappingUtils.ts`, the `processMapping` function already includes validation for `mappingObject.CampaignDetails.campaignDetails.boundaries` before accessing it, so additional null checks are unnecessary.
health-services/project-factory/src/server/validators/campaignValidators.ts (1)
Learnt from: nitish-egov
PR: egovernments/health-campaign-services#1082
File: health-services/project-factory/src/server/utils/campaignMappingUtils.ts:640-646
Timestamp: 2024-12-04T12:30:22.889Z
Learning: In `health-services/project-factory/src/server/utils/campaignMappingUtils.ts`, the `processMapping` function already includes validation for `mappingObject.CampaignDetails.campaignDetails.boundaries` before accessing it, so additional null checks are unnecessary.
🪛 Biome (1.9.4)
health-services/project-factory/src/server/utils/transforms/projectTypeUtils.ts
[error] 99-99: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 98-98: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
health-services/project-factory/src/server/utils/boundaryUtils.ts
[error] 77-80: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts
[error] 380-380: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
health-services/project-factory/src/server/utils/targetUtils.ts
[error] 146-147: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
health-services/project-factory/src/server/utils/processTrackUtils.ts
[error] 311-311: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 313-313: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
health-services/project-factory/src/server/api/campaignApis.ts
[error] 1769-1769: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'let' instead.
(lint/style/noVar)
[error] 1770-1770: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'let' instead.
(lint/style/noVar)
health-services/project-factory/src/server/utils/campaignUtils.ts
[error] 993-993: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 1049-1049: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1054-1054: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1081-1081: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
[error] 1186-1186: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1181-1184: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
[error] 1209-1209: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
[error] 1237-1237: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 1257-1257: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 1963-1964: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 2693-2693: This let declares a variable that is only assigned once.
'rowData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
health-services/project-factory/src/server/utils/projectCampaignUtils.ts
[error] 81-81: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 318-318: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 124-124: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 125-125: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (28)
health-services/project-factory/src/server/utils/transforms/projectTypeUtils.ts (1)
Line range hint
6-8: Address TODO comments for critical functionality.There are multiple TODO comments indicating incomplete implementations:
- Configuration object needs appropriate values
- Project type master fetching logic is missing
These implementations are crucial for the proper functioning of the project type system.
Would you like me to help implement these missing functionalities or create GitHub issues to track them?
Also applies to: 11-11
health-services/project-factory/src/server/utils/targetUtils.ts (1)
108-108: Verify boundary access path change.The change in accessing boundaries (
CampaignDetails.boundariesinstead ofboundariesCombined) might affect other parts of the codebase.✅ Verification successful
Boundary access change is consistent with codebase patterns
The change from
boundariesCombinedtorequest?.body?.CampaignDetails?.boundariesfollows the established pattern used consistently throughout the codebase. No instances of the old pattern remain, confirming a complete migration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of boundariesCombined to ensure consistent updates rg "boundariesCombined" --type tsLength of output: 33
Script:
#!/bin/bash # Search for various boundary access patterns in request bodies rg "request\?\.body.*boundaries?" --type ts # Search for CampaignDetails structure usage rg "CampaignDetails.*boundaries?" --type ts # Search for any boundary-related properties in request bodies ast-grep --pattern 'request?.body.$_boundaries'Length of output: 5617
health-services/project-factory/src/server/api/genericApis.ts (4)
12-12: ImportedgetBoundaryRelationshipDatacorrectly.The function
getBoundaryRelationshipDatais now imported from./boundaryApis, which is appropriate if the function has been moved to a new module.
223-226: Confirm handling when"${codeColumnName}"is not found in sheet.When the column
codeColumnNameis not found in the sheet, the code logs a warning and continues processing. Please confirm that skipping sheets without this column is the intended behavior and that downstream logic accounts for potentially missing data from these sheets.
193-193: Verify all calls togetTargetSheetDataAfterCodeare updated with new parameter.The parameter of
getTargetSheetDataAfterCodehas changed fromrequesttocampaignDetails. Please ensure all calls to this function are updated accordingly to passcampaignDetailsto prevent runtime errors.To verify that all function calls have been updated, you can run the following script:
✅ Verification successful
All calls to
getTargetSheetDataAfterCodeare properly updatedThe only call to this function exists in
campaignUtils.tsand is already using the correctCampaignDetailsparameter. No instances of the oldrequestparameter were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find calls to `getTargetSheetDataAfterCode` that may still use `request` as the first argument. # Search for function calls where the first argument is `request` ast-grep --pattern 'getTargetSheetDataAfterCode(request, $$$)' --lang tsLength of output: 74
Script:
#!/bin/bash # Search for all calls to getTargetSheetDataAfterCode with any arguments ast-grep --pattern 'getTargetSheetDataAfterCode($_)' --lang ts # Backup search using ripgrep to find any mentions of the function rg "getTargetSheetDataAfterCode" -A 1Length of output: 873
590-590: Ensure compatibility withgetBoundaryRelationshipData.The
getBoundarySheetDatafunction now usesgetBoundaryRelationshipDatato retrieve boundary data. Verify that theparamsobject passed to this function contains the correct properties and that the returned data aligns with the expected structure used in the subsequent logic.If needed, you can check the usage of
getBoundaryRelationshipDatawith the following script:✅ Verification successful
✓ The
getBoundaryRelationshipDataintegration is correctly implementedThe
paramsobject contains all required properties and the returned data structure is properly handled with appropriate null checks and fallback mechanisms.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `getBoundaryRelationshipData` is used correctly across the codebase. # Search for calls to `getBoundaryRelationshipData` and display surrounding lines for context. rg 'getBoundaryRelationshipData\(' -A 5 -B 2Length of output: 4160
Script:
#!/bin/bash # Get more context around the usage in genericApis.ts ast-grep --pattern 'async function getBoundarySheetData($_) { $$$ const boundaryData = await getBoundaryRelationshipData(params); $$$ }'Length of output: 144
Script:
#!/bin/bash # Get the full context of getBoundarySheetData function rg -B 10 -A 10 'async function getBoundarySheetData'Length of output: 1856
Script:
#!/bin/bash # Get extended context after the getBoundaryRelationshipData call rg -B 2 -A 15 'const boundaryData = await getBoundaryRelationshipData\(params\);' health-services/project-factory/src/server/api/genericApis.tsLength of output: 804
health-services/project-factory/src/server/utils/processTrackUtils.ts (1)
324-335: Handle the case when no process details are found.In the function
markProcessStatus, if no processes are found for the given campaign number and process name, it's logged as an error. Depending on the application's requirements, you might want to handle this case more gracefully or take corrective action.Please confirm whether additional handling is needed when no process is found. If this is expected behavior, consider adjusting the log level from
errortowarningorinfo.health-services/project-factory/src/server/utils/campaignUtils.ts (1)
1963-1964: 🧹 Nitpick (assertive)Prefer template literals over string concatenation
Using template literals improves code readability and maintainability. Replace string concatenation with a template literal.
Apply this diff:
- throw new Error('Project Creation process did not complete after 75 attempts.'); + throw new Error(`Project Creation process did not complete after 75 attempts.`);Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1963-1964: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
health-services/project-factory/src/server/controllers/index.ts (2)
4-4: Import statement is correctThe import of
mainProcessControlleris properly added and follows the existing import structure.
10-11: Controllers array updated appropriatelyThe
controllersarray now includes an instance ofmainProcessController, ensuring that the new controller is registered.health-services/project-factory/src/server/api/boundaryApis.ts (1)
1-16: New functiongetBoundaryRelationshipDataimplemented correctlyThe
getBoundaryRelationshipDatafunction is well-defined and integrates correctly with the boundary APIs. The use of optional chaining and default headers is appropriate.health-services/project-factory/src/server/utils/logger/index.ts (1)
35-37: LGTM! Good error handling improvement.The change from error to warn level is appropriate for these scenarios as they are recoverable conditions.
health-services/project-factory/src/server/config/constants.ts (1)
161-170: LGTM! Well-structured constants.The new constants follow the existing pattern and provide clear status definitions for campaign processes.
health-services/project-factory/src/server/utils/excelUtils.ts (1)
10-10: LGTM!The import statement is correctly added and follows the project's import organization.
health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts (2)
13-13: LGTM!The import statement is correctly added and follows the project's import organization.
271-271: LGTM!The modification correctly updates the boundaries by combining parent and new boundaries.
health-services/project-factory/src/server/utils/campaignMappingUtils.ts (1)
60-60: LGTM!The function is correctly exported to support reusability across modules.
health-services/project-factory/src/server/validators/campaignValidators.ts (1)
1049-1049: LGTM! Consistent with campaign details structure.The update to use
CampaignDetails.boundariesaligns with the standardized way of accessing campaign data.health-services/project-factory/migration/main/V20250113163400__create_idx_campaignnumber.sql (1)
1-2: LGTM! Index will improve query performance.The index on
campaignnumberis properly defined with idempotent creation using IF NOT EXISTS.health-services/project-factory/migration/main/V20250113154600__create_campaign_process_table.sql (3)
1-12: LGTM! Well-structured table definition with proper constraints.The table definition includes:
- Primary key on id
- Required fields marked as NOT NULL
- JSONB for flexible additional details
- Audit fields for tracking changes
- Unique constraint to prevent duplicates
14-16: LGTM! Index will improve campaign number lookups.The single-column index on
campaignnumberwill optimize queries filtering by campaign number.
18-20: LGTM! Composite index supports unique constraint.The composite index on (campaignnumber, processname) will:
- Support the unique constraint
- Optimize queries filtering by both columns
health-services/project-factory/migration/main/V20250106120000__create_campaign_project.sql (1)
1-15: 🧹 Nitpick (assertive)🛠️ Refactor suggestion
Consider adding foreign key constraint and index for projectid.
The
projectidcolumn appears to reference another table but lacks a foreign key constraint and index, which could lead to data integrity issues and suboptimal query performance.Reconsider unique constraint design.
Including
isactivein the unique constraint (unique_campaign_boundary_active) might cause issues when trying to soft-delete and recreate records with the same campaign number and boundary code.Consider using TIMESTAMP type for temporal columns.
Using BIGINT for
createdtimeandlastmodifiedtimeinstead of TIMESTAMP WITH TIME ZONE:
- Makes it harder to perform date/time operations
- Doesn't enforce valid timestamp values
- Doesn't handle timezone information properly
Document JSONB structure.
Add comments describing the expected structure and schema of the
additionaldetailsJSONB column to help maintain data consistency.health-services/project-factory/src/server/utils/generateUtils.ts (1)
32-32: Verify the impact of boundaries path change.The path to boundaries has changed from
boundariesCombinedtoCampaignDetails.boundaries. Ensure all consumers of this API are updated to use the new path.Run this script to find potential usages that need updating:
health-services/project-factory/src/server/api/campaignApis.ts (4)
32-32: LGTM! New imports are correctly added.The new imports are properly added and used in the modified functions.
Also applies to: 53-54
1751-1754: LGTM! Function signature and null check improvements.The function signature is improved with separate parameters and early return for null/undefined projectId.
1829-1829: LGTM! Added persistence for campaign project.The function now correctly persists campaign project data after successful creation.
Line range hint
1841-1856: LGTM! Enhanced project update functionality.The function improvements include:
- Added campaign project persistence
- Better error handling with detailed logging
- Type-safe parameter for campaign project ID
🧰 Tools
🪛 Biome (1.9.4)
[error] 1844-1844: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
health-services/project-factory/src/server/utils/transforms/projectTypeUtils.ts
Show resolved
Hide resolved
health-services/project-factory/src/server/utils/transforms/projectTypeUtils.ts
Show resolved
Hide resolved
health-services/project-factory/src/server/utils/transforms/projectTypeUtils.ts
Show resolved
Hide resolved
health-services/project-factory/src/server/utils/transforms/projectTypeUtils.ts
Show resolved
Hide resolved
health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts
Show resolved
Hide resolved
health-services/project-factory/migration/main/V20250106120000__create_campaign_project.sql
Show resolved
Hide resolved
Update Listener.ts
…ps://github.com/egovernments/health-campaign-services into HCMPRE-1788-project-creation-entity-persistence
Update campaignMappingUtils.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (9)
health-services/project-factory/src/server/validators/genericValidator.ts (1)
224-224:⚠️ Potential issueAdd error handling for boundary data retrieval.
The function call has been updated to only pass
params, but it still lacks proper error handling which could lead to unhandled exceptions.Apply this diff to improve error handling:
- const boundaryData = await getBoundaryRelationshipData(params); + let boundaryData; + try { + boundaryData = await getBoundaryRelationshipData(params); + } catch (error) { + throwError("BOUNDARY", 500, "BOUNDARY_RELATIONSHIP_ERROR", "Error retrieving boundary relationship data"); + }Additionally, verify that the updated function signature in
boundaryApis.tsproperly handles all the required data that was previously available through therequestparameter.Run this script to check the implementation:
#!/bin/bash # Description: Check if getBoundaryRelationshipData implementation in boundaryApis.ts handles all required data # Search for the implementation ast-grep --pattern 'export $_ getBoundaryRelationshipData($_) { $$$ }' # Check for any other usages to ensure consistency rg -A 5 'getBoundaryRelationshipData'health-services/project-factory/src/server/kafka/Listener.ts (1)
32-33: 🛠️ Refactor suggestionConsider adding message validation.
Add validation for the message structure before processing to prevent runtime errors.
health-services/project-factory/src/server/api/projectApis.ts (4)
6-34: 🛠️ Refactor suggestionSpecify Return Types and Improve Error Handling
The function
getProjectsWithProjectIdslacks explicit return type annotations and proper error handling for thehttpRequestcall. Additionally, the code afterthrowErroris unreachable.Apply this diff to enhance type safety and remove unreachable code:
-export async function getProjectsWithProjectIds(projectIds: string[], tenantId: string, userUuid: string) { +export async function getProjectsWithProjectIds(projectIds: string[], tenantId: string, userUuid: string): Promise<any[]> { // ...existing code... const response = await httpRequest(url, requestBody, params); + if (!response) { + throwError("PROJECT", 500, "PROJECT_SEARCH_ERROR", "No response from project search"); + } if (Array.isArray(response?.Project)) { return response?.Project; } else { throwError("PROJECT", 500, "PROJECT_SEARCH_ERROR", "Error occurred during project search"); - return []; } }
36-64:⚠️ Potential issueFix Return Type Inconsistency and Remove Unreachable Code
The function
getProjectsCountsWithProjectIdsshould consistently return a number representing the count. Currently, it returnsresponse?.TotalCountor an empty array, which could lead to type inconsistencies.Apply this diff to fix the return type and remove unreachable code:
-export async function getProjectsCountsWithProjectIds(projectIds: string[], tenantId: string, userUuid: string) { +export async function getProjectsCountsWithProjectIds(projectIds: string[], tenantId: string, userUuid: string): Promise<number> { // ...existing code... if (Array.isArray(response?.Project)) { return response?.TotalCount || 0; } else { throwError("PROJECT", 500, "PROJECT_SEARCH_ERROR", "Error occurred during project search"); - return []; } }
66-80: 🛠️ Refactor suggestionAdd Error Handling and Specify Types
The function
updateProjectslacks error handling for thehttpRequestcall and uses genericanytypes.Enhance the function with error handling and type definitions:
-export async function updateProjects(projects: any[], userUuid: string) { +export async function updateProjects(projects: Project[], userUuid: string): Promise<void> { // ...existing code... try { await httpRequest(url, requestBody); } catch (error) { throwError("PROJECT", 500, "PROJECT_UPDATE_ERROR", "Error occurred during project update"); } }Also, define the
Projectinterface for better type safety.
82-103: 🛠️ Refactor suggestionEnsure Consistent Error Handling and Return Types
The function
createProjectsAndGetCreatedProjectsshould include error handling for thehttpRequestand specify return types.Modify the function as follows:
-export async function createProjectsAndGetCreatedProjects(projects: any[], userUuid: string) { +export async function createProjectsAndGetCreatedProjects(projects: Project[], userUuid: string): Promise<Project[]> { // ...existing code... const response = await httpRequest(url, requestBody); if (Array.isArray(response?.Project)) { return response?.Project; } else { throwError("PROJECT", 500, "PROJECT_CREATION_ERROR", "Error occurred during project creation"); - return []; } }This ensures that the function consistently returns an array of
Projectobjects.health-services/project-factory/src/server/utils/projectCampaignUtils.ts (2)
110-110: 🧹 Nitpick (assertive)Use
Number.parseIntInstead of the GlobalparseIntFor consistency with ES2015 standards, replace
parseIntwithNumber.parseInt.Apply these diffs to update the code:
At line 110:
- return parseInt(queryResponse.rows[0].count, 10); + return Number.parseInt(queryResponse.rows[0].count, 10);At lines 124-125:
- createdTime: parseInt(row.createdtime), - lastModifiedTime: parseInt(row.lastmodifiedtime) + createdTime: Number.parseInt(row.createdtime, 10), + lastModifiedTime: Number.parseInt(row.lastmodifiedtime, 10)Also applies to: 124-125
🧰 Tools
🪛 Biome (1.9.4)
[error] 110-110: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
296-296: 🧹 Nitpick (assertive)Avoid Using the
deleteOperator for Better PerformanceUsing the
deleteoperator can impact performance due to changes in object shapes. Consider setting the property toundefinedinstead.Apply this diff to modify the code:
At line 296:
- delete campaignProject.additionalDetails.newTargets; + campaignProject.additionalDetails.newTargets = undefined;At line 318:
- delete boundaryCodesForTargetUpdateMappings[boundaryCode].targets; + boundaryCodesForTargetUpdateMappings[boundaryCode].targets = undefined;Also applies to: 318-318
🧰 Tools
🪛 Biome (1.9.4)
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
health-services/project-factory/src/server/service/mainProcessService.ts (1)
11-27: 🛠️ Refactor suggestionReplace
console.logwith Proper LoggingUsing
console.logis not recommended for logging errors in a production environment. Replace it with a proper logging utility likelogger.errorfor better error tracking and consistency.Apply this diff to update the error handling:
} catch (error: any) { - console.log(error); + logger.error("Error in processProjectCreation:", error); await markProcessStatus(campaignDetailsAndRequestInfo?.CampaignDetails?.campaignNumber, processNamesConstantsInOrder.projectCreation, campaignProcessStatus.failed, error?.message); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
health-services/project-factory/src/server/api/genericApis.ts(7 hunks)health-services/project-factory/src/server/api/projectApis.ts(1 hunks)health-services/project-factory/src/server/kafka/Listener.ts(3 hunks)health-services/project-factory/src/server/service/mainProcessService.ts(1 hunks)health-services/project-factory/src/server/utils/campaignMappingUtils.ts(2 hunks)health-services/project-factory/src/server/utils/genericUtils.ts(2 hunks)health-services/project-factory/src/server/utils/projectCampaignUtils.ts(1 hunks)health-services/project-factory/src/server/utils/targetUtils.ts(3 hunks)health-services/project-factory/src/server/validators/genericValidator.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
health-services/project-factory/src/server/utils/targetUtils.ts
[error] 146-147: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
health-services/project-factory/src/server/utils/projectCampaignUtils.ts
[error] 81-81: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 318-318: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 124-124: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 125-125: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (13)
health-services/project-factory/src/server/validators/genericValidator.ts (1)
13-13: LGTM! Good code organization.Moving the
getBoundaryRelationshipDatafunction toboundaryApisimproves code organization by properly separating API-related functions into a dedicated module.health-services/project-factory/src/server/utils/campaignMappingUtils.ts (2)
60-60: LGTM! Function export change looks good.The
getAllBoundariesfunction is now exported, making it accessible to other modules. The function's implementation is robust with proper error handling and data sanitization.
685-688: Verify the impact of removing boundary merging.The commented-out code suggests that boundary merging from parent campaigns has been removed. Please ensure this change:
- Is intentional and aligns with the new requirements
- Won't break existing functionality that might depend on merged boundaries
- Has been properly tested with parent-child campaign scenarios
Run this script to check for potential impacts:
✅ Verification successful
Boundary merging removal is safe - functionality exists elsewhere with better validation
The removal of boundary merging code is intentional and safe because:
- Boundary merging is properly handled in
onGoingCampaignUpdateUtils.tswith explicit validation to prevent duplicate boundaries- The system maintains parent-child boundary relationships through
getBoundariesFromCampaignSearchResponse- The removed code was redundant and less robust than current implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for dependencies on boundary merging functionality # Look for references to boundary merging in the codebase # Search for boundary-related function calls echo "Searching for boundary-related function calls..." rg -l "boundaries.*parent" # Search for tests related to parent campaign boundaries echo "Searching for related test cases..." rg -l "test.*parent.*campaign.*boundar"Length of output: 459
Script:
#!/bin/bash # Check actual implementations and usages # Check boundary usage in campaign utils echo "=== Campaign Utils Implementation ===" rg -A 5 -B 5 "boundaries.*parent|parent.*boundaries" health-services/project-factory/src/server/utils/campaignUtils.ts echo -e "\n=== Ongoing Campaign Update Utils Implementation ===" rg -A 5 -B 5 "boundaries.*parent|parent.*boundaries" health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts # Look for any boundary-related tests with different patterns echo -e "\n=== Boundary-related Tests ===" rg -l "test.*boundar|spec.*boundar" # Check the API expectations in Postman collection echo -e "\n=== Postman Collection Boundary Usage ===" rg -A 5 -B 5 "boundaries.*parent|parent.*boundaries" health-services/project-factory/postman_collection.jsonLength of output: 18489
health-services/project-factory/src/server/kafka/Listener.ts (4)
6-6: LGTM! Import statement follows project conventions.The new import statement correctly imports the campaign processing handlers from the utils directory, maintaining consistency with the project's structure.
32-33: LGTM! Improved logging placement.Moving the logging statements before the switch provides better visibility into all incoming messages, regardless of their topic.
20-21: Verify topic configuration in config file.Ensure that the new Kafka topics are properly defined in the config file.
✅ Verification successful
Kafka topics are properly configured ✅
Both
KAFKA_PROCESS_HANDLER_TOPICandKAFKA_SUB_PROCESS_HANDLER_TOPICare correctly defined in the config file with appropriate default values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new Kafka topics are defined in the config file rg -A 2 "KAFKA_PROCESS_HANDLER_TOPIC|KAFKA_SUB_PROCESS_HANDLER_TOPIC" health-services/project-factory/src/server/config/Length of output: 829
39-44: 🛠️ Refactor suggestionEnhance error handling for new message processors.
While the async/await usage is correct, consider adding dedicated error boundaries for each handler to provide better error context and recovery options.
case config.kafka.KAFKA_PROCESS_HANDLER_TOPIC: - await handleCampaignProcessing(messageObject); + try { + await handleCampaignProcessing(messageObject); + } catch (error) { + logger.error(`Error processing campaign: ${error.message}`, { + topic: message.topic, + error: error.stack + }); + throw error; // Re-throw to trigger message retry + } break; case config.kafka.KAFKA_SUB_PROCESS_HANDLER_TOPIC: - await handleCampaignSubProcessing(messageObject); + try { + await handleCampaignSubProcessing(messageObject); + } catch (error) { + logger.error(`Error processing campaign sub-process: ${error.message}`, { + topic: message.topic, + error: error.stack + }); + throw error; // Re-throw to trigger message retry + } break;Likely invalid or redundant comment.
health-services/project-factory/src/server/utils/targetUtils.ts (4)
108-108: Verify the assignment ofCampaignDetails.boundariesThe code changes the source of
boundariesfromrequest?.body?.boundariesCombinedtorequest?.body?.CampaignDetails?.boundaries. Please verify thatCampaignDetails.boundariesis correctly populated and that this change aligns with the updated data structure.
133-154: Add type definitions for parameters and return valueThe function
getTargetListForCampaignwould benefit from explicit type definitions for its parameters and return value, enhancing type safety and code clarity.🧰 Tools
🪛 Biome (1.9.4)
[error] 146-147: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
156-164: Add type definitions for better type safetyAdding explicit type definitions for
campaignDetailsand the return type ingetTargetFileIdFromCampaignDetailswill enhance code maintainability and reduce potential runtime errors.
146-147: 🧹 Nitpick (assertive)Use template literals instead of string concatenation
Template literals improve readability and maintainability by embedding expressions within strings.
Apply this diff to refactor the code:
- "codesTargetMapping mapping :: " + - getFormattedStringForDebug(targetData) + `codesTargetMapping mapping :: ${getFormattedStringForDebug(targetData)}`Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 146-147: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
health-services/project-factory/src/server/api/genericApis.ts (1)
223-225: Verify the handling of missing code column in sheet data.If the column specified by
codeColumnName("Boundary Code") is not found in the sheet, the current implementation logs a warning and continues processing. Should this scenario be treated as an error instead? Continuing without the required column may lead to incorrect or incomplete data processing downstream.health-services/project-factory/src/server/utils/projectCampaignUtils.ts (1)
81-81: 🧹 Nitpick (assertive)Remove Unnecessary Type Annotation for
fetchCountOnlyThe type of
fetchCountOnlycan be inferred from its default value. It's redundant to declare it explicitly.Apply this diff to simplify the code:
- fetchCountOnly: boolean = false + fetchCountOnly = falseLikely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
health-services/project-factory/src/server/utils/projectCampaignUtils.ts
Show resolved
Hide resolved
…t-creation-entity-persistence
…t-creation-entity-persistence
…t-creation-entity-persistence
Update campaignEmployeesUtils.ts
Update excelUtils.ts
Summary by CodeRabbit
Release Notes
New Features
Database Changes
API Improvements
Performance Enhancements
Bug Fixes