Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console recent changes to merged in master #1274

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
4 changes: 1 addition & 3 deletions health-services/project-factory/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,4 @@ COPY . .
EXPOSE 3000

CMD ["yarn", "prod"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding DEBUG environment variable

According to the summary, the prod script's behavior depends on the DEBUG environment variable, but it's not defined in the Dockerfile.

Add the DEBUG environment variable definition:

 EXPOSE 3000
+
+# Set DEBUG environment variable (default to false for production)
+ENV DEBUG=false
 
 CMD ["yarn", "prod"]

Committable suggestion skipped: line range outside the PR's diff.

# Replaced by CMD ["yarn", "prod"]


# Replaced by CMD ["yarn", "prod"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Remove redundant comment

The comment about CMD replacement is redundant as Git history already tracks these changes. Consider removing this line for cleaner code.

-# Replaced by CMD ["yarn", "prod"]

4 changes: 2 additions & 2 deletions health-services/project-factory/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@
"build": "yarn run build-ts",
"build-ts": "tsc",
"clean": "rm -rf ./dist",
"serve": "node dist/index.js",
"serve": "if [ \"$DEBUG\" = \"true\" ]; then node --inspect=0.0.0.0:9229 dist/index.js; else node dist/index.js; fi",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security: Restrict debug port binding

The debug port is currently bound to all network interfaces (0.0.0.0), which could expose the debugger in production environments. Consider restricting it to localhost or specific interfaces.

Apply this change to improve security:

-    "serve": "if [ \"$DEBUG\" = \"true\" ]; then node --inspect=0.0.0.0:9229 dist/index.js; else node dist/index.js; fi",
+    "serve": "if [ \"$DEBUG\" = \"true\" ]; then node --inspect=127.0.0.1:9229 dist/index.js; else node dist/index.js; fi",
📝 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.

Suggested change
"serve": "if [ \"$DEBUG\" = \"true\" ]; then node --inspect=0.0.0.0:9229 dist/index.js; else node dist/index.js; fi",
"serve": "if [ \"$DEBUG\" = \"true\" ]; then node --inspect=127.0.0.1:9229 dist/index.js; else node dist/index.js; fi",

"start": "yarn run dev",
"test": "jest",
"dev": "ts-node-dev --respawn src/server/index.ts",
"prod": "yarn build && yarn serve",
"prod": "if [ \"$DEBUG\" = \"true\" ]; then cp tsconfig.debug.json tsconfig.json; fi && yarn build && yarn serve",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

⚠️ Potential issue

Reconsider debug configuration in production

The current implementation has several concerns:

  1. Copying debug configuration in production is unusual and could lead to unexpected behavior
  2. No validation if tsconfig.debug.json exists
  3. No cleanup of temporary configuration changes
  4. Could affect build artifacts

Consider:

  1. Using separate npm scripts for debug and production builds
  2. Implementing proper error handling for missing configuration files
  3. Cleaning up temporary configurations after use

Suggested refactor:

-    "prod": "if [ \"$DEBUG\" = \"true\" ]; then cp tsconfig.debug.json tsconfig.json; fi && yarn build && yarn serve",
+    "prod": "yarn build && yarn serve",
+    "debug": "[ -f tsconfig.debug.json ] && cp tsconfig.debug.json tsconfig.json && yarn build && yarn serve || echo 'Error: tsconfig.debug.json not found'",
📝 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.

Suggested change
"prod": "if [ \"$DEBUG\" = \"true\" ]; then cp tsconfig.debug.json tsconfig.json; fi && yarn build && yarn serve",
"prod": "yarn build && yarn serve",
"debug": "[ -f tsconfig.debug.json ] && cp tsconfig.debug.json tsconfig.json && yarn build && yarn serve || echo 'Error: tsconfig.debug.json not found'",

"watch-ts": "tsc --watch"
},
"repository": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1344,7 +1344,6 @@ async function performAndSaveResourceActivity(
createAndSearchConfig?.createBulkDetails?.createPath,
chunkData
);
creationTime = Date.now();
if (type == "facility" || type == "facilityMicroplan") {
await handeFacilityProcess(
request,
Expand Down
2 changes: 1 addition & 1 deletion health-services/project-factory/src/server/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class App {
extended: true,
})
);
this.app.use(bodyParser.json());
// this.app.use(bodyParser.json());
this.app.use(tracingMiddleware);
this.app.use(requestMiddleware);
this.app.use(errorLogger);
Expand Down
5 changes: 5 additions & 0 deletions health-services/project-factory/src/server/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ const config = {
},
facility: {
facilityTab: process.env.FACILITY_TAB_NAME || "HCM_ADMIN_CONSOLE_FACILITIES",
facilityCodeColumn : "HCM_ADMIN_CONSOLE_FACILITY_CODE",
facilityType : "facility"
},
user: {
userTab: process.env.USER_TAB_NAME || "HCM_ADMIN_CONSOLE_USER_LIST",
Expand Down Expand Up @@ -95,6 +97,8 @@ const config = {
defaultLocale: process.env.LOCALE || "en_MZ",
boundaryPrefix: "hcm-boundary",
localizationModule: process.env.LOCALIZATION_MODULE || "hcm-admin-schemas",
localizationWaitTimeInBoundaryCreation: parseInt(process.env.LOCALIZATION_WAIT_TIME_IN_BOUNDARY_CREATION || "30000"),
localizationChunkSizeForBoundaryCreation: parseInt(process.env.LOCALIZATION_CHUNK_SIZE_FOR_BOUNDARY_CREATION || "2000"),
Comment on lines +100 to +101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use Number.parseInt instead of global parseInt

For better consistency and to adhere to modern JavaScript standards, use Number.parseInt instead of the global parseInt function.

Apply this diff to update the code:

-        localizationWaitTimeInBoundaryCreation: parseInt(process.env.LOCALIZATION_WAIT_TIME_IN_BOUNDARY_CREATION || "30000"),
-        localizationChunkSizeForBoundaryCreation: parseInt(process.env.LOCALIZATION_CHUNK_SIZE_FOR_BOUNDARY_CREATION || "2000"),
+        localizationWaitTimeInBoundaryCreation: Number.parseInt(process.env.LOCALIZATION_WAIT_TIME_IN_BOUNDARY_CREATION || "30000"),
+        localizationChunkSizeForBoundaryCreation: Number.parseInt(process.env.LOCALIZATION_CHUNK_SIZE_FOR_BOUNDARY_CREATION || "2000"),
📝 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.

Suggested change
localizationWaitTimeInBoundaryCreation: parseInt(process.env.LOCALIZATION_WAIT_TIME_IN_BOUNDARY_CREATION || "30000"),
localizationChunkSizeForBoundaryCreation: parseInt(process.env.LOCALIZATION_CHUNK_SIZE_FOR_BOUNDARY_CREATION || "2000"),
localizationWaitTimeInBoundaryCreation: Number.parseInt(process.env.LOCALIZATION_WAIT_TIME_IN_BOUNDARY_CREATION || "30000"),
localizationChunkSizeForBoundaryCreation: Number.parseInt(process.env.LOCALIZATION_CHUNK_SIZE_FOR_BOUNDARY_CREATION || "2000"),
🧰 Tools
🪛 Biome (1.9.4)

[error] 100-100: 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] 101-101: 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)

},
// targetColumnsForSpecificCampaigns: {
// bedNetCampaignColumns: ["HCM_ADMIN_CONSOLE_TARGET"],
Expand Down Expand Up @@ -148,6 +152,7 @@ const config = {
localizationSearch: process.env.EGOV_LOCALIZATION_SEARCH || "localization/messages/v1/_search",
localizationCreate: "localization/messages/v1/_upsert",
projectTypeSearch: "project-factory/v1/project-type/search",
cacheBurst: process.env.CACHE_BURST || "localization/messages/cache-bust",
boundaryRelationshipCreate: "boundary-service/boundary-relationships/_create",
healthIndividualSearch: process.env.EGOV_HEALTH_INDIVIDUAL_SEARCH || "health-individual/v1/_search",
projectFacilitySearch: process.env.EGOV_HEALTH_PROJECT_FACILITY_SEARCH || "health-project/facility/v1/_search",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { logger } from "../../utils/logger";
import { httpRequest } from "../../utils/request";
import config from "../../config/index";
import { convertLocalisationResponseToMap } from "../../utils/localisationUtils";
import { defaultRequestInfo } from "../../api/coreApis";

let cachedResponse = {};

Expand Down Expand Up @@ -82,6 +83,20 @@ class Localisation {
);
cachedResponse = { ...this.cachedResponse };
};

// Calls the cache burst API of localization
public cacheBurst = async (
) => {
logger.info(`Calling localization cache burst api`);
const RequestInfo = defaultRequestInfo;
const requestBody = {
RequestInfo
}
await httpRequest(
this.localizationHost + config.paths.cacheBurst,
requestBody)
};


private checkCacheAndDeleteIfExists = (module: string, locale: "string") => {
logger.info(
Expand Down Expand Up @@ -132,6 +147,7 @@ class Localisation {
// Log and handle any errors that occur during the process
console.log(e);
logger.error(String(e));
throw new Error(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Rethrow the original error to preserve stack trace

Using throw new Error(e); replaces the original error's stack trace, making debugging difficult. Instead, rethrow the original error to maintain the stack trace and error message.

Apply this diff to rethrow the original error:

-          throw new Error(e);
+          throw e;

If additional context is needed, wrap the error while preserving the original error:

-          throw new Error(e);
+          throw new Error(`Failed to create localisation: ${e.message}`);
+          // Optionally, include the original error object
+          // throw new Error(`Failed to create localisation: ${e.message}`, { cause: e });

Committable suggestion skipped: line range outside the PR's diff.

}
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,7 @@ async function enrichAndPersistCampaignWithError(requestBody: any, error: any) {
requestBody.CampaignDetails.status = campaignStatuses?.failed;
// requestBody.CampaignDetails.isActive = false;
requestBody.CampaignDetails.boundaryCode =
getRootBoundaryCode(requestBody?.CampaignDetails?.boundaries) || null;
getRootBoundaryCode(requestBody?.boundariesCombined) || null;
requestBody.CampaignDetails.projectType =
requestBody?.CampaignDetails?.projectType || null;
requestBody.CampaignDetails.hierarchyType =
Expand Down Expand Up @@ -1035,7 +1035,7 @@ async function enrichAndPersistCampaignForCreate(
request.body.CampaignDetails.status =
action == "create" ? campaignStatuses.started : campaignStatuses.drafted;
request.body.CampaignDetails.boundaryCode = getRootBoundaryCode(
request.body.CampaignDetails.boundaries
request.body?.boundariesCombined
);
request.body.CampaignDetails.projectType =
request?.body?.CampaignDetails?.projectType || null;
Expand Down Expand Up @@ -1111,7 +1111,7 @@ async function enrichAndPersistCampaignForUpdate(
? campaignStatuses.started
: campaignStatuses.drafted;
const boundaryCode = !request?.body?.CampaignDetails?.projectId
? getRootBoundaryCode(request.body.CampaignDetails.boundaries)
? getRootBoundaryCode(request.body?.boundariesCombined)
: request?.body?.CampaignDetails?.boundaryCode ||
ExistingCampaignDetails?.boundaryCode;
request.body.CampaignDetails.boundaryCode = boundaryCode;
Expand Down Expand Up @@ -2414,6 +2414,7 @@ async function processAfterPersist(request: any, actionInUrl: any) {
localizationMap
);
}
delete request.body?.boundariesCombined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid using the delete operator on objects

Deleting properties from objects can negatively impact performance. Consider setting the property to undefined instead.

Apply this diff:

-        delete request.body?.boundariesCombined;
+        request.body.boundariesCombined = undefined;

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Biome (1.9.4)

[error] 2417-2417: Avoid the delete operator which can impact performance.

(lint/performance/noDelete)

} catch (error: any) {
console.log(error);
logger.error(error);
Expand Down
52 changes: 28 additions & 24 deletions health-services/project-factory/src/server/utils/genericUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ async function fullProcessFlowForNewEntry(newEntryResponse: any, generatedResour
const localizationMapModule = await getLocalizedMessagesHandler(request, request?.query?.tenantId);
const localizationMap = { ...localizationMapHierarchy, ...localizationMapModule };
let fileUrlResponse: any;
if(type != 'boundaryManagement' && request?.query?.campaignId != 'default' && type != 'boundaryGeometryManagement'){
if (type != 'boundaryManagement' && request?.query?.campaignId != 'default' && type != 'boundaryGeometryManagement') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Improve condition readability with early return pattern.

The nested condition checks can be simplified using early returns for better readability.

-    if (type != 'boundaryManagement' && request?.query?.campaignId != 'default' && type != 'boundaryGeometryManagement') {
+    if (type === 'boundaryManagement' || type === 'boundaryGeometryManagement' || request?.query?.campaignId === 'default') {
+      return;
+    }

Committable suggestion skipped: line range outside the PR's diff.

const responseFromCampaignSearch = await getCampaignSearchResponse(request);
const campaignObject = responseFromCampaignSearch?.CampaignDetails?.[0];
logger.info(`checks for parent campaign for type: ${type}`)
Expand Down Expand Up @@ -407,7 +407,7 @@ async function fullProcessFlowForNewEntry(newEntryResponse: any, generatedResour
await produceModifiedMessages(generatedResourceNew, updateGeneratedResourceTopic);
request.body.generatedResource = finalResponse;
}
else if (type == 'boundaryManagement' || type === 'boundaryGeometryManagement'){
else if (type == 'boundaryManagement' || type === 'boundaryGeometryManagement') {
// get boundary data from boundary relationship search api
logger.info("Generating Boundary Data")
const boundaryDataSheetGeneratedBeforeDifferentTabSeparation = await getBoundaryDataService(request, false);
Expand Down Expand Up @@ -601,10 +601,10 @@ function setAndFormatHeaders(worksheet: any, mainHeader: any, headerSet: any) {
async function createReadMeSheet(request: any, workbook: any, mainHeader: any, localizationMap = {}) {
const isSourceMicroplan = await isMicroplanRequest(request);
let readMeConfig: any;
if(isSourceMicroplan) {
if (isSourceMicroplan) {
readMeConfig = await getReadMeConfigForMicroplan(request);
}
else{
else {
readMeConfig = await getReadMeConfig(request);
}
const headerSet = new Set();
Expand Down Expand Up @@ -749,13 +749,13 @@ async function createFacilityAndBoundaryFile(facilitySheetData: any, boundaryShe
hideUniqueIdentifierColumn(facilitySheet, createAndSearch?.["facility"]?.uniqueIdentifierColumn);
changeFirstRowColumnColour(facilitySheet, 'E06666');

let receivedDropdowns=request.body?.dropdowns;
logger.info("started adding dropdowns in facility",JSON.stringify(receivedDropdowns))
let receivedDropdowns = request.body?.dropdowns;
logger.info("started adding dropdowns in facility", JSON.stringify(receivedDropdowns))

if(!receivedDropdowns||Object.keys(receivedDropdowns)?.length==0){
if (!receivedDropdowns || Object.keys(receivedDropdowns)?.length == 0) {
logger.info("No dropdowns found");
receivedDropdowns= setDropdownFromSchema(request,schema,localizationMap);
logger.info("refetched drodowns",JSON.stringify(receivedDropdowns))
receivedDropdowns = setDropdownFromSchema(request, schema, localizationMap);
logger.info("refetched drodowns", JSON.stringify(receivedDropdowns))
Comment on lines +752 to +758
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve error handling for dropdown initialization.

The code lacks proper error handling when setting dropdowns. If setDropdownFromSchema fails, it could lead to undefined behavior.

Add error handling:

  let receivedDropdowns = request.body?.dropdowns;
  logger.info("started adding dropdowns in facility", JSON.stringify(receivedDropdowns))
  if (!receivedDropdowns || Object.keys(receivedDropdowns)?.length == 0) {
    logger.info("No dropdowns found");
+   try {
      receivedDropdowns = setDropdownFromSchema(request, schema, localizationMap);
      logger.info("refetched drodowns", JSON.stringify(receivedDropdowns))
+   } catch (error) {
+     logger.error("Failed to set dropdowns:", error);
+     throwError("COMMON", 500, "DROPDOWN_INITIALIZATION_FAILED", error?.message);
+   }
  }
📝 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.

Suggested change
let receivedDropdowns = request.body?.dropdowns;
logger.info("started adding dropdowns in facility", JSON.stringify(receivedDropdowns))
if(!receivedDropdowns||Object.keys(receivedDropdowns)?.length==0){
if (!receivedDropdowns || Object.keys(receivedDropdowns)?.length == 0) {
logger.info("No dropdowns found");
receivedDropdowns= setDropdownFromSchema(request,schema,localizationMap);
logger.info("refetched drodowns",JSON.stringify(receivedDropdowns))
receivedDropdowns = setDropdownFromSchema(request, schema, localizationMap);
logger.info("refetched drodowns", JSON.stringify(receivedDropdowns))
let receivedDropdowns = request.body?.dropdowns;
logger.info("started adding dropdowns in facility", JSON.stringify(receivedDropdowns))
if (!receivedDropdowns || Object.keys(receivedDropdowns)?.length == 0) {
logger.info("No dropdowns found");
try {
receivedDropdowns = setDropdownFromSchema(request, schema, localizationMap);
logger.info("refetched drodowns", JSON.stringify(receivedDropdowns))
} catch (error) {
logger.error("Failed to set dropdowns:", error);
throwError("COMMON", 500, "DROPDOWN_INITIALIZATION_FAILED", error?.message);
}

}
await handledropdownthings(facilitySheet, receivedDropdowns);
await handleHiddenColumns(facilitySheet, request.body?.hiddenColumns);
Expand All @@ -777,7 +777,7 @@ async function handledropdownthings(sheet: any, dropdowns: any) {
logger.info("Dropdowns provided:", dropdowns);
for (const key of Object.keys(dropdowns)) {
if (dropdowns[key]) {
logger.info(`Processing dropdown key: ${key} with values: ${dropdowns[key]}`);
logger.info(`Processing dropdown key: ${key} with values: ${dropdowns[key]}`);
const firstRow = sheet.getRow(1);
firstRow.eachCell({ includeEmpty: true }, (cell: any, colNumber: any) => {
if (cell.value === key) {
Expand Down Expand Up @@ -815,7 +815,7 @@ async function handledropdownthings(sheet: any, dropdowns: any) {

async function handleHiddenColumns(sheet: any, hiddenColumns: any) {
// logger.info(sheet)
logger.info("hiddenColumns",hiddenColumns);
logger.info("hiddenColumns", hiddenColumns);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Improve logging with structured format.

The logging statement could be more informative with structured data.

- logger.info("hiddenColumns", hiddenColumns);
+ logger.info("Processing hidden columns:", { count: hiddenColumns?.length, columns: hiddenColumns });
📝 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.

Suggested change
logger.info("hiddenColumns", hiddenColumns);
logger.info("Processing hidden columns:", { count: hiddenColumns?.length, columns: hiddenColumns });

if (hiddenColumns) {
for (const columnName of hiddenColumns) {
const firstRow = sheet.getRow(1);
Expand Down Expand Up @@ -849,13 +849,13 @@ async function createUserAndBoundaryFile(userSheetData: any, boundarySheetData:
addDataToSheet(request, userSheet, userSheetData, undefined, undefined, true, false, localizationMap, fileUrl, schema);
hideUniqueIdentifierColumn(userSheet, createAndSearch?.["user"]?.uniqueIdentifierColumn);

let receivedDropdowns=request.body?.dropdowns;
logger.info("started adding dropdowns in user",JSON.stringify(receivedDropdowns))
let receivedDropdowns = request.body?.dropdowns;
logger.info("started adding dropdowns in user", JSON.stringify(receivedDropdowns))

if(!receivedDropdowns||Object.keys(receivedDropdowns)?.length==0){
if (!receivedDropdowns || Object.keys(receivedDropdowns)?.length == 0) {
logger.info("No dropdowns found");
receivedDropdowns= setDropdownFromSchema(request,schema,localizationMap);
logger.info("refetched drodowns",JSON.stringify(receivedDropdowns))
receivedDropdowns = setDropdownFromSchema(request, schema, localizationMap);
logger.info("refetched drodowns", JSON.stringify(receivedDropdowns))
}
await handledropdownthings(userSheet, receivedDropdowns);
await handleHiddenColumns(userSheet, request.body?.hiddenColumns);
Expand All @@ -870,6 +870,8 @@ async function createUserAndBoundaryFile(userSheetData: any, boundarySheetData:


async function generateFacilityAndBoundarySheet(tenantId: string, request: any, localizationMap?: { [key: string]: string }, filteredBoundary?: any, fileUrl?: any) {
const type = request?.query?.type || request?.body?.ResourceDetails?.type;
const typeWithoutWith = type.includes('With') ? type.split('With')[0] : type;
Comment on lines +873 to +874
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Standardize type extraction logic.

The type extraction logic is duplicated across functions. Consider extracting it into a helper function.

Create a helper function:

+ function extractTypeFromRequest(request: any): string {
+   return request?.query?.type || request?.body?.ResourceDetails?.type;
+ }

- const type = request?.query?.type || request?.body?.ResourceDetails?.type;
+ const type = extractTypeFromRequest(request);

Also applies to: 908-909

// Get facility and boundary data
logger.info("Generating facilities started");
const allFacilities = await getAllFacilities(tenantId, request.body);
Expand All @@ -881,10 +883,10 @@ async function generateFacilityAndBoundarySheet(tenantId: string, request: any,
if (fileUrl) {
/* fetch facility from processed file
and generate facility sheet data */
schema = await callMdmsTypeSchema(request, tenantId, true, typeWithoutWith, "all");
const processedFacilitySheetData = await getSheetData(fileUrl, localizedFacilityTab, false, undefined, localizationMap);
const modifiedProcessedFacilitySheetData = modifyProcessedSheetData(request, processedFacilitySheetData, localizationMap);
const modifiedProcessedFacilitySheetData = modifyProcessedSheetData(typeWithoutWith, processedFacilitySheetData, schema, localizationMap);
Comment on lines +886 to +888
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Improve schema handling consistency.

The schema handling logic is similar in both functions but with slight variations. Consider unifying the approach.

Create a unified schema handler:

+ async function getSchemaForType(request: any, tenantId: string, isUpdate: boolean, type: string): Promise<any> {
+   return await callMdmsTypeSchema(request, tenantId, isUpdate, type, "all");
+ }

Also applies to: 911-912

facilitySheetData = modifiedProcessedFacilitySheetData;
schema = await callMdmsTypeSchema(request, tenantId, true, "facility", "all");
setDropdownFromSchema(request, schema, localizationMap);
}
else {
Expand All @@ -903,9 +905,11 @@ async function generateFacilityAndBoundarySheet(tenantId: string, request: any,

async function generateUserSheet(request: any, localizationMap?: { [key: string]: string }, filteredBoundary?: any, userData?: any, fileUrl?: any) {
const tenantId = request?.query?.tenantId;
const type = request?.query?.type || request?.body?.ResourceDetails?.type;
const typeWithoutWith = type.includes('With') ? type.split('With')[0] : type;
let schema: any;
const isUpdate = fileUrl ? true : false;
schema = await callMdmsTypeSchema(request, tenantId, isUpdate, "user");
schema = await callMdmsTypeSchema(request, tenantId, isUpdate, typeWithoutWith);
setDropdownFromSchema(request, schema, localizationMap);
const headers = schema?.columns;
const localizedHeaders = getLocalizedHeaders(headers, localizationMap);
Expand All @@ -917,7 +921,7 @@ async function generateUserSheet(request: any, localizationMap?: { [key: string]
/* fetch facility from processed file
and generate facility sheet data */
const processedUserSheetData = await getSheetData(fileUrl, localizedUserTab, false, undefined, localizationMap);
const modifiedProcessedUserSheetData = modifyProcessedSheetData(request, processedUserSheetData, localizationMap);
const modifiedProcessedUserSheetData = modifyProcessedSheetData(typeWithoutWith, processedUserSheetData, schema, localizationMap);
userSheetData = modifiedProcessedUserSheetData;
}
else {
Expand Down Expand Up @@ -1075,7 +1079,7 @@ async function handleGenerateError(newEntryResponse: any, generatedResource: any
code: error.code,
description: error.description,
message: error.message
} || String(error)
}
}
})
generatedResource = { generatedResource: newEntryResponse };
Expand Down Expand Up @@ -1303,12 +1307,12 @@ async function getDataSheetReady(boundaryData: any, request: any, localizationMa
mappedRowData[boundaryCodeIndex] = boundaryCode;
return mappedRowData;
});
if(type == "boundaryManagement"){
if (type == "boundaryManagement") {
logger.info("Processing data for boundaryManagement type")
const latLongBoundaryMap = await getLatLongMapForBoundaryCodes(request, boundaryCodeList);
for (let d of data) {
const boundaryCode = d[d.length - 1]; // Assume last element is the boundary code

Comment on lines +1310 to +1315
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve type safety in boundary management processing.

The boundary code access assumes array structure without validation.

Add validation:

  if (type == "boundaryManagement") {
    logger.info("Processing data for boundaryManagement type")
    const latLongBoundaryMap = await getLatLongMapForBoundaryCodes(request, boundaryCodeList);
    for (let d of data) {
+     if (!Array.isArray(d) || d.length === 0) {
+       logger.error("Invalid data structure for boundary management");
+       continue;
+     }
      const boundaryCode = d[d.length - 1];
      if (latLongBoundaryMap[boundaryCode]) {
        const [latitude = null, longitude = null] = latLongBoundaryMap[boundaryCode];
        d.push(latitude);
        d.push(longitude);
      }
    }
  }
📝 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.

Suggested change
if (type == "boundaryManagement") {
logger.info("Processing data for boundaryManagement type")
const latLongBoundaryMap = await getLatLongMapForBoundaryCodes(request, boundaryCodeList);
for (let d of data) {
const boundaryCode = d[d.length - 1]; // Assume last element is the boundary code
if (type == "boundaryManagement") {
logger.info("Processing data for boundaryManagement type")
const latLongBoundaryMap = await getLatLongMapForBoundaryCodes(request, boundaryCodeList);
for (let d of data) {
if (!Array.isArray(d) || d.length === 0) {
logger.error("Invalid data structure for boundary management");
continue;
}
const boundaryCode = d[d.length - 1]; // Assume last element is the boundary code
🧰 Tools
🪛 Biome (1.9.4)

[error] 1313-1313: This let declares a variable that is only assigned once.

'd' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

if (latLongBoundaryMap[boundaryCode]) {
const [latitude = null, longitude = null] = latLongBoundaryMap[boundaryCode]; // Destructure lat/long
d.push(latitude); // Append latitude
Expand Down Expand Up @@ -1359,7 +1363,7 @@ function modifyDataBasedOnDifferentTab(boundaryData: any, differentTabsBasedOnLe
async function getLocalizedMessagesHandler(request: any, tenantId: any, module = config.localisation.localizationModule, overrideCache = false) {
const localisationcontroller = Localisation.getInstance();
const locale = getLocaleFromRequest(request);
const localizationResponse = await localisationcontroller.getLocalisedData(module, locale, tenantId,overrideCache);
const localizationResponse = await localisationcontroller.getLocalisedData(module, locale, tenantId, overrideCache);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider caching optimization for localization.

The localization data fetching could benefit from caching to improve performance.

Consider implementing a caching mechanism for frequently accessed localization data to reduce database load and improve response times. This could be achieved using the existing appCache utility or a dedicated localization cache.

return localizationResponse;
}

Expand Down
36 changes: 36 additions & 0 deletions health-services/project-factory/src/server/utils/gzipHandler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { Request } from "express";
import * as zlib from "zlib";
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Update Node.js import to use the node: protocol

For better clarity and explicit Node.js module identification, update the zlib import.

-import * as zlib from "zlib";
+import * as zlib from "node:zlib";
📝 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.

Suggested change
import { Request } from "express";
import * as zlib from "zlib";
import { Request } from "express";
import * as zlib from "node:zlib";
🧰 Tools
🪛 Biome (1.9.4)

[error] 2-2: A Node.js builtin module should be imported with the node: protocol.

Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.

(lint/style/useNodejsImportProtocol)


export const handleGzipRequest = async (req: Request): Promise<void> => {
const buffers: Buffer[] = [];

// Collect data chunks from the request
await new Promise<void>((resolve, reject) => {
req.on("data", (chunk: any) => buffers.push(chunk));
req.on("end", resolve);
req.on("error", reject);
});

// Concatenate and decompress the data
const gzipBuffer = Buffer.concat(buffers);
try {
const decompressedData = await decompressGzip(gzipBuffer);
req.body = decompressedData; // Assign the parsed data to req.body
} catch (err: any) {
throw new Error(`Failed to process Gzip data: ${err.message}`);
}
};
Comment on lines +4 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add memory safeguards for buffer collection

The current implementation collects all chunks without size limits, which could lead to memory issues with large requests.

Add a size limit check:

 export const handleGzipRequest = async (req: Request): Promise<void> => {
     const buffers: Buffer[] = [];
+    let totalSize = 0;
+    const MAX_SIZE = 10 * 1024 * 1024; // 10MB limit
 
     // Collect data chunks from the request
     await new Promise<void>((resolve, reject) => {
-        req.on("data", (chunk: any) => buffers.push(chunk));
+        req.on("data", (chunk: Buffer) => {
+            totalSize += chunk.length;
+            if (totalSize > MAX_SIZE) {
+                reject(new Error("Request payload too large"));
+                return;
+            }
+            buffers.push(chunk);
+        });
         req.on("end", resolve);
         req.on("error", reject);
     });
📝 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.

Suggested change
export const handleGzipRequest = async (req: Request): Promise<void> => {
const buffers: Buffer[] = [];
// Collect data chunks from the request
await new Promise<void>((resolve, reject) => {
req.on("data", (chunk: any) => buffers.push(chunk));
req.on("end", resolve);
req.on("error", reject);
});
// Concatenate and decompress the data
const gzipBuffer = Buffer.concat(buffers);
try {
const decompressedData = await decompressGzip(gzipBuffer);
req.body = decompressedData; // Assign the parsed data to req.body
} catch (err: any) {
throw new Error(`Failed to process Gzip data: ${err.message}`);
}
};
export const handleGzipRequest = async (req: Request): Promise<void> => {
const buffers: Buffer[] = [];
let totalSize = 0;
const MAX_SIZE = 10 * 1024 * 1024; // 10MB limit
// Collect data chunks from the request
await new Promise<void>((resolve, reject) => {
req.on("data", (chunk: Buffer) => {
totalSize += chunk.length;
if (totalSize > MAX_SIZE) {
reject(new Error("Request payload too large"));
return;
}
buffers.push(chunk);
});
req.on("end", resolve);
req.on("error", reject);
});
// Concatenate and decompress the data
const gzipBuffer = Buffer.concat(buffers);
try {
const decompressedData = await decompressGzip(gzipBuffer);
req.body = decompressedData; // Assign the parsed data to req.body
} catch (err: any) {
throw new Error(`Failed to process Gzip data: ${err.message}`);
}
};


// Helper function to decompress Gzip data
const decompressGzip = (gzipBuffer: Buffer): Promise<any> => {
return new Promise((resolve, reject) => {
zlib.gunzip(gzipBuffer, (err, result) => {
if (err) return reject(err);
try {
resolve(JSON.parse(result.toString()));
} catch (parseErr) {
reject(new Error("Invalid JSON format in decompressed data"));
}
});
});
};
Comment on lines +24 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling and type safety in decompressGzip

The current implementation could be improved with more specific error handling and type safety.

-const decompressGzip = (gzipBuffer: Buffer): Promise<any> => {
+const decompressGzip = (gzipBuffer: Buffer): Promise<unknown> => {
     return new Promise((resolve, reject) => {
         zlib.gunzip(gzipBuffer, (err, result) => {
-            if (err) return reject(err);
+            if (err) {
+                return reject(new Error(`Gzip decompression failed: ${err.message}`));
+            }
             try {
-                resolve(JSON.parse(result.toString()));
+                const parsed = JSON.parse(result.toString('utf-8'));
+                if (typeof parsed !== 'object' || parsed === null) {
+                    throw new Error('Decompressed content must be a JSON object');
+                }
+                resolve(parsed);
             } catch (parseErr) {
-                reject(new Error("Invalid JSON format in decompressed data"));
+                reject(new Error(`Invalid JSON format: ${parseErr.message}`));
             }
         });
     });
 };
📝 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.

Suggested change
// Helper function to decompress Gzip data
const decompressGzip = (gzipBuffer: Buffer): Promise<any> => {
return new Promise((resolve, reject) => {
zlib.gunzip(gzipBuffer, (err, result) => {
if (err) return reject(err);
try {
resolve(JSON.parse(result.toString()));
} catch (parseErr) {
reject(new Error("Invalid JSON format in decompressed data"));
}
});
});
};
// Helper function to decompress Gzip data
const decompressGzip = (gzipBuffer: Buffer): Promise<unknown> => {
return new Promise((resolve, reject) => {
zlib.gunzip(gzipBuffer, (err, result) => {
if (err) {
return reject(new Error(`Gzip decompression failed: ${err.message}`));
}
try {
const parsed = JSON.parse(result.toString('utf-8'));
if (typeof parsed !== 'object' || parsed === null) {
throw new Error('Decompressed content must be a JSON object');
}
resolve(parsed);
} catch (parseErr) {
reject(new Error(`Invalid JSON format: ${parseErr.message}`));
}
});
});
};

Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,8 @@ export async function getUserDataFromMicroplanSheet(request: any, fileStoreId: a
export function getAllUserData(request: any, userMapping: any, localizationMap: any) {
const emailKey = getLocalizedName("HCM_ADMIN_CONSOLE_USER_EMAIL_MICROPLAN", localizationMap);
const nameKey = getLocalizedName("HCM_ADMIN_CONSOLE_USER_NAME_MICROPLAN", localizationMap);
const phoneNumberKey = getLocalizedName("HCM_ADMIN_CONSOLE_USER_PHONE_NUMBER_MICROPLAN", localizationMap);
validateInConsistency(request, userMapping, emailKey, nameKey);
validateNationalDuplicacy(request, userMapping, phoneNumberKey);
validateNationalDuplicacy(request, userMapping, localizationMap);
const dataToCreate: any = [];
for (const phoneNumber of Object.keys(userMapping)) {
const roles = userMapping[phoneNumber].map((user: any) => user.role).join(',');
Expand Down Expand Up @@ -94,30 +93,30 @@ function validateInConsistency(request: any, userMapping: any, emailKey: any, na
request.body.sheetErrorDetails = Array.isArray(request.body.sheetErrorDetails) ? [...request.body.sheetErrorDetails, ...overallInconsistencies] : overallInconsistencies;
}

function validateNationalDuplicacy(request: any, userMapping: any, phoneNumberKey: any) {
function validateNationalDuplicacy(request: any, userMapping: any, localizationMap: any) {
const duplicates: any[] = [];

for (const phoneNumber in userMapping) {
const roleMap: any = {};
const users = userMapping[phoneNumber];

for (const user of users) {
if (user.role?.startsWith("Root ")) {
const userRole = user?.role;
if (userRole?.startsWith("ROOT_")) {
// Trim the role
const trimmedRole = user.role.replace("Root ", "").trim().toLowerCase();
const trimmedRoleWithCapital = trimmedRole.charAt(0).toUpperCase() + trimmedRole.slice(1);
const trimmedRole = userRole.replace("ROOT_", "");

// Check for duplicates in the roleMap
if (roleMap[trimmedRole] && roleMap[trimmedRole]["!sheet#name!"] != user["!sheet#name!"]) {
const errorMessage: any = `An user with ${trimmedRoleWithCapital} role can’t be assigned to ${user.role} role`;
const errorMessage: any = `An user with ${getLocalizedName(trimmedRole, localizationMap)} role can’t be assigned to ${getLocalizedName(userRole, localizationMap)} role`;
duplicates.push({ rowNumber: user["!row#number!"], sheetName: user["!sheet#name!"], status: "INVALID", errorDetails: errorMessage });
} else {
roleMap[trimmedRole] = user;
}
}
else {
const trimmedRole = user.role.toLowerCase();
const errorMessage: any = `An user with ${"Root " + trimmedRole} role can’t be assigned to ${user.role} role`;
const trimmedRole = userRole;
const errorMessage: any = `An user with ${getLocalizedName("ROOT_" + trimmedRole, localizationMap)} role can’t be assigned to ${getLocalizedName(userRole, localizationMap)} role`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Use template literals and correct the grammatical error in the error message

To enhance readability and maintain consistency, prefer using template literals over string concatenation.

Also, replace 'An user' with 'A user' for grammatical correctness in the error message.

Apply this diff to implement the changes:

- const errorMessage: any = `An user with ${getLocalizedName("ROOT_" + trimmedRole, localizationMap)} role can’t be assigned to ${getLocalizedName(userRole, localizationMap)} role`;
+ const errorMessage: any = `A user with ${getLocalizedName(`ROOT_${trimmedRole}`, localizationMap)} role can’t be assigned to ${getLocalizedName(userRole, localizationMap)} role`;
📝 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.

Suggested change
const errorMessage: any = `An user with ${getLocalizedName("ROOT_" + trimmedRole, localizationMap)} role cant be assigned to ${getLocalizedName(userRole, localizationMap)} role`;
const errorMessage: any = `A user with ${getLocalizedName(`ROOT_${trimmedRole}`, localizationMap)} role can't be assigned to ${getLocalizedName(userRole, localizationMap)} role`;
🧰 Tools
🪛 Biome (1.9.4)

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

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

if (roleMap[trimmedRole] && roleMap[trimmedRole]["!sheet#name!"] != user["!sheet#name!"]) {
duplicates.push({ rowNumber: user["!row#number!"], sheetName: user["!sheet#name!"], status: "INVALID", errorDetails: errorMessage });
} else {
Expand Down
Loading
Loading