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 26 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
355a8ab
Enabled commands for debug remotely for project factory pod (#1249)
ashish-egov Dec 9, 2024
2bdd736
changed package json for debug (#1258)
ashish-egov Dec 9, 2024
016dc01
boundary code in update flow to be persisted in db (#1259)
nitish-egov Dec 10, 2024
d96bb2b
chnaged header creation logic in generate flow (#1261)
nitish-egov Dec 10, 2024
19283e0
made few changes for fetching boundaries from boundariesCombined (#1263)
nitish-egov Dec 10, 2024
74700da
some boundary bulk and microplan user changes (#1268)
ashish-egov Dec 11, 2024
9dee69b
localisation-cache-fix (#1270)
ashish-egov Dec 11, 2024
3ed33e6
Microplan fix (#1269)
ashish-egov Dec 12, 2024
49b5801
Boundary fix message (#1279)
ashish-egov Dec 12, 2024
b373ac4
Facility Fix (#1282)
ashish-egov Dec 17, 2024
b69ba14
added gzip handler in middleware (#1290)
nitish-egov Dec 19, 2024
ad9a5b8
refactor target generate and validate for generic type (#1296)
nitish-egov Dec 30, 2024
f13f4cf
Facility Singular Creation (#1302)
ashish-egov Dec 31, 2024
9df4256
BUGFIX campaignUtils.ts for update target fix (#1304)
ashish-egov Jan 2, 2025
d832441
Allow facility capacity to be 0 in sheet uploads for microplan (#1303)
ashish-egov Jan 2, 2025
fc6122e
Implemented functionality to include all boundaries present in the fa…
ashish-egov Jan 2, 2025
bfccb0a
Optimized latitude and longitude validation logic for improved effici…
ashish-egov Jan 3, 2025
5420d8e
Implemented locale and campaign check (#1300)
ashish-egov Jan 3, 2025
bd673e7
refactored target tabs validate logic (#1330)
nitish-egov Jan 7, 2025
97b4b32
Update campaignUtils.ts (#1332)
ashish-egov Jan 8, 2025
b5be48e
added config for retry for resource creation completion (#1333)
nitish-egov Jan 9, 2025
396673c
refactored code for header localization (#1338)
nitish-egov Jan 13, 2025
3fc3a41
feat: Implement facility list with inactive by default and toggle act…
ashish-egov Jan 22, 2025
343393e
Console dropdown logics (#1359)
nitish-egov Jan 28, 2025
d94b380
added locale column in generated resource table for generating templa…
nitish-egov Jan 29, 2025
b5aee12
Created User issue fixed (#1370)
ashish-egov Jan 30, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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"
Comment on lines +44 to +45
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)

Make the facility code & type configurable.

Both facilityCodeColumn and facilityType are hardcoded. If there’s a need to adapt or extend these values in the future, consider sourcing them from environment variables instead of hardcoding. This ensures flexibility and easier maintenance.

},
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 +102 to +103
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)
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 improving readability of the boundary code logic.

The nested ternary operator could be simplified for better readability.

Consider this alternative:

- const boundaryCode = !request?.body?.CampaignDetails?.projectId
-   ? getRootBoundaryCode(request.body?.boundariesCombined)
-   : request?.body?.CampaignDetails?.boundaryCode ||
-   ExistingCampaignDetails?.boundaryCode;
+ const boundaryCode = request?.body?.CampaignDetails?.projectId
+   ? (request?.body?.CampaignDetails?.boundaryCode || ExistingCampaignDetails?.boundaryCode)
+   : getRootBoundaryCode(request.body?.boundariesCombined);

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

: 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.

🛠️ Refactor suggestion

Standardize equality operators.

Replace loose equality operators with strict equality for consistent type comparison:

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

- else if (type == 'boundaryManagement' || type === 'boundaryGeometryManagement') {
+ else if (type === 'boundaryManagement' || type === 'boundaryGeometryManagement') {

Also applies to: 387-387

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') {
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)

Unify equality checks for consistency.
You’re using both == and === in a single expression. Prefer === for consistency and avoidance of potential pitfalls with type coercion.

// 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))
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)

Dropdown initialization refetch logic.

Gracefully re-fetching dropdowns if none are found prevents unexpected null references. Consider adding error handling around this fallback.

}
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]}`);
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)

Be mindful of potential clutter in logs.
Logging large arrays or objects can clutter logs. Consider logging only relevant details or using debug-level logs when dealing with big data.

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)

Hidden columns logging.

While printing hidden columns can help debugging, consider logging structured data (like JSON) for better clarity.

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;
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 const for immutable variables.

The variable is never reassigned, use const instead of let:

- let receivedDropdowns = request.body?.dropdowns;
+ const receivedDropdowns = request.body?.dropdowns;
📝 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;
const receivedDropdowns = request.body?.dropdowns;
🧰 Tools
🪛 Biome (1.9.4)

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

'receivedDropdowns' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

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 +883 to +884
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

Improve type safety in request handling.

Add validation and type checking for the extracted type:

- const type = request?.query?.type || request?.body?.ResourceDetails?.type;
+ const type = request?.query?.type || request?.body?.ResourceDetails?.type;
+ if (!type) {
+   throwError("COMMON", 400, "VALIDATION_ERROR", "Type is required");
+ }
+ if (typeof type !== 'string') {
+   throwError("COMMON", 400, "VALIDATION_ERROR", "Type must be a string");
+ }

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

// 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);
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)

Modifying processed data.

Calling modifyProcessedSheetData allows post-processing. Confirm that the transformations match your updated schema definitions.

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)
}
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

Improve error object handling.

Add type checking and validation for error properties to ensure consistent error reporting:

 item.status = generatedResourceStatuses.failed,
 item.additionalDetails = {
   ...item.additionalDetails,
   error: {
-    status: error.status,
-    code: error.code,
-    description: error.description,
-    message: error.message
+    status: error.status || 500,
+    code: error.code || 'UNKNOWN_ERROR',
+    description: error.description || null,
+    message: typeof error.message === 'string' ? error.message : 'Unknown error occurred'
   }
 }

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

}
})
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) {
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 const instead of let.

The variable declared here is never reassigned. Switching to const clarifies its immutable nature.

- let d of data
+ const d of data
📝 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
for (let d of data) {
for (const d of data) {
🧰 Tools
🪛 Biome (1.9.4)

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

'd' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

const boundaryCode = d[d.length - 1]; // Assume last element is the boundary code

Comment on lines +1323 to +1328
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
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) {
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)

Add TypeScript type annotations for function parameters.

The function parameters lack type annotations, which could lead to runtime errors.

-function validateNationalDuplicacy(request: any, userMapping: any, localizationMap: any) {
+interface UserMappingType {
+  [phoneNumber: string]: Array<{
+    role?: string;
+    "!sheet#name!"?: string;
+    "!row#number!"?: number;
+  }>;
+}
+
+function validateNationalDuplicacy(
+  request: { body: { ResourceDetails: { status: string }, sheetErrorDetails?: any[] } },
+  userMapping: UserMappingType,
+  localizationMap: { [key: string]: string }
+) {

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_", "");
Comment on lines +104 to +107
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

Fix inconsistent role handling logic.

The role handling logic is inconsistent between the if and else blocks:

  • The if block removes the "ROOT_" prefix
  • The else block adds the "ROOT_" prefix
    This could lead to incorrect role validation.

Apply this diff to fix the logic:

  if (userRole?.startsWith("ROOT_")) {
    const trimmedRole = userRole.replace("ROOT_", "");
    // ... existing code ...
  } else {
-   const trimmedRole = userRole;
-   const errorMessage: any = `An user with ${getLocalizedName("ROOT_" + trimmedRole, localizationMap)} role can't be assigned to ${getLocalizedName(userRole, localizationMap)} role`;
+   const trimmedRole = userRole;
+   const errorMessage: any = `A user with ${getLocalizedName(trimmedRole, localizationMap)} role can't be assigned to ${getLocalizedName(userRole, localizationMap)} role`;
    // ... existing code ...
  }

Also applies to: 118-119


// 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
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ function getCreatedResourceIds(resources: any, type: any) {
const processedType = type === 'boundary'
? 'boundaryWithTarget'
: (type.includes('With') ? type.split('With')[0] : type);
return resources
.filter((item: any) => item.type === processedType)
.map((item: any) => item.createResourceId);
return resources
.filter((item: any) => item.type === processedType)
.map((item: any) => item.createResourceId);
}

function buildSearchCriteria(request: any, createdResourceId: any, type: any) {
Expand Down Expand Up @@ -76,23 +76,10 @@ async function fetchFileUrls(request: any, processedFileStoreIdForUSerOrFacility



function modifyProcessedSheetData(request: any, sheetData: any, localizationMap?: any) {
// const type = request?.query?.type || request?.body?.ResourceDetails?.type;
// const typeWithoutWith = type.includes('With') ? type.split('With')[0] : type;
function modifyProcessedSheetData(type: any, sheetData: any, schema: any, localizationMap?: any) {
if (!sheetData || sheetData.length === 0) return [];

// Find the row with the maximum number of keys
const maxLengthRow = sheetData.reduce((maxRow: any, row: any) => {
return Object.keys(row).length > Object.keys(maxRow).length ? row : maxRow;
}, {});

// Extract headers from the keys of the row with the maximum number of keys
const originalHeaders = Object.keys(maxLengthRow);
const statusIndex = originalHeaders.indexOf('#status#');
// Insert 'errordetails' after '#status#' if found
if (statusIndex !== -1) {
originalHeaders.splice(statusIndex + 1, 0, '#errorDetails#');
}
const originalHeaders = type === config?.facility?.facilityType ? [config?.facility?.facilityCodeColumn, ...schema?.columns] : schema?.columns;
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

Fix unsafe optional chaining to prevent potential TypeError

The use of optional chaining with config?.facility?.facilityType and schema?.columns could lead to a TypeError if any of the objects are undefined. Since you're accessing properties of these objects, ensure they are defined before use.

Apply this diff to safeguard against undefined objects:

+ if (!config || !config.facility || !schema || !schema.columns) {
+   throw new Error('Required configuration or schema is missing');
+ }
  const originalHeaders = type === config.facility.facilityType
    ? [config.facility.facilityCodeColumn, ...schema.columns]
    : schema.columns;

Alternatively, provide default values to prevent undefined access:

  const facilityType = config?.facility?.facilityType || 'defaultFacilityType';
  const facilityCodeColumn = config?.facility?.facilityCodeColumn || 'defaultFacilityCodeColumn';
  const schemaColumns = schema?.columns || [];
  const originalHeaders = type === facilityType
    ? [facilityCodeColumn, ...schemaColumns]
    : schemaColumns;

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

🧰 Tools
🪛 Biome (1.9.4)

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

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

(lint/correctness/noUnsafeOptionalChaining)


let localizedHeaders = getLocalizedHeaders(originalHeaders, localizationMap);

Expand Down
Loading
Loading