Fix/Skapp-3247 - #1957
Fix/Skapp-3247#1957erandi-rootcode wants to merge 9 commits into
Conversation
|
ThinuwanW
left a comment
ThinuwanW
left a comment
There was a problem hiding this comment.
🤖 Claude Code Review
This PR threads a new isSelfDirectTimeEntry Zustand flag plus self-target props through the timesheet tree so that attendance admins/managers can directly add and edit entries on their own timesheet when the manual-entry restriction is enabled. The store/type plumbing is consistent (all 5 setDirectManualTimeEntryEligibleEmployee call sites were paired with the new setter) and the backend already permits self direct entry, but routing the employee's own add/edit through the enterprise /direct-entry endpoint introduces a cache-invalidation gap that leaves the user's own daily log stale, and the page-level Add button now reaches a submit branch that reads the never-cleared selectedDailyRecord.timeRecordId. Several smaller consistency, memoization and derived-state issues are noted below.
Found 15 new issue(s): 🔴 4 important, 🟡 8 suggestion(s), 🟣 3 nit(s)
| ); | ||
| }} | ||
| isDailyLogLoading={isDailyLogLoading} | ||
| targetEmployeeId={selfTargetEmployeeId} |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
Self direct entries won't refresh the page they were made from. Passing targetEmployeeId/targetEmployeeDetails here routes the current user's own add/edit through useAddDirectTimeEntry/useEditDirectTimeEntry (useAddEntry.tsx takes the directManualTimeEntryEligibleEmployee branch). Those mutations call invalidateDirectTimeEntryQueries, which only invalidates manager-records, manager-work-summary, employee-daily-log-by-employeeId and employee-requests. This page reads employee-daily-log (useGetDailyLogs) and employee-work-summary (useGetEmployeeWorkSummary), and the header clock-in widget reads getAttendanceQueryKeys.employeeStatus() — none are invalidated, so after saving, the daily-log rows and the worked/break-hours KPI cards keep showing pre-save values until a manual refetch. The non-direct path (useAddManualTimeEntry/useEditClockInOut) invalidates all three via invalidateAttendanceTimeRecordQueries. Fix by adding attendanceQueryKeys.getEmployeeWorkSummary(), attendanceQueryKeys.getEmployeeDailyLog() and getAttendanceQueryKeys.employeeStatus() to the key list in src/enterprise/attendance/api/utils/invalidateAttendanceQueries.ts (or by reusing invalidateAttendanceTimeRecordQueries there).
| onPrimaryButtonClick={() => { | ||
| setDirectManualTimeEntryEligibleEmployee(null); | ||
| const isEligible = canDirectlyAddOrEditEntry && !!user?.userId; | ||
| setDirectManualTimeEntryEligibleEmployee( |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
Stale selectedDailyRecord can turn this add into an edit of an unrelated record. Setting a non-null directManualTimeEntryEligibleEmployee here makes the modal's submit take the direct-entry branch in useAddEntry.handleTimeEntrySubmit, which does const existingRecordId = selectedDailyRecord?.timeRecordId || undefined; and calls editDirectManualTimeEntryMutate (PATCH) whenever that id is truthy. selectedDailyRecord is never reset — employeeTimesheetModalSlice initializes it to undefined and EmployeeTimesheetPopupController's close effect clears only currentAddTimeChanges and directManualTimeEntryEligibleEmployee. So: user clicks a daily-log row (or visits the individual employee time report), closes the modal, then clicks "Add manual time entry" and picks a different date → the new times are PATCHed onto the previously selected record instead of creating a new entry. Before this PR the button set the eligible employee to null, so this branch was unreachable from this page. Fix: call setSelectedDailyRecord(undefined) (widen the setter to accept undefined) in this click handler, and ideally also reset it in the popup controller's close effect alongside the other resets.
| isDailyLogLoading={isDailyLogLoading} | ||
| targetEmployeeId={selfTargetEmployeeId} | ||
| targetEmployeeDetails={selfTargetEmployeeDetails} | ||
| isSelfTargetEntry={canDirectlyAddOrEditEntry} |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
isSelfTargetEntry uses the raw canDirectlyAddOrEditEntry while targetEmployeeId/targetEmployeeDetails on the two lines above use the narrower isSelfDirectEntryEligible (which also requires user?.userId). The three props are consumed together in TimesheetDailyRecordTableRow.handleRowActivate, so today this is harmless (the self branch is guarded by targetEmployeeDetails && targetEmployeeId), but the inconsistency is easy to misread and will break if the guard ever changes. Pass isSelfTargetEntry={isSelfDirectEntryEligible} for consistency.
| }} | ||
| isDailyLogLoading={isDailyLogLoading} | ||
| targetEmployeeId={selfTargetEmployeeId} | ||
| targetEmployeeDetails={selfTargetEmployeeDetails} |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
Supplying targetEmployeeDetails for the current user silently disables their own in-progress row. In TimesheetDailyRecordTableRow, isDirectEntryView = Boolean(targetEmployeeDetails && targetEmployeeId) now becomes true on my-timesheet, so isOngoingEntryLocked = isDirectEntryView && hasOngoingTimeEntry(record) makes today's row non-actionable with the ongoingEntryCellTooltip message. Previously an eligible admin/manager clicking that row got the ONGOING_TIME_ENTRY_BY_EDIT modal via mutate(). If that loss of affordance is intended (matching the all-timesheets table), fine — but please confirm, since on one's own timesheet the ongoing entry is the most likely row to be clicked.
| set((state: EmployeeTimesheetModalSliceType) => ({ | ||
| ...state, | ||
| directManualTimeEntryEligibleEmployee: value | ||
| })), |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
isSelfDirectTimeEntry is derived state duplicated in the global store. It is fully determined by directManualTimeEntryEligibleEmployee?.employeeId === user?.userId, which AddEditTimeEntry (its only reader) could compute locally with useAuth(). Keeping it as store state means every present and future site that calls setDirectManualTimeEntryEligibleEmployee must remember to call the paired setter — this PR already had to touch four such sites plus the reset effect, and a missed pairing silently leaks a stale true into the next modal (showing/hiding the employee name field incorrectly). Consider deriving it in AddEditTimeEntry and dropping the store field, setter, and the five call sites.
| ? user?.userId | ||
| : undefined; | ||
|
|
||
| const selfTargetEmployeeDetails = isSelfDirectEntryEligible |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
selfTargetEmployeeDetails builds a fresh object literal on every render, so TimesheetDailyLog → TimesheetDailyRecordTable → every TimesheetDailyRecordTableRow receives a new prop identity each time this component re-renders (and it re-renders on each startTime/endTime change and each query settle). Wrap it in useMemo keyed on [isSelfDirectEntryEligible, user?.employee?.firstName, user?.employee?.lastName], matching the useMemo usage elsewhere in this module (e.g. IndividualEmployeeTimeReportBody).
| isDailyLogLoading?: boolean; | ||
| targetEmployeeId?: number; | ||
| targetEmployeeDetails?: EmployeeDetails; | ||
| targetEmployeeDetails?: L1EmployeeType; |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
Changing targetEmployeeDetails to L1EmployeeType aligns this prop with TimesheetDailyRecordTableRow, but it now disagrees with the other caller: IndividualEmployeeTimeReportBody passes useGetEmployeeById(...) data, typed EmployeeDetails, which is a flat shape (firstName/lastName at the top level) with no personal.general. The row reads targetEmployeeDetails.personal?.general?.firstName/lastName, so on the individual employee time report the direct-entry employeeName resolves to an empty string and the disabled "employee" field in AddEditTimeEntry renders blank. Since all L1EmployeeType members are optional, TypeScript won't catch the mismatch. Either map the EmployeeDetails response into the nested shape at that call site (as EmployeeTimesheet does), or have the row accept a plain { firstName?: string; lastName?: string } so both callers can satisfy it.
| import TimesheetDailyLog from "~community/attendance/components/molecules/TimesheetDailyLog/TimesheetDailyLog"; | ||
| import TimesheetDailyLogFilter from "~community/attendance/components/molecules/TimesheetDailyLogFilter/TimesheetDailyLogFilter"; | ||
| import EmployeeTimesheetPopupController from "~community/attendance/components/organisms/EmployeeTimesheetPopupController/EmployeeTimesheetPopupController"; | ||
| import useManualEntryRestriction from "~community/attendance/hooks/useManualEntryRestriction"; |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
📁 File-level observation
No test coverage for the new branching. The existing TimesheetDailyLog.test.tsx and TimesheetDailyRecordTable.test.tsx are prop-less smoke tests, and nothing exercises the new eligible/not-eligible paths. Given this decides whether a save hits the direct-entry endpoint or the approval-request endpoint, a couple of @testing-library/react tests (wrapped in MockTheme) asserting that the employee-name field is hidden when isSelfDirectTimeEntry is true, and that a row click sets the expected store state for self vs. non-self, would be worth adding.
| import { EmployeeTimesheetModalTypes } from "~community/attendance/enums/timesheetEnums"; | ||
| import useManualEntryRestriction from "~community/attendance/hooks/useManualEntryRestriction"; | ||
| import { useAttendanceStore } from "~community/attendance/store/attendanceStore"; | ||
| import { useAuth } from "~community/auth/providers/AuthProvider"; |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
📁 File-level observation
Re-routing self entries to the enterprise direct-entry endpoint is redundant with backend behaviour, and that redundancy is what causes the regressions in this PR. EpTimeServiceImpl.addManualEntryRequest() and editTimeRequest() already check canSelfApproveTimeEntries(currentUser) (isManualEntryRestrictionEnabled() && canManageManualTimeEntries(...) — exactly the same predicate the frontend's canDirectlyAddOrEditEntry computes) and, when true, delegate to applyDirectTimeEntry(currentUser.getEmployee().getEmployeeId(), ...) / applyDirectTimeEntryEdit(...). getRequestedDateTimeAvailability() likewise short-circuits to timeSlotsExists: false for the same users. So an eligible admin/manager submitting through the existing community endpoints already gets an immediately-approved direct entry for themselves — the only thing that was wrong before this PR was the UI copy ("Submit request", the redundant Employee field). By instead setting directManualTimeEntryEligibleEmployee for self, useAddEntry.handleTimeEntrySubmit takes the useAddDirectTimeEntry/useEditDirectTimeEntry branch, which invalidates only manager/admin and by-employeeId query keys, losing invalidateAttendanceTimeRecordQueries() (employee status + getEmployeeWorkSummary + getEmployeeDailyLog) that useAddManualTimeEntry/useEditClockInOut call. Suggested fix: keep self entries on the community mutations (do not set directManualTimeEntryEligibleEmployee when the target is the current user) and use the new isSelfDirectTimeEntry flag purely for presentation (hide the Employee field, use the "Save" button label and the direct-entry toasts). That removes the stale-cache, stale-selectedDailyRecord and bypassed-guard problems in one step instead of patching each.
| isPrimaryBtnDisabled={isRestrictionLoading} | ||
| onPrimaryButtonClick={() => { | ||
| setDirectManualTimeEntryEligibleEmployee(null); | ||
| const isEligible = canDirectlyAddOrEditEntry && !!user?.userId; |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
The "ongoing time entry" guard is silently bypassed for eligible users. Previously this handler always passed null to setDirectManualTimeEntryEligibleEmployee, so submitting went through submitManualTimeEntry → getModalBeforeManualEntry(), whose first check is isOngoingSession (slotType is START/PAUSE/RESUME and the entry date is today) and which opens ONGOING_TIME_ENTRY instead of submitting. Now, for an admin/manager with canDirectlyAddOrEditEntry, handleTimeEntrySubmit short-circuits on directManualTimeEntryEligibleEmployee and POSTs immediately. That check is client-side only — the backend validateRequestParameters() only validates end>start, zone id and same-day, and applyDirectTimeEntry adds no ongoing-session validation — so a currently clocked-in admin can now create a manual entry for today that overlaps their in-progress record with no warning at all. Either keep self entries on the community path (see the file-level comment) or replicate the isOngoingSession check before taking the direct branch in useAddEntry.handleTimeEntrySubmit.
| targetEmployeeId?: number; | ||
| targetEmployeeDetails?: EmployeeDetails; | ||
| targetEmployeeDetails?: L1EmployeeType; | ||
| isSelfTargetEntry?: boolean; |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
Missing paired change at the other call site: IndividualEmployeeTimeReportBody renders TimesheetDailyRecordTable with targetEmployeeId={selectedUser} / targetEmployeeDetails but never passes isSelfTargetEntry, so it always defaults to false. When an admin/manager opens their own individual time report and edits a row, the entry is a self entry but the modal still renders the disabled "Employee" field showing their own name — the exact UX this PR set out to remove, left inconsistent between the two entry points. Pass isSelfTargetEntry={selectedUser === user?.userId} (or derive it inside the table/row from useAuth()/useSessionData() so no call site can forget it).
| ? user?.userId | ||
| : undefined; | ||
|
|
||
| const selfTargetEmployeeDetails = isSelfDirectEntryEligible |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
selfTargetEmployeeDetails fabricates a partial L1EmployeeType with no explicit type annotation. It only structurally satisfies the prop because every field of L1EmployeeType/L2PersonalDetailsType/L3GeneralDetailsType is optional, so nothing checks the shape at the definition site, and firstName/lastName are string | undefined while the real object (from useGetEmployeeById) is a full employee record. If TimesheetDailyRecordTableRow later reads any other branch (employment, common, …) this stand-in silently yields undefined instead of a type error. At minimum annotate it — const selfTargetEmployeeDetails: L1EmployeeType | undefined = ... — so the shape is checked where it is built; better, pass the current user's real employee record or replace the targetEmployeeDetails truthiness test in the row with an explicit isDirectEntryView boolean prop so a synthetic object is not needed at all.
| dailyLogData: DailyLogType[]; | ||
| downloadEmployeeDailyLogCsv: () => void; | ||
| isDailyLogLoading?: boolean; | ||
| targetEmployeeId?: number; |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
Three new props are threaded through TimesheetDailyLog purely as pass-through to TimesheetDailyRecordTable → TimesheetDailyRecordTableRow (four component levels), and the eligibility expression canDirectlyAddOrEditEntry && !!user?.userId is now duplicated verbatim in EmployeeTimesheet.tsx:39 and my-timesheet.tsx:56, with useManualEntryRestriction() additionally called a third time inside TimesheetDailyRecordTable. These two copies can and already do disagree (see the raw-flag mismatch noted in the first pass), and any future change to the rule must be made in both places. Extract a single useSelfDirectTimeEntry() hook in ~community/attendance/hooks/ that returns { isEligible, employeeId, employeeDetails } and consume it in the page and in the row (which is the only component that actually reads the values), removing the pass-through props from TimesheetDailyLog entirely.
| setDirectManualTimeEntryEligibleEmployee(null); | ||
| const isEligible = canDirectlyAddOrEditEntry && !!user?.userId; | ||
| setDirectManualTimeEntryEligibleEmployee( | ||
| isEligible ? { employeeId: user.userId, employeeName: "" } : null |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
employeeName: "" is a placeholder that only works because isSelfDirectTimeEntry happens to hide the field. AddEditTimeEntry renders value={directManualTimeEntryEligibleEmployee.employeeName}, so any future call site that sets the eligible employee without also setting setIsSelfDirectTimeEntry(true) will render a blank, unlabelled "Employee" input rather than something sensible. The user's name is already available here — concatStrings([user.employee?.firstName ?? "", user.employee?.lastName ?? ""]).trim() — so populating it costs nothing and removes the implicit coupling between the two store fields.
| targetEmployeeDetails?: L1EmployeeType; | ||
| isRowInteractive: boolean; | ||
| isManualEntryRestricted: boolean; | ||
| isSelfTargetEntry?: boolean; |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
Naming drift for one concept: the prop is isSelfTargetEntry, the store field is isSelfDirectTimeEntry, and the local derivation in EmployeeTimesheet is isSelfDirectEntryEligible. setIsSelfDirectTimeEntry(isSelfTargetEntry) on line 218 is a rename with no transformation, which is exactly the kind of aliasing that makes the mismatch on EmployeeTimesheet.tsx:76 hard to spot in review. Pick one name (isSelfDirectTimeEntry) and use it for the prop, the store field and the local variable.
3 participants
PR checklist
TaskId: (https://github.com/SkappHQ/skapp/issues/[id])
Summary
How to test
Project Checklist
npm run formatnpm run check-lintOther
PR Checklist
ready-for-code-review)Additional Information