[Feat]: Manage Permissions and Improve Time entries view #3403
Conversation
|
Caution Review failedThe pull request is closed. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/web/app/[locale]/kanban/page.tsxOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces enhancements across several components related to timesheet management. Key modifications include the addition of an optional Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (2)
19-19: Simplify the optional type notationThe current type definition has redundant optional notation. Using either
?or| undefinedis sufficient, but not both.- user?: IUser | undefined + user?: IUser
14-22: Implementation appears incompleteThe changes add user context to support permission management, but the actual permission checks aren't implemented. Consider:
- Adding permission checks where needed (e.g., for sensitive actions or data)
- Documenting the intended permission requirements
- Passing the user prop to child components if they need to handle permissions
Would you like assistance in implementing the permission checks or documenting the requirements?
apps/web/app/hooks/features/useTimesheet.ts (1)
118-120: Consider safer employee ID filteringWhile the functional approach is good, consider adding explicit null checks for employee objects to prevent potential runtime errors.
-employeeIds: isManage - ? employee?.map(({ employee: { id } }) => id).filter(Boolean) - : [user.employee.id], +employeeIds: isManage + ? employee?.map(({ employee }) => employee?.id) + .filter((id): id is string => id !== undefined && id !== null) + : [user.employee?.id].filter((id): id is string => id !== undefined && id !== null),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx(3 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx(4 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx(2 hunks)apps/web/app/[locale]/timesheet/[memberId]/page.tsx(6 hunks)apps/web/app/hooks/features/useTimelogFilterOptions.ts(3 hunks)apps/web/app/hooks/features/useTimesheet.ts(2 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(6 hunks)apps/web/lib/features/task/task-displays.tsx(2 hunks)apps/web/lib/settings/member-table.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/lib/settings/member-table.tsx
🔇 Additional comments (25)
apps/web/app/[locale]/timesheet/[memberId]/components/CalendarView.tsx (2)
14-14: LGTM!
The IUser interface import is correctly added and necessary for the type definitions.
15-20: Verify the necessity of the user prop in BaseCalendarDataViewProps
The user prop is added to the interface but isn't utilized in the BaseCalendarDataView component. If this prop is intended for future use, consider adding a TODO comment. Otherwise, consider removing it to avoid unused props.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (5)
157-161: Enhancement: Add user context for permission checks
The addition of the user parameter and the computation of isManage using isUserAllowedToAccess(user) enhances the component by enabling conditional rendering based on user permissions.
302-302: Conditional rendering of timesheet buttons based on permissions
Using isManage to conditionally render timesheet action buttons ensures that only authorized users can perform management actions.
364-369: Pass logType and user permissions to child components
Passing logType to DisplayTimeForTimesheet and isManage, user to TaskActionMenu allows these components to render content appropriately based on the log type and user permissions.
472-476: Correctly determine edit permissions
The canEdit logic correctly allows a user to edit if they have management permissions (isManage) or if they are the owner of the timesheet entry.
493-508: Conditional rendering of menu items based on user permissions
- The edit option is displayed when the user can edit (
canEdit), ensuring users without proper permissions cannot access this action. - Management actions are only available to users with
isManagepermissions.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx (3)
2-2: Import IUser for user context
Importing IUser allows the component to utilize user information for permission checks and conditional rendering.
7-7: Update TimesheetView to accept user prop
Adding the user prop to TimesheetView enables the component to pass user context to child components for permission-based rendering.
30-30: Pass user context to DataTableTimeSheet
By passing the user prop to DataTableTimeSheet, you ensure that user permissions are considered when rendering the timesheet data.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilter.tsx (4)
6-7: Import user interfaces and hooks
Importing IUser, TimesheetLog, TimesheetStatus, and useTimelogFilterOptions ensures the component has the necessary types and functions for handling user permissions.
18-18: Extend ITimesheetFilter interface with user prop
Adding the optional user prop to the ITimesheetFilter interface allows the filter component to access user information for permission checks.
22-24: Determine manage permissions in TimesheetFilter
Using isUserAllowedToAccess(user) to set isManage enables the component to conditionally render filters and actions based on user permissions.
42-55: Conditional rendering of filter options based on permissions
Wrapping the filter options and add time button inside {isManage && (...)} ensures that only users with manage permissions can access these features.
apps/web/app/hooks/features/useTimelogFilterOptions.ts (3)
1-1: Import user types for permission checks
Importing IUser and RoleNameEnum allows the hook to define and check user roles accurately.
21-28: Implement isUserAllowedToAccess function
The isUserAllowedToAccess function correctly checks if the user's role is within the allowed roles for managing timesheets.
79-79: Expose isUserAllowedToAccess from the hook
By returning isUserAllowedToAccess, you enable other components to utilize this permission check function.
apps/web/lib/features/task/task-displays.tsx (3)
8-8: Import additional icons for log types
Importing CalendarArrowDown, CalendarClock, and UserPlusIcon provides the necessary icons to represent different log types.
77-90: Enhance DisplayTimeForTimesheet with log type icons
- Adding the
logTypeprop allows the component to display different icons based on the type of log entry (TRACKED,MANUAL,IDLE). - The
iconsmapping andresolvedLogTypeensure that the correct icon is displayed, enhancing the UI for users.
92-98: Display time with appropriate icon
The updated component correctly displays the time alongside the icon corresponding to the logType, improving visual clarity.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (2)
24-24: LGTM: Improved date range handling
The change to use startOfWeek and endOfWeek with Monday as the start day is a good improvement for timesheet management, following common business practices.
Also applies to: 49-50
218-218: LGTM: Consistent user context propagation
The addition of the user prop to both view components ensures consistent permission handling throughout the timesheet interface.
Also applies to: 224-224
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (1)
177-187: LGTM: Enhanced button UI and accessibility
The button improvements provide better visual feedback with hover states and clearer hierarchy with the ChevronDown icon. The increased height and font size enhance clickability.
apps/web/app/hooks/features/useTimesheet.ts (2)
98-98: LGTM: Clean permission handling implementation
Good separation of concerns by using isUserAllowedToAccess from the filter options hook to determine management permissions.
Also applies to: 104-104
303-304: LGTM: Well-structured hook return value
Good addition of the groupByDate utility and isManage flag to the hook's return value, maintaining a clean interface for consumers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/web/lib/features/task/task-displays.tsx (1)
77-98: Add aria-labels to icons for accessibilityThe implementation looks good overall! Consider adding aria-labels to the icons for better accessibility.
const icons = { - MANUAL: <UserPlusIcon className={`text-red-500 ${iconClasses}`} />, - TRACKED: <ClockIcon className={`text-green-400 ${iconClasses}`} />, - IDLE: <CalendarArrowDown className={`text-yellow-400 ${iconClasses}`} />, + MANUAL: <UserPlusIcon className={`text-red-500 ${iconClasses}`} aria-label="Manual time entry" />, + TRACKED: <ClockIcon className={`text-green-400 ${iconClasses}`} aria-label="Tracked time" />, + IDLE: <CalendarArrowDown className={`text-yellow-400 ${iconClasses}`} aria-label="Idle time" />, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/[locale]/timesheet/[memberId]/page.tsx(6 hunks)apps/web/lib/features/task/task-displays.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/[locale]/timesheet/[memberId]/page.tsx
🔇 Additional comments (2)
apps/web/lib/features/task/task-displays.tsx (2)
8-8: LGTM: New icon imports
The new imports are correctly placed and necessary for the enhanced log type visualization.
77-77: Verify the usage of different log types
Let's verify how the different log types are being used across the codebase to ensure consistent implementation.
Also applies to: 90-90
✅ Verification successful
Log types are consistently implemented across the codebase
The verification shows that:
- The log types ('TRACKED', 'MANUAL', 'IDLE') are properly defined as an enum in
apps/web/app/interfaces/ITimer.ts - 'TRACKED' is the default log type used consistently across timer-related operations
DisplayTimeForTimesheetcomponent is used in multiple timesheet-related views with proper type checking- The implementation defaults to 'TRACKED' when no log type is provided (
logType ?? 'TRACKED')
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of DisplayTimeForTimesheet component with different log types
rg -A 2 "DisplayTimeForTimesheet.*logType="
# Search for string literals that might define these log types
rg "'(TRACKED|MANUAL|IDLE)'" --type typescript
Length of output: 135
Script:
#!/bin/bash
# Search for usage of DisplayTimeForTimesheet component
rg "DisplayTimeForTimesheet" -A 2
# Search for these log type strings without typescript extension
rg "'(TRACKED|MANUAL|IDLE)'"
# Search for type definitions or enums related to log types
rg -g "*.ts*" "type.*=.*('TRACKED'|'MANUAL'|'IDLE')"
Length of output: 5739
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (1)
157-167: Consider memoizing the member count calculationWhile the unique member count calculation is correct, it could be optimized by moving it to a useMemo hook to prevent unnecessary recalculations on each render.
Consider applying this optimization:
+const uniqueMemberCount = useMemo(() => + Object.values(statusTimesheet) + .flat() + .map(entry => entry.employee.id) + .filter((id, index, array) => array.indexOf(id) === index) + .length, + [statusTimesheet] +); {isManage && ( <TimesheetCard - count={Object.values(statusTimesheet) - .flat() - .map(entry => entry.employee.id) - .filter((id, index, array) => array.indexOf(id) === index) - .length} + count={uniqueMemberCount} title="Members Worked" description="People worked since last time" icon={<User2 className="font-bold" />} classNameIcon="bg-[#30B366] shadow-[#30b3678f]" /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/app/[locale]/timesheet/[memberId]/page.tsx(7 hunks)
🔇 Additional comments (4)
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (4)
49-50: LGTM: Improved date range initialization
The change to use startOfWeek and endOfWeek with Monday as the start day is a good improvement for business timesheet views.
92-96: LGTM: Well-implemented duration calculation
The total duration calculation correctly aggregates timesheet durations with proper error handling.
52-54:
Consider safer null handling for date parameters
The non-null assertions (!) on dateRange.from and dateRange.to could lead to runtime errors if the dates are null.
Apply this diff for safer null handling:
-startDate: dateRange.from!,
-endDate: dateRange.to!,
+startDate: dateRange.from ?? startOfWeek(new Date(), { weekStartsOn: 1 }),
+endDate: dateRange.to ?? endOfWeek(new Date(), { weekStartsOn: 1 }),201-201: Verify user prop usage in child components
The user prop is consistently passed to TimesheetFilter, TimesheetView, and CalendarView components. Let's verify its implementation in these components.
Also applies to: 226-226, 232-232
✅ Verification successful
Based on the search results, I can now provide a final response as we have found the necessary information about how the user prop is implemented in all three components:
User prop is properly implemented across components
The verification shows that:
TimesheetFiltercomponent properly defines and uses the user prop in its interfaceITimesheetFilterTimesheetViewcomponent properly defines and uses the user prop in its props interfaceCalendarViewcomponent properly defines and uses the user prop in its props interface
All components consistently implement the user prop as an optional IUser type, which aligns with how it's being passed from the parent component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the user prop is used in the child components
# Check TimesheetFilter implementation
ast-grep --pattern 'interface TimesheetFilterProps {
$$$
user: $_
$$$
}'
# Check TimesheetView implementation
ast-grep --pattern 'interface TimesheetViewProps {
$$$
user: $_
$$$
}'
# Check CalendarView implementation
ast-grep --pattern 'interface CalendarViewProps {
$$$
user: $_
$$$
}'
Length of output: 237
Script:
#!/bin/bash
# Let's try a different approach to find these components and their props
# Search for TimesheetFilter component definition and usage
rg "TimesheetFilter" -A 5 -B 5
# Search for TimesheetView component definition and usage
rg "TimesheetView" -A 5 -B 5
# Search for CalendarView component definition and usage
rg "CalendarView" -A 5 -B 5
Length of output: 32191
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx (4)
97-97: LGTM! Consider using design tokens for consistency.The styling changes are valid, but the highly specific padding values (
0.3312rem,0.2875rem) might be better represented using design tokens for maintainability.Consider using design system variables:
-className={clsxm('rounded-sm h-auto !px-[0.3312rem] py-[0.2875rem] text-white bg-primary')} +className={clsxm('rounded-sm h-auto !px-2 py-1 text-white bg-primary')}
Line range hint
249-259: Add input validation and handle edge cases in time conversion.The time conversion logic has potential issues with edge cases:
- No validation for malformed time inputs
- Midnight (12 AM) is not handled correctly
- No error handling for invalid time formats
Consider this safer implementation:
const convertToMinutesHour = (time: string): number => { + if (!time || !/^\d{1,2}:\d{2} [AP]M$/.test(time)) { + throw new Error('Invalid time format. Expected "HH:MM AM/PM"'); + } const [hourMinute, period] = time.split(' '); const [hours, minutes] = hourMinute.split(':').map(Number); + + if (hours < 1 || hours > 12 || minutes < 0 || minutes > 59) { + throw new Error('Invalid hours or minutes'); + } - let totalMinutes = (hours % 12) * 60 + minutes; + let totalMinutes = (hours === 12 ? 0 : hours) * 60 + minutes; if (period === 'PM') totalMinutes += 720; return totalMinutes; }
Line range hint
28-53: Implement comprehensive form validation and error handling.The form lacks proper validation feedback:
- No error messages for required fields
- No validation before form submission
- Missing error states for invalid inputs
Consider implementing form validation:
interface FormErrors { task?: string; notes?: string; shifts?: { startTime?: string; endTime?: string }[]; } // Add to component state const [errors, setErrors] = React.useState<FormErrors>({}); // Add validation function const validateForm = (): boolean => { const newErrors: FormErrors = {}; if (!task.trim()) { newErrors.task = t('errors.TASK_REQUIRED'); } if (!notes.trim()) { newErrors.notes = t('errors.NOTES_REQUIRED'); } // Validate shifts const shiftErrors = shifts.map(shift => ({ startTime: !shift.startTime ? t('errors.START_TIME_REQUIRED') : undefined, endTime: !shift.endTime ? t('errors.END_TIME_REQUIRED') : undefined })); if (shiftErrors.some(err => err.startTime || err.endTime)) { newErrors.shifts = shiftErrors; } setErrors(newErrors); return Object.keys(newErrors).length === 0; }; // Update submit handler const handleSubmit = () => { if (!validateForm()) { return; } // Proceed with submission };
Line range hint
307-393: Optimize component structure and state management.The current implementation has several architectural concerns:
- Complex prop drilling through multiple levels
- Scattered state management
- Tightly coupled components
Consider these improvements:
- Use React Context for sharing state and callbacks
- Extract shift validation logic into a custom hook
- Implement proper component composition
Example implementation:
// Create ShiftContext const ShiftContext = React.createContext<{ timeOptions: string[]; onShiftChange: (index: number, field: keyof Shift, value: string) => void; t: TranslationHooks; } | null>(null); // Create custom hook for shift validation const useShiftValidation = () => { const validateShift = (start: string, end: string): boolean => { // Move validation logic here }; return { validateShift }; }; // Simplify ShiftManagement component const ShiftManagement = ({ index, value }: { index: number; value: Shift }) => { const context = React.useContext(ShiftContext); if (!context) throw new Error('ShiftManagement must be used within ShiftContext'); const { timeOptions, onShiftChange, t } = context; return ( // Simplified JSX ); };apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
Line range hint
472-508: Consider refactoring permission-based rendering.The component could be improved by:
- Consolidating permission checks
- Adding error handling for the edit modal
- Extracting menu items into separate components
Consider this refactor:
const TaskActionMenu = ({ dataTimesheet, isManage, user }: { dataTimesheet: TimesheetLog, isManage?: boolean, user?: IUser | undefined }) => { const { isOpen: isEditTask, openModal: isOpenModalEditTask, closeModal: isCloseModalEditTask } = useModal(); + const [error, setError] = useState<Error | null>(null); const t = useTranslations(); const canEdit = isManage || user?.id === dataTimesheet.employee.user.id; + const handleEdit = () => { + try { + isOpenModalEditTask(); + } catch (error) { + setError(error as Error); + // Handle error appropriately + } + }; + const renderMenuItems = () => { + const items = []; + if (canEdit) { + items.push( + <DropdownMenuItem key="edit" className="cursor-pointer" onClick={handleEdit}> + {t('common.EDIT')} + </DropdownMenuItem> + ); + } + if (isManage) { + items.push( + <StatusTask key="status" timesheet={dataTimesheet} />, + <DropdownMenuItem key="delete" className="text-red-600 hover:!text-red-600 cursor-pointer"> + {t('common.DELETE')} + </DropdownMenuItem> + ); + } + return items; + }; return ( <> {<EditTaskModal closeModal={isCloseModalEditTask} isOpen={isEditTask} dataTimesheet={dataTimesheet} />} <DropdownMenu> <DropdownMenuTrigger asChild> <Button variant="ghost" className="w-8 h-8 p-0 text-sm sm:text-base"> <span className="sr-only">Open menu</span> <MoreHorizontal className="w-4 h-4" /> </Button> </DropdownMenuTrigger> <DropdownMenuContent align="end"> - {canEdit && ( - <DropdownMenuItem className="cursor-pointer" onClick={isOpenModalEditTask}> - {t('common.EDIT')} - </DropdownMenuItem> - )} - <DropdownMenuSeparator /> - {isManage && ( - <> - <StatusTask timesheet={dataTimesheet} /> - <DropdownMenuItem className="text-red-600 hover:!text-red-600 cursor-pointer"> - {t('common.DELETE')} - </DropdownMenuItem> - </> - )} + {renderMenuItems()} </DropdownMenuContent> </DropdownMenu> </> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/[locale]/timesheet/[memberId]/components/AddTaskModal.tsx(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(7 hunks)
🔇 Additional comments (3)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)
157-161: LGTM! Clean implementation of user-based permissions.
The changes properly implement permission checks using optional parameters and follow React best practices.
302-302: LGTM! Proper implementation of permission-based UI rendering.
The changes correctly implement conditional rendering based on user permissions and maintain good component composition.
Also applies to: 366-369
472-476: LGTM! Clean implementation of permission checks.
The permission check logic is well-implemented using the combination of isManage and user ID comparison.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
Outdated
Show resolved
Hide resolved
Innocent-Akim
force-pushed
the
feat/manage-permissions-and-time-entries-view
branch
from
3c53830 to
3dd3562
Compare
Description
#3047
#3046
#3219
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of previous status
Current screenshots
Please add here videos or images of previous status
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores