feat(reports/publisher_reports): add filter for statistics on publisher records page #4660
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds optional group-based filtering to core person/report hooks and threads a new publishersGroup prop through publisher report components; replaces month toggle UI with SelectPublishers and SelectPeriod controls and updates YearDetails state/handlers accordingly. Changes
Sequence DiagramsequenceDiagram
participant User
participant YearDetails
participant SelectPublishers
participant SelectPeriod
participant useYearDetails
participant ReportHooks as usePublishers/useAuxiliaryPioneers/...
participant PersonHooks as usePersons/useReportYearly/useReportMonthly
User->>SelectPublishers: choose group
SelectPublishers->>YearDetails: onChange(selectedPublishers)
YearDetails->>useYearDetails: handleChangeSelectedPublishers
useYearDetails->>YearDetails: selectedPublishers
User->>SelectPeriod: choose month/service-year
SelectPeriod->>YearDetails: onChange(selectedMonth)
YearDetails->>useYearDetails: handleChangeSelectedMonth
useYearDetails->>YearDetails: selectedMonth, wholeYear
YearDetails->>ReportHooks: call with publishersGroup + month/year/wholeYear
ReportHooks->>PersonHooks: call usePersons(publishersGroup)
PersonHooks->>PersonHooks: filter persons by fieldGroups membership
PersonHooks-->>ReportHooks: return filtered persons
ReportHooks-->>YearDetails: computed reports using filtered persons
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/reports/publisher_records/years_stats/year_details/index.tsx (1)
40-63: Remove commented-out code.Commented-out code should not be committed to version control. Git history preserves the old implementation if needed. Keeping dead code reduces maintainability and creates confusion.
Apply this diff to remove the commented block:
- {/* <Tooltip - title={t('tr_wholeYearIsSelected')} - delaySpeed={'fast'} - show={wholeYear} - followCursor - sx={{ flex: 1 }} - > - <MonthSelector - year={year} - value={month} - onChange={handleMonthChange} - readOnly={wholeYear} - sx={{ - flex: 1, - }} - /> - </Tooltip> - <Box> - <SwitchWithLabel - label={t('tr_wholeYearSetting')} - checked={wholeYear} - onChange={handleToggleWholeYear} - /> - </Box> */} <SelectPublishers
🧹 Nitpick comments (3)
src/features/reports/publisher_records/years_stats/year_details/useYearDetails.tsx (1)
1-2: Remove unused imports.Line 2 imports
useEffectanduseState, but onlyuseStateis used. TheuseEffectimport is unused, triggering the eslint-disable comment on line 1. Line 4 also redundantly importsuseCallbackwhich is already available from the React import.Apply this diff:
-/* eslint-disable @typescript-eslint/no-unused-vars */ -import { useEffect, useState } from 'react'; +import { useState, useCallback } from 'react'; import { YearDetailsProps } from './index.types'; -import { useCallback } from 'react';src/features/reports/publisher_records/years_stats/year_details/select_period/index.types.ts (1)
1-5: Strengthen type definitions for better type safety.The
unknowntypes foronChangeandvalueare too loose. Based on the component implementation inindex.tsx, these should be more specific:
valueshould bestring(holds either 'service-year' or a month in 'YYYY/MM' format)onChangeshould acceptSelectChangeEvent<string>from MUIApply this diff to improve type safety:
+import { SelectChangeEvent } from '@mui/material'; + export type SelectPeriodProps = { - onChange: (e: unknown) => void; - value: unknown; + onChange: (e: SelectChangeEvent<string>) => void; + value: string; year: string; };src/features/reports/publisher_records/years_stats/year_details/select_period/useSelectPeriod.tsx (1)
3-11: Add proper return type and consider error handling.The return type uses
any[]which bypasses type safety. Based on thebuildServiceYearsListimplementation, months should be typed as{ value: string; label: string }[].Additionally, when the year is not found, an empty array is silently returned. Consider whether this is the desired behavior or if a warning/fallback would be more appropriate.
Apply this diff to add proper typing:
-const useSelectPeriod = ({ year }: { year: string }) => { +const useSelectPeriod = ({ year }: { year: string }): { months: { value: string; label: string }[] } => { const serviceYears = buildServiceYearsList(); const yearObj = serviceYears.find((y) => y.year === year); const months = yearObj ? yearObj.months : []; return { months, }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/locales/en/general.jsonis excluded by!**/*.json
📒 Files selected for processing (19)
src/features/persons/hooks/usePersons.tsx(2 hunks)src/features/reports/hooks/useReportMonthly.tsx(1 hunks)src/features/reports/hooks/useReportYearly.tsx(1 hunks)src/features/reports/publisher_records/years_stats/auxiliary_pioneers/index.types.ts(1 hunks)src/features/reports/publisher_records/years_stats/auxiliary_pioneers/useAuxiliaryPioneers.tsx(1 hunks)src/features/reports/publisher_records/years_stats/fulltime_servants/index.types.ts(1 hunks)src/features/reports/publisher_records/years_stats/fulltime_servants/useFulltimeServants.tsx(1 hunks)src/features/reports/publisher_records/years_stats/publishers/index.types.ts(1 hunks)src/features/reports/publisher_records/years_stats/publishers/usePublishers.tsx(3 hunks)src/features/reports/publisher_records/years_stats/total_statistics/index.types.ts(1 hunks)src/features/reports/publisher_records/years_stats/total_statistics/useTotalStatistics.tsx(2 hunks)src/features/reports/publisher_records/years_stats/year_details/index.tsx(3 hunks)src/features/reports/publisher_records/years_stats/year_details/select_period/index.tsx(1 hunks)src/features/reports/publisher_records/years_stats/year_details/select_period/index.types.ts(1 hunks)src/features/reports/publisher_records/years_stats/year_details/select_period/useSelectPeriod.tsx(1 hunks)src/features/reports/publisher_records/years_stats/year_details/select_publishers/index.tsx(1 hunks)src/features/reports/publisher_records/years_stats/year_details/select_publishers/index.types.ts(1 hunks)src/features/reports/publisher_records/years_stats/year_details/select_publishers/useSelectPublishers.tsx(1 hunks)src/features/reports/publisher_records/years_stats/year_details/useYearDetails.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/features/reports/publisher_records/years_stats/publishers/usePublishers.tsx (1)
src/features/reports/publisher_records/years_stats/publishers/index.types.ts (1)
PublishersProps(1-6)
src/features/reports/publisher_records/years_stats/fulltime_servants/useFulltimeServants.tsx (1)
src/features/reports/publisher_records/years_stats/fulltime_servants/index.types.ts (1)
FulltimeServantsProps(1-6)
src/features/persons/hooks/usePersons.tsx (2)
src/states/field_service_groups.ts (1)
fieldWithLanguageGroupsState(24-62)src/states/persons.ts (1)
personsActiveState(25-34)
src/features/reports/hooks/useReportMonthly.tsx (3)
src/states/field_service_groups.ts (1)
fieldWithLanguageGroupsState(24-62)src/states/field_service_reports.ts (1)
congFieldServiceReportsState(14-22)src/states/persons.ts (1)
personsState(17-17)
src/features/reports/publisher_records/years_stats/year_details/select_period/useSelectPeriod.tsx (1)
src/utils/date.ts (1)
buildServiceYearsList(233-292)
src/features/reports/publisher_records/years_stats/total_statistics/useTotalStatistics.tsx (1)
src/features/reports/publisher_records/years_stats/total_statistics/index.types.ts (1)
TotalStatisticsProps(1-4)
src/features/reports/publisher_records/years_stats/year_details/select_publishers/useSelectPublishers.tsx (1)
src/states/field_service_groups.ts (1)
fieldWithLanguageGroupsState(24-62)
src/features/reports/publisher_records/years_stats/auxiliary_pioneers/useAuxiliaryPioneers.tsx (1)
src/features/reports/publisher_records/years_stats/auxiliary_pioneers/index.types.ts (1)
AuxiliaryPioneersProps(1-6)
src/features/reports/publisher_records/years_stats/year_details/select_period/index.tsx (1)
src/features/reports/publisher_records/years_stats/year_details/select_period/index.types.ts (1)
SelectPeriodProps(1-5)
src/features/reports/hooks/useReportYearly.tsx (3)
src/states/field_service_groups.ts (1)
fieldWithLanguageGroupsState(24-62)src/states/field_service_reports.ts (1)
congFieldServiceReportsState(14-22)src/states/persons.ts (1)
personsState(17-17)
src/features/reports/publisher_records/years_stats/year_details/select_publishers/index.tsx (1)
src/features/reports/publisher_records/years_stats/year_details/select_publishers/index.types.ts (1)
SelectPublishersProps(1-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SonarCloud
- GitHub Check: Code QL
- GitHub Check: Summary
🔇 Additional comments (12)
src/features/reports/publisher_records/years_stats/fulltime_servants/index.types.ts (1)
5-5: LGTM!The addition of
publishersGroupis consistent with the group-filtering feature and follows the same pattern applied to other Props types in this PR.src/features/reports/publisher_records/years_stats/publishers/index.types.ts (1)
5-5: LGTM!Consistent addition of
publishersGroupproperty to support group-based filtering.src/features/reports/publisher_records/years_stats/auxiliary_pioneers/index.types.ts (1)
5-5: LGTM!Consistent with the group-filtering pattern applied throughout this PR.
src/features/reports/publisher_records/years_stats/fulltime_servants/useFulltimeServants.tsx (1)
14-22: LGTM!The
publishersGroupparameter is correctly destructured and passed to the dependent hooks (usePersons,useReportYearly,useReportMonthly), enabling group-scoped data retrieval.src/features/reports/publisher_records/years_stats/total_statistics/index.types.ts (1)
3-3: LGTM!Addition of
publishersGroupaligns with the group-filtering feature being introduced.src/features/reports/publisher_records/years_stats/total_statistics/useTotalStatistics.tsx (1)
11-18: LGTM! Clean parameter threading.The addition of
publishersGroupparameter and its propagation tousePersonsis implemented correctly, enabling group-based filtering for statistics.src/features/reports/publisher_records/years_stats/year_details/select_period/index.tsx (1)
9-38: LGTM! Well-structured component.The component properly integrates the custom hook, translation, and MUI components to render period selection UI. The static "service-year" option followed by dynamic month options provides clear structure.
src/features/reports/publisher_records/years_stats/year_details/select_publishers/index.tsx (1)
9-38: LGTM! Consistent and clean implementation.The component follows the same well-structured pattern as
SelectPeriod, with proper hook integration, translations, and rendering of both "all" and group-specific options.src/features/reports/publisher_records/years_stats/year_details/select_publishers/useSelectPublishers.tsx (1)
5-21: LGTM! Clean data transformation.The hook properly transforms field service groups into a simple structure for the UI, with appropriate fallback naming using translation keys and 1-based indexing.
src/features/reports/publisher_records/years_stats/publishers/usePublishers.tsx (1)
10-22: Clean implementation of group-based filtering.The addition of
publishersGroupparameter and its propagation to all dependent hooks (usePersons,useReportYearly,useReportMonthly) is consistent and correctly implemented.src/features/reports/publisher_records/years_stats/auxiliary_pioneers/useAuxiliaryPioneers.tsx (1)
10-22: LGTM!The implementation follows the same clean pattern as
usePublishers.tsx, correctly propagatingpublishersGroupto all dependent hooks.src/features/reports/publisher_records/years_stats/year_details/index.tsx (1)
75-96: Correct propagation of group-based filtering.The updated component props correctly pass
selectedMonth,selectedPublishers, and the derivedpublishersGroupto all child statistics components.
src/features/reports/publisher_records/years_stats/year_details/index.tsx
Outdated
Show resolved
Hide resolved
src/features/reports/publisher_records/years_stats/year_details/index.tsx
Outdated
Show resolved
Hide resolved
| export type SelectPublishersProps = { | ||
| onChange: (e: unknown) => void; | ||
| value: unknown; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace unknown types with specific types for better type safety.
Both onChange and value are typed as unknown, which bypasses TypeScript's type checking. Based on the context, value should be string (matching publishersGroup across other components), and onChange should accept a React change event.
Apply this diff:
export type SelectPublishersProps = {
- onChange: (e: unknown) => void;
- value: unknown;
+ onChange: (e: React.ChangeEvent<HTMLSelectElement>) => void;
+ value: string;
};🤖 Prompt for AI Agents
In
src/features/reports/publisher_records/years_stats/year_details/select_publishers/index.types.ts
lines 1-4, replace the loose unknown types: change value to string and type
onChange as a React change event handler (e.g. (e:
React.ChangeEvent<HTMLSelectElement>) => void). Add the necessary React import
(or import { ChangeEvent } from 'react') if not already present so the file
compiles.
src/features/reports/publisher_records/years_stats/year_details/useYearDetails.tsx
Outdated
Show resolved
Hide resolved
src/features/reports/publisher_records/years_stats/year_details/index.tsx
Outdated
Show resolved
Hide resolved
src/features/reports/publisher_records/years_stats/year_details/index.tsx
Outdated
Show resolved
Hide resolved
| justifyContent="space-between" | ||
| > | ||
| <Tooltip | ||
| {/* <Tooltip |
There was a problem hiding this comment.
@FussuChalice: if no longer needed, this can be removed too.
src/features/reports/publisher_records/years_stats/year_details/useYearDetails.tsx
Outdated
Show resolved
Hide resolved
|
@FussuChalice: does this replace the #4193 entirely? |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/features/reports/publisher_records/years_stats/year_details/useYearDetails.tsx (2)
1-3: Combine React imports to satisfy static analysis and simplify the module headerYou can merge the two
reactimports into one to address the Sonar warning and keep the file header cleaner:-import { useState } from 'react'; +import { useState, useCallback } from 'react'; import { YearDetailsProps } from './index.types'; -import { useCallback } from 'react';
9-15: Consider typing the change handlers’ event parameter
eis currently implicitly typed, which can weaken TypeScript checks (especially withnoImplicitAnyenabled). Consider annotating it (or lifting the type fromSelectPublishers/SelectPeriod) so the handler signatures stay aligned with the underlying components’onChangecontract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/features/persons/hooks/usePersons.tsx(2 hunks)src/features/reports/hooks/useReportMonthly.tsx(1 hunks)src/features/reports/hooks/useReportYearly.tsx(1 hunks)src/features/reports/publisher_records/years_stats/year_details/index.tsx(1 hunks)src/features/reports/publisher_records/years_stats/year_details/select_publishers/index.types.ts(1 hunks)src/features/reports/publisher_records/years_stats/year_details/useYearDetails.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/features/reports/hooks/useReportMonthly.tsx
- src/features/reports/publisher_records/years_stats/year_details/select_publishers/index.types.ts
- src/features/persons/hooks/usePersons.tsx
- src/features/reports/hooks/useReportYearly.tsx
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
src/features/reports/publisher_records/years_stats/year_details/useYearDetails.tsx
[warning] 1-1: 'react' imported multiple times.
[warning] 3-3: 'react' imported multiple times.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (1)
src/features/reports/publisher_records/years_stats/year_details/index.tsx (1)
1-65: Wiring of year details, filters, and publishersGroup looks consistent
YearDetailscleanly consumesuseYearDetailsand threadsselectedMonth,wholeYear, andselectedPublishersinto the selects and downstream sections (FulltimeServants,AuxiliaryPioneers,Publishers,TotalStatistics) in a consistent way; imports are minimal and relevant.
Yes. |
2 participants
Description
Added a filter for groups and periods to the statistics on the publisher records page.
Thanks to Igor Selin, from whom I took a few lines of code.
These are the fixes for draft #4193.
Fixes # (issue)
Type of change
Checklist: