[Feat]: Add role-based access control for team dashboard #3601
Conversation
WalkthroughThe changes add an optional boolean property "isManage" to several components in the team dashboard. The Changes
Sequence Diagram(s)sequenceDiagram
participant P as TeamDashboard Page
participant H as useReportActivity Hook
participant DH as DashboardHeader
participant TF as TeamDashboardFilter
P->>H: Invoke useReportActivity()
H-->>P: Return data including isManage
P->>DH: Render DashboardHeader(isManage)
DH->>TF: Pass isManage prop
alt isManage true
TF->>TF: Render employee selection component
else isManage false
TF->>TF: Skip employee selection component
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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/hooks/features/useReportActivity.tsOops! 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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: 1
🧹 Nitpick comments (5)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-dashboard-filter.tsx (2)
83-113: Consider UX improvements for employee filtering.While the implementation correctly implements role-based access control, consider these improvements:
- Add a helper text to indicate that employee options are filtered based on selected teams.
- Memoize the filtered members list to optimize performance.
Here's how you could implement these improvements:
+const memoizedFilteredMembers = React.useMemo( + () => allteamsState.flatMap((team) => { + const members = team.members ?? []; + return members.filter((member) => member && member.employee); + }), + [allteamsState] +); {isManage && ( <div className=""> <label className="flex justify-between mb-1 text-sm text-gray-600"> <span className="text-[12px]">{t('manualTime.EMPLOYEE')}</span> + <span className="text-xs text-gray-400"> + {t('common.FILTERED_BY_SELECTED_TEAMS')} + </span> <span className={cn( 'text-primary/10', alluserState.length > 0 && 'text-primary dark:text-primary-light' )} > {t('common.CLEAR')} ({alluserState.length}) </span> </label> <MultiSelect localStorageKey="team-dashboard-select-filter-employee" removeItems={shouldRemoveItems} - items={allteamsState.flatMap((team) => { - const members = team.members ?? []; - return members.filter((member) => member && member.employee); - })} + items={memoizedFilteredMembers} itemToString={(member) => { if (!member?.employee) return ''; return member.employee.fullName || t('manualTime.EMPLOYEE'); }} itemId={(item) => item.id} onValueChange={(selectedItems) => setAllUserState(selectedItems as any)} multiSelect={true} triggerClassName="dark:border-gray-700" /> </div> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
99-102: Use optional chaining for safer member access.Consider using optional chaining for a more concise and safer member access.
items={allteamsState.flatMap((team) => { - const members = team.members ?? []; + const members = team?.members ?? []; return members.filter((member) => member && member.employee); })}🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web/app/hooks/features/useReportActivity.ts (1)
96-99: Consider adding error handling for edge cases.While the implementation is correct, consider handling edge cases where:
alluserStateis undefined/nulluser.employee.idis undefined-employeeIds: isManage - ? alluserState?.map(({ employee: { id } }) => id).filter(Boolean) - : [user.employee.id], +employeeIds: isManage && alluserState + ? alluserState.map(({ employee: { id } }) => id).filter(Boolean) + : user?.employee?.id ? [user.employee.id] : [],apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx (1)
24-24: Consider breaking down destructured properties for better readability.The line is quite long with many destructured properties. Consider grouping related properties.
-const { rapportChartActivity, updateDateRange, updateFilters, loadingTimeLogReportDailyChart, rapportDailyActivity, loadingTimeLogReportDaily, statisticsCounts,loadingTimesheetStatisticsCounts, isManage} = useReportActivity(); +const { + // Report data + rapportChartActivity, + rapportDailyActivity, + statisticsCounts, + // Loading states + loadingTimeLogReportDailyChart, + loadingTimeLogReportDaily, + loadingTimesheetStatisticsCounts, + // Actions + updateDateRange, + updateFilters, + // Access control + isManage +} = useReportActivity();apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/dashboard-header.tsx (1)
8-12: Consider adding JSDoc documentation for the interface.Adding documentation would help explain the purpose of the isManage flag and its impact on component behavior.
+/** + * Props for the DashboardHeader component + * @property {Function} onUpdateDateRange - Callback for date range updates + * @property {Function} onUpdateFilters - Callback for filter updates + * @property {boolean} [isManage] - Flag indicating if user has management access + */ interface DashboardHeaderProps { onUpdateDateRange: (startDate: Date, endDate: Date) => void; onUpdateFilters: (filters: Partial<Omit<ITimeLogReportDailyChartProps, 'organizationId' | 'tenantId'>>) => void; isManage?: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/dashboard-header.tsx(1 hunks)apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-dashboard-filter.tsx(3 hunks)apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx(2 hunks)apps/web/app/hooks/features/useReportActivity.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-dashboard-filter.tsx
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
🔇 Additional comments (4)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-dashboard-filter.tsx (1)
10-13: LGTM! Clean interface definition for role-based access control.The interface and component signature changes are well-structured and align with the PR objectives.
apps/web/app/hooks/features/useReportActivity.ts (1)
64-64: LGTM! Role-based access control is properly implemented.The implementation correctly determines management access by:
- Using
isUserAllowedToAccessfrom theuseTimelogFilterOptionshook- Checking both user existence and access rights
Also applies to: 73-73
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx (1)
57-61: LGTM! Props are correctly passed to DashboardHeader.The isManage prop is properly passed down to the DashboardHeader component.
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/dashboard-header.tsx (1)
14-14: LGTM! Component properly handles and forwards the isManage prop.The implementation correctly:
- Accepts isManage in the component signature
- Forwards it to TeamDashboardFilter
Also applies to: 26-26
…Either include it or remove the dependency array.
Description
Please include a summary of the changes and the related issues.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of the previous status
Current screenshots
Please add here videos or images of the current (new) status
Summary by CodeRabbit