Changes to supervisor start flow #1654
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple components in the microplan module. Key changes include the addition of an Changes
Possibly related PRs
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 (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/ActivityCard.js (1)
Line range hint
12-36: Consider extracting styles and improving grid responsiveness.While the component structure is good, there are some improvements that could enhance maintainability and user experience:
- Move inline styles to CSS modules
- Enhance grid layout for better responsiveness
- Consider adding aria-labels for accessibility
Example implementation:
+ import styles from './ActivityCard.module.css' // ... - <Card className="fsm" style={{ backgroundColor: "transparent", boxShadow: "none", paddingTop: "0" }}> + <Card className={`fsm ${styles.transparentCard}`}> // ... - <div style={{ display: "grid", gridTemplateColumns: "33.3% 33.3% 33.3%", textAlign: "-webkit-center", gap: "10px" }}> + <div className={styles.cardGrid}> // ... <Card key={i.name} - style={{ minWidth: "100px", cursor: i.disable ? "not-allowed" : "pointer", opacity: i.disable ? 0.6 : 1, margin: "10px" }} + className={`${styles.actionCard} ${i.disable ? styles.disabled : ''}`} + aria-disabled={i.disable} + aria-label={t(i.name)}Create a CSS module file:
.transparentCard { background-color: transparent; box-shadow: none; padding-top: 0; } .cardGrid { display: grid; grid-template-columns: repeat(auto-fit, minmax(200px, 1fr)); gap: 1rem; text-align: center; } .actionCard { min-width: 100px; margin: 10px; cursor: pointer; } .disabled { cursor: not-allowed; opacity: 0.6; }health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanCard.js (1)
Line range hint
5-13: Consider moving ROLES configuration to a central locationThe ROLES configuration should be maintained in a central configuration file rather than being defined within this component. This would:
- Improve maintainability
- Enable reuse across components
- Make role management more consistent
Consider:
- Moving ROLES to a separate configuration file (e.g.,
config/roles.js)- Creating a custom hook for role-based checks (e.g.,
useUserRoles)
📜 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/ActivityCard.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanCard.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/ActivityCard.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanCard.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js (1)
Pattern
**/*.js: check
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/ActivityCard.js (1)
Learnt from: Ramkrishna-egov PR: egovernments/DIGIT-Frontend#1545 File: health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/ActivityCard.js:30-35 Timestamp: 2024-10-17T12:00:51.749Z Learning: In our codebase, the 'Card' component is commonly used by passing children via the 'children' prop.
🔇 Additional comments (7)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/ActivityCard.js (1)
6-6: LGTM! Props destructuring follows best practices.The implementation correctly provides a default empty function for
onClickCardand uses rest operator for remaining props.health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanCard.js (1)
42-42: Verify translation key usage and role-checking patternsLet's verify the consistency of translation keys and role-checking patterns across the codebase.
✅ Verification successful
Based on the results, I can now generate the final response since we have clear evidence about the usage of these translation keys and role-checking patterns.
Translation keys and role-checking implementation are properly scoped
The verification shows that:
- The translation keys
MICROPLAN_MODULE_PROCESSandMICROPLAN_MODULE_SETUPare only used in this specific component, indicating they are properly scoped for this feature.- The supervisor role check (
isSupervisorLoggedIn) is consistently implemented within the same file, along with proper role-based link generation usingROLES.SUPERVISOR.- The conditional rendering based on the supervisor role is self-contained within this component and follows a clear pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for translation key usage echo "Checking translation key usage..." rg "MICROPLAN_MODULE_PROCESS|MICROPLAN_MODULE_SETUP" -t js # Check for similar role-checking patterns echo "Checking role-checking patterns..." rg "isSupervisorLoggedIn|ROLES\.SUPERVISOR" -t jsLength of output: 1014
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js (2)
131-131: Verify onClickCard prop handling in ActivityHomeCard.The addition of onClickCard prop looks good, but we should verify its implementation in the ActivityHomeCard component.
Let's verify the prop handling:
#!/bin/bash # Description: Check ActivityHomeCard implementation # Expected: Find onClickCard prop handling in ActivityCard component ast-grep --pattern 'const ActivityHomeCard = ({ $$$, onClickCard, $$$ }) => { $$$ }'
69-98: LGTM! Verify role-status combinations.The addition of optionKeys and role-based disable conditions looks good. The status checks are appropriately mapped to each activity's workflow stage.
Let's verify the role-status combinations are correctly configured:
✅ Verification successful
Let me try a different approach to verify the role configurations and workflow states.
Let me try one final verification to check the
isCardDisabledfunction implementation to ensure the role-status combinations are properly validated.
Based on the gathered evidence, I can now provide a final response:
Role-status combinations are correctly implemented
The verification shows that:
- The workflow states (
EXECUTION_TO_BE_DONE,CENSUS_DATA_APPROVAL_IN_PROGRESS, etc.) are properly defined in the UI configurations and consistently used across the application.- The
isCardDisabledfunction implementation correctly validates:
- User roles against the required roles for each action
- Current plan status against the valid statuses for each action
- The role-status combinations in the activity cards align with the workflow stages defined in
UICustomizations.jsThe disable logic is robust and follows proper role-based access control patterns:
- Population data approval requires
POPULATION_DATA_APPROVERorROOT_POPULATION_DATA_APPROVERroles duringEXECUTION_TO_BE_DONEorCENSUS_DATA_APPROVAL_IN_PROGRESSstates- Facility assignment requires
FACILITY_CATCHMENT_MAPPERorROOT_FACILITY_CATCHMENT_MAPPERroles duringCENSUS_DATA_APPROVEDstate- Plan estimation approval requires
PLAN_ESTIMATION_APPROVERorROOT_PLAN_ESTIMATION_APPROVERroles duringRESOURCE_ESTIMATION_IN_PROGRESSstate🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify role definitions exist in MDMS # Expected: Find role definitions matching those used in the code rg -A 2 '"code":.*("POPULATION_DATA_APPROVER"|"ROOT_POPULATION_DATA_APPROVER"|"FACILITY_CATCHMENT_MAPPER"|"ROOT_FACILITY_CATCHMENT_MAPPER"|"PLAN_ESTIMATION_APPROVER"|"ROOT_PLAN_ESTIMATION_APPROVER"|"MICROPLAN_VIEWER")'Length of output: 182
Script:
#!/bin/bash # Search for role definitions in JSON files fd -e json -x rg -l "POPULATION_DATA_APPROVER|ROOT_POPULATION_DATA_APPROVER|FACILITY_CATCHMENT_MAPPER|ROOT_FACILITY_CATCHMENT_MAPPER|PLAN_ESTIMATION_APPROVER|ROOT_PLAN_ESTIMATION_APPROVER|MICROPLAN_VIEWER" {} # Search for workflow states in the codebase rg "EXECUTION_TO_BE_DONE|CENSUS_DATA_APPROVAL_IN_PROGRESS|CENSUS_DATA_APPROVED|RESOURCE_ESTIMATION_IN_PROGRESS" -A 2Length of output: 7620
Script:
#!/bin/bash # Search for isCardDisabled function implementation ast-grep --pattern 'const isCardDisabled = ($$$) => { $$$ }' # Also search with ripgrep as fallback rg "function isCardDisabled|const isCardDisabled" -A 10Length of output: 4292
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js (3)
310-311: Verify that setting URL parameters to null removes themAt lines 310-311, you are updating URL parameters
isLastVerticalStepandinternalKeyby setting them tonull. Please confirm that setting these parameters tonulleffectively removes them from the URL or behaves as intended, ensuring no unintendednullvalues are introduced in the query string.
387-388: Verify that setting URL parameters to null removes themAt lines 387-388, you set
isFormulaLastVerticalStepandformulaInternalKeyin the URL parameters tonull. Please ensure that this action removes the parameters from the URL as expected and does not result innullstrings appearing in the URL.
544-545: Verify that setting URL parameters to null removes themAt lines 544-545, you again set
isLastVerticalStepandinternalKeyURL parameters tonull. Please confirm that this effectively removes these parameters from the URL and doesn't introduce unintended behavior due tonullvalues.
...th/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/ActivityCard.js
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanCard.js
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanCard.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js
Show resolved
Hide resolved
.../micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
Show resolved
Hide resolved
Summary by CodeRabbit
Release Notes
New Features
ActivityHomeCardto support click actions with a newonClickCardprop for improved interactivity.optionKeyinChooseActivityfor better identification of activity actions.updatePlanfunction to facilitate plan updates based on user interactions.Improvements
MicroplanCardbased on user roles.ChooseActivityfor improved user experience during updates.Bug Fixes