HCM Admin Console v0.3 Release code changes

[jagankumar-egov]

Summary by CodeRabbit

• New Features

  • Introduced new validation functions for microplan campaigns, including checks for phone numbers, emails, and names.
  • Added methods to manage ongoing campaign updates and handle microplan data.
  • Enhanced the campaign management controller with new routes for retrying campaigns and fetching from microplans.
  • Expanded the functionality of the transformAndCreateLocalisation function to include chunked uploads for improved efficiency.
  • Added a new ownership entry for project factory directory management.
  • Introduced several asynchronous functions for managing data integration with microplan services, including data manipulation in worksheets and fetching facility data.
  • Implemented a polling mechanism for downloading campaign templates and managing their generation.
  • Added new functions for validating microplan requests and enhancing error handling across various modules.
  • Added a new asynchronous function fetchProductVariants for retrieving product variant data in chunks.
  • Enhanced the Campaign Details Table by adding new columns parentId and active.
  • Removed the unique constraint on campaignName, allowing for greater flexibility in naming campaigns.
  • Introduced a new function consolidateBoundaries for managing boundary data.
  • Updated Dockerfile to use Node.js version 20 and set memory allocation options.

• Bug Fixes

  • Improved error handling and validation logic across various modules, ensuring data integrity and better user feedback.

• Documentation

  • Updated function signatures and added author annotations for better clarity and maintainability.

• Chores

  • Refactored existing functions to improve readability and structure, including changes to how project types and campaign details are processed.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between 175a995 and 0999d44.

📒 Files selected for processing (2)

• health-services/project-factory/package.json (2 hunks)
• health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts (1 hunks)

🧰 Additional context used

📓 Learnings (1)

health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts (2)

Learnt from: jagankumar-egov
PR: egovernments/health-campaign-services#1082
File: health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts:464-465
Timestamp: 2024-12-04T11:51:07.331Z
Learning: In the `health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts` file, the `throwError` function logs errors internally. Therefore, when catching errors and rethrowing using `throwError`, it's not necessary to add extra `logger.error` calls before invoking `throwError`.

Learnt from: jagankumar-egov
PR: egovernments/health-campaign-services#1082
File: health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts:498-499
Timestamp: 2024-12-04T11:51:01.303Z
Learning: In the `health-services/project-factory` codebase, the `throwError()` function already logs errors internally. Therefore, explicit error logging before calling `throwError()` is unnecessary.

🪛 Biome (1.9.4)

health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts

[error] 359-359: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

[error] 390-390: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

[error] 437-437: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

[error] 454-454: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

[error] 471-471: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

[error] 489-489: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

[error] 227-227: Reassigning a function parameter is confusing.

The parameter is declared here:

Use a local variable instead.

(lint/style/noParameterAssign)

---

[error] 238-238: Reassigning a function parameter is confusing.

The parameter is declared here:

Use a local variable instead.

(lint/style/noParameterAssign)

🔇 Additional comments (7)

health-services/project-factory/package.json (2)

3-3: LGTM: Version bump aligns with v0.3 release

The version increment from 0.2.0 to 0.3.0 follows semantic versioning and correctly reflects this being a minor release with new features.

---

29-29: 🧹 Nitpick (assertive)

Verify the impact of allowing minor version updates for exceljs

Changing from fixed version "4.4.0" to "^4.4.0" allows automatic minor version updates. This is inconsistent with other dependencies that use fixed versions and could potentially introduce compatibility issues.

Let's verify the latest versions and any breaking changes:

Consider maintaining consistency with other dependencies by either:

1. Keep using fixed version "4.4.0" like most other dependencies
2. Or document the rationale for allowing minor updates for this specific package

health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts (5)

27-28: 🛠️ Refactor suggestion

Remove redundant logger.error call before throwError

Since throwError already logs errors internally, the explicit call to logger.error at line 27 is unnecessary and can be removed to prevent duplicate logging.

---

227-227: 🛠️ Refactor suggestion

Avoid reassigning function parameters

Reassigning the function parameter newData can lead to confusion and unintended side effects. It's better to use a new variable for the transformed data.

Also applies to: 238-238

🧰 Tools

🪛 Biome (1.9.4)

[error] 227-227: Reassigning a function parameter is confusing.

The parameter is declared here:

Use a local variable instead.

(lint/style/noParameterAssign)

---

359-359: 🧹 Nitpick (assertive)

Use template literals instead of string concatenation

Using template literals improves readability and consistency in logging statements.

Apply these changes:

At line 359:

- logger.info("Project search params " + JSON.stringify(projectSearchParams))
+ logger.info(`Project search params ${JSON.stringify(projectSearchParams)}`)

At line 390:

- logger.info("Project search params " + JSON.stringify(projectSearchParams))
+ logger.info(`Project search params ${JSON.stringify(projectSearchParams)}`)

At line 454:

- logger.info("Project search params " + JSON.stringify(projectSearchParams))
+ logger.info(`Project search params ${JSON.stringify(projectSearchParams)}`)

At line 489:

- logger.info("Project search params " + JSON.stringify(projectSearchParams))
+ logger.info(`Project search params ${JSON.stringify(projectSearchParams)}`)

Also applies to: 390-390, 454-454, 489-489

🧰 Tools

🪛 Biome (1.9.4)

[error] 359-359: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

437-437: ⚠️ Potential issue

Ensure safe usage of optional chaining to prevent TypeErrors

Destructuring from a potentially undefined object can lead to a TypeError if the object is undefined. Add a check to ensure the object exists before destructuring.

Consider modifying the code as follows:

At line 437:

- const { tenantId } = request?.body?.parentCampaign || request?.parentCampaign;
+ const parentCampaign = request?.body?.parentCampaign || request?.parentCampaign;
+ if (parentCampaign) {
+   const { tenantId } = parentCampaign;
+   // Continue with logic using tenantId
+ } else {
+   // Handle the case when parentCampaign is undefined
+ }

At line 471:

- const { tenantId } = request?.body?.parentCampaign || request?.parentCampaign;
+ const parentCampaign = request?.body?.parentCampaign || request?.parentCampaign;
+ if (parentCampaign) {
+   const { tenantId } = parentCampaign;
+   // Continue with logic using tenantId
+ } else {
+   // Handle the case when parentCampaign is undefined
+ }

Also applies to: 471-471

🧰 Tools

🪛 Biome (1.9.4)

[error] 437-437: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

71-73: 🛠️ Refactor suggestion

Use throwError for consistent error handling

In the catch block, you're rethrowing the error using throw error;. To maintain consistent error handling across the application, consider using the throwError function.

Apply this diff:

} catch (error) {
  logger.error("Error fetching file URLs:", error);
- throw error;
+ throwError("FILE_FETCH", 500, "FILE_FETCH_ERROR", "Error fetching file URLs");
}

Likely invalid or redundant comment.

[coderabbitai]

Actionable comments posted: 5

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between 0999d44 and 406451e.

📒 Files selected for processing (5)

• .github/workflows/buildWorkbenchUI.yml (0 hunks)
• .github/workflows/publishAllPackages.yml (0 hunks)
• .github/workflows/publishAllPackagesRelease.yml (0 hunks)
• .github/workflows/publishProjectFactory.yml (1 hunks)
• health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts (1 hunks)

💤 Files with no reviewable changes (3)

• .github/workflows/publishAllPackagesRelease.yml
• .github/workflows/publishAllPackages.yml
• .github/workflows/buildWorkbenchUI.yml

🧰 Additional context used

📓 Learnings (1)

health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts (2)

Learnt from: jagankumar-egov
PR: egovernments/health-campaign-services#1082
File: health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts:464-465
Timestamp: 2024-12-04T11:51:07.331Z
Learning: In the `health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts` file, the `throwError` function logs errors internally. Therefore, when catching errors and rethrowing using `throwError`, it's not necessary to add extra `logger.error` calls before invoking `throwError`.

Learnt from: jagankumar-egov
PR: egovernments/health-campaign-services#1082
File: health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts:498-499
Timestamp: 2024-12-04T11:51:01.303Z
Learning: In the `health-services/project-factory` codebase, the `throwError()` function already logs errors internally. Therefore, explicit error logging before calling `throwError()` is unnecessary.

🪛 actionlint (1.7.4)

.github/workflows/publishProjectFactory.yml

21-21: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

---

28-28: shellcheck reported issue in this script: SC2086:info:1:46: Double quote to prevent globbing and word splitting

(shellcheck)

---

35-35: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions

(deprecated-commands)

---

47-47: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions

(deprecated-commands)

---

60-60: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

---

63-63: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🪛 yamllint (1.35.1)

.github/workflows/publishProjectFactory.yml

[warning] 3-3: truthy value should be one of [false, true]

(truthy)

---

[error] 5-5: trailing spaces

(trailing-spaces)

---

[error] 11-11: trailing spaces

(trailing-spaces)

🪛 Biome (1.9.4)

health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts

[error] 360-360: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

[error] 391-391: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

[error] 438-438: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

[error] 455-455: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

[error] 472-472: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

[error] 490-490: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

[error] 228-228: Reassigning a function parameter is confusing.

The parameter is declared here:

Use a local variable instead.

(lint/style/noParameterAssign)

---

[error] 239-239: Reassigning a function parameter is confusing.

The parameter is declared here:

Use a local variable instead.

(lint/style/noParameterAssign)

🔇 Additional comments (9)

health-services/project-factory/src/server/utils/onGoingCampaignUpdateUtils.ts (9)

27-28: Remove redundant logger.error call before throwError

Since the throwError function logs errors internally, the explicit call to logger.error at line 27 is unnecessary and can be removed to prevent duplicate logging.

---

321-322: Remove redundant logger.error call before throwError

Since throwError already logs errors internally, the logger.error(error); at line 321 is unnecessary and can be removed to prevent duplicate logging.

---

228-239: 🧹 Nitpick (assertive)

Avoid reassigning function parameter newData

Reassigning the function parameter newData can lead to confusion and unintended side effects. It's better to use a new local variable for transformations.

Consider the following changes:

At line 228:

- newData = newData.map((newRow: any) => {
+ const transformedData = newData.map((newRow: any) => {

At line 239:

- newData = newData.map((newRow: any, rowIndex: number) => {
+ const updatedData = transformedData.map((newRow: any, rowIndex: number) => {

Then, use updatedData in subsequent operations.

🧰 Tools

🪛 Biome (1.9.4)

[error] 228-228: Reassigning a function parameter is confusing.

The parameter is declared here:

Use a local variable instead.

(lint/style/noParameterAssign)

---

[error] 239-239: Reassigning a function parameter is confusing.

The parameter is declared here:

Use a local variable instead.

(lint/style/noParameterAssign)

---

360-360: 🧹 Nitpick (assertive)

Use template literals instead of string concatenation

At line 360, string concatenation is used in a logger statement. Using template literals improves readability and consistency.

Suggested change:

- logger.info("Project search params " + JSON.stringify(projectSearchParams))
+ logger.info(`Project search params ${JSON.stringify(projectSearchParams)}`)

🧰 Tools

🪛 Biome (1.9.4)

[error] 360-360: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

391-391: 🧹 Nitpick (assertive)

Use template literals instead of string concatenation

At line 391, string concatenation is used in a logger statement. Consider using template literals for consistency.

Suggested change:

- logger.info("Project search params " + JSON.stringify(projectSearchParams))
+ logger.info(`Project search params ${JSON.stringify(projectSearchParams)}`)

🧰 Tools

🪛 Biome (1.9.4)

[error] 391-391: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

438-438: ⚠️ Potential issue

Potential TypeError due to unsafe optional chaining

At line 438, destructuring tenantId from request?.body?.parentCampaign || request?.parentCampaign without checking if the result is defined can lead to a TypeError if both are undefined.

Consider adding a null check before destructuring:

const parentCampaign = request?.body?.parentCampaign || request?.parentCampaign;
if (parentCampaign) {
  const { tenantId } = parentCampaign;
  // Use tenantId as needed
} else {
  // Handle the case where parentCampaign is undefined
}

🧰 Tools

🪛 Biome (1.9.4)

[error] 438-438: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

472-472: ⚠️ Potential issue

Potential TypeError due to unsafe optional chaining

At line 472, destructuring tenantId from request?.body?.parentCampaign || request?.parentCampaign without verifying that the result is defined can cause a TypeError if both are undefined.

Consider modifying the code to ensure safety:

const parentCampaign = request?.body?.parentCampaign || request?.parentCampaign;
if (parentCampaign) {
  const { tenantId } = parentCampaign;
  // Proceed with tenantId
} else {
  // Handle the undefined case appropriately
}

🧰 Tools

🪛 Biome (1.9.4)

[error] 472-472: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

490-490: 🧹 Nitpick (assertive)

Use template literals instead of string concatenation

At line 490, string concatenation is used in a logger statement. For better readability, use template literals.

Suggested change:

- logger.info("Project search params " + JSON.stringify(projectSearchParams))
+ logger.info(`Project search params ${JSON.stringify(projectSearchParams)}`)

🧰 Tools

🪛 Biome (1.9.4)

[error] 490-490: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

---

71-73: 🧹 Nitpick (assertive)

Use throwError for consistent error handling

At line 73, the error is rethrown using throw error;. To maintain consistent error handling across the application, consider using throwError instead of directly throwing the error.

Suggested change:

- throw error;
+ throwError("FILE_FETCH", 500, "FILE_FETCH_ERROR", "Error fetching file URLs");

Ensure that this aligns with your application's error handling practices.

Likely invalid or redundant comment.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between 406451e and 33a103f.

📒 Files selected for processing (1)

• health-services/project-factory/src/server/utils/transforms/localisationMessageConstructor.ts (1 hunks)

🔇 Additional comments (4)

health-services/project-factory/src/server/utils/transforms/localisationMessageConstructor.ts (4)

15-17: Specify explicit types instead of any for function parameters

Using any undermines TypeScript's type safety features and can lead to runtime errors. Defining explicit types for function parameters enhances code reliability and maintainability.

---

58-59: Specify explicit types instead of any for function parameters

Using any undermines TypeScript's type safety features and can lead to runtime errors. Defining explicit types for function parameters enhances code reliability and maintainability.

---

34-34: ⚠️ Potential issue

Fix parameter order in boundaryMap.forEach callback

In the boundaryMap.forEach method, the callback parameters appear to be reversed. The forEach method for a Map provides (value, key) as arguments. Currently, code is being treated as the key and boundary as the value, which may lead to incorrect data processing.

Apply this diff to correct the parameter order:

-boundaryMap.forEach((code: string, boundary: any) => {
+boundaryMap.forEach((boundary: any, code: string) => {

---

111-116: ⚠️ Potential issue

Propagate upload failures to the caller

If any chunks fail to upload after exhausting retries, the current implementation logs a warning but does not inform the caller, potentially leading to silent data loss. It's crucial to notify the calling function of these failures so that appropriate action can be taken.

Apply this diff to propagate failures:

 const uploadInChunks = async (messages: any, chunkSize: any, tenantId: any, request: any) => {
+  let allSuccess = true;
   // Existing code...
       }
     if (!success) {
+      allSuccess = false;
       logger.warn(`Skipping chunk ${Math.floor(i / chunkSize) + 1} after exhausting retries`);
     }
   }
   logger.info("Finished processing all chunks");
+  if (!allSuccess) {
+    throw new Error("One or more chunks failed to upload");
+  }
 };

[kavi-egov]

approving based on jagan's feedback