G1 - HR Module#228
Conversation
Implemented all HR workflows - G1 Pair 2
vikrantwiz02
left a comment
vikrantwiz02
left a comment
There was a problem hiding this comment.
Code Review — G1 HR Module (Frontend)
The refactor is meaningful — useFetchData hook reduces boilerplate across pages, the services/api.js centralisation is the right move, and SubstituteNomination/SubstituteInbox add genuinely new functionality. Critical blocker: the entire venv/ directory (1,099 Python files) is committed — this must be removed before merge. Other issues inline.
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
ESLint exhaustive-deps warning on deps. Passing the deps array as a direct variable to useEffect triggers the react-hooks/exhaustive-deps rule because ESLint cannot statically analyse it. This is a known pattern trade-off — if you keep it, add // eslint-disable-next-line react-hooks/exhaustive-deps above the useEffect to make the suppression explicit, or document the requirement that callers must memoize their deps argument.
| export const handle_leave_academic_responsibility = `${host}/hr2/api/handle_leave_academic_responsibility`; | ||
| export const handle_leave_administrative_responsibility = `${host}/hr2/api/handle_leave_administrative_responsibility`; | ||
| export const download_leave_form_pdf = `${host}/hr2/api/download_leave_form_pdf`; | ||
| export const download_leave_form_pdf = (formId) => |
There was a problem hiding this comment.
download_leave_form_pdf changed from a string constant to a function — breaking change for existing callers. Any component that previously used it as a URL string (e.g. href={download_leave_form_pdf}) will now render function (formId) {...} as the href. Audit all existing usages across the module before merging this change.
| <Layout> | ||
| <HR /> | ||
| </Layout> | ||
| } |
There was a problem hiding this comment.
/hr/* route has no RequireAuth guard. All other protected routes in this file (dashboard, academics, profile, examination, etc.) are wrapped in <RequireAuth>. The HR route is not. An unauthenticated user who navigates directly to /hr will hit the HR module instead of being redirected to login.
| role && | ||
| (facultyRoles.includes(role) || role.toLowerCase().includes("faculty")); | ||
|
|
||
| const filterModules = Modules.filter((module) => { |
There was a problem hiding this comment.
HR sidebar item is only shown for faculty roles — students and others cannot access it at all. The filter shows the HR module only when isFaculty is true. If students or admin staff also need HR access (e.g. leave forms), this gates them out entirely. Verify the intended access matrix and either expand the condition or document that HR is faculty-only.
| @@ -0,0 +1,565 @@ | |||
| // src/Modules/HR/services/hrApi.js | |||
There was a problem hiding this comment.
File header comment says src/Modules/HR/services/hrApi.js but the actual filename is api.js. Update the comment to match, or rename the file — stale path comments in the header cause confusion when searching the codebase.
|
Critical:
Fix: git rm -r --cached venv/
echo 'venv/' >> .gitignore
git add .gitignore
git commit -m "Remove committed venv, add to gitignore"
git pushAlso: |
G1 - HR Module