fixed summary and campaign creation #1479
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve significant updates to the Changes
Possibly related PRs
Suggested reviewers
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: 5
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Line range hint
1-1556: Summary of changes and recommendations
- The new
restructureDatafunction is a good improvement in terms of code clarity and conciseness.- There's a recurring issue where only the first delivery rule is being processed, potentially leading to data loss. This should be fixed by processing all rules.
- The styling change (setting
noCardStyletotrue) needs clarification on its purpose and impact.To improve the code:
- Implement a helper function to consistently handle the restructuring of delivery rules across the component.
- Review and update all instances where delivery rules are processed to ensure all data is included.
- Clarify the reasoning behind the styling change and remove any unnecessary commented-out code.
These changes will enhance the robustness and maintainability of the component.
Consider breaking down this large component into smaller, more manageable sub-components. This will improve readability, maintainability, and potentially performance through more granular rendering control.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (0 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (14 hunks)
💤 Files with no reviewable changes (1)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js
🧰 Additional context used
📓 Path-based instructions (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js
[error] 414-414: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 421-421: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🔇 Additional comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (3)
636-638:⚠️ Potential issueConsistent usage of restructureData function needed.
This code segment repeats the same pattern as seen earlier, potentially leading to data loss by only processing the first delivery rule.
Apply the same fix suggested earlier to ensure all delivery rules are processed:
-const temp = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule?.[0]); -payloadData.deliveryRules = temp; +payloadData.deliveryRules = totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule?.map(restructureData) || [];Consider creating a helper function to handle this restructuring consistently across the component.
765-767:⚠️ Potential issueRepeated pattern of restructureData usage.
This is the third instance of the same pattern, which potentially leads to data loss by only processing the first delivery rule.
To address this recurring issue, consider implementing a global fix:
- Create a helper function to handle the restructuring:
function restructureDeliveryRules(deliveryRules) { return deliveryRules?.map(restructureData) || []; }
- Replace all instances of the current pattern with this helper function:
-const temp = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule?.[0]); -payloadData.deliveryRules = temp; +payloadData.deliveryRules = restructureDeliveryRules(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule);This approach will ensure consistency and prevent potential data loss across all usage points.
1521-1521: Clarify the removal of conditional card styling.The
noCardStyleprop is now always set totrue, removing the previous conditional styling for step 7.Could you provide more context on why the conditional styling was removed? If it's no longer needed, consider removing the commented-out code for better maintainability.
✅ Verification successful
Conditional Card Styling Removal Verified
The
noCardStyleprop is consistently set totrueacross the codebase, and the previous conditional logic referencingcurrentStep === 7is no longer active.Consider removing the commented-out code to enhance code maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any references to step 7 styling in the codebase rg "currentStep === 7" --type jsLength of output: 829
| const temp = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule?.[0]); | ||
| payloadData.deliveryRules = temp; | ||
| // payloadData.deliveryRules = totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule; |
There was a problem hiding this comment.
Clarify usage of restructureData function.
The restructureData function is now being called with only the first element of the deliveryRule array. This might lead to loss of data if there are multiple delivery rules.
Consider modifying the code to process all delivery rules:
-const temp = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule?.[0]);
-payloadData.deliveryRules = temp;
+payloadData.deliveryRules = totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule?.map(restructureData) || [];This change ensures all delivery rules are processed and maintains consistency with the array structure.
📝 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 = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule?.[0]); | |
| payloadData.deliveryRules = temp; | |
| // payloadData.deliveryRules = totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule; | |
| payloadData.deliveryRules = totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule?.map(restructureData) || []; | |
| // payloadData.deliveryRules = totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule; |
| function restructureData(inputData) { | ||
| const result = { | ||
| cycleIndex: inputData?.cycleIndex, | ||
| active: inputData?.active, | ||
| deliveries: inputData?.deliveries?.map((delivery) => ({ | ||
| deliveryIndex: delivery.deliveryIndex, | ||
| active: delivery.active, | ||
| deliveryRules: delivery.deliveryRules.map((rule) => ({ | ||
| ruleKey: rule.ruleKey, | ||
| delivery: rule.delivery, | ||
| attributes: rule.attributes.map((attr) => ({ | ||
| key: attr.key, | ||
| attribute: attr.attribute.code, // Directly using code from attribute | ||
| operator: attr.operator.code, // Directly using code from operator | ||
| value: attr.value, | ||
| })), | ||
| products: rule?.products.map((product) => ({ | ||
| key: product.key, | ||
| value: product.value, | ||
| count: product.count, | ||
| name: product.name, | ||
| })), | ||
| })), | ||
| })), | ||
| }; | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improved data restructuring function.
The new restructureData function is a more concise and clear implementation compared to the previous version. It effectively maintains the hierarchy of cycles, deliveries, and rules while simplifying the code.
Consider adding type checking or default values to handle potential undefined properties, e.g.:
function restructureData(inputData) {
const result = {
- cycleIndex: inputData?.cycleIndex,
- active: inputData?.active,
+ cycleIndex: inputData?.cycleIndex ?? null,
+ active: inputData?.active ?? false,
deliveries: inputData?.deliveries?.map((delivery) => ({
// ... rest of the code
})),
};
return result;
}📝 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.
| function restructureData(inputData) { | |
| const result = { | |
| cycleIndex: inputData?.cycleIndex, | |
| active: inputData?.active, | |
| deliveries: inputData?.deliveries?.map((delivery) => ({ | |
| deliveryIndex: delivery.deliveryIndex, | |
| active: delivery.active, | |
| deliveryRules: delivery.deliveryRules.map((rule) => ({ | |
| ruleKey: rule.ruleKey, | |
| delivery: rule.delivery, | |
| attributes: rule.attributes.map((attr) => ({ | |
| key: attr.key, | |
| attribute: attr.attribute.code, // Directly using code from attribute | |
| operator: attr.operator.code, // Directly using code from operator | |
| value: attr.value, | |
| })), | |
| products: rule?.products.map((product) => ({ | |
| key: product.key, | |
| value: product.value, | |
| count: product.count, | |
| name: product.name, | |
| })), | |
| })), | |
| })), | |
| }; | |
| return result; | |
| } | |
| function restructureData(inputData) { | |
| const result = { | |
| cycleIndex: inputData?.cycleIndex ?? null, | |
| active: inputData?.active ?? false, | |
| deliveries: inputData?.deliveries?.map((delivery) => ({ | |
| deliveryIndex: delivery.deliveryIndex, | |
| active: delivery.active, | |
| deliveryRules: delivery.deliveryRules.map((rule) => ({ | |
| ruleKey: rule.ruleKey, | |
| delivery: rule.delivery, | |
| attributes: rule.attributes.map((attr) => ({ | |
| key: attr.key, | |
| attribute: attr.attribute.code, // Directly using code from attribute | |
| operator: attr.operator.code, // Directly using code from operator | |
| value: attr.value, | |
| })), | |
| products: rule?.products.map((product) => ({ | |
| key: product.key, | |
| value: product.value, | |
| count: product.count, | |
| name: product.name, | |
| })), | |
| })), | |
| })), | |
| }; | |
| return result; | |
| } |
| // 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 data; |
There was a problem hiding this comment.
Uncomment or remove the reverseDeliveryRemap function
The reverseDeliveryRemap function has its core logic commented out from lines 88 to 120, causing it to simply return the input data without any processing. Additionally, there is an unreachable return reversedData; statement at line 121, which will never be executed because the function already returns data at line 121.
This could lead to confusion and potential bugs if other parts of the code expect processed data from this function. If the function is still required, please uncomment and update it accordingly. If it is no longer needed, consider removing the function to clean up the code.
| data?.[0]?.deliveryRules && data?.[0]?.deliveryRules.map((item) => item.cycleIndex)?.length > 0 | ||
| ? Math.max(...data?.[0]?.deliveryRules.map((item) => item.cycleIndex)) |
There was a problem hiding this comment.
Fix unsafe optional chaining in delivery rules
At lines 413-414, there is unsafe usage of optional chaining. If data?.[0]?.deliveryRules is undefined, attempting to call .map() on it will result in a TypeError because you cannot call map on undefined.
To fix this issue, ensure that deliveryRules is an array before calling .map(). You can modify the condition to safely handle undefined or null values.
Apply this diff to fix the unsafe optional chaining:
- data?.[0]?.deliveryRules && data?.[0]?.deliveryRules.map((item) => item.cycleIndex)?.length > 0
- ? Math.max(...data?.[0]?.deliveryRules.map((item) => item.cycleIndex))
+ Array.isArray(data?.[0]?.deliveryRules) && data[0].deliveryRules.length > 0
+ ? Math.max(...data[0].deliveryRules.map((item) => item.cycleIndex))
: t("CAMPAIGN_SUMMARY_NA"),📝 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.
| data?.[0]?.deliveryRules && data?.[0]?.deliveryRules.map((item) => item.cycleIndex)?.length > 0 | |
| ? Math.max(...data?.[0]?.deliveryRules.map((item) => item.cycleIndex)) | |
| Array.isArray(data?.[0]?.deliveryRules) && data[0].deliveryRules.length > 0 | |
| ? Math.max(...data[0].deliveryRules.map((item) => item.cycleIndex)) | |
| : t("CAMPAIGN_SUMMARY_NA"), |
🧰 Tools
🪛 Biome
[error] 414-414: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
| data?.[0]?.deliveryRules && data?.[0]?.deliveryRules?.flatMap((rule) => rule?.deliveries.map((delivery) => delivery?.deliveryIndex))?.length > 0 | ||
| ? Math.max(...data?.[0]?.deliveryRules?.flatMap((rule) => rule?.deliveries.map((delivery) => delivery?.deliveryIndex))) | ||
| : t("CAMPAIGN_SUMMARY_NA"), |
There was a problem hiding this comment.
Fix unsafe optional chaining in delivery indices
At lines 420-422, there is unsafe usage of optional chaining with data?.[0]?.deliveryRules?.flatMap(). If deliveryRules is undefined, attempting to call flatMap() on it will throw a TypeError.
To resolve this issue, ensure that deliveryRules is an array before invoking flatMap(). Additionally, check that rule.deliveries is an array before mapping over it.
Apply this diff to fix the unsafe optional chaining:
- data?.[0]?.deliveryRules && data?.[0]?.deliveryRules?.flatMap((rule) => rule?.deliveries.map((delivery) => delivery?.deliveryIndex))?.length > 0
- ? Math.max(...data?.[0]?.deliveryRules?.flatMap((rule) => rule?.deliveries.map((delivery) => delivery?.deliveryIndex)))
+ Array.isArray(data?.[0]?.deliveryRules) && data[0].deliveryRules.length > 0
+ ? Math.max(
+ ...data[0].deliveryRules.flatMap((rule) =>
+ Array.isArray(rule?.deliveries)
+ ? rule.deliveries.map((delivery) => delivery?.deliveryIndex)
+ : []
+ )
+ )
: t("CAMPAIGN_SUMMARY_NA"),📝 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.
| data?.[0]?.deliveryRules && data?.[0]?.deliveryRules?.flatMap((rule) => rule?.deliveries.map((delivery) => delivery?.deliveryIndex))?.length > 0 | |
| ? Math.max(...data?.[0]?.deliveryRules?.flatMap((rule) => rule?.deliveries.map((delivery) => delivery?.deliveryIndex))) | |
| : t("CAMPAIGN_SUMMARY_NA"), | |
| Array.isArray(data?.[0]?.deliveryRules) && data[0].deliveryRules.length > 0 | |
| ? Math.max( | |
| ...data[0].deliveryRules.flatMap((rule) => | |
| Array.isArray(rule?.deliveries) | |
| ? rule.deliveries.map((delivery) => delivery?.deliveryIndex) | |
| : [] | |
| ) | |
| ) | |
| : t("CAMPAIGN_SUMMARY_NA"), |
🧰 Tools
🪛 Biome
[error] 421-421: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Line range hint
138-159: Consider removing or uncommenting the large blocks of commented-out code.The code contains entire functions that are commented out, such as
parseCondition,convertOperator, andreverseDeliveryRemap. Keeping large blocks of commented-out code can make the codebase harder to read and maintain.If these functions are no longer needed, consider removing them. If they are needed, consider uncommenting and integrating them properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (14 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Pattern
**/*.js: check
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (2)
354-356: Verify the impact of commenting outreverseDeliveryRemapondeliveryRule.The
reverseDeliveryRemapfunction has been commented out, anddeliveryRuleis now assigned directly fromdelivery. This change might affect howdeliveryRuleis structured and could impact functionalities that rely on the transformed data.Please ensure that the
deliveryRuledata structure remains consistent with what downstream code expects.
1521-1521: Re-evaluate hardcodingnoCardStyletotrue.Previously, the
noCardStyleprop was set conditionally based oncurrentStep:noCardStyle={currentStep === 7 ? false : true}By changing it to always
true, the styling for step 7 might be affected. If step 7 requiresnoCardStyleto befalsefor proper display, this change might cause UI issues.Consider restoring the conditional logic or verifying that this change does not affect the user interface.
| attributes: rule.attributes.map((attr) => ({ | ||
| key: attr.key, | ||
| attribute: attr.attribute.code, // Directly using code from attribute | ||
| operator: attr.operator.code, // Directly using code from operator | ||
| value: attr.value, | ||
| })), |
There was a problem hiding this comment.
Add null checks to prevent potential runtime errors in property access.
In the restructureData function, when mapping over rule.attributes, the code accesses attr.attribute.code and attr.operator.code directly. If attr.attribute or attr.operator is undefined, this will cause a TypeError.
Consider using optional chaining or providing default values to prevent runtime errors:
attributes: rule.attributes.map((attr) => ({
key: attr.key,
- attribute: attr.attribute.code, // Directly using code from attribute
- operator: attr.operator.code, // Directly using code from operator
+ attribute: attr.attribute?.code ?? null, // Safely accessing code
+ operator: attr.operator?.code ?? null, // Safely accessing code
value: attr.value,
})),📝 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.
| attributes: rule.attributes.map((attr) => ({ | |
| key: attr.key, | |
| attribute: attr.attribute.code, // Directly using code from attribute | |
| operator: attr.operator.code, // Directly using code from operator | |
| value: attr.value, | |
| })), | |
| attributes: rule.attributes.map((attr) => ({ | |
| key: attr.key, | |
| attribute: attr.attribute?.code ?? null, // Safely accessing code | |
| operator: attr.operator?.code ?? null, // Safely accessing code | |
| value: attr.value, | |
| })), |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes