HCMPRE-700, HCMPRE-701 and HCMPRE-741 #1471
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to several files within the campaign manager module and related components. Key changes involve updating external CSS stylesheet links in HTML files, refactoring functions in JavaScript components, and introducing a new hook for product variant search. Additionally, several functions have been commented out or removed, indicating a shift in how data is processed and managed within the application. The overall structure of the components remains largely intact, with a focus on enhancing data handling and user interactions. Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range comments (2)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
Line range hint
12-30: Consider optimizing CSS imports and removing commented codeThere are multiple CSS stylesheets being loaded, and some are commented out. This could potentially lead to styling conflicts or unexpected behavior. Additionally, keeping commented-out code in production files is generally not recommended.
Consider the following suggestions:
- Review and consolidate the CSS imports to ensure only the necessary stylesheets are loaded.
- Remove commented-out code if it's no longer needed, or add clear comments explaining why certain lines are kept for future reference.
- If different stylesheet versions are being tested, consider using a more systematic approach for managing these variations, such as feature flags or environment-specific configurations.
Would you like assistance in optimizing these imports or cleaning up the commented code?
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (1)
Line range hint
41-46: Consider increasing the minimum length for product names.The validation now allows product names as short as 1 character. This might lead to less meaningful or potentially confusing product names. Consider increasing the minimum length to ensure more descriptive product names.
-if (i?.name?.length >= 1 && i?.name?.length < 101) { +if (i?.name?.length >= 3 && i?.name?.length < 101) {This change would require product names to be at least 3 characters long, which is more likely to result in meaningful names while still allowing flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (13)
- health/micro-ui/web/micro-ui-internals/example/public/index.html (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (0 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (7 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1 hunks)
- health/micro-ui/web/public/index.html (1 hunks)
💤 Files with no reviewable changes (1)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
🧰 Additional context used
📓 Path-based instructions (10)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js
[error] 286-286: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 294-294: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js
[error] 15-15: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (23)
health/micro-ui/web/micro-ui-internals/example/public/index.html (3)
Line range hint
32-32: Verify global configuration loadingThe file loads a global configuration script using an environment variable (%REACT_APP_GLOBAL%).
While using environment variables for configuration is a good practice, please ensure that:
- The correct configuration is loaded in each environment (development, staging, production).
- Sensitive information is not exposed through this global configuration.
Consider running the following script to check the usage of this global configuration:
#!/bin/bash # Description: Check usage of global configuration # Test: Search for usage of globalConfigs or window.globalConfigs rg --type javascript --type typescript -g '!node_modules' -i '(globalConfigs|window\.globalConfigs)' # Test: Look for any TODO or FIXME comments related to configuration rg --type javascript --type typescript -g '!node_modules' -i '(TODO|FIXME).*config'
21-21: Confirm the update of digit-ui-components-css versionThe version of
digit-ui-components-csshas been updated from 0.0.2-beta.34 to 0.0.2-beta.36.While this is a smaller version jump compared to the previous change, please ensure that this update is intentional and compatible with the rest of the application. Consider running the following script to check for any potential issues:
#!/bin/bash # Description: Check for potential issues with digit-ui-components-css update # Test: Search for custom component styles that might be affected by the update rg --type css -g '!node_modules' -i 'digit-ui-components' # Test: Look for any component-related TODO or FIXME comments rg --type html --type javascript --type typescript -g '!node_modules' -i '(TODO|FIXME).*component'
17-17: Verify the intentional update of digit-ui-css versionThe version of
digit-ui-csshas been updated from 1.8.2-beta.11 to 1.8.2-beta.34. This is a significant jump within the same minor version.Please confirm that this update is intentional and that it doesn't introduce any breaking changes or unexpected behavior in the UI. Consider running the following script to check for any potential issues:
health/micro-ui/web/public/index.html (1)
11-12: 🧹 Nitpick (assertive)CSS library versions updated - verify visual consistency
The changes update two CSS libraries to newer versions:
- @egovernments/digit-ui-css: 1.8.2-beta.11 → 1.8.2-beta.34
- @egovernments/digit-ui-components-css: 0.0.2-beta.34 → 0.0.2-beta.36
While these updates likely include improvements and bug fixes, please ensure that:
- The application's visual appearance and functionality remain consistent after these updates.
- You've reviewed the changelogs for these libraries to understand any breaking changes or new features.
To verify the impact of these changes, please run the following commands:
Consider the following improvements:
- Add integrity hashes to the link tags for enhanced security. For example:
<link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css@1.8.2-beta.34/dist/index.css" integrity="sha384-[calculated-hash]" crossorigin="anonymous">- Host these CSS files locally to reduce dependency on external CDNs and improve load reliability.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (3)
Line range hint
51-56: Approved: Improved validation for product variants.The change to require at least 2 characters for product variants is a good improvement. It ensures more meaningful variant names while maintaining the upper limit of 100 characters.
89-96: No additional comments needed.This segment has been addressed in the previous comment about efficient handling of unique product-variant combinations.
90-96: No additional comments needed.This segment has been addressed in the previous comments about efficient handling of unique product-variant combinations.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
Line range hint
123-123: Verify usage of CreateQuestionContextThe mapping of
CreateQuestiontoCreateQuestionContextin thecomponentsToRegisterobject looks good. This change suggests a refactoring of theCreateQuestioncomponent, possibly to use React's Context API.To ensure this change doesn't introduce any issues, please verify that all usages of
CreateQuestionin the application are compatible with the newCreateQuestionContext. Run the following script to check for any potential issues:Review the results to ensure that all usages are consistent with this change.
✅ Verification successful
Verification Successful: CreateQuestionContext Mapping
The mapping of
CreateQuestiontoCreateQuestionContexthas been successfully verified. All usages are consistent and compatible with the new context mapping.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of CreateQuestion and CreateQuestionContext # Search for CreateQuestion usages echo "Searching for CreateQuestion usages:" rg --type js "CreateQuestion" -g '!Module.js' # Search for CreateQuestionContext usages echo "Searching for CreateQuestionContext usages:" rg --type js "CreateQuestionContext" -g '!Module.js'Length of output: 3863
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (3)
77-77:⚠️ Potential issueVerify potential off-by-one error when decrementing
toValue.At line 77, you subtract 1 from
toValue:toValue: toValue ? toValue - 1 : null,Subtracting 1 from
toValuemay introduce an off-by-one error, depending on the intended logical conditions. Ensure that this decrement aligns with the business logic and that it does not exclude the intended upper boundary value.Suggested Action:
Double-check the requirement for the
toValue. If the condition should include themaxValue, you might not need to subtract 1. If the subtraction is intentional, consider adding a comment to explain why.
135-144:⚠️ Potential issueEnsure consistent attribute configuration keys across functions.
In
generateBednetConfig, the attribute configuration objects do not includefromValueandtoValuekeys, unlike the objects ingenerateMRDNConfig. This inconsistency may lead to issues when consuming these configurations elsewhere in your application.Suggested Fix:
Include
fromValueandtoValuein the attribute configurations withingenerateBednetConfig, even if they arenullor unused. This ensures a consistent data structure.return { key: index + 1 + subIndex, label: "Custom", attrType: criteria.attrType, attrValue: value?.variable, operatorValue: operatorValue, value: value?.comparisonValue, + fromValue: fromValue, + toValue: toValue, };Likely invalid or redundant comment.
153-163:⚠️ Potential issueHandle asynchronous functions correctly in
deliveryConfig.If
generateMRDNConfigandgenerateBednetConfigbecome asynchronous due to the use ofawait, thedeliveryConfigfunction should also be marked asasync. Additionally, ensure that all callers ofdeliveryConfighandle the returned Promise appropriately.Suggested Fix:
Mark
deliveryConfigasasync:- const deliveryConfig = ({ data, projectType }) => { + const deliveryConfig = async ({ data, projectType }) => {Update the switch cases to await the asynchronous functions:
switch (projectType) { case "MR-DN": - return generateMRDNConfig(data?.cycles?.[0]?.deliveries, projectType); + return await generateMRDNConfig(data?.cycles?.[0]?.deliveries, projectType); case "LLIN-mz": case "IRS-mz": - return generateBednetConfig(data?.cycles?.[0]?.deliveries, projectType); + return await generateBednetConfig(data?.cycles?.[0]?.deliveries, projectType); default: return []; }Ensure that any function calling
deliveryConfighandles it as a Promise.Likely invalid or redundant comment.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (1)
6-6: Import statement forgetDeliveryConfigis correctThe new import statement correctly brings in the
getDeliveryConfigutility function.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (2)
77-77: LGTM!The return statement correctly spreads the object
iand returns it. The code is clear and functions as intended.
88-125: Ensure commenting outreverseDeliveryRemapdoes not affect functionalityThe entire logic of the
reverseDeliveryRemapfunction has been commented out, and it now simply returns the inputdata. If other parts of the application rely on the processing done within this function, this change could lead to unexpected behavior or bugs.Run the following script to identify all usages of
reverseDeliveryRemapand assess the impact:This will help you determine if the function can be safely altered or if the original logic needs to be preserved.
✅ Verification successful
Verified:
reverseDeliveryRemapis not used in active codeAfter reviewing the codebase, all usages of
reverseDeliveryRemapare currently commented out. This ensures that commenting out the function does not impact the application's functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of 'reverseDeliveryRemap' in the codebase to verify its usage. # Expected: List all places where 'reverseDeliveryRemap' is invoked. rg --type js 'reverseDeliveryRemap' -A 5 -B 5Length of output: 11858
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (2)
5-5: Verify the import path forgetDeliveryConfigPlease ensure that the
getDeliveryConfigmodule exists at the specified path"../../../utils/getDeliveryConfig"and that it exports the required function correctly. This helps prevent import errors at runtime.
26-27: Confirm the MDMS module name and data structureThe module name
"HCM-PROJECT-TYPES"is used in theuseCustomMDMShook. Please verify that this is the correct MDMS module name and that it contains theprojectTypesdata as expected.Run the following script to check the available MDMS modules and their contents:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (7)
177-222: Remove obsolete commented-out functionsThe
restructureDatafunction and its helper functions are commented out from lines 177 to 222. If these functions are no longer required, consider deleting them to maintain a clean codebase.
433-434: EnsuredeliveryRuleis in the expected format after removingreverseDeliveryRemapBy directly assigning
deliveryRule: deliveryon line 434, verify that thedeliveryobject structure matches what is expected downstream. Previously,reverseDeliveryRemapmight have transformeddeliveryinto the required format. Confirm that all parts of the application usingdeliveryRulecan handle the new structure without issues.
500-573: Clean up commented-outrestructureDatafunctionThe
restructureDatafunction, along with its internal logic, is commented out from lines 500 to 573. If this code is no longer necessary, removing it will enhance code clarity.
614-616: Verify the structure ofpayloadData.deliveryRulesafter bypassingrestructureDataOn line 616,
payloadData.deliveryRulesis assigned directly fromtotalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule. Ensure that this data now aligns with what the backend API expects, asrestructureDatawas previously used to format it appropriately.
686-688: ConfirmdeliveryRulesformat is acceptable for the APIAt line 688, check that the direct assignment of
payloadData.deliveryRuleswithout restructuring still conforms to the API's expected data schema. This is crucial to prevent potential data processing errors on the server side.
751-753: EnsuredeliveryRulesmeet backend requirements after changesBy directly assigning
payloadData.deliveryRuleson line 753, confirm that the data meets all necessary backend requirements, especially since the transformation viarestructureDatais no longer applied.
815-817: CheckdeliveryRulesconsistency with expected data structureOn line 817, since
restructureDatais no longer used, verify thatpayloadData.deliveryRuleshas the correct structure and includes all required fields as per the backend's expectations.
| @@ -61,6 +61,7 @@ const CampaignModule = ({ stateCode, userType, tenants }) => { | |||
| }); | |||
|
|
|||
| const hierarchyData = Digit.Hooks.campaign.useBoundaryRelationshipSearch({BOUNDARY_HIERARCHY_TYPE,tenantId}); | |||
| // const modulePrefix = "hcm"; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing or explaining commented-out code
There are two instances of commented-out code related to modulePrefix:
- Line 64:
// const modulePrefix = "hcm"; - Line 73:
// modulePrefix,
Commented-out code can lead to confusion and make the codebase harder to maintain. If this code is no longer needed, consider removing it entirely. If it's kept for future use, add a TODO comment explaining why it's there and when it might be needed.
If you decide to keep the code, consider adding a comment like this:
// TODO: modulePrefix might be needed for future implementation of X feature. Keeping it commented out until then.
// const modulePrefix = "hcm";Also applies to: 73-73
| const temp = rowsData.map((i) => { | ||
| if (i?.operator?.code === "IN_BETWEEN") { | ||
| return { | ||
| ...i, | ||
| value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`, | ||
| operator: t(i?.operator?.code) || t(i?.operator), | ||
| attribute: i?.attribute?.code || i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`) : "", | ||
| }; | ||
| } | ||
| else{ | ||
| return{ | ||
| ...i, | ||
| operator: t(i?.operator), | ||
| attribute: i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.toUpperCase()}`) : "", | ||
| })); | ||
| operator: t(i?.operator?.code) || t(i?.operator), | ||
| attribute: i?.attribute?.code | ||
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute.code.toUpperCase()}`) | ||
| : i?.attribute | ||
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute}`) | ||
| : "", | ||
| } | ||
| }}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor mapping function to reduce duplication and improve readability
The current mapping function contains duplicated code and complex conditional logic. Refactoring can enhance readability and maintainability.
Consider the following refactored version:
const temp = rowsData.map((i) => {
const attribute = i?.attribute?.code
? t(`CAMPAIGN_ATTRIBUTE_${i.attribute.code.toUpperCase()}`)
: i?.attribute
? t(`CAMPAIGN_ATTRIBUTE_${i.attribute}`)
: "";
const operator = t(i?.operator?.code) || t(i?.operator);
const value =
i?.operator?.code === "IN_BETWEEN"
? `${i?.fromValue ? i?.fromValue : "N/A"} to ${i?.toValue ? i?.toValue : "N/A"}`
: i?.value;
return {
...i,
attribute,
operator,
value,
};
});| attribute: i?.attribute?.code | ||
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute.code.toUpperCase()}`) | ||
| : i?.attribute | ||
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute}`) | ||
| : "", |
There was a problem hiding this comment.
Ensure safe optional chaining for 'attribute' properties
To prevent potential runtime errors when i.attribute or i.attribute.code is undefined, ensure that optional chaining is consistently used.
Suggested change:
attribute: i?.attribute?.code
- ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute.code.toUpperCase()}`)
+ ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`)
: i?.attribute
- ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute}`)
+ ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute}`)
: "",📝 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.
| attribute: i?.attribute?.code | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute.code.toUpperCase()}`) | |
| : i?.attribute | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute}`) | |
| : "", | |
| attribute: i?.attribute?.code | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`) | |
| : i?.attribute | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute}`) | |
| : "", |
| if (i?.operator?.code === "IN_BETWEEN") { | ||
| return { | ||
| ...i, | ||
| value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`, |
There was a problem hiding this comment.
Reverse 'fromValue' and 'toValue' in the range display
In the value assignment, the 'toValue' and 'fromValue' appear to be reversed. Typically, a range is displayed as 'fromValue to toValue'. Consider swapping them for correct representation.
Suggested change:
- value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`,
+ value: `${i?.fromValue ? i?.fromValue : "N/A"} to ${i?.toValue ? i?.toValue : "N/A"}`,📝 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.
| value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`, | |
| value: `${i?.fromValue ? i?.fromValue : "N/A"} to ${i?.toValue ? i?.toValue : "N/A"}`, |
| ...i, | ||
| value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`, | ||
| operator: t(i?.operator?.code) || t(i?.operator), | ||
| attribute: i?.attribute?.code || i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`) : "", |
There was a problem hiding this comment.
Correct logical error in 'attribute' field assignment
The current logic for the 'attribute' field may not work as intended due to operator precedence. The expression could always evaluate to t(...), even when i?.attribute?.code is undefined.
Suggested change:
- attribute: i?.attribute?.code || i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`) : "",
+ attribute: i?.attribute?.code
+ ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`)
+ : i?.attribute
+ ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute}`)
+ : "",📝 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.
| attribute: i?.attribute?.code || i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`) : "", | |
| attribute: i?.attribute?.code | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`) | |
| : i?.attribute | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute}`) | |
| : "", |
| // function loopAndReturn(dataa, t) { | ||
| // let newArray = []; | ||
| // const data = dataa?.map((i) => ({ ...i, operator: i?.operator, attribute: i?.attribute })); | ||
|
|
||
| data.forEach((item) => { | ||
| // Check if an object with the same attribute already exists in the newArray | ||
| const existingIndex = newArray.findIndex((element) => element.attribute === item.attribute); | ||
| if (existingIndex !== -1) { | ||
| // If an existing item is found, replace it with the new object | ||
| const existingItem = newArray[existingIndex]; | ||
| newArray[existingIndex] = { | ||
| attribute: existingItem.attribute, | ||
| operator: "IN_BETWEEN", | ||
| toValue: existingItem.value && item.value ? Math.min(existingItem.value, item.value) : null, | ||
| fromValue: existingItem.value && item.value ? Math.max(existingItem.value, item.value) : null, | ||
| }; | ||
| } else if (item?.operator === "EQUAL_TO") { | ||
| newArray.push({ | ||
| ...item, | ||
| value: item?.value ? t(item?.value) : null, | ||
| }); | ||
| } else { | ||
| newArray.push(item); | ||
| } | ||
| }); | ||
| // data.forEach((item) => { | ||
| // // Check if an object with the same attribute already exists in the newArray | ||
| // const existingIndex = newArray.findIndex((element) => element.attribute === item.attribute); | ||
| // if (existingIndex !== -1) { | ||
| // // If an existing item is found, replace it with the new object | ||
| // const existingItem = newArray[existingIndex]; | ||
| // newArray[existingIndex] = { | ||
| // attribute: existingItem.attribute, | ||
| // operator: "IN_BETWEEN", | ||
| // toValue: existingItem.value && item.value ? Math.min(existingItem.value, item.value) : null, | ||
| // fromValue: existingItem.value && item.value ? Math.max(existingItem.value, item.value) : null, | ||
| // }; | ||
| // } else if (item?.operator === "EQUAL_TO") { | ||
| // newArray.push({ | ||
| // ...item, | ||
| // value: item?.value ? t(item?.value) : null, | ||
| // }); | ||
| // } else { | ||
| // newArray.push(item); | ||
| // } | ||
| // }); | ||
|
|
||
| const withKey = newArray.map((i, c) => ({ key: c + 1, ...i })); | ||
| const format = withKey.map((i) => { | ||
| if (i.operator === "IN_BETWEEN") { | ||
| return { | ||
| ...i, | ||
| value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`, | ||
| }; | ||
| } | ||
| return { | ||
| ...i, | ||
| }; | ||
| }); | ||
| return format; | ||
| } | ||
| // const withKey = newArray.map((i, c) => ({ key: c + 1, ...i })); | ||
| // const format = withKey.map((i) => { | ||
| // if (i.operator === "IN_BETWEEN") { | ||
| // return { | ||
| // ...i, | ||
| // value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`, | ||
| // }; | ||
| // } | ||
| // return { | ||
| // ...i, | ||
| // }; | ||
| // }); | ||
| // return format; | ||
| // } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing the commented-out function loopAndReturn
The function loopAndReturn has been commented out. If this function is no longer needed, consider removing it to keep the codebase clean and maintainable.
💡 Codebase verification
References to loopAndReturn and reverseDeliveryRemap exist in multiple files, which may lead to runtime errors since these functions are commented out. Please review and remove or update these references accordingly:
src/pages/employee/SetupCampaign.jssrc/components/DeliveryDetailsSummary.jssrc/components/CampaignSummary.js
🔗 Analysis chain
Verify there are no remaining references to the commented-out functions
Since loopAndReturn and reverseDeliveryRemap have been commented out, ensure that there are no remaining references to these functions elsewhere in the codebase to prevent potential runtime errors.
Run the following script to check for any references:
Also applies to: 82-123
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to `loopAndReturn` and `reverseDeliveryRemap` in the codebase.
# Test: Find all occurrences of `loopAndReturn` and `reverseDeliveryRemap` excluding commented lines.
# Expect: No matches found.
rg '^[^/\n]*\b(loopAndReturn|reverseDeliveryRemap)\b' --type js --type jsx --type ts --type tsx --glob '!DataUploadSummary.js'
Length of output: 156
Script:
#!/bin/bash
# Description: Search for references to `loopAndReturn` and `reverseDeliveryRemap` in the codebase.
# Test: Find all occurrences of `loopAndReturn` and `reverseDeliveryRemap` excluding commented lines.
# Expect: No matches found.
rg '^[^/\n]*\b(loopAndReturn|reverseDeliveryRemap)\b' \
-g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx' \
--glob '!DataUploadSummary.js'
Length of output: 1362
| // function reverseDeliveryRemap(data, t) { | ||
| // if (!data) return null; | ||
| // const reversedData = []; | ||
| // let currentCycleIndex = null; | ||
| // let currentCycle = null; | ||
|
|
||
| data.forEach((item, index) => { | ||
| if (currentCycleIndex !== item.cycleNumber) { | ||
| currentCycleIndex = item.cycleNumber; | ||
| currentCycle = { | ||
| cycleIndex: currentCycleIndex.toString(), | ||
| startDate: item?.startDate ? Digit.Utils.date.convertEpochToDate(item?.startDate) : null, | ||
| endDate: item?.endDate ? Digit.Utils.date.convertEpochToDate(item?.endDate) : null, | ||
| active: index === 0, // Initialize active to false | ||
| deliveries: [], | ||
| }; | ||
| reversedData.push(currentCycle); | ||
| } | ||
| // data.forEach((item, index) => { | ||
| // if (currentCycleIndex !== item.cycleNumber) { | ||
| // currentCycleIndex = item.cycleNumber; | ||
| // currentCycle = { | ||
| // cycleIndex: currentCycleIndex.toString(), | ||
| // startDate: item?.startDate ? Digit.Utils.date.convertEpochToDate(item?.startDate) : null, | ||
| // endDate: item?.endDate ? Digit.Utils.date.convertEpochToDate(item?.endDate) : null, | ||
| // active: index === 0, // Initialize active to false | ||
| // deliveries: [], | ||
| // }; | ||
| // reversedData.push(currentCycle); | ||
| // } | ||
|
|
||
| const deliveryIndex = item.deliveryNumber.toString(); | ||
| // const deliveryIndex = item.deliveryNumber.toString(); | ||
|
|
||
| let delivery = currentCycle.deliveries.find((delivery) => delivery.deliveryIndex === deliveryIndex); | ||
| // let delivery = currentCycle.deliveries.find((delivery) => delivery.deliveryIndex === deliveryIndex); | ||
|
|
||
| if (!delivery) { | ||
| delivery = { | ||
| deliveryIndex: deliveryIndex, | ||
| active: item.deliveryNumber === 1, // Set active to true only for the first delivery | ||
| deliveryRules: [], | ||
| }; | ||
| currentCycle.deliveries.push(delivery); | ||
| } | ||
| // if (!delivery) { | ||
| // delivery = { | ||
| // deliveryIndex: deliveryIndex, | ||
| // active: item.deliveryNumber === 1, // Set active to true only for the first delivery | ||
| // deliveryRules: [], | ||
| // }; | ||
| // currentCycle.deliveries.push(delivery); | ||
| // } | ||
|
|
||
| delivery.deliveryRules.push({ | ||
| ruleKey: item.deliveryRuleNumber, | ||
| delivery: {}, | ||
| attributes: loopAndReturn(item.conditions, t), | ||
| products: [...item.products], | ||
| }); | ||
| }); | ||
| // delivery.deliveryRules.push({ | ||
| // ruleKey: item.deliveryRuleNumber, | ||
| // delivery: {}, | ||
| // attributes: loopAndReturn(item.conditions, t), | ||
| // products: [...item.products], | ||
| // }); | ||
| // }); | ||
|
|
||
| return reversedData; | ||
| } | ||
| // return reversedData; | ||
| // } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing the commented-out function reverseDeliveryRemap
The function reverseDeliveryRemap has been commented out. If it's no longer required, removing it can improve code readability and reduce clutter.
| const temp= getDeliveryConfig({data: data?.["HCM-PROJECT-TYPES"], projectType:selectedProjectType}); | ||
| return temp; |
There was a problem hiding this comment.
Correct the data access path in getDeliveryConfig
The data being passed to getDeliveryConfig is accessed as data?.["HCM-PROJECT-TYPES"]. However, the MDMS response typically contains data under the MdmsRes key. To ensure the correct data is retrieved, please adjust the access path.
Apply this diff to fix the data access:
- const temp= getDeliveryConfig({data: data?.["HCM-PROJECT-TYPES"], projectType:selectedProjectType});
+ const temp= getDeliveryConfig({data: data?.MdmsRes?.["HCM-PROJECT-TYPES"], projectType: selectedProjectType});📝 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.
| const temp= getDeliveryConfig({data: data?.["HCM-PROJECT-TYPES"], projectType:selectedProjectType}); | |
| return temp; | |
| const temp= getDeliveryConfig({data: data?.MdmsRes?.["HCM-PROJECT-TYPES"], projectType: selectedProjectType}); | |
| return temp; |
| // function reverseDeliveryRemap(data) { | ||
| // if (!data) return null; | ||
| // const reversedData = []; | ||
| // let currentCycleIndex = null; | ||
| // let currentCycle = null; | ||
|
|
||
| const deliveryIndex = item.deliveryNumber.toString(); | ||
| // data.forEach((item, index) => { | ||
| // if (currentCycleIndex !== item.cycleNumber) { | ||
| // currentCycleIndex = item.cycleNumber; | ||
| // currentCycle = { | ||
| // cycleIndex: currentCycleIndex.toString(), | ||
| // active: index === 0, // Initialize active to false | ||
| // deliveries: [], | ||
| // }; | ||
| // reversedData.push(currentCycle); | ||
| // } | ||
|
|
||
| let delivery = currentCycle.deliveries.find((delivery) => delivery.deliveryIndex === deliveryIndex); | ||
| // const deliveryIndex = item.deliveryNumber.toString(); | ||
|
|
||
| if (!delivery) { | ||
| delivery = { | ||
| deliveryIndex: deliveryIndex, | ||
| active: item.deliveryNumber === 1, // Set active to true only for the first delivery | ||
| deliveryRules: [], | ||
| }; | ||
| currentCycle.deliveries.push(delivery); | ||
| } | ||
| // let delivery = currentCycle.deliveries.find((delivery) => delivery.deliveryIndex === deliveryIndex); | ||
|
|
||
| delivery.deliveryRules.push({ | ||
| ruleKey: item.deliveryRuleNumber, | ||
| delivery: {}, | ||
| deliveryType: item?.deliveryType, | ||
| attributes: loopAndReturn(item.conditions), | ||
| products: [...item.products], | ||
| }); | ||
| }); | ||
| // if (!delivery) { | ||
| // delivery = { | ||
| // deliveryIndex: deliveryIndex, | ||
| // active: item.deliveryNumber === 1, // Set active to true only for the first delivery | ||
| // deliveryRules: [], | ||
| // }; | ||
| // currentCycle.deliveries.push(delivery); | ||
| // } | ||
|
|
||
| // delivery.deliveryRules.push({ | ||
| // ruleKey: item.deliveryRuleNumber, | ||
| // delivery: {}, | ||
| // deliveryType: item?.deliveryType, | ||
| // attributes: loopAndReturn(item.conditions), | ||
| // products: [...item.products], | ||
| // }); | ||
| // }); | ||
|
|
||
| // return reversedData; | ||
| // } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing commented-out code for better maintainability
The large block of commented-out code from lines 134 to 175 (reverseDeliveryRemap function) can be removed to improve code readability and maintainability. If this code is no longer needed, it's advisable to remove it from the codebase to keep it clean and avoid confusion.
| // useEffect(async () => { | ||
| // if (totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule) { | ||
| // const temp = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule); | ||
| // } | ||
| // }, [shouldUpdate]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing unused useEffect hook
The useEffect hook commented out from lines 580 to 584 is no longer in use. To improve code readability and maintainability, consider removing it if it's not needed.
Bhavya-egov
force-pushed
the
HCMPRE-701
branch
from
a44eb1e to
25e91b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (13)
- health/micro-ui/web/micro-ui-internals/example/public/index.html (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (0 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (7 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1 hunks)
- health/micro-ui/web/public/index.html (1 hunks)
💤 Files with no reviewable changes (1)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
🧰 Additional context used
📓 Path-based instructions (10)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js
[error] 286-286: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 294-294: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js
[error] 15-15: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (22)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js (1)
1-1: LGTM: Function signature and structure are well-defined.The hook is correctly exported as a named constant and uses an async arrow function, which is appropriate for handling API calls. The destructuring of input parameters is a clean approach.
health/micro-ui/web/micro-ui-internals/example/public/index.html (3)
17-17: LGTM! Verify compatibility with the new CSS version.The update to the newer version of
digit-ui-css(1.8.2-beta.34) is approved. This change likely introduces new features or bug fixes.Please ensure that this new version is compatible with your current implementation and doesn't introduce any unexpected visual changes or breaks in the UI. Consider running the following command to check for any potential issues:
#!/bin/bash # Description: Check for potential CSS-related issues after updating digit-ui-css version # Test: Search for any TODO comments or console.log statements related to CSS issues rg --type js --type jsx -i "(TODO|FIXME|XXX|console\.log).*css" ./src
17-20: Summary: CSS stylesheet versions updatedBoth
digit-ui-cssanddigit-ui-components-csshave been updated to newer versions. These changes are likely to introduce improvements, new features, or bug fixes to the UI and components.To ensure these updates don't introduce any unexpected issues, please:
- Test the application thoroughly, paying special attention to UI elements and component styling.
- Review the changelogs for both
digit-ui-cssanddigit-ui-components-cssto understand what changes have been introduced in these new versions.- Run the following command to check for any recent CSS-related changes in your codebase that might be affected by these updates:
#!/bin/bash # Description: Check for recent CSS-related changes in the codebase # Test: List CSS files modified in the last 30 days git log --name-only --since="30 days ago" --pretty=format: | sort -u | grep '\.css$'
20-20: LGTM! Verify compatibility with the new components CSS version.The update to the newer version of
digit-ui-components-css(0.0.2-beta.36) is approved. This minor version bump likely includes small improvements or bug fixes.Please ensure that this new version is compatible with your current implementation and doesn't introduce any unexpected changes in component styling. Consider running the following command to check for any potential issues:
health/micro-ui/web/public/index.html (1)
11-12: 🧹 Nitpick (assertive)Verify compatibility with updated CSS versions
The stylesheet versions have been updated:
- @egovernments/digit-ui-css: 1.8.2-beta.11 → 1.8.2-beta.34
- @egovernments/digit-ui-components-css: 0.0.2-beta.34 → 0.0.2-beta.36
While these updates likely include improvements and bug fixes, please ensure:
- The application's UI remains consistent with these new versions.
- There are no unintended side effects or breaking changes introduced.
To verify the impact of these changes, you can run the following script:
Consider consolidating the different versions of @egovernments/digit-ui-css being used (1.8.0-alpha.6, 1.8.2-beta.34, 1.0.75-campaign, 1.0.50-microplan) to maintain consistency and reduce potential conflicts.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (4)
Line range hint
1-185: Overall assessment: Changes improve data handling with minor adjustments to validation.The modifications to this file enhance data integrity by preventing duplicate processing of product-variant combinations. The relaxed validation rules for product names and variants provide more flexibility but should be verified against business requirements. Consider the suggested refactoring to further improve code readability. Overall, the changes appear to be well-implemented and align with the intended objectives.
87-96: 🧹 Nitpick (assertive)Efficient handling of unique product-variant combinations: Consider refactoring.
The introduction of
usedCombinationsSet effectively prevents duplicate processing of product-variant combinations, improving data integrity. This is a good improvement.As suggested in a previous review, consider extracting the combination check into a separate function for improved readability:
const isUniqueCombination = (name, variant) => { const combination = `${name}-${variant}`; if (!usedCombinations.has(combination)) { usedCombinations.add(combination); return true; } return false; }; // Then use it in the find function: const target = formData?.["addProduct"]?.find((f) => f.name === i.name && isUniqueCombination(f.name, f.variant) );This refactoring would improve code readability and maintainability.
Line range hint
51-56: Product variant validation adjusted: Verify business requirements.The minimum length requirement for product variants has been slightly relaxed, now allowing 2-character variants. Ensure this aligns with your business requirements and doesn't conflict with any existing variant naming conventions.
Let's check for consistency across the codebase:
#!/bin/bash # Search for other product variant validations rg --type js 'variant.*length.*(?:>=|>).*2.*&&.*variant.*length.*<.*101' -g '!AddProduct.js'
Line range hint
41-46: Product name validation relaxed: Consider implications.The minimum length requirement for product names has been reduced from 3 characters to 1. While this provides more flexibility, it may lead to less descriptive product names. Consider if this aligns with your product naming conventions and user expectations.
To ensure consistency, let's check if this change is reflected in other parts of the codebase:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
65-65: LGTM: Improved code formattingThe reformatting of the
hierarchyDatavariable assignment improves readability without changing functionality.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (1)
14-33: Previous review comments remain applicableThe issues and suggestions highlighted in the previous reviews for these lines are still valid. Please refer to them to address the outstanding concerns.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1)
77-77:⚠️ Potential issueVerify potential off-by-one error in
toValuecalculation.In the line:
toValue: toValue ? toValue - 1 : null,Subtracting 1 from
toValuemay introduce an off-by-one error if the intention is to include thetoValuein the range. Please verify whether this subtraction is necessary and aligns with the business logic.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (2)
6-6: ImportgetDeliveryConfigadded correctlyThe import statement for
getDeliveryConfigis properly added and aligns with its usage in the code.
78-79: MDMS module and entity names updated appropriatelyThe MDMS module name is updated to
"HCM-PROJECT-TYPES"and the entity name to"projectTypes", which is consistent with the expected data structure.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (1)
126-126: Verify the structure ofcycleDataafter changesSince
reverseDeliveryRemapnow returns the unmodifieddata, ensure thatcycleData(assigned at line 267) has the expected structure for subsequent operations. The code later maps overcycleDatato render cycle components, which may rely on a specific data format produced by the originalreverseDeliveryRemapfunction. Verify that this change doesn't introduce runtime errors or affect the rendering of cycles.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (3)
5-5: Import statement is correctThe
getDeliveryConfigmodule is correctly imported. This will help in fetching the delivery configuration based on the project type.
30-31: Data access path may be incorrectThe MDMS response data is typically nested under the
MdmsReskey. Currently, the code accesses data usingdata?.["HCM-PROJECT-TYPES"]. To ensure accurate data retrieval, consider updating the access path to includeMdmsRes.
26-27: Verify the MDMS module and master namesEnsure that the MDMS module name
"HCM-PROJECT-TYPES"and the master[ { name: "projectTypes" } ]are correct and exist in the MDMS configuration.Run the following script to confirm the existence of the module and master:
This script searches the
mdms-datadirectory for MDMS definitions matching the provided module and master names.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (2)
134-175: Reminder: Remove the commented-outreverseDeliveryRemapfunctionThe
reverseDeliveryRemapfunction from lines 134 to 175 is still commented out and not in use. To maintain code cleanliness and readability, consider removing this block if it's no longer needed.
580-584: Reminder: Remove the commented-outuseEffecthookThe
useEffecthook from lines 580 to 584 remains commented out and unused. To improve code readability, consider removing it if it's no longer necessary.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js (2)
41-80: No issues with commenting outloopAndReturnThe function
loopAndReturnhas been commented out. If this is intentional and temporary, it's acceptable. Ensure that any references to this function are removed or handled appropriately to prevent potential runtime errors.
82-123: No issues with commenting outreverseDeliveryRemapThe function
reverseDeliveryRemaphas been commented out. If this is intentional and temporary, it's acceptable. Make sure there are no remaining references to this function elsewhere in the codebase that could cause issues.
| const data = await Digit.CustomService.getResponse({ | ||
| url: `/product/variant/v1/_search`, | ||
| params: { | ||
| tenantId: tenantId, | ||
| limit: 100, | ||
| offset: 0 | ||
| }, | ||
| body: { | ||
| ProductVariant: { | ||
| id: [variantId], | ||
| }, | ||
| }, | ||
| config: { | ||
| enabled: variantId ? true : false, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding pagination support and error handling.
The API call configuration is generally correct, but there are two areas for improvement:
-
Pagination: The current implementation uses a fixed limit of 100 and an offset of 0. Consider making these configurable to support pagination for larger datasets.
-
Error Handling: Add a try-catch block to handle potential API errors gracefully.
Here's a suggested refactor addressing these points:
export const useProductVariantSearch = async ({ variantId, tenantId, limit = 100, offset = 0 }) => {
try {
const data = await Digit.CustomService.getResponse({
url: `/product/variant/v1/_search`,
params: { tenantId, limit, offset },
body: { ProductVariant: { id: [variantId] } },
config: { enabled: !!variantId },
});
return data?.ProductVariant?.[0]?.sku;
} catch (error) {
console.error('Error fetching product variant:', error);
throw error; // or handle it as per your error handling strategy
}
};This refactored version includes error handling and allows for pagination by making limit and offset configurable.
🧰 Tools
🪛 Biome
[error] 15-15: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
| }, | ||
| }, | ||
| config: { | ||
| enabled: variantId ? true : false, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify the boolean expression in the enabled config.
The current ternary operator is unnecessary and can be simplified.
Replace the line with:
enabled: !!variantId,This change uses the double negation operator to ensure a boolean value, which is more concise and achieves the same result.
🧰 Tools
🪛 Biome
[error] 15-15: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
| enabled: variantId ? true : false, | ||
| }, | ||
| }); | ||
| return data?.ProductVariant?.[0]?.sku; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling empty API results.
The current return statement uses optional chaining, which is good. However, it doesn't handle the case where the API returns an empty result.
Consider adding a fallback value or throwing an error for empty results:
const sku = data?.ProductVariant?.[0]?.sku;
if (!sku) {
console.warn('No product variant found');
return null; // or throw new Error('No product variant found');
}
return sku;This approach provides more explicit handling of empty results, making it easier for the consumer of this hook to handle different scenarios.
| const hierarchyData = Digit.Hooks.campaign.useBoundaryRelationshipSearch({BOUNDARY_HIERARCHY_TYPE,tenantId}); | ||
| const modulePrefix = "hcm"; | ||
|
|
||
| const moduleCode = ["hr", "campaignmanager", "workbench", "mdms", "schema", "admin-schemas", `boundary-${BOUNDARY_HIERARCHY_TYPE}`]; |
There was a problem hiding this comment.
💡 Codebase verification
Residual references to "hcm-admin-schemas" detected
The verification script found that "hcm-admin-schemas" is still referenced in the following files:
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.jshealth/micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/Module.js
These residual references may lead to inconsistencies or break existing functionality. Please update these instances to "admin-schemas" to ensure consistency across the codebase.
🔗 Analysis chain
Verify the impact of removing the "hcm" prefix from "admin-schemas"
The removal of the "hcm" prefix from "admin-schemas" in the moduleCode array might have implications for how the module interacts with admin schemas. Please ensure that this change is consistent with the rest of the codebase and doesn't break any existing functionality.
To verify the impact of this change, you can run the following script:
This script will help identify any remaining references to "hcm-admin-schemas" that might need updating, and confirm that "admin-schemas" is being used correctly throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to "hcm-admin-schemas" in the codebase
# Search for "hcm-admin-schemas" in all JavaScript and TypeScript files
echo "Searching for 'hcm-admin-schemas' references:"
rg --type js --type ts "hcm-admin-schemas"
# Search for "admin-schemas" to ensure it's being used correctly
echo "\nSearching for 'admin-schemas' usage:"
rg --type js --type ts "admin-schemas"
Length of output: 1337
| } | ||
|
|
||
| return { | ||
| key: index + 1 + subIndex, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure uniqueness of keys in attributeConfig.
The key for each attribute is calculated as:
key: index + 1 + subIndex,There is a possibility of duplicate keys if index and subIndex combinations result in the same value. To prevent potential rendering issues or incorrect data reference, consider modifying the key generation logic to ensure uniqueness across all attributes.
| return reversedData; | ||
| // return reversedData; | ||
|
|
||
| return data; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Assess the necessity of reverseDeliveryRemap function
At line 126, the reverseDeliveryRemap function simply returns data without any transformation. Consider whether this function is still necessary. If it doesn't perform any operations, you might remove the function and use the data directly where it's called to simplify the code.
|
|
||
| // function restructureData(data) { | ||
| // return data.map(cycle => ({ | ||
| // mandatoryWaitSinceLastCycleInDays: null, | ||
| // id: parseInt(cycle.cycleIndex), | ||
| // deliveries: cycle.deliveries.map(delivery => ({ | ||
| // id: parseInt(delivery.deliveryIndex), | ||
| // mandatoryWaitSinceLastDeliveryInDays: null, | ||
| // doseCriteria: delivery.deliveryRules.map(rule => ({ | ||
| // condition: parseCondition(rule.attributes), | ||
| // ProductVariants: rule.products.map(product => ({ | ||
| // productVariantId: product.value, | ||
| // name: product.name | ||
| // })) | ||
| // })) | ||
| // })) | ||
| // })); | ||
| // } | ||
|
|
||
| // function parseCondition(attributes) { | ||
| // const conditions = attributes.map(attr => { | ||
| // const attributeCode = attr.attribute.code; | ||
| // const operator = attr.operator.code; | ||
| // if (operator === "IN_BETWEEN") { | ||
| // const fromValue = attr.fromValue; | ||
| // const toValue = attr.toValue; | ||
| // return `${fromValue} <= ${attributeCode} < ${toValue}`; | ||
| // } | ||
|
|
||
| // const value = attr.value; | ||
| // return `${attributeCode}${convertOperator(operator)}${value}`; | ||
| // }); | ||
|
|
||
| // // Join conditions into a single string | ||
| // return conditions.join(' and '); | ||
| // } | ||
|
|
||
| // function convertOperator(operatorCode) { | ||
| // const operators = { | ||
| // "LESS_THAN_EQUAL_TO": "<=", | ||
| // "GREATER_THAN_EQUAL_TO": ">=", | ||
| // "LESS_THAN": "<", | ||
| // "GREATER_THAN": ">", | ||
| // "EQUAL_TO": "=", | ||
| // }; | ||
| // return operators[operatorCode] || "="; | ||
| // } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing the commented-out restructureData and related functions
The functions restructureData, parseCondition, and convertOperator from lines 177 to 222 are commented out and not being used. Removing unused code can improve maintainability and reduce potential confusion.
| // deliveryRule: reverseDeliveryRemap(delivery), | ||
| deliveryRule: delivery |
There was a problem hiding this comment.
Ensure deliveryRule has the correct data format
By replacing deliveryRule: reverseDeliveryRemap(delivery) with deliveryRule: delivery, ensure that the delivery object conforms to the expected structure required downstream. The reverseDeliveryRemap function might have performed necessary transformations.
| // function restructureData(data) { | ||
| // const dateData = totalFormData?.HCM_CAMPAIGN_CYCLE_CONFIGURE?.cycleConfigure?.cycleData; | ||
| // const restructuredData = []; | ||
|
|
||
| // data.forEach((cycle) => { | ||
| // cycle.deliveries.forEach((delivery, index) => { | ||
| // delivery.deliveryRules.forEach((rule) => { | ||
| // const restructuredRule = { | ||
| // startDate: Digit.Utils.pt.convertDateToEpoch(dateData?.find((i) => i.key == cycle.cycleIndex)?.fromDate, "daystart"), // Hardcoded for now | ||
| // endDate: Digit.Utils.pt.convertDateToEpoch(dateData?.find((i) => i?.key == cycle?.cycleIndex)?.toDate), // Hardcoded for now | ||
| // cycleNumber: parseInt(cycle.cycleIndex), | ||
| // deliveryNumber: parseInt(delivery.deliveryIndex), | ||
| // deliveryType: rule?.deliveryType, | ||
| // deliveryRuleNumber: parseInt(rule.ruleKey), // New key added | ||
| // products: [], | ||
| // conditions: [], | ||
| // }; | ||
|
|
||
| // rule.attributes.forEach((attribute) => { | ||
| // if (attribute?.operator?.code === "IN_BETWEEN") { | ||
| // restructuredRule.conditions.push({ | ||
| // attribute: attribute?.attribute?.code | ||
| // ? attribute?.attribute?.code | ||
| // : typeof attribute?.attribute === "string" | ||
| // ? attribute?.attribute | ||
| // : null, | ||
| // operator: "LESS_THAN_EQUAL_TO", | ||
| // value: attribute.fromValue ? Number(attribute.fromValue) : null, | ||
| // }); | ||
|
|
||
| // restructuredRule.conditions.push({ | ||
| // attribute: attribute?.attribute?.code | ||
| // ? attribute?.attribute?.code | ||
| // : typeof attribute?.attribute === "string" | ||
| // ? attribute?.attribute | ||
| // : null, | ||
| // operator: "GREATER_THAN_EQUAL_TO", | ||
| // value: attribute.toValue ? Number(attribute.toValue) : null, | ||
| // }); | ||
| // } else { | ||
| // restructuredRule.conditions.push({ | ||
| // attribute: attribute?.attribute?.code | ||
| // ? attribute?.attribute?.code | ||
| // : typeof attribute?.attribute === "string" | ||
| // ? attribute?.attribute | ||
| // : null, | ||
| // operator: attribute.operator ? attribute.operator.code : null, | ||
| // value: | ||
| // attribute?.attribute?.code === "Gender" && attribute?.value?.length > 0 | ||
| // ? attribute?.value | ||
| // : attribute?.attribute?.code === "TYPE_OF_STRUCTURE" | ||
| // ? attribute?.value | ||
| // : attribute?.value | ||
| // ? Number(attribute?.value) | ||
| // : null, | ||
| // }); | ||
| // } | ||
| // }); | ||
|
|
||
| // rule.products.forEach((prod) => { | ||
| // restructuredRule.products.push({ | ||
| // value: prod?.value, | ||
| // name: prod?.name, | ||
| // count: prod?.count, | ||
| // }); | ||
| // }); | ||
|
|
||
| // restructuredData.push(restructuredRule); | ||
| // }); | ||
| // }); | ||
| // }); | ||
|
|
||
| return restructuredData; | ||
| } | ||
| // return restructuredData; | ||
| // } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing the redundant commented-out restructureData function
The restructureData function from lines 500 to 573 is a duplicate of the earlier commented-out function. If it's no longer needed, removing this redundant code can enhance code clarity.
| // const temp = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule); | ||
| // payloadData.deliveryRules = temp; | ||
| payloadData.deliveryRules = totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule; |
There was a problem hiding this comment.
Verify that payloadData.deliveryRules is correctly formatted
In lines 614-616, 686-688, 751-753, and 815-817, the restructureData function is commented out, and payloadData.deliveryRules is directly assigned from totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule. Please ensure that this data matches the format expected by the backend API. Previously, restructureData may have been essential for proper data transformation.
Also applies to: 686-688, 751-753, 815-817
Bhavya-egov
force-pushed
the
HCMPRE-701
branch
from
b238070 to
96105c7
Compare
Bhavya-egov
force-pushed
the
HCMPRE-701
branch
from
1616655 to
0d67f0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range comments (16)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
Line range hint
1-41: Consider cleaning up and documenting stylesheet usage.The file contains several commented-out stylesheet links and scripts, as well as multiple versions of stylesheets. This could lead to confusion and maintenance issues in the future.
Suggestions:
- Remove any unnecessary commented-out code.
- Document the purpose of each stylesheet, especially those with specific versions (e.g., 1.0.75-campaign, 1.0.50-microplan).
- Consider using environment variables or a configuration file to manage different stylesheet versions for various environments, if applicable.
- Ensure that all necessary stylesheets are properly loaded and that there are no conflicts between different versions.
#!/bin/bash # Description: Check for usage of different stylesheet versions across the project echo "Checking for usage of different digit-ui-css versions:" rg --type html "digit-ui-css@[0-9.a-z-]+" | sort | uniq -c echo "Checking for usage of different digit-ui-components-css versions:" rg --type html "digit-ui-components-css@[0-9.a-z-]+" | sort | uniq -chealth/micro-ui/web/public/index.html (1)
Line range hint
23-23: Consider reviewing the commented out script tag.There's a commented out script tag for
globalConfigs.js:<!-- <script src="https://s3.ap-south-1.amazonaws.com/egov-dev-assets/globalConfigs.js"></script> -->If this script is no longer needed, consider removing the commented line to keep the code clean. If it's still required but commented out for a specific reason, it might be helpful to add a comment explaining why it's currently disabled.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js (1)
Line range hint
1-280: Consider adding error boundaries and improving error handlingThe component handles errors in various places, but it might benefit from a more structured approach to error handling. Consider implementing React Error Boundaries to catch and handle errors more gracefully. Also, review the error handling in the API calls and state updates to ensure all potential error scenarios are covered.
Here's an example of how you could implement an Error Boundary:
import React from 'react'; class ErrorBoundary extends React.Component { constructor(props) { super(props); this.state = { hasError: false }; } static getDerivedStateFromError(error) { return { hasError: true }; } componentDidCatch(error, errorInfo) { // You can log the error here console.error('Error caught by boundary:', error, errorInfo); } render() { if (this.state.hasError) { return <h1>Something went wrong.</h1>; } return this.props.children; } } // Usage <ErrorBoundary> <DataUploadSummary /> </ErrorBoundary>Consider wrapping the main component or its subcomponents with this ErrorBoundary to catch and handle unexpected errors.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (1)
Line range hint
111-116: Add missing dependency array to useEffect to prevent unintended behaviorThe
useEffecton lines 111-116 lacks a dependency array, causing it to execute after every render. This may lead to performance issues or unintended side effects, such as infinite loops. To ensure the effect runs only when intended, you should add an appropriate dependency array.Consider revising the effect:
If the intention is to run this effect only once on component mount, add an empty dependency array:
useEffect(() => { if (executionCount < 5) { onSelect("cycleConfigure", state); setExecutionCount((prevCount) => prevCount + 1); } - }, ); + }, []);If the effect should run when specific variables change, include those variables in the dependency array, ensuring that it doesn't create an unintended loop:
useEffect(() => { if (executionCount < 5) { onSelect("cycleConfigure", state); setExecutionCount((prevCount) => prevCount + 1); } - }, ); + }, [executionCount, state]);Please verify the desired behavior and adjust the dependencies accordingly to prevent potential issues.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (2)
Line range hint
142-142: Improve error handling infetchcdfunctionIn the
catchblock of thefetchcdfunction, the error is logged usingconsole.log, but the function does not return any value or handle the error appropriately. This could lead toundefinedbeing returned, which may cause issues in the code that relies on this function.Consider modifying the function to handle errors properly by returning a default value or rethrowing the error. Here's how you can adjust the code:
} catch (e) { console.log("error", e); + return null; // Return a default value or handle the error as needed }Ensure that any code using
fetchcdaccounts for the possibility of anullreturn value.
Line range hint
271-279: Refactor unused asynchronous function and variableWithin the
selectfunction of the data fetching hook, the asynchronous functionssis declared and immediately invoked, but the returned value is not utilized. Additionally, the variableprocessidis assigned a value insidessbut is not used elsewhere. This may lead to unnecessary complexity and potential confusion.Consider removing the unused function and variable to simplify the code. If fetching resource details is required for future implementation, ensure that the data is used appropriately.
Apply this diff to remove the unused code:
let processid; setprojectId(data?.[0]?.projectId); setCards(data?.cards); - const ss = async () => { - let temp = await fetchResourceFile(tenantId, resourceIdArr); - processid = temp; - return; - }; - ss(); + // If resource details are needed, fetch and use them here + // const resourceDetails = await fetchResourceFile(tenantId, resourceIdArr); + // Use resourceDetails as neededIf you need to perform asynchronous operations inside
select, ensure that they are properly handled, considering thatselectcannot be an async function. Alternatively, move the asynchronous logic to auseEffecthook within the component.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/TimelineComponent.js (9)
Line range hint
138-149: Prevent potentialTypeErrorwithresourceId.lengthcheckIn the condition
resourceId.length === 0, ifresourceIdisundefinedornull, it will cause aTypeError. Consider adding a null check or using optional chaining to prevent this error.Apply this diff to safeguard against potential errors:
- if ( - resourceId.length === 0 && + if ( + (!resourceId || resourceId.length === 0) && lastCompletedProcess?.type === "campaign-creation" && lastCompletedProcess?.status === "completed" && !dataFetched ) { // ... }
Line range hint
45-45: Consider usinguseLocationhook for consistentlocationaccessThe
locationobject may not be available globally in all environments, especially in server-side rendering or strict mode. For better compatibility and to follow React best practices, consider using theuseLocationhook fromreact-router-dom.Here's how you can modify the code:
+ import { useLocation } from 'react-router-dom'; // ... + const location = useLocation(); const searchParams = new URLSearchParams(location.search);
Line range hint
69-74: Remove commented-out code to clean up the codebaseThere's a block of commented-out code that seems to be obsolete. Removing unused code can improve readability and maintainability.
[code_smell]
Apply this diff to remove the unnecessary code:
- // useEffect(() => { - // if (newResourceId?.length > 0) { - // fetchUser(); - // } - // }, [newResourceId && lastCompletedProcess]);
Line range hint
237-259: Avoid using Immediately Invoked Function Expressions (IIFEs) inside JSXUsing IIFEs within JSX can make the code harder to read and maintain. It can also lead to unintended side effects. Consider extracting the logic into a separate function or component.
Refactor the code as follows:
- Extract the rendering logic into a separate function:
const renderTimelines = () => { const processesToRender = []; let foundFirstFailed = false; let allCompleted = true; // ...existing logic... return ( <TimelineMolecule hideFutureLabel={true} hidePastLabel={!allCompleted} {...(!allCompleted && { initialVisibleCount: processesToRender.length })} {...(allCompleted && { initialVisibleCount: 1 })} > {processesToRender.reverse()} </TimelineMolecule> ); };
- Replace the IIFE in the JSX with the function call:
- {progessTrack ? ( - (() => { - // rendering logic... - })() - ) : ( - <p></p> - )} + {progessTrack ? renderTimelines() : <p></p>}
Line range hint
57-68: Handle errors in thefetchUserfunctionThe
fetchUserfunction does not currently handle errors. If the API call fails, the application may experience unhandled promise rejections. Consider adding a try-catch block to handle potential errors gracefully.Apply this diff to add error handling:
const fetchUser = async () => { + try { const responseTemp = await Digit.CustomService.getResponse({ url: `/project-factory/v1/data/_search`, body: { SearchCriteria: { tenantId: tenantId, id: newResourceId, }, }, }); const response = responseTemp?.ResourceDetails?.map((i) => i?.processedFilestoreId); if (response?.[0]) { setUserCredential({ fileStoreId: response?.[0], customName: "userCredential" }); } + } catch (error) { + console.error("Error fetching user credentials:", error); + } };
Line range hint
200-204: Ensure proper dependency array inuseEffecthookThe
useEffecthook depends onlastCompletedProcess, but it may not update correctly iffailedProcesschanges. Consider addingfailedProcessto the dependency array to ensure the interval is cleared or reset appropriately.Apply this diff to update the dependency array:
useEffect(() => { let intervalId; if (failedProcess?.status !== "failed" && lastCompletedProcess?.type !== "campaign-creation") { intervalId = setInterval(() => { refetch(); }, baseTimeOut?.["HCM-ADMIN-CONSOLE"]?.baseTimeOut?.[0]?.timelineRefetch); } return () => { if (intervalId) { clearInterval(intervalId); } }; - }, [lastCompletedProcess]); + }, [lastCompletedProcess, failedProcess]);
Line range hint
33-35: Improve date formatting with locale supportThe
epochToDateTimefunction manually formats date and time components. Consider usingtoLocaleStringfor better locale support and cleaner code.Apply this diff to simplify the function:
- // Existing code formatting date and time manually + const epochToDateTime = (epoch) => { + const date = new Date(epoch); + return date.toLocaleString(); + };This approach automatically formats the date and time according to the user's locale settings.
Line range hint
235-236: Provide a loading indicator while data is being fetchedCurrently, if
progessTrackisnullor undefined, the component renders an empty<p></p>element. For better user experience, consider displaying a loading indicator.Apply this diff to add a loading state:
- ) : ( - <p></p> // You can replace this with a loading spinner or any other indicator - )} + ) : ( + <div className="loading-indicator">{t("COMMON_LOADING")}</div> // Display a loading message or spinner + )}Ensure that you have appropriate styling for the loading indicator.
Line range hint
229-232: Avoid unnecessary comments and commented-out codeThere are several blocks of commented-out code and unnecessary comments that can be removed to improve code clarity.
[code_smell]
Apply this diff to clean up the code:
- // useEffect(()=>{ - // const lastCompletedProcess = ... - // }, [progessTrack])Remove any other commented-out code that is no longer needed.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Line range hint
10-13: Possible inversion offromValueandtoValueinloopAndReturnfunctionIn the
loopAndReturnfunction, the assignment offromValueandtoValuemight be inverted. Typically,fromValueshould be the smaller value, andtoValueshould be the larger value. Currently,fromValueis assigned usingMath.max(...), andtoValueis assigned usingMath.min(...), which may result infromValuebeing greater thantoValue. This could lead to incorrect range representations in conditions.Consider swapping the assignments to ensure the correct range:
- toValue: existingItem.value && item.value ? Math.min(existingItem.value, item.value) : null, - fromValue: existingItem.value && item.value ? Math.max(existingItem.value, item.value) : null, + fromValue: existingItem.value && item.value ? Math.min(existingItem.value, item.value) : null, + toValue: existingItem.value && item.value ? Math.max(existingItem.value, item.value) : null,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (15)
- health/micro-ui/web/micro-ui-internals/example/public/index.html (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js (0 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/TimelineComponent.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (0 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (8 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1 hunks)
- health/micro-ui/web/public/index.html (1 hunks)
💤 Files with no reviewable changes (2)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
🧰 Additional context used
📓 Path-based instructions (11)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/TimelineComponent.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js
[error] 286-286: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 294-294: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js
[error] 15-15: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (25)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useProductVariantSearch.js (1)
1-3: Function signature and API call setup look good.The async function signature and the use of
Digit.CustomService.getResponsefor the API call are well-implemented. The destructuring of parameters in the function signature provides clear expectations for the hook's usage.health/micro-ui/web/micro-ui-internals/example/public/index.html (2)
20-20: Confirm the update of digit-ui-components-css version.The
digit-ui-components-csspackage has been updated from version0.0.2-beta.34to0.0.2-beta.36.Please ensure:
- This update was intentional.
- The application has been tested with this new version to confirm compatibility.
- Any changes or improvements in this version have been noted and communicated to the team if necessary.
#!/bin/bash # Description: Check for any other occurrences of the old version and verify if all have been updated. # Test: Search for any remaining occurrences of the old version echo "Checking for any remaining occurrences of version 0.0.2-beta.34:" rg --type html "0\.0\.2-beta\.34" # Test: Verify if the new version is consistently used across the project echo "Verifying consistent usage of version 0.0.2-beta.36:" rg --type html "0\.0\.2-beta\.36"
17-17: Verify the compatibility of the updated digit-ui-css version.The
digit-ui-csspackage has been updated from version1.8.2-beta.11to1.8.2-beta.34. This is a significant version jump that might introduce breaking changes or new features.Please confirm:
- Was this update intentional?
- Have you tested the application with this new version to ensure compatibility?
- Are there any new features or breaking changes in this version that the team should be aware of?
✅ Verification successful
digit-ui-css version update is verified successfully.
- No remaining occurrences of version
1.8.2-beta.11found.- Version
1.8.2-beta.34is consistently used across all relevant files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other occurrences of the old version and verify if all have been updated. # Test: Search for any remaining occurrences of the old version echo "Checking for any remaining occurrences of version 1.8.2-beta.11:" rg --type html "1\.8\.2-beta\.11" # Test: Verify if the new version is consistently used across the project echo "Verifying consistent usage of version 1.8.2-beta.34:" rg --type html "1\.8\.2-beta\.34"Length of output: 668
Script:
#!/bin/bash # Description: Check for any remaining occurrences of the old version and verify if all have been updated. # Test: Search for any remaining occurrences of the old version echo "Checking for any remaining occurrences of version 1.8.2-beta.11:" rg --type html "1\.8\.2-beta\.11" # Test: Verify if the new version is consistently used across the project echo "Verifying consistent usage of version 1.8.2-beta.34:" rg --type html "1\.8\.2-beta\.34"Length of output: 646
health/micro-ui/web/public/index.html (1)
11-12: CSS library versions have been updated.The versions of two CSS libraries have been updated:
- @egovernments/digit-ui-css: 1.8.2-beta.11 → 1.8.2-beta.34
- @egovernments/digit-ui-components-css: 0.0.2-beta.34 → 0.0.2-beta.36
These updates may introduce new styles or fix existing styling issues. Please ensure that:
- The application's UI remains consistent with these new versions.
- Any custom styles that may have been overriding the previous versions are still effective.
- The changes in these libraries are compatible with the current application version.
To verify the impact of these changes, you can run the following script:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/AddProduct.js (2)
Line range hint
51-55: Approved: Minor adjustment to variant length validation.The change to allow product variants with exactly two characters is a reasonable adjustment. It provides slightly more flexibility without significantly impacting data quality or meaningfulness of variant names.
Line range hint
41-45: Consider the implications of allowing single-character product names.The validation for product names has been relaxed to allow names with a single character. While this provides more flexibility, it might lead to less meaningful or distinguishable product names. Consider if this aligns with your product naming conventions and user experience goals.
To ensure this change doesn't conflict with any existing naming conventions or cause issues in other parts of the system, you may want to run the following check:
✅ Verification successful
Validation for product name lengths is consistent across the codebase.
The relaxation to allow product names with a single character in
AddProduct.jsaligns with existing validations inUserProfile.js, ensuring consistency in name length requirements throughout the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing product name validations or usages rg --type js 'product.*name.*length' -g '!*test*'Length of output: 51
Script:
#!/bin/bash # Search for any existing name length validations across the codebase rg --type js 'name\s*\.\s*length\s*(?:>=|<=|>|<|==|!=)\s*\d+' -g '!*test*'Length of output: 276
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
67-67: Verify the impact of removing the "hcm" prefix from "admin-schemas"The removal of the "hcm" prefix from "admin-schemas" in the
moduleCodearray might have implications for how the module interacts with admin schemas. Please ensure that this change is consistent with the rest of the codebase and doesn't break any existing functionality.To verify the impact of this change, you can run the following script:
This script will help identify any remaining references to "hcm-admin-schemas" that might need updating, and confirm that "admin-schemas" is being used correctly throughout the codebase.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js (4)
41-80: Consider removing the commented-out functionloopAndReturnThe
loopAndReturnfunction has been entirely commented out. If this function is no longer needed, it's best to remove it completely rather than leaving it as commented code. This will improve code readability and maintainability.
82-123: Consider removing the commented-out functionreverseDeliveryRemapThe
reverseDeliveryRemapfunction has been entirely commented out. If this function is no longer needed, it's best to remove it completely rather than leaving it as commented code. This will improve code readability and maintainability.
Line range hint
191-268: Review the updated structure of the cards in the componentThe structure of the cards rendered in the component has been significantly changed. Please ensure that this new structure aligns with the intended design and functionality of the DataUploadSummary component. Verify that all necessary information is still being displayed and that the edit functionality for each section is working as expected.
#!/bin/bash # Description: Check for any potential issues with the new card structure # Test: Search for references to the card names and their associated components # Expect: Consistent usage of card names and components rg '\b(target|facility|user)\b.*\bname:' health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js # Test: Check for the CampaignDocumentsPreview component usage # Expect: Consistent usage across all cards rg '\bCampaignDocumentsPreview\b' health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.js
Line range hint
270-278: Verify the removal of user credential download functionalityThe user credential download button and its associated functionality have been commented out. Please confirm if this removal is intentional. If it's no longer needed, consider removing the commented code entirely. If it's temporary, add a TODO comment explaining why it's commented out and when it should be re-enabled.
✅ Verification successful
User Credential Download Functionality Removal Verified
The search for
userCredentialanddownloadUserCredyielded no results outside of commented code. Therefore, the removal of the user credential download functionality is intentional and complete.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to userCredential or downloadUserCred # Test: Search for userCredential or downloadUserCred in the file # Expect: No matches found outside of commented code rg '\buserCredential\b|\bdownloadUserCred\b' health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DataUploadSummary.jsLength of output: 473
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (2)
27-31: Ensure consistent optional chaining for 'attribute' propertiesTo prevent potential runtime errors when
i.attributeori.attribute.codeis undefined, ensure that optional chaining is consistently applied.
18-18: Correct the order of 'fromValue' and 'toValue' in the range displayThe
valueassignment in theIN_BETWEENcondition displaystoValuebeforefromValue. Typically, a range is displayed as "fromValuetotoValue".health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (6)
80-89: Avoid using React Hooks outside of functional components or other Hooks.The
useProductVariantSearchis a React Hook, and according to React's Rules of Hooks, Hooks can only be called inside React functional components or other custom Hooks. Currently, it's being called inside a regular functiongenerateMRDNConfig, which is neither a React component nor a Hook. This can lead to runtime errors and unpredictable behavior.
80-89: Handle asynchronous operations correctly when usingmap.In
generateMRDNConfig, themapfunction is used with anasynccallback oncriteria.ProductVariants. This results inproductConfigbeing an array of unresolved Promises rather than the desired product configuration objects.
178-180: Ensure the asynchronousdeliveryConfigis properly awaited inconvertToConfig.Since
deliveryConfigmay return a Promise due to asynchronous operations, theconvertToConfigfunction should handle this accordingly. Without proper handling, the returned configuration might be incomplete or incorrect.
42-43: Handle unknown operators explicitly inparseCondition.In the
defaultcase of your switch statement,operatorValueis set to"UNKNOWN"without any error handling. This could lead to silent failures or unexpected behavior downstream.
136-136: Ensure uniqueness of keys inattributeConfig.The key for each attribute is calculated as:
key: index + 1 + subIndex,There's a possibility of duplicate keys if
indexandsubIndexcombinations result in the same value. Duplicate keys can cause rendering issues or incorrect data references. Consider enhancing the key generation logic to ensure uniqueness across all attributes.
115-115: Consistent retrieval of product names across configurations.In
generateBednetConfig, product names are obtained directly fromvariant.name:name: variant.name,Whereas in
generateMRDNConfig, product names are fetched asynchronously usinguseProductVariantSearch. This inconsistency might lead to discrepancies in product naming or data retrieval methods. Consider standardizing the method of obtaining product names to maintain consistency across your configurations.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (1)
6-6: Import statement is correctThe import of
getDeliveryConfigis correctly added and used appropriately in the code.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/TimelineComponent.js (2)
84-84:⚠️ Potential issueTypo in variable name
progessTrackThe variable
progessTrackappears to be misspelled. It should likely beprogressTrackfor better readability and to prevent confusion.Apply this diff to correct the typo:
- const { data: progessTrack , refetch } = Digit.Hooks.useCustomAPIHook(reqCriteria); + const { data: progressTrack , refetch } = Digit.Hooks.useCustomAPIHook(reqCriteria);Remember to update all occurrences of
progessTracktoprogressTrackthroughout the code.Likely invalid or redundant comment.
Line range hint
150-163: EnsuresearchAPIDatahandles campaign ID correctlyIn the
searchAPIDatafunction, theCampaignDetailsobject in the request body includesids: [campaignId]. Verify thatcampaignIdis always an array or adjust the code accordingly.Run the following script to check the type of
campaignIdthroughout the codebase:Ensure that
campaignIdis correctly used as an array when necessary.✅ Verification successful
Verification Successful:
campaignIdis consistently used as a string and appropriately handled as an array where necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `campaignId` is used consistently as an array or a string. # Search for declarations and usages of `campaignId` rg --type js --type jsx 'campaignId' -A 5 -B 5Length of output: 74
Script:
#!/bin/bash # Description: Verify if `campaignId` is used consistently as an array or a string. # Search for declarations and usages of `campaignId` in .js and .jsx files rg --type js --type javascript --type ts --type tsx 'campaignId' -A 5 -B 5Length of output: 109
Script:
#!/bin/bash # Description: Verify if `campaignId` is used consistently as an array or a string. # Search for declarations and usages of `campaignId` in .js, .jsx, .ts, and .tsx files rg 'campaignId' -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx' -A 5 -B 5Length of output: 86173
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (3)
5-5: Import statement correctly addedThe
getDeliveryConfigmodule is properly imported from"../../../utils/getDeliveryConfig".
26-27: Verify the MDMS module and master namesThe
useCustomMDMShook is now fetching data from the module"HCM-PROJECT-TYPES"and the master"projectTypes". Please ensure that these names are accurate and correspond to the correct MDMS configurations.
30-31: Previous review comment is still applicableThe concern regarding the data access path in
getDeliveryConfigremains unresolved. The data is still accessed asdata?.["HCM-PROJECT-TYPES"], but it should be accessed viadata?.MdmsRes?.["HCM-PROJECT-TYPES"]to correctly retrieve the MDMS response.
| params: { | ||
| tenantId: tenantId, | ||
| limit: 100, | ||
| offset: 0 | ||
| }, | ||
| body: { | ||
| ProductVariant: { | ||
| id: [variantId], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing API call parameters for better flexibility.
The API call parameters and body are structured well, but there are opportunities for improvement:
-
Pagination: The current implementation uses fixed
limitandoffsetvalues. Consider making these configurable to support flexible pagination for larger datasets. -
Request Body: While the current structure is correct, evaluate if additional fields in the
ProductVariantobject might be beneficial for more specific searches.
Here's a suggested refactor to address these points:
export const useProductVariantSearch = async ({ variantId, tenantId, limit = 100, offset = 0, additionalFields = {} }) => {
const data = await Digit.CustomService.getResponse({
url: `/product/variant/v1/_search`,
params: {
tenantId,
limit,
offset
},
body: {
ProductVariant: {
id: [variantId],
...additionalFields
},
},
// ... rest of the configuration
});
// ... rest of the function
};This refactor allows for customizable pagination and the inclusion of additional search criteria if needed.
| config: { | ||
| enabled: variantId ? true : false, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify the enabled configuration.
The current implementation of the enabled property uses an unnecessary ternary operator. This can be simplified for better readability and to address the static analysis hint.
Replace the line:
enabled: variantId ? true : false,with:
enabled: !!variantId,This change uses the double negation operator to ensure a boolean value, which is more concise and achieves the same result.
🧰 Tools
🪛 Biome
[error] 15-15: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
| return data?.ProductVariant?.[0]?.sku; | ||
| }; |
There was a problem hiding this comment.
Enhance error handling and empty result management.
While the use of optional chaining in the return statement is good, the function could benefit from more robust error handling and empty result management.
Consider implementing the following improvements:
- Add a try-catch block to handle potential API errors.
- Implement a check for empty results.
Here's a suggested refactor:
export const useProductVariantSearch = async ({ variantId, tenantId }) => {
try {
const data = await Digit.CustomService.getResponse({
// ... existing configuration
});
const sku = data?.ProductVariant?.[0]?.sku;
if (!sku) {
console.warn('No product variant found');
return null;
}
return sku;
} catch (error) {
console.error('Error fetching product variant:', error);
throw error; // or handle it as per your error handling strategy
}
};This approach provides more explicit handling of errors and empty results, making it easier for consumers of this hook to handle different scenarios.
| export const useProductVariantSearch = async ({ variantId, tenantId }) => { | ||
| const data = await Digit.CustomService.getResponse({ | ||
| url: `/product/variant/v1/_search`, | ||
| params: { | ||
| tenantId: tenantId, | ||
| limit: 100, | ||
| offset: 0 | ||
| }, | ||
| body: { | ||
| ProductVariant: { | ||
| id: [variantId], | ||
| }, | ||
| }, | ||
| config: { | ||
| enabled: variantId ? true : false, | ||
| }, | ||
| }); | ||
| return data?.ProductVariant?.[0]?.sku; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Overall, the hook implementation is solid but has room for improvement.
The useProductVariantSearch hook provides a good foundation for fetching product variant data. To enhance its functionality and robustness, consider implementing the suggested improvements:
- Add support for flexible pagination.
- Allow for additional search criteria in the request body.
- Simplify the
enabledconfiguration. - Implement error handling and empty result management.
These changes will make the hook more versatile and easier to use in various scenarios.
If you'd like assistance in implementing any of these suggestions or have any questions about the proposed changes, please let me know. I'd be happy to provide more detailed guidance or code examples.
🧰 Tools
🪛 Biome
[error] 15-15: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
| const usedCombinations = new Set(); | ||
| const variantPayload = resData.map((i) => { | ||
| const target = formData?.["addProduct"]?.find((f) => f.name === i.name); | ||
| const target = formData?.["addProduct"]?.find((f) => { | ||
| const combination = `${f.name}-${f.variant}`; | ||
| if (f.name === i.name && !usedCombinations.has(combination)) { | ||
| usedCombinations.add(combination); | ||
| return true; | ||
| } | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approved: Efficient handling of unique product-variant combinations.
The introduction of usedCombinations Set and the modified logic for finding target products effectively prevents duplicate processing of product-variant combinations. This is a good improvement for data integrity.
As suggested in a previous review, consider extracting the combination check into a separate function for improved readability:
const usedCombinations = new Set();
const isUniqueCombination = (name, variant) => {
const combination = `${name}-${variant}`;
if (!usedCombinations.has(combination)) {
usedCombinations.add(combination);
return true;
}
return false;
};
const variantPayload = resData.map((i) => {
const target = formData?.["addProduct"]?.find((f) =>
f.name === i.name && isUniqueCombination(f.name, f.variant)
);
// ... rest of the code
});This refactoring would improve code readability and maintainability.
| @@ -61,15 +61,18 @@ const CampaignModule = ({ stateCode, userType, tenants }) => { | |||
| }, | |||
| }); | |||
|
|
|||
| const hierarchyData = Digit.Hooks.campaign.useBoundaryRelationshipSearch({ BOUNDARY_HIERARCHY_TYPE, tenantId }); | |||
| const hierarchyData = Digit.Hooks.campaign.useBoundaryRelationshipSearch({BOUNDARY_HIERARCHY_TYPE,tenantId}); | |||
| const modulePrefix = "hcm"; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add documentation for the modulePrefix variable
While the introduction of modulePrefix resolves the previous issue with commented-out code, it would be beneficial to add a brief comment explaining its purpose and significance. This will improve code readability and maintainability.
Example:
// The "hcm" prefix is used to identify and scope store data specific to the Health Campaign Manager module
const modulePrefix = "hcm";| const { path, url } = useRouteMatch(); | ||
| const language = Digit.StoreData.getCurrentLanguage(); | ||
| const { isLoading, data: store } = Digit.Services.useStore({ | ||
| stateCode, | ||
| moduleCode, | ||
| language, | ||
| modulePrefix, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Update useStore hook documentation if necessary
The addition of modulePrefix to the useStore hook call is consistent with the earlier introduction of the modulePrefix variable. This change appears to be correct and aligns with the module's updated structure.
Consider updating the documentation of the useStore hook (if it exists) to reflect this new parameter and its purpose in scoping or identifying store data.
| const data = React.useMemo(() => { | ||
| const temp = rowsData.map((i) => ({ | ||
| const temp = rowsData.map((i) => { | ||
| if (i?.operator?.code === "IN_BETWEEN") { | ||
| return { | ||
| ...i, | ||
| value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`, | ||
| operator: t(i?.operator?.code) || t(i?.operator), | ||
| attribute: i?.attribute?.code || i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`) : "", | ||
| }; | ||
| } | ||
| else{ | ||
| return{ | ||
| ...i, | ||
| operator: t(i?.operator), | ||
| attribute: i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.toUpperCase()}`) : "", | ||
| })); | ||
| operator: t(i?.operator?.code) || t(i?.operator), | ||
| attribute: i?.attribute?.code | ||
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute.code.toUpperCase()}`) | ||
| : i?.attribute | ||
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute}`) | ||
| : "", | ||
| } | ||
| }}); | ||
| return temp; | ||
| }, [rowsData]); |
There was a problem hiding this comment.
Include 't' in the dependency array of the 'data' useMemo
The translation function t is used within the data computation but is not included in the dependency array of the useMemo hook. This could lead to stale translations if the language changes, as data would not recompute.
Apply this diff to add t to the dependency array:
}, [rowsData]);
+ }, [rowsData, t]);📝 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.
| const data = React.useMemo(() => { | |
| const temp = rowsData.map((i) => ({ | |
| const temp = rowsData.map((i) => { | |
| if (i?.operator?.code === "IN_BETWEEN") { | |
| return { | |
| ...i, | |
| value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`, | |
| operator: t(i?.operator?.code) || t(i?.operator), | |
| attribute: i?.attribute?.code || i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`) : "", | |
| }; | |
| } | |
| else{ | |
| return{ | |
| ...i, | |
| operator: t(i?.operator), | |
| attribute: i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.toUpperCase()}`) : "", | |
| })); | |
| operator: t(i?.operator?.code) || t(i?.operator), | |
| attribute: i?.attribute?.code | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute.code.toUpperCase()}`) | |
| : i?.attribute | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute}`) | |
| : "", | |
| } | |
| }}); | |
| return temp; | |
| }, [rowsData]); | |
| const data = React.useMemo(() => { | |
| const temp = rowsData.map((i) => { | |
| if (i?.operator?.code === "IN_BETWEEN") { | |
| return { | |
| ...i, | |
| value: `${i?.toValue ? i?.toValue : "N/A"} to ${i?.fromValue ? i?.fromValue : "N/A"}`, | |
| operator: t(i?.operator?.code) || t(i?.operator), | |
| attribute: i?.attribute?.code || i?.attribute ? t(`CAMPAIGN_ATTRIBUTE_${i?.attribute?.code?.toUpperCase()}`) : "", | |
| }; | |
| } | |
| else{ | |
| return{ | |
| ...i, | |
| operator: t(i?.operator?.code) || t(i?.operator), | |
| attribute: i?.attribute?.code | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute.code.toUpperCase()}`) | |
| : i?.attribute | |
| ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute}`) | |
| : "", | |
| } | |
| }}); | |
| return temp; | |
| }, [rowsData, t]); |
| switch (operator) { | ||
| case "<=": | ||
| operatorValue = "LESS_THAN_EQUAL_TO"; | ||
| break; | ||
| case ">=": | ||
| operatorValue = "GREATER_THAN_EQUAL_TO"; | ||
| break; | ||
| case "<": | ||
| operatorValue = "LESS_THAN"; | ||
| break; | ||
| case ">": | ||
| operatorValue = "GREATER_THAN"; | ||
| break; | ||
| case "=": | ||
| operatorValue = "EQUAL_TO"; | ||
| break; | ||
| default: | ||
| operatorValue = "UNKNOWN"; | ||
| } |
There was a problem hiding this comment.
Handle additional operators like '!=' in parseCondition.
Currently, the parseCondition function does not handle the 'not equal to' operator !=. If your conditions might include this operator, it's important to parse and process it correctly.
Apply this diff to add support for the != operator:
const operatorRegex = /([a-zA-Z_]+)\s*(<=?|>=?|=|<|>)\s*([a-zA-Z0-9_]+)/;
+const notEqualRegex = /([a-zA-Z_]+)\s*!=\s*([a-zA-Z0-9_]+)/;
if (betweenRegex.test(condition)) {
// existing code
} else if (operatorRegex.test(condition)) {
// existing code
+} else if (notEqualRegex.test(condition)) {
+ const match = condition.match(notEqualRegex);
+ const variable = match[1];
+ const comparisonValue = match[2];
+ operatorValue = "NOT_EQUAL_TO";
+ value = { variable, comparisonValue };
} else {
throw new Error(`Unknown condition format: ${condition}`);
}This modification adds a new regular expression to detect the != operator and handles it accordingly.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
SetupCampaigncomponent by removing complex restructuring logic.Chores