changes for the draft flow #1780
Conversation
📝 WalkthroughWalkthroughThe changes involve modifications across several components in the campaign management module. 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: 6
🧹 Outside diff range comments (8)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (2)
Line range hint
26-40: Improve code structure and readability.The nested ternary operations and complex conditional logic make the code harder to maintain.
Consider restructuring the attribute translation logic:
- attribute: i?.attribute?.code - ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute.code.toUpperCase()}`) - : i?.attribute - ? t(`CAMPAIGN_ATTRIBUTE_${i.attribute}`) - : "", + attribute: getAttributeTranslation(i?.attribute),Add a helper function at the top of the file:
const getAttributeTranslation = (attribute) => { if (!attribute) return ""; const code = attribute.code || attribute; return t(`CAMPAIGN_ATTRIBUTE_${code.toUpperCase()}`); };
Line range hint
13-40: Consider memoizing translation function calls.The component re-renders might trigger unnecessary translation calls for static content.
Consider memoizing frequently used translations or moving them outside the memoized data calculation:
const memoizedTranslations = React.useMemo(() => ({ inBetween: t('IN_BETWEEN'), na: t('N/A') }), [t]);health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (2)
Line range hint
105-135: Add error handling and optimize the implementation.The implementation could benefit from several improvements:
- Error handling:
return data?.deliveries?.map(delivery => { + if (!delivery?.doseCriteria?.length) { + return { delivery: delivery.id, conditionConfig: [] }; + } const conditionConfig = delivery.doseCriteria.map((dose, index) => {
- Performance and maintainability:
- const productConfig = dose.ProductVariants.map(variant => ({ - key: 1, - name: variant.name, - count: variant.quantity, - value: variant.productVariantId - })); + const productConfig = dose.ProductVariants?.map((variant, idx) => ({ + key: idx + 1, + name: variant.name, + count: variant.quantity, + value: variant.productVariantId + })) ?? [];
- Dynamic attribute type:
- attrType: "dropdown", + attrType: dose.attrType || "dropdown",
Line range hint
105-135: Consider extracting mapping logic into separate functions.The nested mapping operations make the code harder to maintain. Consider extracting the product and attribute configuration logic into separate functions:
+const mapProductConfig = (variants = []) => + variants.map((variant, idx) => ({ + key: idx + 1, + name: variant.name, + count: variant.quantity, + value: variant.productVariantId + })); +const mapAttributeConfig = (condition, dose) => { + const { operatorValue, value } = parseCondition(condition); + return { + key: 1, + label: "Custom", + attrType: dose.attrType || "dropdown", + attrValue: value.variable, + fromValue: value.minValue || value.comparisonValue, + toValue: value.maxValue || null, + operatorValue + }; +}; const generateMRDNConfig = (data) => { return data?.deliveries?.map(delivery => { const conditionConfig = delivery.doseCriteria.map((dose, index) => { - const productConfig = dose.ProductVariants.map(variant => ({ - key: 1, - name: variant.name, - count: variant.quantity, - value: variant.productVariantId - })); + const productConfig = mapProductConfig(dose.ProductVariants); const conditions = dose.condition.split('and'); - const attributeConfigs = conditions.map(condition => { - const { operatorValue, value } = parseCondition(condition); - return { - key: 1, - label: "Custom", - attrType: "dropdown", - attrValue: value.variable, - fromValue: value.minValue || value.comparisonValue, - toValue: value.maxValue || null, - operatorValue - }; - }); + const attributeConfigs = conditions.map(condition => + mapAttributeConfig(condition, dose) + );health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (2)
Line range hint
77-91: Simplify the enabled property in useCustomMDMS hook.The boolean expression can be simplified by directly using the selectedProjectType variable.
- enabled: selectedProjectType ? true : false, + enabled: !!selectedProjectType,🧰 Tools
🪛 Biome
[error] 84-84: 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)
The executionCount implementation is unnecessary and can be removed
The executionCount implementation in CycleConfiguration.js is redundant because:
- The component already has a dedicated useEffect hook that updates the state through
onSelect("cycleConfigure", state)wheneverstatechanges- The additional useEffect with executionCount creates unnecessary re-renders by forcing the same state update up to 5 times without any conditional logic
The following code block should be removed as it's redundant:
useEffect(() => { if (executionCount < 5) { onSelect("cycleConfigure", state); setExecutionCount((prevCount) => prevCount + 1); } });🔗 Analysis chain
Line range hint
134-140: Review the necessity of executionCount implementation.The useEffect with executionCount seems to force state updates up to 5 times. This could indicate an underlying issue with state management that should be investigated.
Consider refactoring to avoid the need for forced state updates. If you need assistance with the refactoring, I can help propose a more robust solution.
Let me search for the usage pattern of executionCount to understand its implementation better.
Let me try a different approach to find the executionCount usage pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns of execution counting in the codebase rg -A 2 "executionCount.*useState.*0"Length of output: 8871
Script:
#!/bin/bash # Search for useEffect patterns with executionCount in CycleConfiguration.js rg -A 10 "useEffect.*executionCount" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.jsLength of output: 152
Script:
#!/bin/bash # Get the content of CycleConfiguration.js to understand the executionCount implementation cat health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.jsLength of output: 12631
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/setupCampaignHelpers.js (1)
Line range hint
1-400: Consider improving documentation and code organization.While the code is functional, it could benefit from:
- Adding JSDoc comments for each exported function
- Breaking down complex functions into smaller, more focused ones
- Adding type annotations for better maintainability
Example improvement for one of the functions:
/** * Restructures campaign delivery data into a standardized format * @param {Object} data - Raw campaign data * @param {Object} cycleData - Cycle configuration data * @param {Array} DeliveryConfig - Available delivery configurations * @param {string} projectType - Type of the project * @returns {Array} Restructured delivery configuration */ export const restructureData = (data, cycleData, DeliveryConfig, projectType) => { // ... existing implementation }health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Line range hint
1-24: Consider architectural improvements for better maintainability.The component would benefit from several architectural improvements:
- Consider using a reducer for complex state management
- Implement proper error boundaries
- Split the component into smaller, more focused components
- Add proper TypeScript types for better type safety
Example reducer structure:
const campaignReducer = (state, action) => { switch (action.type) { case 'SET_DRAFT': return { ...state, isDraft: true, currentKey: action.payload }; case 'UPDATE_FORM_DATA': return { ...state, formData: { ...state.formData, ...action.payload } }; // ... other cases } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
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/pages/employee/CycleConfiguration.js(4 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/setupCampaignHelpers.js(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/setupCampaignHelpers.js (1)
Pattern **/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js
[error] 84-84: 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 (5)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/DetailsTable.js (1)
26-28: 🛠️ Refactor suggestion
Verify translation key existence for dynamic values.
The direct translation of i.value might fail if the translation key doesn't exist in the translation files. Consider adding a fallback mechanism.
Consider implementing a safer translation approach:
- value: typeof i?.value === "string"
- ? t(i.value)
- : i?.value,
+ value: typeof i?.value === "string"
+ ? t(i.value, { defaultValue: i.value })
+ : i?.value,health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/getDeliveryConfig.js (1)
Line range hint 105-135: Verify the impact of disableDeliveryType flag.
The implementation sets disableDeliveryType: index === 0, which means only the first delivery type will be enabled. Let's verify if this change is intentional and properly handled in the UI.
✅ Verification successful
Let me gather more information about how this flag is used in the UI component.
Let me check one more thing to understand the delivery strategy selection implementation.
The disableDeliveryType flag is correctly implemented and handled in the UI
The verification shows that:
- The flag is properly used in
AddDeliverycontext.jsto disable the delivery type selection for the first dose - The UI component correctly handles this through a
disabledprop that evaluates thedisableDeliveryTypeflag - The implementation allows changing delivery types for subsequent doses while keeping the first dose's delivery type fixed
This is an intentional design choice to maintain consistency in the delivery strategy, where the first dose's delivery type serves as the base configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that handle the disableDeliveryType prop
rg -l "disableDeliveryType" --type js
# Search for any UI components that might be affected by this flag
ast-grep --pattern 'disabled={$_?.disableDeliveryType}'
Length of output: 326
Script:
#!/bin/bash
# Check the implementation in AddDeliverycontext.js
rg "disableDeliveryType" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/AddDeliverycontext.js -B 2 -A 2
# Look for any dropdown or select components that might be using this flag
rg -A 5 "Select|Dropdown" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/AddDeliverycontext.js
Length of output: 2943
Script:
#!/bin/bash
# Look for the delivery strategy selection component and its usage
rg "deliveryStrategy|updateDeliveryType" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/deliveryRule/AddDeliverycontext.js -B 2 -A 5
Length of output: 933
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js (1)
1-2: LGTM: Loading state management follows React best practices.
The addition of the Loader component and loading state management is implemented correctly.
Also applies to: 75-75
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/setupCampaignHelpers.js (2)
5-6: Great optimization on lodash import!
The change from importing the entire lodash library to importing just the cloneDeep function will help reduce the bundle size through better tree-shaking.
127-127: Verify consistent usage of deliveryType across the codebase.
The renaming from deliveryStrategy to deliveryType with a default value of "DIRECT" has been applied consistently in this file. Let's verify this change across the entire codebase to ensure all references have been updated.
Also applies to: 143-143, 164-164
...icro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js
Show resolved
Hide resolved
...icro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CycleConfiguration.js
Show resolved
Hide resolved
...i/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/setupCampaignHelpers.js
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js
Show resolved
Hide resolved
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor