Hcmpre 1290 #1834
Hcmpre 1290 #1834
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple components and configuration files related to campaign management. Key changes include the addition of a new link in 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: 20
🧹 Outside diff range comments (7)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js (2)
Line range hint
29-53: Remove or implement commented API integration codeThere's a significant block of commented code related to API integration for count functionality. Consider either:
- Implementing the count feature if it's required
- Removing the commented code and tracking it in a separate issue
- Adding a TODO comment with the issue reference if implementation is planned
Line range hint
1-82: Clean up commented count properties across link definitionsMultiple links have commented-out count properties. This pattern appears throughout the links array and should be addressed consistently:
{ label: t("MICROPLAN_CAMPAIGN"), link: `/${window?.contextPath}/employee/campaign/my-microplan`, roles: ROLES.CAMPAIGN_MANAGER, - // count: isLoading?"-":data },health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Line range hint
122-133: Fix inconsistent popup behavior between onClick and onFocus handlersThe
productType!=="microplan"check is present in onClick but missing in onFocus handler, which could lead to inconsistent behavior.onClick={(e) => { if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate && productType!=="microplan") { setShowPopUp(true); return; } return; }} onFocus={(e) => { - if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate) { + if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate && productType!=="microplan") { setShowPopUp(true); return; } return; }}🧰 Tools
🪛 Biome
[error] 119-135: 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/SetupCampaign.js (4)
Line range hint
1-24: Add JSDoc documentation for component props.The component lacks proper documentation for its props. Add JSDoc to document the expected props and their types.
+/** + * SetupCampaign component for managing campaign setup workflow + * @param {Object} props + * @param {string} props.hierarchyType - Type of hierarchy for the campaign + * @param {Object} props.hierarchyData - Data structure defining the hierarchy + * @returns {React.ReactElement} + */ const SetupCampaign = ({ hierarchyType, hierarchyData }) => {
Line range hint
32-45: Consider implementing error boundaries.The component handles complex state management and API calls but lacks error boundaries. This could lead to unhandled runtime errors affecting the user experience.
Consider implementing React Error Boundaries to gracefully handle and recover from runtime errors:
class CampaignErrorBoundary extends React.Component { state = { hasError: false }; static getDerivedStateFromError(error) { return { hasError: true }; } render() { if (this.state.hasError) { return <ErrorFallback onReset={() => this.setState({ hasError: false })} />; } return this.props.children; } }
Line range hint
46-52: Consider memoizing search params.The search params are being parsed on every render. Consider memoizing them using useMemo to optimize performance.
-const searchParams = new URLSearchParams(location.search); -const id = searchParams.get("id"); -const isPreview = searchParams.get("preview"); +const searchParams = useMemo(() => { + const params = new URLSearchParams(location.search); + return { + id: params.get("id"), + isPreview: params.get("preview"), + // ... other params + }; +}, [location.search]);
Based on the search results, I can now provide a final response as there's clear evidence of inconsistent error handling patterns in the codebase:
Error handling needs standardization across API calls
The codebase shows multiple inconsistent patterns in error handling:
- Some handlers only log to console and show toast:
console.log(error); setShowToast()- Some show toast without logging:
setShowToast({ key: "error", label: error })- Some include additional error state management:
setLoader(false); setShowToast()- Inconsistent error message formatting:
- Direct error message:
error?.message- Translated error codes:
t(error?.response?.data?.Errors?.[0]?.code)- Generic messages:
t("FAILED_TO_UPDATE_RESOURCE")Specific locations needing standardization:
SetupCampaign.js: Mixes console logging with conditional toast messagesUpdateCampaign.js: Similar inconsistent patterns as SetupCampaignCreateMicroplan.js: Uses different toast message structureHypothesisWrapper.js: Includes console.error with toast🔗 Analysis chain
Line range hint
391-394: Verify API error handling consistency.The error handling in API calls is inconsistent. Some error callbacks only log to console while others show toasts. Ensure consistent error handling across all API calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for inconsistent error handling patterns in API calls rg -A 5 "onError: \(error" .Length of output: 22306
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (8)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myCampaignConfig.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myMicroplanConfig.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js(1 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/pages/employee/index.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myCampaignConfig.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myMicroplanConfig.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.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/pages/employee/index.js (1)
Pattern **/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
[error] 348-348: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js
[error] 24-24: 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)
[error] 40-47: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (7)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js (2)
40-48: 🛠️ Refactor suggestion
Fix component syntax and contextPath usage
- The InboxSearchComposer should be self-closing
- Verify window.contextPath availability to prevent runtime errors
- <InboxSearchComposer
- configs={MicroplanCampaignSearchConfig?.[0]}
- additionalConfig={{
- resultsTable: {
- onClickRow,
- },
- }}
- ></InboxSearchComposer>
+ <InboxSearchComposer
+ configs={MicroplanCampaignSearchConfig?.[0]}
+ additionalConfig={{
+ resultsTable: {
+ onClickRow,
+ },
+ }}
+ />#!/bin/bash
# Check for contextPath definition and usage
rg "contextPath" 🧰 Tools
🪛 Biome
[error] 40-47: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
9-11:
Initialize defaultSearchValues with expected search parameters
The empty defaultSearchValues object might cause issues if the search configuration expects certain default values. Consider initializing it with the expected search parameters from MicroplanCampaignSearchConfig.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myMicroplanConfig.js (2)
73-89: Verify column customization implementation
All columns are marked with additionalCustomization: true, but the implementation details are not clear. Please ensure the customization logic is properly implemented.
#!/bin/bash
# Search for column customization implementation
rg -A 10 "additionalCustomization.*PlanConfiguration" --type js63-63: Verify the campaign type population function
The dropdown uses a custom function populateCampaignTypeReqCriteria. Please ensure this function is properly implemented and available.
✅ Verification successful
Function is properly implemented and available
The custom function populateCampaignTypeReqCriteria is correctly implemented in the UICustomizations.js file. It's used consistently across different campaign configurations (ongoing, completed, upcoming, drafts) to populate campaign type dropdown options by making an MDMS API call to fetch project types. The implementation includes proper error handling and data transformation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the custom function implementation
rg -A 5 "populateCampaignTypeReqCriteria" --type js
Length of output: 65
Script:
#!/bin/bash
# Search for the function with a broader context
rg -A 10 "populateCampaignTypeReqCriteria" --type js --type jsx --type ts --type tsx
# Search in campaign manager related files
fd . health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager -e js -e jsx -e ts -e tsx --exec rg -l "populateCampaignTypeReqCriteria" {}
# Search for any campaign type related functions
rg -A 5 "campaign.*type.*criteria" --type js --type jsx --type ts --type tsx -i
Length of output: 694
Script:
#!/bin/bash
# Let's check the implementation in UICustomizations.js
cat health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
# Also check the usage in other config files
cat health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myCampaignConfig.js
Length of output: 62242
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js (2)
17-17: LGTM! Import statement follows existing patterns.
The import statement for MyMicroplans is correctly placed with other page component imports.
Line range hint 92-106: Verify session storage cleanup requirements for microplan route.
The useEffect cleanup logic specifically checks for 'setup-campaign' and 'response' routes. Verify if the new microplan route requires similar session storage cleanup.
Let's check for any session storage usage in the MyMicroplans component:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myCampaignConfig.js (1)
527-532: Verify impact of removing CAMPAIGN_END_DATE field
The removal of the campaign end date field could impact features that depend on campaign duration tracking. Please ensure:
- No existing features rely on this field
- Campaign lifecycle management remains intact without the end date
.../web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js
Show resolved
Hide resolved
.../web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js
Outdated
Show resolved
Hide resolved
| const onTabChange = (n) => { | ||
|
|
||
| setTabData((prev) => prev.map((i, c) => ({ ...i, active: c === n ? true : false }))); //setting tab enable which is being clicked | ||
| // setConfig(TabSearchconfig?.TabSearchconfig?.[n]);// as per tab number filtering the config | ||
| const url = new URL(window.location.href); | ||
| url.searchParams.set("tabId", `${n}`); | ||
| window.history.replaceState({}, "", url); | ||
|
|
||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Fix undefined tabData state
The onTabChange function references tabData and setTabData but these are not defined in the component. The state needs to be initialized using useState hook.
- Remove or properly implement the
onTabChangefunction as it's currently not being used by any tab component in the rendered JSX - If tabs are needed, initialize the tabData state:
const [tabData, setTabData] = useState([]);
🔗 Analysis chain
Fix incomplete tab implementation
The onTabChange function has several issues:
- It references undefined variables (tabData, setTabData)
- Contains commented code suggesting incomplete implementation
- Updates URL params but the tab state isn't properly managed
Let me search for the state definitions and React imports in the specific file to verify if tabData is properly initialized.
Let me check the complete file content to see how tabData is initialized and used in the component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for tab-related state definitions in the file
rg "tabData|setTabData"
Length of output: 88832
Script:
#!/bin/bash
# Search for tabData state definition in the specific file
rg -A 5 "tabData|setTabData" "health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js"
# Search for useState imports
rg "useState" "health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js"
Length of output: 806
Script:
#!/bin/bash
# Get more context around the file to see tabData initialization
cat "health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js"
Length of output: 1947
🧰 Tools
🪛 Biome
[error] 24-24: 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)
...o-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js
Outdated
Show resolved
Hide resolved
| resultsJsonPath: "PlanConfiguration", | ||
|
|
||
| enableColumnSort: true, | ||
| // resultsJsonPath: "mdms", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented code
Remove the commented out resultsJsonPath line as it's no longer needed.
- // resultsJsonPath: "mdms",📝 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.
| // resultsJsonPath: "mdms", |
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js
Outdated
Show resolved
Hide resolved
| data.body.PlanConfigurationSearchCriteria.tenantId = Digit.ULBService.getCurrentTenantId(); | ||
| data.body.PlanConfigurationSearchCriteria.userUuid = Digit.UserService.getUser().info.uuid; | ||
| // delete data.body.PlanConfigurationSearchCriteria.pagination | ||
| data.body.PlanConfigurationSearchCriteria.status = status?.status; |
There was a problem hiding this comment.
Verify correct assignment of status in PlanConfigurationSearchCriteria
At line 317, data.body.PlanConfigurationSearchCriteria.status = status?.status;. Since status is destructured from data?.state?.searchForm || {}, ensure that status?.status is the intended value. It might need to be status?.code or simply status.
Please confirm that the correct value is being assigned to status.
...-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
Show resolved
Hide resolved
|
|
||
| case "CAMPAIGN_TYPE": | ||
| if (value && value != "NA") { | ||
| return <p>{t(Digit.Utils.locale.getTransformedLocale("MICROPLAN_TYPE_" + value))}</p>; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use template literals instead of string concatenation
Line 348 concatenates strings using +. For better readability and to adhere to best practices, use template literals.
Modify line 348 as follows:
- return <p>{t(Digit.Utils.locale.getTransformedLocale("MICROPLAN_TYPE_" + value))}</p>;
+ return <p>{t(Digit.Utils.locale.getTransformedLocale(`MICROPLAN_TYPE_${value}`))}</p>;📝 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 <p>{t(Digit.Utils.locale.getTransformedLocale("MICROPLAN_TYPE_" + value))}</p>; | |
| return <p>{t(Digit.Utils.locale.getTransformedLocale(`MICROPLAN_TYPE_${value}`))}</p>; |
🧰 Tools
🪛 Biome
[error] 348-348: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| data.body.PlanConfigurationSearchCriteria.name = name; | ||
| data.body.PlanConfigurationSearchCriteria.tenantId = Digit.ULBService.getCurrentTenantId(); | ||
| data.body.PlanConfigurationSearchCriteria.userUuid = Digit.UserService.getUser().info.uuid; | ||
| // delete data.body.PlanConfigurationSearchCriteria.pagination | ||
| data.body.PlanConfigurationSearchCriteria.status = status?.status; | ||
| data.body.PlanConfigurationSearchCriteria.name = data?.state?.searchForm?.microplanName; |
There was a problem hiding this comment.
Avoid duplicate assignments to PlanConfigurationSearchCriteria.name
In the preProcess method, data.body.PlanConfigurationSearchCriteria.name is assigned twice, at lines 313 and 318. This could lead to unintended behavior or confusion.
Consider consolidating the assignments to ensure only one definitive value is set:
- data.body.PlanConfigurationSearchCriteria.name = name;
...
- data.body.PlanConfigurationSearchCriteria.name = data?.state?.searchForm?.microplanName;
+ data.body.PlanConfigurationSearchCriteria.name = data?.state?.searchForm?.microplanName || name;📝 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.body.PlanConfigurationSearchCriteria.name = name; | |
| data.body.PlanConfigurationSearchCriteria.tenantId = Digit.ULBService.getCurrentTenantId(); | |
| data.body.PlanConfigurationSearchCriteria.userUuid = Digit.UserService.getUser().info.uuid; | |
| // delete data.body.PlanConfigurationSearchCriteria.pagination | |
| data.body.PlanConfigurationSearchCriteria.status = status?.status; | |
| data.body.PlanConfigurationSearchCriteria.name = data?.state?.searchForm?.microplanName; | |
| data.body.PlanConfigurationSearchCriteria.name = data?.state?.searchForm?.microplanName || name; | |
| data.body.PlanConfigurationSearchCriteria.tenantId = Digit.ULBService.getCurrentTenantId(); | |
| data.body.PlanConfigurationSearchCriteria.userUuid = Digit.UserService.getUser().info.uuid; | |
| // delete data.body.PlanConfigurationSearchCriteria.pagination | |
| data.body.PlanConfigurationSearchCriteria.status = status?.status; |
...o-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js
Outdated
Show resolved
Hide resolved
...o-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js
Outdated
Show resolved
Hide resolved
...o-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js
Outdated
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
Outdated
Show resolved
Hide resolved
...ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myMicroplanConfig.js
Outdated
Show resolved
Hide resolved
...ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myMicroplanConfig.js
Outdated
Show resolved
Hide resolved
...ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myMicroplanConfig.js
Outdated
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range comments (6)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignName.js (2)
Line range hint
41-54: Consolidate error handling logicThe error handling is spread across multiple useEffect hooks, making it harder to maintain and understand the validation flow.
Consider consolidating the validation logic:
+ const validateName = (name, isSubmitting) => { + if (isSubmitting && !name) { + return "CAMPAIGN_FIELD_ERROR_MANDATORY"; + } + if (startValidation && !name) { + return "CAMPAIGN_NAME_FIELD_ERROR"; + } + return null; + }; useEffect(() => { - if (props?.props?.isSubmitting && !name) { - setError({ message: "CAMPAIGN_FIELD_ERROR_MANDATORY" }); - } else { - setError(null); - } - }, [props?.props?.isSubmitting]); - - useEffect(() => { - if (startValidation && !name) { - setError({ message: "CAMPAIGN_NAME_FIELD_ERROR" }); - } else if (name) { - setError(null); - onSelect("campaignName", name); - } + const errorMessage = validateName(name, props?.props?.isSubmitting); + setError(errorMessage ? { message: errorMessage } : null); + if (!errorMessage && name) { + onSelect("campaignName", name); + } - }, [name, props?.props?.sessionData?.HCM_CAMPAIGN_NAME?.campaignName]); + }, [name, props?.props?.isSubmitting, startValidation]);
Line range hint
65-73: Improve URL parameter management and event handlingThe current implementation has potential race conditions with window events and scattered URL parameter management.
Consider these improvements:
- useEffect(() =>{ - setKey(currentKey); - setCurrentStep(currentKey); - }, [currentKey]) - useEffect(() => { - updateUrlParams({ key: key }); - window.dispatchEvent(new Event("checking")); - }, [key]); + useEffect(() => { + if (currentKey) { + const newKey = parseInt(currentKey); + setKey(newKey); + setCurrentStep(newKey); + updateUrlParams({ key: newKey }); + // Use a more specific custom event name and add necessary data + window.dispatchEvent(new CustomEvent('campaignKeyUpdate', { + detail: { key: newKey } + })); + } + }, [currentKey]);health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Line range hint
122-126: Fix undefined variable and extract condition checkThere are several issues in this code segment:
productTypeis undefined, which could cause unexpected behavior- The condition check is duplicated between onClick and onFocus handlers
Apply this fix:
const checkIfMicroplan = () => { return source === "microplan"; }; // In JSX onClick={(e) => { if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate && !checkIfMicroplan()) { setShowPopUp(true); return; } }} onFocus={(e) => { if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate && !checkIfMicroplan()) { setShowPopUp(true); return; } }}🧰 Tools
🪛 Biome
[error] 119-135: 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/utils/setupCampaignValidators.js (3)
Line range hint
219-221: Fix date comparison logicThere's a bug in the date comparison. The
getTime()method is used on one side but not the other.Apply this fix:
- } else if (endDateObj.getTime() < startDateObj) { + } else if (endDateObj.getTime() < startDateObj.getTime()) {
Line range hint
1-400: Standardize error handling patternThe error handling pattern varies across different validation cases. Some use direct string keys, others use template literals with
t(), and some include additional parameters.Consider standardizing the error handling pattern:
- Use consistent error message keys
- Always use the translation function
- Include relevant parameters in the error object
Example pattern:
setShowToast({ key: "error", label: errorKey, labelParams: params, // Optional parameters for translation transitionTime: time // Optional transition time });
Line range hint
286-321: Add security validations for file uploadsThe file upload validation lacks important security checks:
- File type validation
- File size limits
- Path traversal prevention
Consider adding these security checks:
case "uploadBoundary": + const allowedTypes = ['csv', 'xlsx']; + const maxSize = 5 * 1024 * 1024; // 5MB + + if (!formData?.uploadBoundary?.uploadedFile?.[0]) { + setShowToast({ key: "error", label: t("HCM_FILE_MISSING") }); + return false; + } + + const file = formData.uploadBoundary.uploadedFile[0]; + const fileType = file.name.split('.').pop().toLowerCase(); + + if (!allowedTypes.includes(fileType)) { + setShowToast({ key: "error", label: t("HCM_INVALID_FILE_TYPE") }); + return false; + } + + if (file.size > maxSize) { + setShowToast({ key: "error", label: t("HCM_FILE_TOO_LARGE") }); + return false; + } + if (formData?.uploadBoundary?.isValidation) {Apply similar changes to
uploadFacilityanduploadUsercases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignName.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js(7 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myMicroplanConfig.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js(1 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/pages/employee/index.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/setupCampaignValidators.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignName.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myMicroplanConfig.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.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/pages/employee/index.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/setupCampaignValidators.js (1)
Pattern **/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
[error] 345-345: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js
[error] 20-27: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (9)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js (2)
1-5: LGTM! Imports are well-organized and necessary.
All imported modules are being used in the component, and the import structure follows good practices.
7-9: LGTM! Component structure follows React best practices.
The component is properly defined as a functional component with necessary hooks initialization.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js (1)
41-44:
Fix incorrect URL query parameter format.
The URL format for query parameters is incorrect. The first parameter should use ? instead of &.
- link: `/${window?.contextPath}/employee/campaign/setup-microplan&userId=${userId}&status=${microplanStatus}`,
+ link: `/${window?.contextPath}/employee/campaign/setup-microplan?userId=${userId}&status=${microplanStatus}`,The previous review comments about route path mismatch and translation keys are still valid and should be addressed.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myMicroplanConfig.js (2)
48-64: 🧹 Nitpick (assertive)
Remove commented code block
Remove the commented campaign type dropdown configuration to improve code maintainability.
72-88: Verify additionalCustomization implementation
The columns are configured with additionalCustomization: true, but we need to ensure the customization logic is properly implemented.
✅ Verification successful
Implementation for additionalCustomization is properly handled
The codebase shows that timestamp formatting is properly implemented:
- The
lastModifiedTimefield is handled byDigit.DateUtils.ConvertEpochToDate()in UICustomizations.js - The codebase has consistent timestamp formatting using various date utilities:
Digit.DateUtils.ConvertEpochToDate()for display formattingDigit.Utils.date.convertDateToEpoch()for epoch conversionepochToDateTime()for timeline displays
The additionalCustomization flag is working as intended to enable this date formatting functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for additionalCustomization implementation for these columns
rg -A 10 "additionalCustomization.*MicroplanCampaignSearchConfig" --type js
# Search for timestamp formatting implementation
rg -A 5 "lastModifiedTime.*format" --type js
Length of output: 124
Script:
#!/bin/bash
# Search for any implementation related to additionalCustomization in campaign manager
rg -A 10 "additionalCustomization" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/
# Search for any table component or search result component that might handle these customizations
rg -A 10 "SearchResult|Table" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/
# Look for timestamp formatting in campaign manager
rg -A 5 "lastModifiedTime" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/
Length of output: 90409
Script:
#!/bin/bash
# Search for the implementation of additionalCustomization in UICustomizations.js for timestamp formatting
rg -A 10 "LAST_MODIFIED_TIME.*additionalCustomization" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
# Look for any utility functions that handle timestamp formatting
rg -A 5 "epochToDateTime|formatDate" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/
Length of output: 9831
Script:
#!/bin/bash
# Search for the implementation of additionalCustomization in UICustomizations.js for lastModifiedTime
rg -B 5 -A 15 "case \"LAST_MODIFIED_TIME\"" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
# Look for any date formatting utilities in the codebase
rg -A 5 "Digit.*date|DateFormatter|formatDateTime" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/
Length of output: 37204
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignName.js (1)
7-7: 🧹 Nitpick (assertive)
Verify props usage and consider prop drilling alternatives
The component receives several props through destructuring, but some might be unnecessary. Additionally, the nested props.props structure suggests potential prop drilling.
Let's analyze the props usage:
Consider using React Context or a state management solution if this component is deeply nested in the component tree.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js (1)
17-17: LGTM!
The import statement follows the existing pattern and is correctly placed with other page imports.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/setupCampaignValidators.js (1)
194-195: 🛠️ Refactor suggestion
Verify the impact of reducing campaign name length limit
The maximum allowed length for campaign names has been significantly reduced from 250 to 30 characters. This is a breaking change that could affect existing campaigns and integrations.
Let's verify the impact:
Consider improving the validation:
- } else if (formData.campaignName.length > 30) {
- setShowToast({ key: "error", label: "CAMPAIGN_NAME_LONG_ERROR" });
+ } else if (formData.campaignName.trim().length > 30) {
+ setShowToast({
+ key: "error",
+ label: "CAMPAIGN_NAME_LONG_ERROR",
+ labelParams: { MAX_LENGTH: 30, CURRENT_LENGTH: formData.campaignName.trim().length }
+ });The improvements:
- Use
trim()to accurately count characters without leading/trailing spaces - Add dynamic parameters to the error message for better user feedback
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (1)
309-309:
Fix Typo: Use 'url.includes' Instead of 'urlIncludes'
At line 309, urlIncludes appears to be a typo. The correct method is url.includes.
Apply this diff to fix the typo:
- const queryString = urlIncludes("?") ? url.split("?")[1] : url.split("&").slice(1).join("&");
+ const queryString = url.includes("?") ? url.split("?")[1] : url.split("&").slice(1).join("&");Likely invalid or redundant comment.
| const { t } = useTranslation(); | ||
| const history = useHistory(); | ||
| const onClickRow = ({ original: row }) => { | ||
| console.log("row" , row); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove console.log statement.
Remove debugging statements before deploying to production.
- console.log("row" , row);📝 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("row" , row); |
| <InboxSearchComposer | ||
| configs={MicroplanCampaignSearchConfig?.[0]} | ||
| additionalConfig={{ | ||
| resultsTable: { | ||
| onClickRow, | ||
| }, | ||
| }} | ||
| ></InboxSearchComposer> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use self-closing tag for InboxSearchComposer.
Since the InboxSearchComposer component doesn't have any children, it should use a self-closing tag.
- <InboxSearchComposer
- configs={MicroplanCampaignSearchConfig?.[0]}
- additionalConfig={{
- resultsTable: {
- onClickRow,
- },
- }}
- ></InboxSearchComposer>
+ <InboxSearchComposer
+ configs={MicroplanCampaignSearchConfig?.[0]}
+ additionalConfig={{
+ resultsTable: {
+ onClickRow,
+ },
+ }}
+ />📝 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.
| <InboxSearchComposer | |
| configs={MicroplanCampaignSearchConfig?.[0]} | |
| additionalConfig={{ | |
| resultsTable: { | |
| onClickRow, | |
| }, | |
| }} | |
| ></InboxSearchComposer> | |
| </div> | |
| <InboxSearchComposer | |
| configs={MicroplanCampaignSearchConfig?.[0]} | |
| additionalConfig={{ | |
| resultsTable: { | |
| onClickRow, | |
| }, | |
| }} | |
| /> | |
| </div> |
🧰 Tools
🪛 Biome
[error] 20-27: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
| const updatedName = row.name.replace(/&/g, "and"); | ||
| history.push(`/${window.contextPath}/employee/campaign/setup-campaign?id=${row.campaignId}&draft=true&fetchBoundary=true&draftBoundary=true&source=microplanµName=${updatedName}`); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing string handling and URL construction.
The current implementation has potential areas for improvement:
- The string replacement might need more robust handling for special characters
- URL parameters should be properly encoded
- const updatedName = row.name.replace(/&/g, "and");
- history.push(`/${window.contextPath}/employee/campaign/setup-campaign?id=${row.campaignId}&draft=true&fetchBoundary=true&draftBoundary=true&source=microplanµName=${updatedName}`);
+ const updatedName = encodeURIComponent(row.name.replace(/&/g, "and"));
+ const params = new URLSearchParams({
+ id: row.campaignId,
+ draft: "true",
+ fetchBoundary: "true",
+ draftBoundary: "true",
+ source: "microplan",
+ microName: updatedName
+ });
+ history.push(`/${window.contextPath}/employee/campaign/setup-campaign?${params}`);📝 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 updatedName = row.name.replace(/&/g, "and"); | |
| history.push(`/${window.contextPath}/employee/campaign/setup-campaign?id=${row.campaignId}&draft=true&fetchBoundary=true&draftBoundary=true&source=microplanµName=${updatedName}`); | |
| const updatedName = encodeURIComponent(row.name.replace(/&/g, "and")); | |
| const params = new URLSearchParams({ | |
| id: row.campaignId, | |
| draft: "true", | |
| fetchBoundary: "true", | |
| draftBoundary: "true", | |
| source: "microplan", | |
| microName: updatedName | |
| }); | |
| history.push(`/${window.contextPath}/employee/campaign/setup-campaign?${params}`); |
| const userId = Digit.UserService.getUser().info.uuid; | ||
| const microplanStatus = "RESOURCE_ESTIMATIONS_APPROVED" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider improving variable management and error handling.
- The hardcoded
microplanStatusshould be moved to a constants file for better maintainability. - Add error handling for the user service call.
+ import { MICROPLAN_STATUSES } from "../constants";
+
const CampaignCard = () => {
- const userId = Digit.UserService.getUser().info.uuid;
- const microplanStatus = "RESOURCE_ESTIMATIONS_APPROVED"
+ const userId = Digit.UserService.getUser()?.info?.uuid;
+ if (!userId) {
+ throw new Error("User information not available");
+ }
+ const microplanStatus = MICROPLAN_STATUSES.RESOURCE_ESTIMATIONS_APPROVED;Committable suggestion skipped: line range outside the PR's diff.
|
|
||
|
|
||
| const tenantId = Digit.ULBService.getCurrentTenantId(); | ||
| const mdms_context_path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable declaration
The mdms_context_path variable is declared but never used in the configuration.
- const mdms_context_path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2";📝 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 mdms_context_path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2"; |
| //if user comes from set up microplan | ||
| } else if (source === "microplan") { | ||
| setCurrentKey(2); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the microplan source condition.
The condition for source === "microplan" lacks proper documentation explaining when and why this flow is triggered. Add comments to clarify the business logic.
+ // When campaign is initiated from microplan setup,
+ // skip to step 2 to maintain workflow continuity
} else if (source === "microplan") {
setCurrentKey(2);📝 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 user comes from set up microplan | |
| } else if (source === "microplan") { | |
| setCurrentKey(2); | |
| // When campaign is initiated from microplan setup, | |
| // skip to step 2 to maintain workflow continuity | |
| } else if (source === "microplan") { | |
| setCurrentKey(2); |
| const source = searchParams.get("source"); | ||
| const microplanName = searchParams.get("microName"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused variable declaration.
The microplanName variable is declared but never used in the code. Either remove it if not needed or document its intended purpose.
const source = searchParams.get("source");
- const microplanName = searchParams.get("microName");📝 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 source = searchParams.get("source"); | |
| const microplanName = searchParams.get("microName"); | |
| const source = searchParams.get("source"); |
| //if user comes from set up microplan | ||
| } else if (source === "microplan") { | ||
| setCurrentKey(2); | ||
| //if the campaign is in draft and the start date is passed | ||
| }else if (isDateRestricted === "true") { | ||
| setCurrentKey(3); | ||
| } | ||
| else { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve code readability and fix indentation.
The else-if chain has inconsistent indentation and could be simplified for better readability. Consider using early returns or consistent indentation.
- //if user comes from set up microplan
- } else if (source === "microplan") {
- setCurrentKey(2);
- //if the campaign is in draft and the start date is passed
- }else if (isDateRestricted === "true") {
- setCurrentKey(3);
- }
+ // Handle microplan setup flow
+ } else if (source === "microplan") {
+ setCurrentKey(2);
+ // Handle date restricted campaigns
+ } else if (isDateRestricted === "true") {
+ setCurrentKey(3);
+ }Also, consider adding JSDoc comments to explain the purpose of different conditions and their impact on the campaign setup flow.
📝 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 user comes from set up microplan | |
| } else if (source === "microplan") { | |
| setCurrentKey(2); | |
| //if the campaign is in draft and the start date is passed | |
| }else if (isDateRestricted === "true") { | |
| setCurrentKey(3); | |
| } | |
| else { | |
| // Handle microplan setup flow | |
| } else if (source === "microplan") { | |
| setCurrentKey(2); | |
| // Handle date restricted campaigns | |
| } else if (isDateRestricted === "true") { | |
| setCurrentKey(3); | |
| } else { |
| retryCampaign(row, searchResult); | ||
| break; |
There was a problem hiding this comment.
'searchResult' May Be Undefined in 'onActionSelect'
At lines 1030-1031, retryCampaign(row, searchResult); is called within the onActionSelect function, but searchResult is not defined in this scope. This could lead to a ReferenceError when onActionSelect is executed.
Consider passing searchResult as a parameter to onActionSelect or retrieving it within the function. Here's a possible fix:
- const onActionSelect = (value, row) => {
+ const onActionSelect = (value, row, searchResult) => {
switch (value?.code) {
case "ACTION_LABEL_VIEW_TIMELINE":
setTimeline(true);
break;
case "ACTION_LABEL_RETRY":
retryCampaign(row, searchResult);
break;Ensure that when onActionSelect is called, searchResult is passed appropriately.
Committable suggestion skipped: line range outside the PR's diff.
| options={[ | ||
| { key: 1, code: "ACTION_LABEL_VIEW_TIMELINE", i18nKey: t("ACTION_LABEL_VIEW_TIMELINE") }, | ||
| { key: 2, code: "ACTION_LABEL_RETRY", i18nKey: t("ACTION_LABEL_RETRY") }, | ||
| ].filter((obj) => Digit.Utils.didEmployeeHasAtleastOneRole(["SYSTEM_ADMINISTRATOR"] || obj?.key != 2))} //added retry for system adminstrator for failed campaign |
There was a problem hiding this comment.
Correct Logical Error in Filtering Action Options
The filter function in lines 1060-1063 may not work as intended due to incorrect use of the logical || operator. The condition should ensure that the 'Retry' option is only available to users with the SYSTEM_ADMINISTRATOR role.
Adjust the filter logic as follows:
- ].filter((obj) => Digit.Utils.didEmployeeHasAtleastOneRole(["SYSTEM_ADMINISTRATOR"] || obj?.key != 2))
+ ].filter((obj) => obj?.key !== 2 || Digit.Utils.didEmployeeHasAtleastOneRole(["SYSTEM_ADMINISTRATOR"]))This change ensures that:
- If the option's
keyis not2(not 'Retry'), it is included. - If the option's
keyis2(the 'Retry' action), it is included only if the user has theSYSTEM_ADMINISTRATORrole.
📝 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.
| options={[ | |
| { key: 1, code: "ACTION_LABEL_VIEW_TIMELINE", i18nKey: t("ACTION_LABEL_VIEW_TIMELINE") }, | |
| { key: 2, code: "ACTION_LABEL_RETRY", i18nKey: t("ACTION_LABEL_RETRY") }, | |
| ].filter((obj) => Digit.Utils.didEmployeeHasAtleastOneRole(["SYSTEM_ADMINISTRATOR"] || obj?.key != 2))} //added retry for system adminstrator for failed campaign | |
| options={[ | |
| { key: 1, code: "ACTION_LABEL_VIEW_TIMELINE", i18nKey: t("ACTION_LABEL_VIEW_TIMELINE") }, | |
| { key: 2, code: "ACTION_LABEL_RETRY", i18nKey: t("ACTION_LABEL_RETRY") }, | |
| ].filter((obj) => obj?.key !== 2 || Digit.Utils.didEmployeeHasAtleastOneRole(["SYSTEM_ADMINISTRATOR"]))} //added retry for system adminstrator for failed campaign |
| @@ -92,6 +93,8 @@ const CampaignBreadCrumb = ({ location, defaultPath }) => { | |||
| */ | |||
| const App = ({ path, BOUNDARY_HIERARCHY_TYPE, hierarchyData }) => { | |||
| const location = useLocation(); | |||
| const userId = Digit.UserService.getUser().info.uuid; | |||
There was a problem hiding this comment.
remove them from here, it is just query params from the link.
keep it similar to update campaign URL
Also URL say setup-from-microplan
.../web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js
Outdated
Show resolved
Hide resolved
| }, | ||
| }, | ||
| masterName: "commonUiConfig", | ||
| moduleName: "MicroplanCampaignSearchConfig", |
There was a problem hiding this comment.
just config, moduleName is differnt for microplan UI and here ?
...o-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js
Outdated
Show resolved
Hide resolved
...o-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Line range hint
122-126: Extract complex condition into a readable functionThe popup display condition is becoming complex. Consider extracting it into a descriptive function for better readability and maintainability.
const shouldShowPopup = () => { return props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate && source !== "microplan"; }; // Usage if (shouldShowPopup()) { setShowPopUp(true); return; }🧰 Tools
🪛 Biome
[error] 119-135: 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/index.js (1)
Line range hint
31-89: Add breadcrumb entry for setup-from-microplan routeTo maintain navigation consistency, add a breadcrumb entry for the new route in the crumbs array:
const crumbs = [ // ... existing crumbs + { + path: pathVar === "setup-from-microplan" ? "" : `/${window?.contextPath}/employee/campaign/setup-from-microplan`, + content: t("SETUP_FROM_MICROPLAN"), + show: pathVar === "setup-from-microplan" ? true : false, + }, // ... remaining crumbs ];Also ensure to add the translation key "SETUP_FROM_MICROPLAN" to your localization files.
📜 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/CampaignCard.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js (1)
Pattern **/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js
[error] 33-40: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (5)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/MyMicroplans.js (2)
28-42: Component structure looks good!
The component follows React best practices with proper use of fragments and clear separation of concerns. The search configuration is properly passed to the InboxSearchComposer.
🧰 Tools
🪛 Biome
[error] 33-40: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
18-23: 🧹 Nitpick (assertive)
Clarify the status of user-specific filtering.
The commented code suggests that user-specific filtering was intended but not implemented. This could affect the functionality of the microplans list.
Let me check if this functionality is implemented elsewhere:
Would you like me to help implement the user-specific filtering functionality?
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js (3)
17-17: LGTM!
The import statement follows the existing pattern and is correctly placed with other page component imports.
96-96: LGTM!
The user ID retrieval follows the standard practice using Digit.UserService.
154-154: LGTM!
The route configuration:
- Uses the correct path format as suggested
- Follows the existing routing pattern
- Is properly placed within the AppContainer
| // export const updateUrlParams=(params) =>{ | ||
| // const url = new URL(window.location.href); | ||
| // Object.entries(params).forEach(([key, value]) => { | ||
| // url.searchParams.set(key, value); | ||
| // }); | ||
| // window.history.replaceState({}, "", url); | ||
| // } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented-out utility function.
The updateUrlParams function is commented out and unused. If this functionality is needed in the future, it can be retrieved from version control history.
| { | ||
| label: t("ACTION_TEST_SETUP_CAMPAIGN_FROM_MICROPLAN"), | ||
| link: `/${window?.contextPath}/employee/campaign/setup-campaign`, | ||
| link: `/${window?.contextPath}/employee/campaign/setup-from-microplan&userId=${userId}&status=${microplanStatus}`, |
There was a problem hiding this comment.
💡 Codebase verification
Fix URL query parameter syntax and ensure route consistency
The search results confirm two issues:
- The URL uses incorrect query parameter syntax (
&instead of?for first parameter) - Other similar links in the codebase consistently use the correct
?syntax for query parameters
Apply this fix:
- link: `/${window?.contextPath}/employee/campaign/setup-from-microplan&userId=${userId}&status=${microplanStatus}`,
+ link: `/${window?.contextPath}/employee/campaign/setup-from-microplan?userId=${encodeURIComponent(userId)}&status=${encodeURIComponent(microplanStatus)}`,🔗 Analysis chain
Fix URL query parameter syntax and add URL encoding
The link URL has incorrect query parameter syntax and missing URL encoding:
Apply this fix:
- link: `/${window?.contextPath}/employee/campaign/setup-from-microplan&userId=${userId}&status=${microplanStatus}`,
+ link: `/${window?.contextPath}/employee/campaign/setup-from-microplan?userId=${encodeURIComponent(userId)}&status=${encodeURIComponent(microplanStatus)}`,Let's verify the URL format consistency across the codebase:
The search results confirm that there's a mismatch between the route definition and the link URL. Let me verify the complete routing structure to ensure we provide accurate guidance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check URL patterns in campaign setup routes
rg "setup-from-microplan" --type js -A 2
Length of output: 962
Script:
#!/bin/bash
# Check the campaign routes and URL patterns
rg "campaign.*Route" --type js -A 3
# Also check other similar link constructions
rg "link:.*campaign" --type js -A 1
Length of output: 3616
| @@ -19,6 +19,7 @@ const CampaignSelection = ({ onSelect, formData, formState, ...props }) => { | |||
| const searchParams = new URLSearchParams(location.search); | |||
| const [currentStep , setCurrentStep] = useState(1); | |||
| const currentKey = searchParams.get("key"); | |||
| const source = searchParams.get("source"); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate URL parameter handling into a custom hook
Following up on previous review feedback, the URL parameter handling should be extracted into a custom hook for better maintainability and reusability.
Create a new hook useCampaignParams:
const useCampaignParams = () => {
const validSources = ["microplan"];
const searchParams = new URLSearchParams(location.search);
const rawSource = searchParams.get("source");
return {
source: validSources.includes(rawSource) ? rawSource : null,
key: searchParams.get("key") ? parseInt(searchParams.get("key")) : 1,
isMicroplan: () => rawSource === "microplan",
updateParams: (params) => {
const url = new URL(window.location.href);
Object.entries(params).forEach(([key, value]) => {
url.searchParams.set(key, value);
});
window.history.replaceState({}, "", url);
}
};
};| disabled = {source === "microplan"} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove whitespace and extract condition into a function
- Remove the unnecessary whitespace after the disabled prop
- Extract the condition into a descriptive function as suggested in previous review
- disabled = {source === "microplan"}
-
+ disabled={checkIfMicroplan()}Add this function at the component level:
const checkIfMicroplan = () => {
return source === "microplan";
};
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
These changes enhance the functionality and user experience of the campaign management interface.