not selected added in single value list #1824
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications 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: 3
🧹 Outside diff range comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (2)
Line range hint
590-614: Enhance error handling in onSubmit functionThe current error handling could be improved to provide more specific feedback and better error tracking.
Consider applying this enhancement:
try { const data = await mutateAsync(payload); - // Handle successful checklist creation if (data.success) { const localisations = uniqueLocal; const localisationResult = await localisationMutateAsync(localisations); if (!localisationResult.success) { setShowToast({ label: "CHECKLIST_CREATED_LOCALISATION_ERROR", isError: "true" }); + console.error("Localization failed:", localisationResult.error); return; } history.push(`/${window.contextPath}/employee/campaign/response?isSuccess=${true}`, { message: "ES_CHECKLIST_CREATE_SUCCESS_RESPONSE", preText: "ES_CHECKLIST_CREATE_SUCCESS_RESPONSE_PRE_TEXT", actionLabel: "CS_CHECKLIST_NEW_RESPONSE_ACTION", actionLink: `/${window.contextPath}/employee/campaign/checklist/search?name=${projectName}&campaignId=${campaignId}&projectType=${projectType}`, secondaryActionLabel: "MY_CAMPAIGN", secondaryActionLink: `/${window?.contextPath}/employee/campaign/my-campaign`, }); } else { - setShowToast({ label: "CHECKLIST_CREATED_FAILED", isError: "true" }); + setShowToast({ + label: data.error?.message || "CHECKLIST_CREATED_FAILED", + isError: "true" + }); + console.error("Checklist creation failed:", data.error); } } catch (error) { - setShowToast({ label: "CHECKLIST_CREATED_FAILED", isError: "true" }); + const isNetworkError = error.name === 'NetworkError'; + setShowToast({ + label: isNetworkError ? "NETWORK_ERROR" : "CHECKLIST_CREATED_FAILED", + isError: "true" + }); + console.error("Error in checklist creation:", error); } finally { setSubmitting(false); }
Line range hint
1-24: Consider component decomposition and code organizationThe component is quite large and handles multiple responsibilities. Consider breaking it down into smaller, more manageable pieces.
Suggestions for improvement:
- Extract utility functions (
generateCodes,createQuestionObject,transformQuestions) into a separate utils file- Create custom hooks for form state management and API interactions
- Split the preview functionality into a separate component
Example structure:
// utils/checklistUtils.js export const generateCodes = (questions) => { ... } export const createQuestionObject = (item, tenantId, idCodeMap) => { ... } export const transformQuestions = (questions, tenantId, idCodeMap) => { ... } // hooks/useChecklistForm.js export const useChecklistForm = (initialData) => { const [formData, setFormData] = useState(initialData); // ... other form-related state and logic return { formData, ... }; } // components/ChecklistPreview.js export const ChecklistPreview = ({ data, onSubmit, onClose }) => { ... } // CreateChecklist.js const CreateChecklist = () => { const { formData, ... } = useChecklistForm(initialData); // ... reduced component logic }health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js (2)
Line range hint
1-24: Consider extracting toast configuration to constantsThe toast timeout and other UI-related constants should be defined as named constants at the top of the file.
Add these constants:
const TOAST_TIMEOUT_MS = 5000; const LOADING_STATES = { IDLE: 'idle', SUBMITTING: 'submitting', ERROR: 'error', SUCCESS: 'success' };
Line range hint
1-700: Consider restructuring for better maintainabilityThe component is handling too many responsibilities and could benefit from better separation of concerns.
Consider these architectural improvements:
Extract utility functions to a separate file:
generateCodescreateQuestionObjecttransformQuestionsorganizeQuestionsCreate separate custom hooks for:
- Form state management
- API interactions
- Toast notifications
Split the component into smaller, focused components:
- ChecklistForm
- ChecklistPreview
- ChecklistHeader
This would improve maintainability, testability, and reusability of the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js (1)
Pattern **/*.js: check
| console.log("hieeee", String(item?.type?.code)); | ||
| if(String(item?.type?.code) === "SingleValueList"){ | ||
| labelsArray.push("NOT_SELECTED"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log and improve type comparison
The code has two issues:
- Console.log statement should be removed from production code
- String comparison can be made more robust
Apply this diff to fix both issues:
- console.log("hieeee", String(item?.type?.code));
- if(String(item?.type?.code) === "SingleValueList"){
+ if(String(item?.type?.code)?.toLowerCase() === "singlevaluelist"){
labelsArray.push("NOT_SELECTED");
}📝 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.
| console.log("hieeee", String(item?.type?.code)); | |
| if(String(item?.type?.code) === "SingleValueList"){ | |
| labelsArray.push("NOT_SELECTED"); | |
| } | |
| if(String(item?.type?.code)?.toLowerCase() === "singlevaluelist"){ | |
| labelsArray.push("NOT_SELECTED"); | |
| } |
| if(String(item?.type?.code) === "SingleValueList"){ | ||
| labelsArray.push("NOT_SELECTED"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling specificity
The error handling could be more specific about what went wrong during the update process.
Consider enhancing the error handling:
} catch (error) {
- setShowToast({ label: "CHECKLIST_UPDATE_FAILED", isError: "true" });
+ const errorMessage = error.response?.data?.message || "CHECKLIST_UPDATE_FAILED";
+ setShowToast({
+ label: errorMessage,
+ isError: "true",
+ details: error.message
+ });
// console.error("Error creating checklist:", error);
}Also applies to: 279-281
🧹 Nitpick (assertive)
Improve string comparison robustness
The string comparison for question type could be more resilient to edge cases.
Consider this improvement:
-if(String(item?.type?.code) === "SingleValueList"){
+if(String(item?.type?.code)?.toUpperCase() === "SINGLEVALUELIST"){This change makes the comparison case-insensitive and more maintainable.
📝 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.
| if(String(item?.type?.code) === "SingleValueList"){ | |
| labelsArray.push("NOT_SELECTED"); | |
| } | |
| if(String(item?.type?.code)?.toUpperCase() === "SINGLEVALUELIST"){ | |
| labelsArray.push("NOT_SELECTED"); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (8)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js (4)
Line range hint
56-67: Consider adding error handling for MDMS data fetchThe
useEffectfor processing MDMS data should handle potential errors and edge cases.useEffect(() => { const data = dataBT?.mdms; - if(data) + if (!data) { + console.error('Failed to fetch MDMS roles data'); + return; + } + try { const newCodesOpt = data.map((item) => ({ code: `ACCESSCONTROL_ROLES_ROLES_${item?.data?.code}` })); setCodesOpt(newCodesOpt) + } catch (error) { + console.error('Error processing MDMS roles data:', error); + setCodesOpt([]); + } }, [dataBT]);
Line range hint
82-93: Add error handling for HCM data processingSimilar to the previous comment, the HCM data processing should include error handling.
useEffect(() => { let data = HCM?.mdms; - if(data) + if (!data) { + console.error('Failed to fetch HCM data'); + return; + } + try { const newListsOpt = data.map((item) => ({ list: `HCM_CHECKLIST_TYPE_${item?.data?.code}` })); setListsOpt(newListsOpt) + } catch (error) { + console.error('Error processing HCM data:', error); + setListsOpt([]); + } }, [HCM]);
Line range hint
119-122: Improve loading state handlingThe current loading state implementation could be enhanced to provide better user feedback.
-if (isFetching) return <div></div>; +if (isFetching) { + return ( + <div className="loader-container"> + <Loader /> {/* Assuming you have a Loader component */} + </div> + ); +}
Line range hint
95-98: Remove commented codeThere's commented code that appears to be obsolete. If it's no longer needed, it should be removed for better code maintainability.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CreateQuestion.js (2)
Line range hint
28-108: Consider refactoring option management for better maintainabilityThe option management logic (handleAddOption, handleUpdateOption, handleOptionDependency, etc.) follows similar patterns and could be consolidated using a reducer pattern. This would:
- Centralize option state management
- Reduce code duplication
- Make the logic more maintainable
Consider refactoring the option management code to use a reducer:
+const optionReducer = (state, action) => { + switch (action.type) { + case 'ADD_OPTION': + return [...state, { + id: crypto.randomUUID(), + key: state.length + 1, + parentQuestionId: action.parentId, + label: `${action.t("HCM_CHECKLIST_OPTION")} ${state.length + 1}`, + optionDependency: false, + optionComment: false, + }]; + case 'UPDATE_OPTION': + return state.map(item => + item.key === action.payload.id + ? { ...item, label: action.payload.value } + : item + ); + case 'TOGGLE_DEPENDENCY': + return state.map(option => + option.id === action.id + ? { ...option, optionDependency: !option.optionDependency } + : option + ); + // Add other cases... + default: + return state; + } +}; + +const [options, dispatch] = useReducer(optionReducer, initialOptions); + +const handleAddOption = () => { + dispatch({ type: 'ADD_OPTION', parentId: field.id, t }); +}; + +const handleUpdateOption = (param) => { + dispatch({ type: 'UPDATE_OPTION', payload: param }); +};
Line range hint
437-611: Consider splitting the component for better separation of concernsThe CreateQuestion component currently handles multiple responsibilities:
- Question creation
- Question updates
- View mode rendering
- Option management
- Sub-question handling
This makes the component complex and harder to maintain. Consider splitting it into smaller, more focused components:
- QuestionForm (base form structure)
- QuestionOptions (option management)
- SubQuestions (nested question handling)
This would:
- Improve code organization
- Make testing easier
- Reduce cognitive load when maintaining the code
- Make it easier to add new features or modify existing ones
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (2)
Line range hint
590-596: Consider using a cleanup function in useEffectThe current implementation might lead to race conditions if multiple toasts are triggered in quick succession. Consider using a cleanup function.
useEffect(()=>{ + let timeoutId; if(showToast !== null) { setShowPopUp(false); + timeoutId = setTimeout(() => setShowToast(null), 5000); } + return () => { + if (timeoutId) clearTimeout(timeoutId); + }; }, [showToast])
Line range hint
526-548: Enhance error handling in onSubmitThe error handling could be more specific to help users understand and resolve issues better.
} catch (error) { - // Handle error scenario - setShowToast({ label: "CHECKLIST_CREATED_FAILED", isError: "true" }); - // console.error("Error creating checklist:", error); + const errorMessage = error.response?.data?.message || "CHECKLIST_CREATED_FAILED"; + setShowToast({ + label: errorMessage, + isError: "true", + details: error.response?.data?.errors?.map(e => e.message).join(', ') + }); } finally {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CreateQuestion.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CreateQuestion.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js (1)
Pattern **/*.js: check
🔇 Additional comments (5)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (1)
18-18: Verify the impact of filtering only active templates.
The addition of isActive: true will filter out inactive checklist templates from search results. While this aligns with showing active status in the results table, please ensure this is the intended behavior.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js (1)
53-53: MDMS filtering looks good, but verify data consistency
The addition of isActive filters and type constraints in the MDMS criteria will help ensure only relevant data is fetched. However, we should verify that this doesn't accidentally filter out valid entries.
Also applies to: 75-76
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CreateQuestion.js (1)
259-259: LGTM: Filtering for active field types
The addition of isActive: true improves the UX by filtering out inactive field types from the dropdown options.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (2)
75-76: LGTM: Improved MDMS search criteria
Adding isActive: true to the search criteria is a good optimization that ensures only active checklist templates are retrieved.
333-335: Past review comment is still applicable
A previous review has already addressed the string comparison improvement needed in this code segment.
| @@ -15,6 +15,7 @@ | |||
| MdmsCriteria: { | |||
| tenantId: tenantId, | |||
| schemaCode: `HCM-ADMIN-CONSOLE.ChecklistTemplates`, | |||
| isActive: true, | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the active-only filtering behavior.
Consider adding a code comment to document that this configuration only returns active checklist templates. This helps other developers understand the filtering behavior.
schemaCode: `HCM-ADMIN-CONSOLE.ChecklistTemplates`,
+// Only fetch active checklist templates
isActive: true,
filters : {},📝 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.
| isActive: true, | |
| schemaCode: `HCM-ADMIN-CONSOLE.ChecklistTemplates`, | |
| // Only fetch active checklist templates | |
| isActive: true, | |
| filters : {}, |
Choose the appropriate template for your PR:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements