project type config simplified #1539
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe pull request introduces substantial modifications across multiple components related to campaign management. Key changes include the refactoring of the 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: 21
🧹 Outside diff range comments (12)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignUpdateSummary.js (1)
Potential Missing Error Handling.
The commented-out
useEffecthook for processingsummaryErrorssuggests that error handling logic may no longer be active. Our search did not find any newuseEffecthooks or alternative mechanisms handling these errors.
- File: health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignUpdateSummary.js
- Commented-out error handling code indicates potential removal without replacement.
🔗 Analysis chain
Line range hint
93-123: Clarify new error handling approach and clean up commented code.A significant portion of error handling logic in the
useEffecthook has been commented out. This suggests a change in how errors are processed and displayed.Please clarify the new approach for handling and displaying errors, particularly for
deliveryErrors,targetErrors,facilityErrors, anduserErrors. If this logic has been moved elsewhere or is no longer needed, consider removing the commented code to improve maintainability.To verify the current error handling mechanism, you can run the following script:
This script will help identify where and how errors are currently being handled in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for new error handling logic in the codebase # Search for error-related state updates echo "Searching for error state updates:" rg -n --type js 'set(Delivery|Target|Facility|User|Summary)Errors' # Search for error-related useEffect hooks echo "\nSearching for error-related useEffect hooks:" rg -n --type js 'useEffect.*Errors' # Search for error display components echo "\nSearching for error display components:" rg -n --type js '(Error|Toast).*component'Length of output: 29621
🧰 Tools
🪛 Biome
[error] 217-217: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/AddDeliverycontext.js (11)
Line range hint
58-73: Refactor duplicatedselectValuefunctions to improve maintainabilityThe
selectValuefunction in bothAddAttributeFieldandAddCustomAttributeFieldcomponents contains identical logic. Consider extracting this function into a shared utility function or a higher-order component to reduce code duplication and enhance maintainability.Also applies to: 167-182
Line range hint
100-112: Consolidate duplicatedselectOperatorfunctionsThe
selectOperatorfunction is duplicated in bothAddAttributeFieldandAddCustomAttributeField. Refactoring this into a shared function will reduce redundancy and make future updates easier.Also applies to: 195-207
Line range hint
138-160: Ensure safe access when updating attributes to prevent potential runtime errorsIn the functions
selectValue,selectOperator, andselectToFromValue, you access properties on the result ofitem.attributes.find(...)without checking if the result is defined. Iffindreturnsundefined, this will lead to a TypeError. Add a null check to ensure the attribute exists before attempting to set its properties.Apply this diff to add null checks:
const updatedData = deliveryRules.map((item, index) => { if (item.ruleKey === deliveryRuleIndex) { const attributeItem = item.attributes.find((i) => i.key === attribute.key); + if (attributeItem) { attributeItem.value = val; + } } return item; });Also applies to: 230-252
Line range hint
475-478: Remove unusedreviseIndexKeysfunctionThe
reviseIndexKeysfunction inAddAttributeWrapperis defined but never used. Removing unused code helps keep the codebase clean and improves readability.Apply this diff to remove the unused function:
- const reviseIndexKeys = () => { - setAttributes((prev) => prev.map((unit, index) => ({ ...unit, key: index + 1 }))); - };
Line range hint
678-686: Clean up commented-out code for better readabilityThere is commented-out code in
AddDeliveryRuleWrapperwhich can be removed to reduce clutter and improve code clarity.Apply this diff to remove the commented code:
- {/* {filteredDeliveryConfig?.projectType === "IRS-mz"?} */} - {/* {((!filteredDeliveryConfig?.deliveryAddDisable && deliveryRules?.length < 5) || isIRSDelivery) && ( */} - {/* <Button */} - {/* variation="secondary" */} - {/* label={t(`CAMPAIGN_ADD_MORE_DELIVERY_BUTTON`)} */} - {/* className={"add-rule-btn hover"} */} - {/* icon={<AddIcon styles={{ height: "1.5rem", width: "1.5rem" }} fill={PRIMARY_COLOR} />} */} - {/* onButtonClick={addMoreDelivery} */} - {/* /> */} - {/* )} */}
Line range hint
370-375: Add cleanup function inuseEffectto prevent memory leaksIn the
useEffecthook withinAddDeliveryRule, you set a timeout but do not clear it on component unmounting. This could lead to memory leaks. Include a cleanup function to clear the timeout.Apply this diff to add the cleanup:
useEffect(() => { if (showToast) { - setTimeout(closeToast, 5000); + const timer = setTimeout(closeToast, 5000); + return () => clearTimeout(timer); } }, [showToast]);
Line range hint
265-270: Use stable keys instead of index in list renderingUsing the index as a key in list rendering can lead to issues, especially if the list can change. Consider using a unique identifier like
attribute.keyfor thekeyprop.Apply this diff to update the key:
- key={index} + key={item.key}
Line range hint
380-386: Add null check forprodRef.currentinconfirmResourcesIn the
confirmResourcesfunction, you accessprodRef.currentwithout checking if it's defined. This could lead to errors ifprodRef.currentisnullorundefined. Add a null check before using it.Apply this diff to add a null check:
const confirmResources = () => { + if (!prodRef.current) { + setShowToast({ key: "error", label: "CAMPAIGN_PRODUCT_MISSING_ERROR" }); + return; + } const isValid = prodRef.current.every((item) => item?.count !== null && item?.value !== null); if (!isValid) { setShowToast({ key: "error", label: "CAMPAIGN_PRODUCT_MISSING_ERROR" }); return; } // Rest of the code...
Line range hint
578-581: Consider memoizing components to improve performanceComponents like
AddAttributeWrapperandAddDeliveryRulecan benefit fromReact.memoto prevent unnecessary re-renders when their props have not changed.
Line range hint
58-73: Enhance input validation inselectValuefunctionThe
selectValuefunction's current validation may not handle all edge cases. For instance, entering multiple dots or non-numeric characters in rapid succession could bypass the validation. Consider using more robust validation methods or input masks to ensure only valid numeric input is accepted.Also applies to: 167-182
Line range hint
138-160: Avoid mutating state directlyIn your
selectToFromValueand other functions, ensure you are not directly mutating state or its nested objects. Instead, create copies of the objects before making changes to prevent unexpected side effects.Apply this diff to avoid direct mutation:
const updatedData = deliveryRules.map((item, index) => { if (item.ruleKey === deliveryRuleIndex) { const attributeItem = item.attributes.find((i) => i.key === attribute.key); if (attributeItem) { + const updatedAttribute = { ...attributeItem }; if (range === "to") { - attributeItem.toValue = val; + updatedAttribute.toValue = val; } else { - attributeItem.fromValue = val; + updatedAttribute.fromValue = val; } + const updatedAttributes = item.attributes.map(attr => attr.key === attribute.key ? updatedAttribute : attr); + return { ...item, attributes: updatedAttributes }; } } return item; });Also applies to: 230-252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
- 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/CampaignUpdateSummary.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (7 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (6 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/AddDeliverycontext.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/index.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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/components/CampaignUpdateSummary.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/pages/employee/SetupCampaign.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/AddDeliverycontext.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/CampaignSummary.js
[error] 110-110: This let declares a variable that is only assigned once.
'conditionParts' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 111-111: This let declares a variable that is only assigned once.
'rules' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 115-115: This let declares a variable that is only assigned once.
'attributes' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignUpdateSummary.js
[error] 217-217: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 237-237: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 257-257: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js
[error] 111-111: This let declares a variable that is only assigned once.
'conditionParts' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 112-112: This let declares a variable that is only assigned once.
'rules' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 118-118: This let declares a variable that is only assigned once.
'attributes' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 91-91: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1)
110-121: EnsureparseConditionhandles all possible conditionsThe
parseConditionfunction is critical for correctly interpreting condition strings. In theattributeConfigmapping (lines 110-121), ensure thatparseConditionrobustly handles all expected condition formats to prevent runtime errors or incorrect configurations.Please verify that
parseConditioncan handle conditions outside the current regex patterns and gracefully handles unexpected formats.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DeliveryDetailsSummary.js (1)
412-414: Verify the logic inonStepClickfunctionWhen
currentStepequals1, the function setskeyto8. Please confirm that this mapping is intentional and aligns with the expected navigation flow.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js (1)
88-97: Operator mapping is well-definedThe
operatorMappingobject is correctly defined, providing a clear mapping between operator symbols and their corresponding codes.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
73-85: Addition ofgetOperatorSymbolfunction enhances code clarityThe
getOperatorSymbolfunction effectively maps operator codes to their corresponding symbols, improving code readability and maintainability.
| @@ -214,7 +214,7 @@ const CampaignUpdateSummary = (props) => { | |||
| }, | |||
| cardHeader: { value: t("TARGET_DETAILS"), inlineStyles: { marginTop: 0, fontSize: "1.5rem" } }, | |||
| cardSecondaryAction: noAction !== "false" && ( | |||
| <div className="campaign-preview-edit-container" onClick={() => handleRedirect(2)}> | |||
| <div className="campaign-preview-edit-container" onClick={() => handleRedirect(4)}> | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approve changes to handleRedirect function with a minor suggestion.
The updates to the handleRedirect function improve URL parameter handling using URLSearchParams. This approach is more robust and maintainable. However, consider extracting the URL parameter update logic into a separate function for better readability and reusability.
Consider refactoring the URL parameter update logic into a separate function:
const updateUrlParams = (params) => {
const urlParams = new URLSearchParams(window.location.search);
Object.entries(params).forEach(([key, value]) => urlParams.set(key, value));
return `${window.location.pathname}?${urlParams.toString()}`;
};
const handleRedirect = (step, activeCycle) => {
const params = { key: step, preview: false };
if (activeCycle) params.activeCycle = activeCycle;
const newUrl = updateUrlParams(params);
history.push(newUrl);
};This refactoring improves code organization and makes it easier to update URL parameters consistently across the component.
Also applies to: 237-237, 257-257
🧰 Tools
🪛 Biome
[error] 217-217: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
Update redirection steps and improve accessibility for secondary actions.
The changes to the redirection steps in the onClick handlers for "target", "facility", and "user" sections appear to be intentional, likely reflecting a reorganization of the campaign setup process. However, there are accessibility concerns that need to be addressed.
To improve accessibility, add keyboard event handlers to these clickable div elements. Here's an example of how to modify the "target" section (apply similar changes to "facility" and "user" sections):
<div
className="campaign-preview-edit-container"
onClick={() => handleRedirect(4)}
onKeyDown={(e) => e.key === 'Enter' && handleRedirect(4)}
tabIndex={0}
role="button"
aria-label={t("CAMPAIGN_EDIT_TARGET")}
>
<span>{t(`CAMPAIGN_EDIT`)}</span>
<EditIcon />
</div>These changes ensure that the edit actions are accessible to keyboard users, improving the overall accessibility of the component.
Also applies to: 237-237, 257-257
🧰 Tools
🪛 Biome
[error] 217-217: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
| conditionConfig: delivery.deliveries.map((deliveryItem, conditionIndex) => { | ||
| return { | ||
| disableDeliveryType: deliveryItem.deliveryStrategy === "DIRECT", | ||
| deliveryType: deliveryItem.deliveryStrategy, | ||
| attributeConfig: deliveryItem.doseCriteria.map((criteria, attrIndex) => { | ||
| const { operatorValue, value } = parseCondition(criteria.condition); | ||
|
|
||
| return { | ||
| key: `${conditionIndex + 1}-${attrIndex + 1}`, | ||
| label: "Custom", | ||
| attrType: criteria.attrType || "dropdown", | ||
| attrValue: value.variable, | ||
| operatorValue, | ||
| fromValue: Number(value.minValue), | ||
| toValue: Number(value.maxValue) - 1 | ||
| }; | ||
| }), | ||
| productConfig: deliveryItem.doseCriteria.flatMap((criteria, prodIndex) => { | ||
| return criteria.ProductVariants.map((variant, varIndex) => { | ||
| // if(variant?.productVariantId){ | ||
| // productName = useProductVariantSearch({ variantId: variant.productVariantId, tenantId: "mz" }); | ||
| // } | ||
| // const productName = useProductVariantSearch({ variantId: variant.productVariantId, tenantId: "mz" }); | ||
|
|
||
| return { | ||
| key: `${conditionIndex + 1}-${varIndex + 1}`, | ||
| count: variant.quantity, | ||
| value: variant.productVariantId, | ||
| name: "Product Name" | ||
| }; | ||
| }); | ||
| }) | ||
| }; | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve variable naming for clarity
The usage of delivery.deliveries may cause confusion due to repetitive naming. Consider renaming variables to enhance readability. For example, renaming the inner deliveries to deliveryItems or subDeliveries can make the code more understandable.
Apply this suggestion for better code clarity:
- conditionConfig: delivery.deliveries.map((deliveryItem, conditionIndex) => {
+ conditionConfig: delivery.subDeliveries.map((deliveryItem, conditionIndex) => {Ensure that all occurrences of delivery.deliveries are updated accordingly.
Committable suggestion was skipped due to low confidence.
| const deliveryConfig = ({ data, projectType }) => { | ||
| switch (projectType) { | ||
| case "MR-DN": | ||
| return generateMRDNConfig(data?.cycles?.[0]?.deliveries, projectType); | ||
| return generateMRDNConfig(data?.cycles, projectType); |
There was a problem hiding this comment.
Remove unnecessary parameter projectType
In the deliveryConfig function (line 147), generateMRDNConfig is called with data?.cycles and projectType, but the generateMRDNConfig function definition only accepts a single deliveries parameter. Passing projectType is unnecessary and could lead to confusion.
Apply the following change to remove the extraneous argument:
- return generateMRDNConfig(data?.cycles, projectType);
+ return generateMRDNConfig(data?.cycles);📝 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.
| return generateMRDNConfig(data?.cycles, projectType); | |
| return generateMRDNConfig(data?.cycles); |
| code: data?.code, | ||
| projectType: data?.code, | ||
| attrAddDisable: data?.attrAddDisable || false, | ||
| deliveryAddDisable: data?.deliveryAddDisable || false, | ||
| customAttribute: data?.customAttribute || true, | ||
| productCountHide: data?.productCountHide || false, | ||
| eligibilityCriteria: data?.eligibilityCriteria, | ||
| dashboardUrls: data?.dashboardUrls, | ||
| taskProcedure: data?.taskProcedure, | ||
| resources: data?.resources, |
There was a problem hiding this comment.
Handle potential undefined properties in convertToConfig
In the convertToConfig function (lines 159-168), several properties are accessed from data, such as beneficiaryType, eligibilityCriteria, dashboardUrls, taskProcedure, and resources. If these properties are undefined, it may lead to unintended behavior.
Consider providing default values to ensure stability:
return {
beneficiaryType: data?.beneficiaryType || "",
code: data?.code || "",
projectType: data?.code || "",
attrAddDisable: data?.attrAddDisable || false,
deliveryAddDisable: data?.deliveryAddDisable || false,
customAttribute: data?.customAttribute || true,
productCountHide: data?.productCountHide || false,
eligibilityCriteria: data?.eligibilityCriteria || [],
dashboardUrls: data?.dashboardUrls || [],
taskProcedure: data?.taskProcedure || {},
resources: data?.resources || [],📝 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.
| code: data?.code, | |
| projectType: data?.code, | |
| attrAddDisable: data?.attrAddDisable || false, | |
| deliveryAddDisable: data?.deliveryAddDisable || false, | |
| customAttribute: data?.customAttribute || true, | |
| productCountHide: data?.productCountHide || false, | |
| eligibilityCriteria: data?.eligibilityCriteria, | |
| dashboardUrls: data?.dashboardUrls, | |
| taskProcedure: data?.taskProcedure, | |
| resources: data?.resources, | |
| beneficiaryType: data?.beneficiaryType || "", | |
| code: data?.code || "", | |
| projectType: data?.code || "", | |
| attrAddDisable: data?.attrAddDisable || false, | |
| deliveryAddDisable: data?.deliveryAddDisable || false, | |
| customAttribute: data?.customAttribute || true, | |
| productCountHide: data?.productCountHide || false, | |
| eligibilityCriteria: data?.eligibilityCriteria || [], | |
| dashboardUrls: data?.dashboardUrls || [], | |
| taskProcedure: data?.taskProcedure || {}, | |
| resources: data?.resources || [], |
| const generateMRDNConfig = (deliveries) => { | ||
| const deliveryConfig = deliveries.map((delivery, deliveryIndex) => { | ||
| return { | ||
| delivery: delivery.id, | ||
| conditionConfig: delivery.deliveries.map((deliveryItem, conditionIndex) => { | ||
| return { | ||
| disableDeliveryType: deliveryItem.deliveryStrategy === "DIRECT", | ||
| deliveryType: deliveryItem.deliveryStrategy, | ||
| attributeConfig: deliveryItem.doseCriteria.map((criteria, attrIndex) => { | ||
| const { operatorValue, value } = parseCondition(criteria.condition); | ||
|
|
||
| return { | ||
| key: `${conditionIndex + 1}-${attrIndex + 1}`, | ||
| label: "Custom", | ||
| attrType: criteria.attrType || "dropdown", | ||
| attrValue: value.variable, | ||
| operatorValue, | ||
| fromValue: Number(value.minValue), | ||
| toValue: Number(value.maxValue) - 1 | ||
| }; | ||
| }), | ||
| productConfig: deliveryItem.doseCriteria.flatMap((criteria, prodIndex) => { | ||
| return criteria.ProductVariants.map((variant, varIndex) => { | ||
| // if(variant?.productVariantId){ | ||
| // productName = useProductVariantSearch({ variantId: variant.productVariantId, tenantId: "mz" }); | ||
| // } | ||
| // const productName = useProductVariantSearch({ variantId: variant.productVariantId, tenantId: "mz" }); | ||
|
|
||
| return { | ||
| key: `${conditionIndex + 1}-${varIndex + 1}`, | ||
| count: variant.quantity, | ||
| value: variant.productVariantId, | ||
| name: "Product Name" | ||
| }; | ||
| }); | ||
| }) | ||
| }; | ||
| }) | ||
| }; | ||
| }); | ||
| return deliveryConfig; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding product names and remove commented-out code
In the generateMRDNConfig function, within the productConfig construction (lines 123-135), the product name is hardcoded as "Product Name". This may lead to incorrect product display and hinder future scalability. Instead, consider retrieving the product name dynamically from the variant object, assuming it contains a name property.
Additionally, there is commented-out code related to useProductVariantSearch. It's a good practice to remove such code to maintain code cleanliness and readability.
Apply the following changes to address these issues:
- // if(variant?.productVariantId){
- // productName = useProductVariantSearch({ variantId: variant.productVariantId, tenantId: "mz" });
- // }
- // const productName = useProductVariantSearch({ variantId: variant.productVariantId, tenantId: "mz" });
return {
key: `${conditionIndex + 1}-${varIndex + 1}`,
count: variant.quantity,
value: variant.productVariantId,
- name: "Product Name"
+ name: variant.name || "Unknown Product"
};📝 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 generateMRDNConfig = (deliveries) => { | |
| const deliveryConfig = deliveries.map((delivery, deliveryIndex) => { | |
| return { | |
| delivery: delivery.id, | |
| conditionConfig: delivery.deliveries.map((deliveryItem, conditionIndex) => { | |
| return { | |
| disableDeliveryType: deliveryItem.deliveryStrategy === "DIRECT", | |
| deliveryType: deliveryItem.deliveryStrategy, | |
| attributeConfig: deliveryItem.doseCriteria.map((criteria, attrIndex) => { | |
| const { operatorValue, value } = parseCondition(criteria.condition); | |
| return { | |
| key: `${conditionIndex + 1}-${attrIndex + 1}`, | |
| label: "Custom", | |
| attrType: criteria.attrType || "dropdown", | |
| attrValue: value.variable, | |
| operatorValue, | |
| fromValue: Number(value.minValue), | |
| toValue: Number(value.maxValue) - 1 | |
| }; | |
| }), | |
| productConfig: deliveryItem.doseCriteria.flatMap((criteria, prodIndex) => { | |
| return criteria.ProductVariants.map((variant, varIndex) => { | |
| // if(variant?.productVariantId){ | |
| // productName = useProductVariantSearch({ variantId: variant.productVariantId, tenantId: "mz" }); | |
| // } | |
| // const productName = useProductVariantSearch({ variantId: variant.productVariantId, tenantId: "mz" }); | |
| return { | |
| key: `${conditionIndex + 1}-${varIndex + 1}`, | |
| count: variant.quantity, | |
| value: variant.productVariantId, | |
| name: "Product Name" | |
| }; | |
| }); | |
| }) | |
| }; | |
| }) | |
| }; | |
| }); | |
| return deliveryConfig; | |
| }; | |
| const generateMRDNConfig = (deliveries) => { | |
| const deliveryConfig = deliveries.map((delivery, deliveryIndex) => { | |
| return { | |
| delivery: delivery.id, | |
| conditionConfig: delivery.deliveries.map((deliveryItem, conditionIndex) => { | |
| return { | |
| disableDeliveryType: deliveryItem.deliveryStrategy === "DIRECT", | |
| deliveryType: deliveryItem.deliveryStrategy, | |
| attributeConfig: deliveryItem.doseCriteria.map((criteria, attrIndex) => { | |
| const { operatorValue, value } = parseCondition(criteria.condition); | |
| return { | |
| key: `${conditionIndex + 1}-${attrIndex + 1}`, | |
| label: "Custom", | |
| attrType: criteria.attrType || "dropdown", | |
| attrValue: value.variable, | |
| operatorValue, | |
| fromValue: Number(value.minValue), | |
| toValue: Number(value.maxValue) - 1 | |
| }; | |
| }), | |
| productConfig: deliveryItem.doseCriteria.flatMap((criteria, prodIndex) => { | |
| return criteria.ProductVariants.map((variant, varIndex) => { | |
| return { | |
| key: `${conditionIndex + 1}-${varIndex + 1}`, | |
| count: variant.quantity, | |
| value: variant.productVariantId, | |
| name: variant.name || "Unknown Product" | |
| }; | |
| }); | |
| }) | |
| }; | |
| }) | |
| }; | |
| }); | |
| return deliveryConfig; | |
| }; |
| if (attr?.operator?.code === "IN_BETWEEN") { | ||
| return `${attr.toValue} <= ${attr.attribute.code} < ${attr.fromValue}`; | ||
| } else { | ||
| return `${attr?.attribute?.code}${getOperatorSymbol(attr?.operator?.code)}${attr?.value}`; |
There was a problem hiding this comment.
Potential logical error in IN_BETWEEN condition
In the IN_BETWEEN operator handling, the fromValue and toValue may be used incorrectly. The current condition is:
return `${attr.toValue} <= ${attr.attribute.code} < ${attr.fromValue}`;This could lead to incorrect evaluations. Typically, for an "in between" condition, it should be:
return `${attr.fromValue} <= ${attr.attribute.code} <= ${attr.toValue}`;Apply this diff to correct the condition:
- return `${attr.toValue} <= ${attr.attribute.code} < ${attr.fromValue}`;
+ return `${attr.fromValue} <= ${attr.attribute.code} <= ${attr.toValue}`;Please verify that fromValue and toValue are correctly assigned.
| function restructureData(data, cycle, DeliveryConfig, projectType) { | ||
| const tt = DeliveryConfig?.find((e) => e.code === String(projectType)); | ||
|
|
||
| if (tt) { | ||
| delete tt.cycles; // Remove old cycles | ||
| delete tt.resources; // Remove old resources | ||
| } | ||
|
|
||
| const resourcesMap = new Map(); | ||
|
|
||
| const cycles = data.map(cycle => ({ | ||
| mandatoryWaitSinceLastCycleInDays: null, | ||
| startDate: Digit.Utils.pt.convertDateToEpoch(cycle?.cycleData?.[0]?.fromDate), | ||
| endDate: Digit.Utils.pt.convertDateToEpoch(cycle?.cycleData?.[0]?.toDate), | ||
| id: parseInt(cycle.cycleIndex, 10), | ||
| deliveries: cycle.deliveries.map(delivery => ({ | ||
| id: parseInt(delivery.deliveryIndex, 10), | ||
| deliveryStrategy: delivery.deliveryStrategy || "DIRECT", | ||
| mandatoryWaitSinceLastDeliveryInDays: null, | ||
| doseCriteria: delivery.deliveryRules.map(rule => { | ||
| // Consolidate product variants for the resources array | ||
| rule.products.forEach(product => { | ||
| if (resourcesMap.has(product.value)) { | ||
| resourcesMap.get(product.value).count += product.count; | ||
| } else { | ||
| resourcesMap.set(product.value, { | ||
| productVariantId: product.value, | ||
| isBaseUnitVariant: false, | ||
| name: product.name | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // Build the condition string, handling IN_BETWEEN separately | ||
| const conditions = rule.attributes.map(attr => { | ||
| if (attr?.operator?.code === "IN_BETWEEN") { | ||
| return `${attr.toValue} <= ${attr.attribute.code} < ${attr.fromValue}`; | ||
| } else { | ||
| return `${attr?.attribute?.code}${getOperatorSymbol(attr?.operator?.code)}${attr?.value}`; | ||
| } | ||
| }); | ||
|
|
||
| return { | ||
| condition: conditions.join(" and "), | ||
| ProductVariants: rule.products.map(product => ({ | ||
| productVariantId: product.value, | ||
| name: product.name | ||
| })) | ||
| }; | ||
| }) | ||
| })) | ||
| })); | ||
|
|
||
| const resources = Array.from(resourcesMap.values()); | ||
|
|
||
| if (tt) { | ||
| tt.cycles = cycles; | ||
| tt.resources = resources; | ||
| } | ||
| return [tt]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring restructureData for improved maintainability
The restructureData function is quite lengthy and handles multiple responsibilities, including data transformation, condition building, and resource mapping. Refactoring it into smaller, focused functions can enhance readability and maintainability.
For example, you could extract the condition-building logic into a separate function.
🧰 Tools
🪛 Biome
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 91-91: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| delete tt.cycles; // Remove old cycles | ||
| delete tt.resources; // Remove old resources |
There was a problem hiding this comment.
Avoid using the delete operator for better performance
Using the delete operator can negatively impact performance. Consider setting the properties to undefined or creating a new object without the unwanted properties.
Apply this diff to set the properties to undefined:
- delete tt.cycles; // Remove old cycles
- delete tt.resources; // Remove old resources
+ tt.cycles = undefined; // Remove old cycles
+ tt.resources = undefined; // Remove old resourcesAlternatively, create a new object excluding the unwanted properties:
- const tt = DeliveryConfig?.find((e) => e.code === String(projectType));
+ const ttOriginal = DeliveryConfig?.find((e) => e.code === String(projectType));
+ const { cycles, resources, ...tt } = ttOriginal || {};📝 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.
| delete tt.cycles; // Remove old cycles | |
| delete tt.resources; // Remove old resources | |
| tt.cycles = undefined; // Remove old cycles | |
| tt.resources = undefined; // Remove old resources |
🧰 Tools
🪛 Biome
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 91-91: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| const temp = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule , totalFormData?.HCM_CAMPAIGN_CYCLE_CONFIGURE?.cycleConfigure , DeliveryConfig); | ||
| payloadData.deliveryRules = temp?.[0]; |
There was a problem hiding this comment.
Missing projectType parameter in restructureData function call
The restructureData function now expects four parameters. In this call, the projectType parameter is missing, which may result in errors.
Apply this diff to include the projectType:
- const temp = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule, totalFormData?.HCM_CAMPAIGN_CYCLE_CONFIGURE?.cycleConfigure, DeliveryConfig);
+ const temp = restructureData(
+ totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule,
+ totalFormData?.HCM_CAMPAIGN_CYCLE_CONFIGURE?.cycleConfigure,
+ DeliveryConfig,
+ totalFormData?.HCM_CAMPAIGN_TYPE?.projectType?.code
+ );📝 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 , totalFormData?.HCM_CAMPAIGN_CYCLE_CONFIGURE?.cycleConfigure , DeliveryConfig); | |
| payloadData.deliveryRules = temp?.[0]; | |
| const temp = restructureData( | |
| totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule, | |
| totalFormData?.HCM_CAMPAIGN_CYCLE_CONFIGURE?.cycleConfigure, | |
| DeliveryConfig, | |
| totalFormData?.HCM_CAMPAIGN_TYPE?.projectType?.code | |
| ); | |
| payloadData.deliveryRules = temp?.[0]; |
| const temp = restructureData(totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule, totalFormData?.HCM_CAMPAIGN_CYCLE_CONFIGURE?.cycleConfigure , DeliveryConfig , totalFormData?.HCM_CAMPAIGN_TYPE?.projectType?.code); | ||
| payloadData.deliveryRules = temp; | ||
| // payloadData.deliveryRules = totalFormData?.HCM_CAMPAIGN_DELIVERY_DATA?.deliveryRule; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Assignments to payloadData.deliveryRules Detected
The verification confirms that payloadData.deliveryRules is assigned inconsistently across the codebase, which may lead to unexpected behavior. Specifically:
-
In
SetupCampaign.js:payloadData.deliveryRules = temp;payloadData.deliveryRules = temp?.[0];payloadData.deliveryRules = [temp?.[0]];
-
In
UpdateBoundary.js:payloadData.deliveryRules = CampaignData?.CampaignDetails?.[0]?.deliveryRules;
Please standardize the assignment structure of payloadData.deliveryRules to ensure consistent behavior.
🔗 Analysis chain
Inconsistent assignment to payloadData.deliveryRules
There are inconsistencies in how payloadData.deliveryRules is assigned:
- Line 529:
payloadData.deliveryRules = temp; - Line 594:
payloadData.deliveryRules = temp?.[0]; - Line 658:
payloadData.deliveryRules = [temp?.[0]];
This could lead to unexpected behavior due to differing data structures.
Run the following script to identify all assignments to payloadData.deliveryRules:
Ensure that payloadData.deliveryRules is consistently assigned the correct data structure across the codebase.
Also applies to: 594-596, 658-660
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all assignments to 'payloadData.deliveryRules' to verify consistency.
rg 'payloadData\.deliveryRules\s*=' -A 2
Length of output: 5878
| }; | ||
|
|
||
| const cycles = data?.[0]?.cycles || []; | ||
| const transformedCycles = cycles.map((cycle) => { |
There was a problem hiding this comment.
make them into multiple readble functions
Summary by CodeRabbit