ESTIMATION & FORMULA FIXES #1804
Conversation
…lan/src/components/Hypothesis.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThe pull request introduces several enhancements 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: 7
🧹 Outside diff range comments (7)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1)
Line range hint
82-134: Refactor addNewAssumption for better maintainabilityThe function is complex with multiple nested conditions and repeated logic patterns. Consider these improvements:
- Extract the common logic for adding assumptions into a separate helper function
- Add error handling for edge cases (e.g., null selectedDeletedAssumption)
- Use early returns to reduce nesting
Here's a suggested structure:
const addAssumptionToState = (assumptionToAdd, source = "MDMS") => { if (!assumptionToAdd) return false; setAssumptions(prev => [...prev, assumptionToAdd.key]); if (!assumptionValues.some(assumption => assumption.key === assumptionToAdd.key)) { setAssumptionValues(prev => [...prev, { source, key: assumptionToAdd.key, value: null, category }]); } return true; }; const addNewAssumption = () => { if (!selectedDeletedAssumption) return; let success = false; if (category === "CAMPAIGN_VEHICLES") { success = addAssumptionToState({ key: selectedDeletedAssumption?.model, category }, "CUSTOM"); } else if (selectedDeletedAssumption?.code === "NEW_ASSUMPTION") { success = addAssumptionToState({ key: selectedDeletedAssumption?.name, category }, "CUSTOM"); } else { const assumptionToAdd = deletedAssumptions.find( assumption => assumption === selectedDeletedAssumption.code ); success = addAssumptionToState({ key: assumptionToAdd }); } if (success) { setSelectedDeletedAssumption(null); setAssumptionsPopUp(false); } };health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (2)
Line range hint
228-232: Add defensive checks for form dataThe form validation lacks defensive checks for the formData parameter. Consider adding type checking to prevent potential runtime errors.
- const toastObject = Digit.Utils.microplanv1.formValidator(formData?.[currentConfBody?.key], currentConfBody?.key, state, t); + const toastObject = formData && currentConfBody?.key + ? Digit.Utils.microplanv1.formValidator(formData[currentConfBody.key], currentConfBody.key, state, t) + : { key: 'error', label: 'INVALID_FORM_DATA' };
Line range hint
13-434: Consider breaking down the component for better maintainabilityThe
SetupMicroplancomponent is handling multiple responsibilities including:
- State management
- Form validation
- Navigation
- Session storage
- Event handling
Consider breaking it down into smaller, more focused components to improve maintainability and testing.
Suggestions:
- Extract form handling logic into a custom hook
- Create separate components for different steps
- Consider using React Context for state management instead of direct session storage
- Implement a proper state machine for step management
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (1)
Line range hint
40-47: Consider extracting category-specific logicThe special case handling for "CAMPAIGN_VEHICLES" category is embedded in the deletion validation. Consider extracting this business rule into a configuration or utility function for better maintainability.
+const CATEGORIES_ALLOWING_EMPTY_FORMULAS = ['CAMPAIGN_VEHICLES']; + +const isFormulaDeleteAllowed = (formulas, category) => { + return formulas?.length > 1 || CATEGORIES_ALLOWING_EMPTY_FORMULAS.includes(category); +}; + if (formulas?.length === 1 && category !== "CAMPAIGN_VEHICLES") { - setShowToast({ - key: "error", - label: t("ERR_ATLEAST_ONE_MANDATORY_FORMULA"), - transitionTime: 3000, - }); - return; + if (!isFormulaDeleteAllowed(formulas, category)) { + setShowToast({ + key: "error", + label: t("ERR_ATLEAST_ONE_MANDATORY_FORMULA"), + transitionTime: 3000, + }); + return; + } }health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (3)
Line range hint
146-157: Centralize error handling and ensure consistent error message internationalization.The error handling is scattered and some error messages might not be internationalized. Consider creating a centralized error handling utility.
+const ERROR_TYPES = { + MANDATORY_FIELD: 'ERR_MANDATORY_FIELD', + UPDATE_FAILED: 'FAILED_TO_UPDATE_RESOURCE', +}; + +const handleError = (error, setShowToast, t) => { + const errorMessage = error?.message ? error.message : t(ERROR_TYPES.UPDATE_FAILED); + setShowToast({ + key: "error", + label: t(errorMessage), + transitionTime: 3000 + }); +}; if (formulaConfigValues.filter((row) => row.category === currentCategory) .every((row) => { return row.assumptionValue && row.input && row.output && row.operatorName; })) { // ... existing code } else { - setShowToast({ - key: "error", - label: t("ERR_MANDATORY_FIELD"), - transitionTime: 3000, - }); + handleError(null, setShowToast, t); return; }
Line range hint
16-40: Consider using useReducer for complex state management.The component manages multiple related states that could be better organized using useReducer. This would make state updates more predictable and easier to maintain.
+const formulaReducer = (state, action) => { + switch (action.type) { + case 'SET_FORMULA_CONFIG': + return { ...state, formulaConfigValues: action.payload }; + case 'SET_DELETED_FORMULAS': + return { ...state, deletedFormulas: action.payload }; + case 'SET_CUSTOM_FORMULA': + return { ...state, customFormula: action.payload }; + default: + return state; + } +}; + const FormulaConfigWrapper = ({ onSelect, props: customProps }) => { - const [formulaConfigValues, setFormulaConfigValues] = useState([]); - const [deletedFormulas, setDeletedFormulas] = useState([]); - const [customFormula, setCustomFormula] = useState([]); + const [state, dispatch] = useReducer(formulaReducer, { + formulaConfigValues: [], + deletedFormulas: [], + customFormula: [] + });
Line range hint
1-576: Consider breaking down the component for better maintainability and performance.The component is handling too many responsibilities which makes it hard to maintain and potentially impacts performance. Consider splitting it into smaller, focused components.
- Extract the formula configuration logic into a separate hook:
// useFormulaConfiguration.js export const useFormulaConfiguration = (props) => { // Formula-related state and logic return { formulaState, handleFormulaChange, // ... other formula-related functions }; };
- Extract the stepper logic into a separate component:
// FormulaConfigStepper.js const FormulaConfigStepper = ({ steps, currentStep, onStepClick }) => { // Stepper-related logic };
- Create a dedicated context provider component:
// FormulaContextProvider.js const FormulaContextProvider = ({ children }) => { // Context-related logic return ( <FormulaContext.Provider value={contextValue}> {children} </FormulaContext.Provider> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
Pattern **/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
[error] 314-314: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
[error] 163-163: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (2)
181-181: Verify translation coverage for new placeholder text
The category-specific placeholder is a good UX improvement. However, ensure that all translation keys are properly defined.
#!/bin/bash
# Check if the translation keys exist in the translation files
rg "MP_ENTER_ASSUMPTION_CAMPAIGN_VEHICLES|MP_ENTER_ASSUMPTION" public/localization/Line range hint 41-50: Verify deletion restriction logic against business requirements
The condition ensures at least one assumption remains for non-CAMPAIGN_VEHICLES categories, which is good for data integrity. However, consider these improvements:
- Add a comment explaining why CAMPAIGN_VEHICLES is exempt from this rule
- Consider extracting the error message key to a constant for maintainability
✅ Verification successful
Special handling for CAMPAIGN_VEHICLES category is consistently implemented
The verification reveals that the CAMPAIGN_VEHICLES exception is consistently implemented across the codebase:
-
In Hypothesis.js:
- Deletion restriction is properly enforced (non-CAMPAIGN_VEHICLES categories must retain at least one assumption)
- Special UI handling (custom placeholders, labels, and popup messages)
- Mandatory field indicators are suppressed
-
In FormulaConfiguration.js:
- Same deletion restriction pattern is applied
- Consistent UI treatment for mandatory indicators
The comment about "atleast one assumption in each category has to be mandatory to support draft functionality" adequately explains the business requirement, making the CAMPAIGN_VEHICLES exception clear.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if this deletion restriction is consistently applied across the codebase
rg -l "CAMPAIGN_VEHICLES" . | xargs rg "handleDeleteClick|deletion|remove" -A 5
Length of output: 240
Script:
#!/bin/bash
# Search for CAMPAIGN_VEHICLES usage in the codebase
rg "CAMPAIGN_VEHICLES" -B 2 -A 2
# Search for assumption deletion related logic
rg "handleDeleteClick|setAssumptionToDelete|assumptions\?.length" -B 2 -A 2
Length of output: 14732
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js
Show resolved
Hide resolved
Choose the appropriate template for your PR:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style