Enhance Weekly Financial Report functionality and sorting #95
Conversation
- Added TypeScript support in ESLint configuration by including `@typescript-eslint/eslint-plugin` and `@typescript-eslint/parser` at version 8.39.0. - Enhanced ESLint rules with naming conventions for identifiers, interfaces, classes, and constants to improve code quality and consistency. - Updated `package-lock.json` and `package.json` to reflect the new dependencies and their versions. These changes strengthen the linting process for TypeScript files and enforce better coding standards across the project.
- Modified ESLint configuration to target only TypeScript files (`**/*.ts`) by removing TypeScript React files (`**/*.tsx`). - Enhanced naming convention rules to exclude const variables from the default identifier rule and updated the filter for variable naming to ensure uppercase constants are not matched. - Added `@typescript-eslint/eslint-plugin` and `@typescript-eslint/parser` as devDependencies in `package.json` to support TypeScript linting. These changes refine the linting process for TypeScript files and improve code quality standards across the project.
- Introduced a new archive file detailing ESLint naming conventions based on the naming-cheatsheet principles to enhance code quality and consistency in the TypeScript codebase. - Updated `eslint.config.mjs` to implement comprehensive naming rules, including camelCase for constants, PascalCase for classes and enums, and specific prefixes for boolean variables and functions. - Enhanced the ESLint configuration to allow flexible naming for object properties, MongoDB operators, and dot notation, ensuring compatibility with various coding patterns. - Updated `package.json` to reflect the latest dependencies and scripts for ESLint. These changes significantly improve the linting process and enforce better coding standards across the project, promoting maintainability and readability.
- Updated the archive file to include recent enhancements in ESLint naming conventions, specifically enforcing camelCase for boolean variables and functions while maintaining prefix requirements. - Modified `eslint.config.mjs` to implement camelCase formatting for boolean variables and functions, ensuring better code consistency and readability. - Added new rules to allow specific patterns for class/model variables and snake_case for parameters, enhancing flexibility in naming conventions. These changes improve the linting process and enforce stricter naming standards across the TypeScript codebase, promoting maintainability and clarity.
- Revised the archive file to reflect recent updates in ESLint naming conventions, including the transition to PascalCase for boolean variables and functions, and the addition of new function prefixes such as 'setup', 'create', 'init', and 'build'. - Enhanced `eslint.config.mjs` to implement these changes, ensuring better support for naming patterns and improved variable filtering. - Introduced a new rule for true constants to use UPPER_CASE formatting, further refining the naming standards. These updates enhance code quality and consistency across the TypeScript codebase, promoting maintainability and clarity.
- Integrated `QBORepository` to fetch effective revenue data, enhancing financial report accuracy. - Updated `WeeklyFinancialReportRepository` to include effective revenue calculations and sorting by marginality levels. - Modified report formatting to display effective revenue, margin, and marginality metrics. - Added tests to ensure correct sorting of groups by marginality and effective marginality. These changes improve the financial reporting capabilities, providing clearer insights into revenue and performance metrics.
|
Warning Rate limit exceeded@anatolyshipitz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughIntegrates QBO effectiveRevenue into data fetch and tests; adds required Project.name across schema/types/repo projections; refactors WeeklyFinancialReport to compute and sort groups by marginality and effective metrics; extends formatter (new effective fields and dynamic footer); adds sorting tests and test data builders. Changes
Sequence Diagram(s)sequenceDiagram
participant Activity as fetchFinancialAppData
participant FinApp as FinApp API
participant QBO as QBORepository
Activity->>FinApp: getEmployees()
Activity->>FinApp: getProjects()
Activity->>QBO: getEffectiveRevenue()
par Concurrency
FinApp-->>Activity: employees
FinApp-->>Activity: projects
QBO-->>Activity: revenueMap
end
Activity->>Activity: augment projects with effectiveRevenue by quick_books_id
Activity-->>Activity: write JSON { employees, projects, effectiveRevenue }
sequenceDiagram
participant Repo as WeeklyFinancialReportRepository
participant Aggregator as GroupAggregator
participant Calc as MarginalityCalculator
participant Formatter as WeeklyFinancialReportFormatter
participant Config as qboConfig
Repo->>Repo: collectGroupData(targetUnits, employees, projects)
loop per group
Repo->>Aggregator: aggregate group units
Aggregator-->>Repo: totals (cogs, revenue, effectiveRevenue)
Repo->>Calc: compute marginality
Calc-->>Repo: marginality {level, indicator, value}
Repo->>Repo: build GroupData
end
Repo->>Repo: sortGroupData (level, groupName)
Repo->>Formatter: formatDetail(group metrics incl. effectiveRevenue/Margin/Marginality)
Formatter-->>Repo: detail strings
Repo->>Formatter: formatFooter(using qboConfig.effectiveRevenueMonths window)
Formatter-->>Repo: footer string
Repo-->>Caller: { reportDetails, summary }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
anatolyshipitz
changed the base branch from
main
to
feature/eslint-naming-conventions
🔍 Vulnerabilities of
|
| digest | sha256:da30eca7154c7c4cb2616e31f7411021ea9146d5b15c68adf2a57df4c52b164c |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 218 MB |
| packages | 358 |
📦 Base Image alpine:3
| also known as |
|
| digest | sha256:1c4eef651f65e2f7daee7ee785882ac164b02b78fb74503052a26dc061c90474 |
| vulnerabilities |   |
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description | ||||||||||||
Description
| ||||||||||||
Description
|
- Introduced effective financial parameters (Effective Revenue, Effective Margin, Effective Marginality) into the Weekly Financial Reports system via QuickBooks Online integration. - Updated `WeeklyFinancialReportRepository` to include calculations and sorting by marginality levels. - Enhanced report formatting to display new metrics and explanations regarding effective revenue calculations. - Added comprehensive unit tests to validate effective metrics calculations and sorting functionality. These enhancements improve the accuracy and clarity of financial reporting, providing better insights into revenue and performance metrics.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
workers/main/src/configs/qbo.ts (1)
19-27: Strengthen Zod schema: coerce to number with default and constraints.Move validation into the schema to ensure a typed, validated value. This keeps config concerns declarative and consistent with other env validation.
Apply this diff:
export const qboSchema = z.object({ QBO_API_URL: z.string().url().min(1, 'QBO_API_URL is required'), QBO_BEARER_TOKEN: z.string().optional(), QBO_CLIENT_ID: z.string().min(1, 'QBO_CLIENT_ID is required'), QBO_CLIENT_SECRET: z.string().min(1, 'QBO_CLIENT_SECRET is required'), QBO_COMPANY_ID: z.string().min(1, 'QBO_COMPANY_ID is required'), QBO_REFRESH_TOKEN: z.string(), - QBO_EFFECTIVE_REVENUE_MONTHS: z.string().optional(), + QBO_EFFECTIVE_REVENUE_MONTHS: z.coerce + .number() + .int() + .min(1) + .max(24) + .default(4), });Optionally (outside this hunk), consider parsing process.env with this schema and sourcing qboConfig fields from the parsed object to guarantee type safety end-to-end.
🧹 Nitpick comments (9)
workers/main/src/services/FinApp/types.ts (1)
24-24: Add a short JSDoc for effectiveRevenue to clarify semantics and units.Please document whether this is gross or net revenue, the currency/units, and the date window (derived from qboConfig.effectiveRevenueMonths). This reduces ambiguity for downstream consumers and tests.
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts (2)
31-35: Rename effectiveRevenue to clarify it’s a map; avoids confusion with the project field.The local variable name collides conceptually with the per-project numeric field. Rename to indicate it’s a lookup by QBO customer ref.
Apply this diff:
- const [employees, projects, effectiveRevenue] = await Promise.all([ + const [employees, projects, effectiveRevenueByCustomerRef] = await Promise.all([ repo.getEmployeesByRedmineIds(employeeIds), repo.getProjectsByRedmineIds(projectIds), - qboRepo.getEffectiveRevenue(), + qboRepo.getEffectiveRevenue(), ]);
25-26: Consider dependency injection for QBORepository to improve testability.Instantiating QBORepository inline makes it harder to isolate in unit tests. Passing it as an optional parameter (defaulting to a real instance) mirrors how FinAppRepository could be injected and reduces reliance on module-level mocking.
Example (outside this hunk, for illustration):
export const fetchFinancialAppData = async ( fileLink: string, deps: { finAppRepo?: FinAppRepository; qboRepo?: IQBORepository } = {}, ) => { const repo = deps.finAppRepo ?? new FinAppRepository(); const qboRepo = deps.qboRepo ?? new QBORepository(); ... };workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportSorting.test.ts (2)
65-89: Strengthen assertions to cover within-level tie-breakers.Currently you don’t assert ordering between High Group B vs High Group D. Add a case where their effectiveMarginality differs to prove the secondary comparator works as expected within the same level.
73-89: Reduce brittleness of string indexOf checks.Parsing explicit group lines or returning structured group order from the repository (for test-only path) avoids false positives if names repeat in other sections.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts (1)
182-200: Also assert ordering in the summary to mirror details ordering.To catch divergences between detail and summary ordering, add corresponding summary assertions in this test.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (3)
17-26: Consider adding JSDoc documentation for the GroupData interfaceThis new interface introduces several important fields that would benefit from documentation, particularly to clarify the distinction between regular and "effective" metrics.
+/** + * Represents aggregated financial data for a group + */ interface GroupData { + /** Group identifier name */ groupName: string; + /** Total hours worked by the group */ groupTotalHours: number; + /** Total revenue generated by the group */ groupTotalRevenue: number; + /** Total cost of goods sold for the group */ groupTotalCogs: number; + /** Revenue calculated over the effective period (QBO integration) */ effectiveRevenue: number; + /** Margin calculated using effective revenue */ effectiveMargin: number; + /** Marginality percentage based on effective metrics */ effectiveMarginality: number; + /** Regular marginality calculation result */ marginality: MarginalityResult; }
117-149: Consider renaming function to follow naming conventionsThe function name
processSingleGroupdoesn't follow the established naming pattern. According to the guidelines, it should use the pattern: action + high context + low context.- private processSingleGroup( + private calculateGroupMetrics( targetUnit: TargetUnit, targetUnits: TargetUnit[], employees: Employee[], projects: Project[], ): GroupData {Also update the call site at line 82:
- const groupDataItem = this.processSingleGroup( + const groupDataItem = this.calculateGroupMetrics(
151-166: Simplify function name for better clarityThe function name
createSortedGroupsis misleading since the groups are already sorted. The function is actually categorizing them by marginality level.- private createSortedGroups(groupData: GroupData[]) { + private categorizeGroupsByMarginality(groupData: GroupData[]) {Also update the call site at line 55-56:
const { highGroups, mediumGroups, lowGroups } = - this.createSortedGroups(groupData); + this.categorizeGroupsByMarginality(groupData);
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
memory-bank/archive/archive-TargetUnit-Effective-Params-20250811.mdis excluded by!memory-bank/**
📒 Files selected for processing (7)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts(2 hunks)workers/main/src/configs/qbo.ts(1 hunks)workers/main/src/services/FinApp/types.ts(1 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts(4 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts(3 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts(5 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportSorting.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Follow the function naming pattern: prefix? + action (A) + high context (HC) + low context? (LC), using action verbs such as get, fetch, send, create, validate, handle, calculate, and boolean prefixes is, has, should
Use descriptive, unabbreviated variable names; use singular for single values and plural for collections; ensure variable names are context-specific
Files:
workers/main/src/services/FinApp/types.tsworkers/main/src/configs/qbo.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportSorting.test.tsworkers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts
workers/main/src/configs/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
workers/main/src/configs/**: Environment validation should be implemented in workers/main/src/configs/
Use Zod schemas for environment validation
Files:
workers/main/src/configs/qbo.ts
**/*.test.ts
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests are co-located with source files and should be named with the pattern *.test.ts
Files:
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportSorting.test.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts
🧬 Code Graph Analysis (6)
workers/main/src/services/FinApp/types.ts (2)
workers/main/src/services/QBO/types.ts (1)
IQBORepository(51-53)workers/main/src/services/WeeklyFinancialReport/IWeeklyFinancialReportRepository.ts (1)
GenerateReportInput(4-8)
workers/main/src/configs/qbo.ts (3)
workers/main/src/services/QBO/QBORepository.integration.test.ts (2)
it(150-180)qboConfig(13-34)workers/main/src/services/QBO/QBORepository.errorHandling.test.ts (1)
qboConfig(14-35)workers/main/src/services/QBO/QBORepository.ts (1)
calculateDateWindow(149-159)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportSorting.test.ts (1)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (1)
WeeklyFinancialReportRepository(28-272)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts (6)
workers/main/src/services/QBO/QBORepository.ts (2)
QBORepository(20-172)getEffectiveRevenue(161-171)workers/main/src/common/fileUtils.ts (2)
readJsonFile(6-17)writeJsonFile(19-32)workers/main/src/common/types.ts (1)
TargetUnit(3-14)workers/main/src/services/QBO/QBORepository.integration.test.ts (2)
invoices(107-147)invoices(61-105)workers/main/src/services/QBO/types.ts (1)
IQBORepository(51-53)workers/main/src/services/QBO/QBORepository.test.ts (1)
it(115-173)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (6)
workers/main/src/services/WeeklyFinancialReport/MarginalityCalculator.ts (2)
MarginalityResult(12-17)MarginalityCalculator(19-47)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (1)
WeeklyFinancialReportFormatter(32-112)workers/main/src/common/types.ts (1)
TargetUnit(3-14)workers/main/src/services/FinApp/types.ts (2)
Employee(5-12)Project(14-26)workers/main/src/services/WeeklyFinancialReport/GroupAggregator.ts (1)
GroupAggregator(3-15)workers/main/src/services/WeeklyFinancialReport/IWeeklyFinancialReportRepository.ts (1)
AggregateGroupDataInput(16-20)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (5)
workers/main/src/common/formatUtils.ts (1)
formatCurrency(1-3)workers/main/src/configs/qbo.ts (1)
qboConfig(3-17)workers/main/src/common/utils.ts (1)
formatDateToISOString(19-25)workers/main/src/configs/weeklyFinancialReport.ts (2)
HIGH_MARGINALITY_THRESHOLD(6-6)MEDIUM_MARGINALITY_THRESHOLD(7-7)workers/main/src/services/WeeklyFinancialReport/IWeeklyFinancialReportRepository.ts (2)
AggregateGroupDataInput(16-20)GenerateReportInput(4-8)
⏰ 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). (2)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (7)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportSorting.test.ts (1)
1-97: Good coverage for level-first, then effectiveMarginality ordering.The test validates the intended sort: High before Medium before Low, with a secondary order by effectiveMarginality. This protects against regressions in the repository’s sort routine.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts (1)
5-71: Nice move to test data builders; improves readability and reuse.createBasicTestData consolidates verbose fixtures and makes tests easier to maintain.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (2)
96-115: LGTM! Sorting implementation is correct and well-structuredThe two-level sorting strategy (first by marginality level, then by effective marginality within each level) is implemented correctly with clear comments explaining the logic.
168-190: Good separation of concerns in formatting logicThe extraction of formatting logic into a separate method with clear return values improves readability and maintainability.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (3)
9-14: Interface naming follows conventions correctlyGood change renaming from
formatSummaryInputtoFormatSummaryInputto follow TypeScript interface naming conventions.
42-54: Effective metrics integration looks goodThe new effective revenue fields are properly integrated into the detail formatting with consistent formatting and clear labeling.
100-111: Footer dynamically adapts to configurationGood implementation making the footer dynamic based on the
qboConfig.effectiveRevenueMonthsconfiguration, providing transparency about the calculation window.
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts
Outdated
Show resolved
Hide resolved
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts
Outdated
Show resolved
Hide resolved
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts
Outdated
Show resolved
Hide resolved
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts
Outdated
Show resolved
Hide resolved
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts
Outdated
Show resolved
Hide resolved
Enhance Weekly Financial Reports with Project Name and Formatting Adjustments - Added 'name' field to the project schema to ensure project identification in financial reports. - Updated `FinAppRepository` to include project names in the data retrieval process. - Removed unnecessary group total hours from report formatting to streamline output. - Adjusted formatting in `WeeklyFinancialReportFormatter` for improved readability and consistency. - Enhanced sorting logic in `WeeklyFinancialReportRepository` to sort groups by name alphabetically. These changes improve the clarity and organization of weekly financial reports, providing better insights into project performance.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
workers/main/src/services/FinApp/FinAppSchemas.ts (1)
27-31: Require Project.name: update schema, interface, and testsWe’ve made
namea required field on the Project schema—let’s align types and tests, and add trimming to avoid accidental whitespace.• In workers/main/src/services/FinApp/FinAppSchemas.ts, update the
namefield:- name: { type: String, required: true }, + name: { type: String, required: true, trim: true },• In workers/main/src/services/FinApp/types.ts, add
nameto the Project interface:export interface Project { name: string; redmine_id: number; quick_books_id?: number; history?: History; effectiveRevenue?: number; [key: string]: unknown; }• In workers/main/src/services/FinApp/FinAppSchemas.test.ts, include a
namevalue in the “should accept valid project” case:it('should accept valid project', async () => { const doc = new ProjectModel({ name: 'Test Project', redmine_id: 456, quick_books_id: 789, history: { rate: { '2024-01-01': 200 } }, }); const err = await validateDoc(doc); expect(err).toBeNull(); });
♻️ Duplicate comments (3)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts (2)
37-46: Tests must assert the new written shape (projects with effectiveRevenue + top-level effectiveRevenue)The success-case in fetchFinancialAppData.test.ts still asserts only employees and projects. Update it to match the new payload by either augmenting mockProjects or asserting via a mapping.
Example adjustment in the test:
expect(writeJsonFile).toHaveBeenCalledWith(expectedFilename, { employees: mockEmployees, projects: mockProjects.map(p => ({ ...p, effectiveRevenue: p.quick_books_id != null ? mockEffectiveRevenue[String(p.quick_books_id)]?.totalAmount ?? 0 : 0, })), effectiveRevenue: mockEffectiveRevenue, });Optional script to find the failing assertion:
#!/bin/bash rg -n "writeJsonFile\\).*\\{[\\s\\S]*employees:.*projects:" workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts -A 10
37-46: Use nullish check and string key when indexing effective revenue; avoid falsy-ID bugIndexing a string-keyed map with a number and using a truthiness check will mis-handle 0-like IDs. Use an explicit nullish check and convert the key to string. Also avoid overshadowing the top-level effectiveRevenue property by using a distinct local variable name.
- await writeJsonFile(filename, { - employees, - projects: projects.map((project) => ({ - ...project, - effectiveRevenue: project.quick_books_id - ? effectiveRevenue[project.quick_books_id]?.totalAmount || 0 - : 0, - })), - effectiveRevenue, - }); + await writeJsonFile(filename, { + employees, + projects: projects.map((project) => ({ + ...project, + effectiveRevenue: + project.quick_books_id != null + ? effectiveRevenueByCustomerRef[String(project.quick_books_id)]?.totalAmount ?? 0 + : 0, + })), + effectiveRevenue: effectiveRevenueByCustomerRef, + });workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (1)
86-96: Harden month subtraction to avoid day overflow (e.g., Mar 31 → Feb 28/29 edge case)Using setMonth with the same day can overshoot into the following month. Clamp the day to the target month length.
- private static calculateDateWindow() { - const endDate = new Date(); - const startDate = new Date(endDate); - - startDate.setMonth(endDate.getMonth() - qboConfig.effectiveRevenueMonths); + private static calculateDateWindow() { + const endDate = new Date(); + // Compute target year/month by subtracting months safely + const monthsToSubtract = qboConfig.effectiveRevenueMonths; + const endYear = endDate.getFullYear(); + const endMonth = endDate.getMonth(); // 0-based + const targetMonthIndex = endMonth - monthsToSubtract; + const targetYear = + endYear + Math.floor(targetMonthIndex / 12); // negative months roll back years + const normalizedTargetMonth = ((targetMonthIndex % 12) + 12) % 12; + // Clamp day to end of target month + const daysInTargetMonth = new Date( + targetYear, + normalizedTargetMonth + 1, + 0, + ).getDate(); + const clampedDay = Math.min(endDate.getDate(), daysInTargetMonth); + const startDate = new Date( + targetYear, + normalizedTargetMonth, + clampedDay, + 0, + 0, + 0, + 0, + ); return { startDate: formatDateToISOString(startDate), endDate: formatDateToISOString(endDate), }; }
🧹 Nitpick comments (4)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (3)
248-248: Translate code comment to EnglishAvoid non-English inline comments for consistency.
- const processedProjects = new Set<number>(); // Отслеживаем обработанные проекты + const processedProjects = new Set<number>(); // Track processed projects to avoid double-counting
251-256: Avoid O(n^2) lookups by pre-indexing employees and projects by redmine_idRepeated Array.find calls inside the loop are quadratic. Precompute maps for O(1) access.
Outside this function, pre-index once:
const employeeById = new Map(employees.map(e => [e.redmine_id, e])); const projectById = new Map(projects.map(p => [p.redmine_id, p]));Then inside the loop:
const employee = employeeById.get(unit.user_id); const project = projectById.get(unit.project_id);
260-263: Potential cross-group double-counting of project effectiveRevenueWithin a group you dedupe by project, which is good. If the same project appears in multiple groups, its full effectiveRevenue will be added to each group, inflating totals across the report.
If projects can span multiple groups in the input targetUnits, consider allocating project.effectiveRevenue proportionally per group, e.g.:
- Compute per-project hours by group.
- Distribute project.effectiveRevenue across groups using hours share.
- Sum the allocated amount in each group.
Do projects in your weekly input belong to exactly one group? If not, we should adjust the allocation.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (1)
74-78: Minor formatting nit: stray leading space before :large_yellow_circle:There's an extra leading space before the medium-level label that makes alignment inconsistent.
- summary += ` :large_yellow_circle: *Marginality is between ${MEDIUM_MARGINALITY_THRESHOLD}-${HIGH_MARGINALITY_THRESHOLD}%*:\n`; + summary += `:large_yellow_circle: *Marginality is between ${MEDIUM_MARGINALITY_THRESHOLD}-${HIGH_MARGINALITY_THRESHOLD}%*:\n`;
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts(2 hunks)workers/main/src/services/FinApp/FinAppRepository.ts(1 hunks)workers/main/src/services/FinApp/FinAppSchemas.ts(1 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts(2 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Follow the function naming pattern: prefix? + action (A) + high context (HC) + low context? (LC), using action verbs such as get, fetch, send, create, validate, handle, calculate, and boolean prefixes is, has, should
Use descriptive, unabbreviated variable names; use singular for single values and plural for collections; ensure variable names are context-specific
Files:
workers/main/src/services/FinApp/FinAppRepository.tsworkers/main/src/services/FinApp/FinAppSchemas.tsworkers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts
🧬 Code Graph Analysis (5)
workers/main/src/services/FinApp/FinAppRepository.ts (2)
workers/main/src/services/FinApp/types.ts (1)
Project(14-25)workers/main/src/services/FinApp/IFinAppRepository.ts (1)
IFinAppRepository(6-15)
workers/main/src/services/FinApp/FinAppSchemas.ts (2)
workers/main/src/services/FinApp/FinAppSchemas.test.ts (1)
it(80-106)workers/main/src/services/FinApp/types.ts (1)
Project(14-25)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts (6)
workers/main/src/services/QBO/QBORepository.ts (2)
QBORepository(20-172)getEffectiveRevenue(161-171)workers/main/src/common/fileUtils.ts (2)
readJsonFile(6-17)writeJsonFile(19-32)workers/main/src/common/types.ts (1)
TargetUnit(3-14)workers/main/src/services/QBO/QBORepository.test.ts (2)
qboRepository(61-174)it(115-173)workers/main/src/services/QBO/QBORepository.integration.test.ts (3)
invoices(107-147)qboRepository(39-181)invoices(61-105)workers/main/src/services/QBO/types.ts (1)
IQBORepository(51-53)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (4)
workers/main/src/common/formatUtils.ts (1)
formatCurrency(1-3)workers/main/src/configs/weeklyFinancialReport.ts (2)
HIGH_MARGINALITY_THRESHOLD(6-6)MEDIUM_MARGINALITY_THRESHOLD(7-7)workers/main/src/configs/qbo.ts (1)
qboConfig(3-17)workers/main/src/common/utils.ts (1)
formatDateToISOString(19-25)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (6)
workers/main/src/services/WeeklyFinancialReport/MarginalityCalculator.ts (2)
MarginalityResult(12-17)MarginalityCalculator(19-47)workers/main/src/services/WeeklyFinancialReport/IWeeklyFinancialReportRepository.ts (3)
IWeeklyFinancialReportRepository(10-14)GenerateReportInput(4-8)AggregateGroupDataInput(16-20)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (1)
WeeklyFinancialReportFormatter(31-109)workers/main/src/common/types.ts (1)
TargetUnit(3-14)workers/main/src/services/FinApp/types.ts (2)
Employee(5-12)Project(14-26)workers/main/src/services/WeeklyFinancialReport/GroupAggregator.ts (1)
GroupAggregator(3-15)
⏰ 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: Docker Security Scanning (temporal, Dockerfile.temporal, temporal-test:latest)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (2)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (1)
171-191: Detail formatting integration looks solidThe new fields (effectiveRevenue, effectiveMargin, effectiveMarginality) are correctly passed to the formatter. Nice separation of concerns.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (1)
9-14: Formatter input changes look consistent with repository outputAddition of lowGroups and effective fields aligns with the updated repository pipeline and tests.
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts
Outdated
Show resolved
Hide resolved
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts
Show resolved
Hide resolved
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts
Outdated
Show resolved
Hide resolved
- Removed alphabetical sorting of high, medium, and low marginality groups in the `WeeklyFinancialReportRepository`. - Added a comment to clarify the intention of preserving the established order in the group data. This change enhances the report's clarity by maintaining the original grouping order, improving the overall organization of financial data.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (2)
150-167: Good: preserved computed order in summary listsAlphabetical re-sorting was removed. This keeps the performance-based ordering produced by sortGroupData intact in the summary.
95-115: Fix comparator: sort within levels by effectiveMarginality (desc), then by groupNameCurrent implementation sorts by level then alphabetically, which contradicts the PR objective/tests expecting effectiveMarginality ordering within each level. Update the comment and comparator accordingly.
- // Sort by marginality level (High -> Medium -> Low), - // then within each level by groupName alphabetically + // Sort by marginality level (High -> Medium -> Low), + // then within each level by effectiveMarginality (desc), + // then by groupName alphabetically groupData.sort((a, b) => { const levelComparison = levelOrder[b.marginality.level] - levelOrder[a.marginality.level]; if (levelComparison !== 0) { return levelComparison; } - // Sort by groupName alphabetically within each level - return a.groupName.localeCompare(b.groupName); + // Next: higher effectiveMarginality first within each level + if (b.effectiveMarginality !== a.effectiveMarginality) { + return b.effectiveMarginality - a.effectiveMarginality; + } + // Finally: alphabetical by name for stability + return a.groupName.localeCompare(b.groupName); });
🧹 Nitpick comments (3)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (3)
237-274: Optional: add a robust fallback for effectiveRevenue when QBO value is missingRight now, projects lacking effectiveRevenue contribute 0 even if we have rate×hours data. To avoid undercounting, aggregate a per-project placeholder (sum of projectRate×hours across all units for that project within the group), then add either the QBO effectiveRevenue or the placeholder once per project. Still dedupes by project.
}: AggregateGroupDataInput) { let groupTotalCogs = 0; let groupTotalRevenue = 0; - let effectiveRevenue = 0; - const processedProjects = new Set<number>(); // Отслеживаем обработанные проекты + let effectiveRevenue = 0; + // Track processed projects to avoid double-counting effective revenue per project + const processedProjects = new Set<number>(); + // Aggregate placeholder effective revenue (rate × hours) per project for cases where QBO value is absent + const placeholderRevenueByProject = new Map<number, number>(); for (const unit of groupUnits) { const employee = employees.find((e) => e.redmine_id === unit.user_id); const project = projects.find((p) => p.redmine_id === unit.project_id); const date = unit.spent_on; const employeeRate = this.safeGetRate(employee?.history, date); const projectRate = this.safeGetRate(project?.history, date); groupTotalCogs += employeeRate * unit.total_hours; groupTotalRevenue += projectRate * unit.total_hours; - if (project && !processedProjects.has(project.redmine_id)) { - effectiveRevenue += project.effectiveRevenue || 0; - processedProjects.add(project.redmine_id); - } + if (project) { + // Accumulate a per-project placeholder only if QBO effectiveRevenue is missing + if (project.effectiveRevenue == null) { + const prev = placeholderRevenueByProject.get(project.redmine_id) ?? 0; + placeholderRevenueByProject.set( + project.redmine_id, + prev + projectRate * unit.total_hours, + ); + } + processedProjects.add(project.redmine_id); + } } + // Sum effective revenue once per project: prefer QBO value, else use placeholder + for (const projectId of processedProjects) { + const proj = projects.find((p) => p.redmine_id === projectId); + effectiveRevenue += + proj?.effectiveRevenue ?? (placeholderRevenueByProject.get(projectId) ?? 0); + }This keeps totals consistent and avoids skew when some projects lack QBO effective revenue.
70-94: Optional: avoid O(n²) scanning by pre-indexing one representative unit per groupcollectGroupData currently iterates targetUnits and re-filters per group later; pre-indexing reduces unnecessary checks and keeps the same behavior by using the first unit per group.
private collectGroupData( targetUnits: TargetUnit[], employees: Employee[], projects: Project[], ) { - const processedGroupIds = new Set<number>(); - const groupData: GroupData[] = []; - - for (const targetUnit of targetUnits) { - if (!processedGroupIds.has(targetUnit.group_id)) { - processedGroupIds.add(targetUnit.group_id); - const groupDataItem = this.processSingleGroup( - targetUnit, - targetUnits, - employees, - projects, - ); - - groupData.push(groupDataItem); - } - } - - return { groupData }; + // One representative unit per group_id + const firstUnitByGroup = new Map<number, TargetUnit>(); + for (const unit of targetUnits) { + if (!firstUnitByGroup.has(unit.group_id)) { + firstUnitByGroup.set(unit.group_id, unit); + } + } + const groupData: GroupData[] = []; + for (const unit of firstUnitByGroup.values()) { + const groupDataItem = this.processSingleGroup( + unit, + targetUnits, + employees, + projects, + ); + groupData.push(groupDataItem); + } + return { groupData }; }
212-226: Optional: prevent UTC off-by-one by using a date formatter instead of toISOString().slice(0, 10)toISOString is UTC; slicing may shift a day depending on timezone. Use the project’s date formatter to produce consistent yyyy-mm-dd strings.
private composeWeeklyReportTitle(currentDate: Date): string { const quarter = Math.floor(currentDate.getMonth() / 3); - const periodStart = new Date(currentDate.getFullYear(), quarter * 3, 1) - .toISOString() - .slice(0, 10); - const periodEnd = new Date( + const periodStart = formatDateToISOString( + new Date(currentDate.getFullYear(), quarter * 3, 1), + ); + const endBound = new Date( currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate() - ((currentDate.getDay() + 6) % 7) - 1, - ) - .toISOString() - .slice(0, 10); + ); + const periodEnd = formatDateToISOString(endBound);Add import (outside this range):
import { getRateByDate, formatDateToISOString } from '../../common/formatUtils';
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Follow the function naming pattern: prefix? + action (A) + high context (HC) + low context? (LC), using action verbs such as get, fetch, send, create, validate, handle, calculate, and boolean prefixes is, has, should
Use descriptive, unabbreviated variable names; use singular for single values and plural for collections; ensure variable names are context-specific
Files:
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts
🧬 Code Graph Analysis (1)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (6)
workers/main/src/services/WeeklyFinancialReport/MarginalityCalculator.ts (2)
MarginalityResult(12-17)MarginalityCalculator(19-47)workers/main/src/services/WeeklyFinancialReport/IWeeklyFinancialReportRepository.ts (3)
IWeeklyFinancialReportRepository(10-14)GenerateReportInput(4-8)AggregateGroupDataInput(16-20)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (3)
WeeklyFinancialReportFormatter(31-109)WeeklyFinancialReportFormatter(27-84)FormatDetailInput(14-23)workers/main/src/common/types.ts (1)
TargetUnit(3-14)workers/main/src/services/FinApp/types.ts (2)
Employee(5-12)Project(14-26)workers/main/src/services/WeeklyFinancialReport/GroupAggregator.ts (1)
GroupAggregator(3-15)
⏰ 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). (2)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
- Updated the calculation of effectiveRevenueMonths in the qboConfig to ensure it handles non-finite values correctly. The new implementation uses a self-invoking function to parse the environment variable and defaults to 4 if the value is not a valid number. This change improves the robustness of the configuration by preventing potential errors from invalid input.
- Updated the date calculation in `WeeklyFinancialReportFormatter` to correctly handle month and year transitions when subtracting months from the current date. This ensures that the start date is accurately set to the last day of the target month, improving the reliability of financial report date ranges. - Cleaned up the code structure for better readability and maintainability. These changes enhance the accuracy of the date window used in weekly financial reports, providing more reliable data for analysis.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (1)
96-116: Sorting logic doesn't match test expectations.Based on the test files, groups should be sorted by:
- Marginality level (High → Medium → Low)
- EffectiveMarginality descending within each level
- GroupName alphabetically as the final tiebreaker
The current implementation only sorts by level and then name, missing the effectiveMarginality comparison. This will cause the test "sorts groups by effectiveMarginality in descending order" to fail.
private sortGroupData(groupData: GroupData[]) { const levelOrder = { [MarginalityLevel.High]: 3, [MarginalityLevel.Medium]: 2, [MarginalityLevel.Low]: 1, }; // Sort by marginality level (High -> Medium -> Low), - // then within each level by groupName alphabetically + // then within each level by effectiveMarginality (desc), + // then by groupName alphabetically groupData.sort((a, b) => { const levelComparison = levelOrder[b.marginality.level] - levelOrder[a.marginality.level]; if (levelComparison !== 0) { return levelComparison; } - // Sort by groupName alphabetically within each level - return a.groupName.localeCompare(b.groupName); + // Sort by effectiveMarginality descending within each level + if (b.effectiveMarginality !== a.effectiveMarginality) { + return b.effectiveMarginality - a.effectiveMarginality; + } + // Finally by groupName alphabetically + return a.groupName.localeCompare(b.groupName); }); }
🧹 Nitpick comments (4)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (4)
17-25: Consider making GroupData interface exportable for better testing and modularity.The
GroupDatainterface is central to this module's data flow. Exporting it would enable type-safe testing of individual components and improve modularity by allowing other components to consume this interface if needed.-interface GroupData { +export interface GroupData { groupName: string; groupTotalRevenue: number; groupTotalCogs: number; effectiveRevenue: number; effectiveMargin: number; effectiveMarginality: number; marginality: MarginalityResult; }
245-245: Fix typo in Russian comment.The code contains a comment in Russian. For consistency and maintainability in an international codebase, comments should be in English.
- const processedProjects = new Set<number>(); // Отслеживаем обработанные проекты + const processedProjects = new Set<number>(); // Track processed projects
118-149: Consider extracting aggregation logic for improved testability.The
processSingleGroupmethod handles multiple responsibilities: fetching group units, aggregating data, and calculating marginality. Consider splitting the aggregation logic into a separate, testable method.Consider refactoring to improve testability:
private processSingleGroup( targetUnit: TargetUnit, targetUnits: TargetUnit[], employees: Employee[], projects: Project[], ): GroupData { const { groupUnits } = GroupAggregator.aggregateGroup( targetUnits, targetUnit.group_id, ); + return this.createGroupData( + targetUnit.group_name, + groupUnits, + employees, + projects, + ); + } + + private createGroupData( + groupName: string, + groupUnits: TargetUnit[], + employees: Employee[], + projects: Project[], + ): GroupData { const { groupTotalCogs, groupTotalRevenue, effectiveRevenue, effectiveMargin, effectiveMarginality, } = this.aggregateGroupData({ groupUnits, employees, projects }); const marginality = MarginalityCalculator.calculate( groupTotalRevenue, groupTotalCogs, ); return { - groupName: targetUnit.group_name, + groupName, groupTotalRevenue, groupTotalCogs, effectiveRevenue, effectiveMargin, effectiveMarginality, marginality, }; }
71-94: Consider using Map for O(1) lookups instead of Set.The method correctly deduplicates groups, but using a Map would be more idiomatic and allow direct access to previously processed groups if needed in the future.
private collectGroupData( targetUnits: TargetUnit[], employees: Employee[], projects: Project[], ) { - const processedGroupIds = new Set<number>(); + const processedGroups = new Map<number, GroupData>(); const groupData: GroupData[] = []; for (const targetUnit of targetUnits) { - if (!processedGroupIds.has(targetUnit.group_id)) { - processedGroupIds.add(targetUnit.group_id); + if (!processedGroups.has(targetUnit.group_id)) { const groupDataItem = this.processSingleGroup( targetUnit, targetUnits, employees, projects, ); - + processedGroups.set(targetUnit.group_id, groupDataItem); groupData.push(groupDataItem); } } return { groupData }; }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts(2 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Follow the function naming pattern: prefix? + action (A) + high context (HC) + low context? (LC), using action verbs such as get, fetch, send, create, validate, handle, calculate, and boolean prefixes is, has, should
Use descriptive, unabbreviated variable names; use singular for single values and plural for collections; ensure variable names are context-specific
Files:
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts
🧬 Code Graph Analysis (1)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (6)
workers/main/src/services/WeeklyFinancialReport/MarginalityCalculator.ts (2)
MarginalityResult(12-17)MarginalityCalculator(19-47)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (1)
WeeklyFinancialReportFormatter(31-136)workers/main/src/common/types.ts (1)
TargetUnit(3-14)workers/main/src/services/FinApp/types.ts (2)
Employee(5-12)Project(14-26)workers/main/src/services/WeeklyFinancialReport/GroupAggregator.ts (1)
GroupAggregator(3-15)workers/main/src/services/WeeklyFinancialReport/IWeeklyFinancialReportRepository.ts (1)
AggregateGroupDataInput(16-20)
⏰ 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). (2)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (1)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (1)
257-260: Good implementation of effective revenue aggregation with deduplication.The logic correctly:
- Uses the QBO-provided
effectiveRevenuefield from the project- Implements deduplication using a Set to prevent double-counting when a project appears multiple times
- Falls back to 0 when
effectiveRevenueis undefined
- Updated the `fetchFinancialAppData` function to utilize the `QBORepository` for fetching effective revenue data, enhancing the accuracy of financial reports. - Adjusted the handling of effective revenue in the report generation process to ensure it correctly maps to project IDs. - Added necessary mocks for `QBORepository` in the corresponding test file to ensure proper testing of the new functionality. These changes improve the integration of financial data from QuickBooks Online, providing more reliable insights into project performance.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts (1)
202-221: Add missing error case: QBO getEffectiveRevenue failure should surface as AppError.You covered readJsonFile, getEmployees, getProjects, and writeJsonFile failures. Include the QBO path to ensure parity and prevent regressions when QBO is unavailable.
Proposed test to add within "error cases":
it('throws AppError when getEffectiveRevenue throws', async () => { readJsonFile.mockResolvedValue(mockTargetUnits); qboRepoInstance.getEffectiveRevenue.mockRejectedValue(new Error('fail-qbo')); await expectAppError( fetchFinancialAppData(fileLink), 'Failed to get Fin App Data', ); });I can open a follow-up PR to include this test if you prefer.
🧹 Nitpick comments (3)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts (3)
98-102: Optional: Strengthen typing around QBO repo mock to catch API drift.Right now qboRepoInstance is typed with a loose shape. Consider importing and using IQBORepository (if exported) to type the mock, or at least hoist a typed getEffectiveRevenueMock for better IDE help. Not blocking.
131-135: QBORepository mock wiring is fine; consider resetting mock implementations.Using vi.clearAllMocks resets call state but preserves implementations. Given you set implementations in beforeEach, this is safe; however, vi.resetAllMocks in afterEach can further reduce coupling between tests by also resetting implementations.
136-140: Inline qboRepoInstance factory is fine.This approach is straightforward. If you plan to assert calls, consider hoisting getEffectiveRevenueMock as a separate variable to assert on it explicitly.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts(7 hunks)workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.test.ts
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests are co-located with source files and should be named with the pattern *.test.ts
Files:
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Follow the function naming pattern: prefix? + action (A) + high context (HC) + low context? (LC), using action verbs such as get, fetch, send, create, validate, handle, calculate, and boolean prefixes is, has, should
Use descriptive, unabbreviated variable names; use singular for single values and plural for collections; ensure variable names are context-specific
Files:
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts
🧬 Code Graph Analysis (1)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts (5)
workers/main/src/services/QBO/types.ts (1)
CustomerRevenueByRef(1-7)workers/main/src/services/QBO/QBORepository.ts (1)
QBORepository(20-172)workers/main/src/services/FinApp/IFinAppRepository.ts (1)
IFinAppRepository(6-15)workers/main/src/services/FinApp/FinAppRepository.ts (1)
FinAppRepository(6-32)workers/main/src/services/QBO/QBORepository.integration.test.ts (2)
invoices(107-147)invoices(61-105)
⏰ 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). (2)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (5)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts (5)
8-8: Type-only FinApp imports are correct and scoped.Importing Employee, IFinAppRepository, and Project as types is precise and avoids runtime coupling.
10-11: QBO type import and module import look correct.CustomerRevenueByRef aligns with the effective revenue map shape used across QBORepository.
34-36: QBO module mocked appropriately.Providing a constructor mock for QBORepository is the right seam for injecting the test double.
61-68: Effective revenue fixture shape is valid.The test data matches CustomerRevenueByRef and uses string keys as expected by QBO aggregation.
117-117: Success-mock of getEffectiveRevenue is in the right place.Stubbing the QBO call keeps the test focused on orchestration and output shaping.
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts
Outdated
Show resolved
Hide resolved
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts
Show resolved
Hide resolved
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts
Outdated
Show resolved
Hide resolved
…nancial Reports - Reformatted import statements in `fetchFinancialAppData.test.ts` for better readability. - Adjusted error handling in `fetchFinancialAppData` to ensure proper disconnection of the mongo pool. - Enhanced test data in `WeeklyFinancialReportRepository.test.ts` to include effective revenue calculations for projects, improving the accuracy of marginality assessments. These changes enhance the clarity and reliability of financial report tests, ensuring more accurate calculations and better organization of test data.
- Modified the `FinAppRepository.test.ts` to include the 'name' field in the projection for project data retrieval, enhancing the accuracy of test expectations. - Added a documentation comment for the 'name' field in the `Project` interface within `types.ts`, clarifying its purpose in the project schema. These changes improve the clarity and reliability of tests related to project data, ensuring that the project name is correctly handled in the repository logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
workers/main/src/services/FinApp/FinAppRepository.test.ts (1)
3-17: Mocking after imports can cause the repo to use real models — hoist vi.mock before importing FinAppRepositoryCurrent order imports FinAppRepository (which imports FinAppSchemas) before calling vi.mock. Depending on transform/hoisting, this can silently bind the real models inside the repository and make tests flaky or hit the DB unintentionally. Hoist the vi.mock call above the FinAppRepository import.
Apply this diff to ensure mocks are applied before the repo is loaded:
import { beforeEach, describe, expect, it, vi } from 'vitest'; import { FinAppRepositoryError } from '../../common/errors'; -import { FinAppRepository } from './FinAppRepository'; -import { EmployeeModel, ProjectModel } from './FinAppSchemas'; - -vi.mock('./FinAppSchemas', () => { - return { - EmployeeModel: { - find: vi.fn(), - }, - ProjectModel: { - find: vi.fn(), - }, - }; -}); +vi.mock('./FinAppSchemas', () => ({ + EmployeeModel: { + find: vi.fn(), + }, + ProjectModel: { + find: vi.fn(), + }, +})); + +import { FinAppRepository } from './FinAppRepository'; +import { EmployeeModel, ProjectModel } from './FinAppSchemas';workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts (1)
217-224: Add test case for QBO repository failureThe test suite covers failures for reading files, fetching employees, fetching projects, and writing files, but doesn't test what happens when the QBO repository fails to fetch effective revenue.
Add this test case after the existing error cases:
it('throws AppError when writeJsonFile throws', async () => { readJsonFile.mockResolvedValue(mockTargetUnits); writeJsonFile.mockRejectedValue(new Error('fail-write')); await expectAppError( fetchFinancialAppData(fileLink), 'Failed to get Fin App Data', ); }); + + it('throws AppError when getEffectiveRevenue throws', async () => { + readJsonFile.mockResolvedValue(mockTargetUnits); + qboRepoInstance.getEffectiveRevenue.mockRejectedValue( + new Error('fail-qbo-revenue'), + ); + await expectAppError( + fetchFinancialAppData(fileLink), + 'Failed to get Fin App Data', + ); + });
♻️ Duplicate comments (3)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts (3)
179-183: Verify that connect and disconnect are actually calledThe current test only checks that calling disconnect doesn't throw, but doesn't verify that the cleanup actually happens during the workflow execution.
Apply this diff to properly verify the cleanup behavior:
it('always disconnects the mongo pool', async () => { setupSuccessMocks(); await fetchFinancialAppData(fileLink).catch(() => {}); - expect(() => mongoPoolInstance.disconnect()).not.toThrow(); + expect(connect).toHaveBeenCalledTimes(1); + expect(disconnect).toHaveBeenCalledTimes(1); });
60-60: Fix type mismatch: quick_books_id should be a numberBased on the Project interface definition and FinAppRepository projection,
quick_books_idis typed asnumber | undefined. Using a string'10'here will cause TypeScript errors and may break revenue lookups that expect numeric keys.Apply this diff to fix the type mismatch:
- quick_books_id: '10', + quick_books_id: 10,
169-176: Strengthen test assertions to verify QBO integrationThe test should verify that the QBO repository was properly instantiated and its method was called to ensure the integration path is fully validated.
Apply this diff to enhance the test coverage:
expect(writeJsonFile).toHaveBeenCalledWith(expectedFilename, { employees: mockEmployees, projects: [ { ...mockProjects[0], effectiveRevenue: 5000, }, ], effectiveRevenue: mockEffectiveRevenue, }); + expect(QBORepository).toHaveBeenCalledTimes(1); + expect(qboRepoInstance.getEffectiveRevenue).toHaveBeenCalledTimes(1);
🧹 Nitpick comments (1)
workers/main/src/services/FinApp/FinAppRepository.test.ts (1)
95-95: Align test descriptions with method names for clarityUsing the exact method names in the describe blocks improves discoverability and consistency.
-describe('getEmployees', () => { +describe('getEmployeesByRedmineIds', () => { ... -describe('getProjects', () => { +describe('getProjectsByRedmineIds', () => {Also applies to: 115-115
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts(7 hunks)workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts(2 hunks)workers/main/src/services/FinApp/FinAppRepository.test.ts(1 hunks)workers/main/src/services/FinApp/types.ts(2 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.ts
- workers/main/src/services/FinApp/types.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.test.ts
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests are co-located with source files and should be named with the pattern *.test.ts
Files:
workers/main/src/services/FinApp/FinAppRepository.test.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.tsworkers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Follow the function naming pattern: prefix? + action (A) + high context (HC) + low context? (LC), using action verbs such as get, fetch, send, create, validate, handle, calculate, and boolean prefixes is, has, should
Use descriptive, unabbreviated variable names; use singular for single values and plural for collections; ensure variable names are context-specific
Files:
workers/main/src/services/FinApp/FinAppRepository.test.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.tsworkers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts
🧬 Code Graph Analysis (3)
workers/main/src/services/FinApp/FinAppRepository.test.ts (1)
workers/main/src/services/FinApp/FinAppRepository.ts (1)
FinAppRepository(6-32)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts (1)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (2)
WeeklyFinancialReportRepository(27-275)WeeklyFinancialReportRepository(30-199)
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts (4)
workers/main/src/services/QBO/types.ts (1)
CustomerRevenueByRef(1-7)workers/main/src/services/QBO/QBORepository.ts (1)
QBORepository(20-172)workers/main/src/services/FinApp/IFinAppRepository.ts (1)
IFinAppRepository(6-15)workers/main/src/services/FinApp/FinAppRepository.ts (1)
FinAppRepository(6-32)
⏰ 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: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
🔇 Additional comments (5)
workers/main/src/services/FinApp/FinAppRepository.test.ts (1)
121-124: No action required: projection already includes ‘name’.The
getProjectsByRedmineIdsmethod inFinAppRepository.tsalready projects{ 'name': 1, 'redmine_id': 1, 'quick_books_id': 1, 'history.rate': 1 }, matching the test’s assertion. No code or test changes are needed.Likely an incorrect or invalid review comment.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts (4)
5-71: LGTM! Excellent refactoring with data builders.The introduction of
createBasicTestData()improves test maintainability by centralizing test data creation and making it reusable across multiple test cases. The data structure is comprehensive and well-organized.
73-132: Well-designed marginality test data with clear mathematical documentation.The
createMarginalityTestData()function provides focused test fixtures specifically for marginality sorting validation. The inline comments clearly explain the mathematical calculations for achieving target marginalities (50%, 75%, 67%), making the test intentions explicit and verifiable.
137-181: Test updated correctly to use data builder pattern.The existing test has been properly refactored to use the new
createBasicTestData()builder while maintaining all its original assertions. The test comprehensively validates both summary and details output, including proper formatting, currency display, and marginality indicators.
198-216: Comprehensive test for effectiveMarginality sorting behavior.The test effectively validates that groups are sorted by effectiveMarginality in descending order (Group B: 75% → Group C: 67% → Group A: 50%). The approach of checking index positions in the details string is appropriate for verifying the sort order in the formatted output.
workers/main/src/activities/weeklyFinancialReports/fetchFinancialAppData.test.ts
Show resolved
Hide resolved
- Updated `WeeklyFinancialReportRepository.test.ts` to enhance test data by including project names and adjusting the sorting logic to ensure groups are sorted alphabetically within the same marginality level. - Modified `WeeklyFinancialReportSorting.test.ts` to reflect the new sorting criteria, ensuring that groups are sorted by marginality level and then by group name alphabetically. - Cleaned up the `WeeklyFinancialReportRepository.ts` interface implementation for better readability. These changes improve the accuracy and clarity of the weekly financial report tests, ensuring proper sorting and organization of group data.
- Added a 'name' field to the project data in `fetchFinancialAppData.test.ts` to improve test accuracy. - Changed the `quick_books_id` type from string to number for consistency in `fetchFinancialAppData.test.ts`. - Cleaned up the implementation of the `WeeklyFinancialReportRepository` class for better readability. These changes enhance the clarity and reliability of the weekly financial report tests and improve the overall structure of the repository code.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (2)
236-260: Speed up lookups with pre-indexed mapsCurrently, each unit performs O(n) searches over employees and projects. Build maps once to reduce lookups to O(1). This can significantly improve performance for larger inputs.
Apply this diff:
private aggregateGroupData({ groupUnits, employees, projects, }: AggregateGroupDataInput) { let groupTotalCogs = 0; let groupTotalRevenue = 0; let effectiveRevenue = 0; - const processedProjects = new Set<number>(); // Отслеживаем обработанные проекты + const processedProjects = new Set<number>(); // Track processed projects to avoid double-counting + const employeesById = new Map(employees.map((e) => [e.redmine_id, e])); + const projectsById = new Map(projects.map((p) => [p.redmine_id, p])); for (const unit of groupUnits) { - const employee = employees.find((e) => e.redmine_id === unit.user_id); - const project = projects.find((p) => p.redmine_id === unit.project_id); + const employee = employeesById.get(unit.user_id); + const project = projectsById.get(unit.project_id); const date = unit.spent_on; const employeeRate = this.safeGetRate(employee?.history, date); const projectRate = this.safeGetRate(project?.history, date); groupTotalCogs += employeeRate * unit.total_hours; groupTotalRevenue += projectRate * unit.total_hours; if (project && !processedProjects.has(project.redmine_id)) { effectiveRevenue += project.effectiveRevenue || 0; processedProjects.add(project.redmine_id); } }
211-225: Ensure date formatting isn’t affected by UTC offsetsUsing
toISOString().slice(0, 10)always formats in UTC, which can shift the date by one day depending on the server’s timezone. We should replace it with a helper that formats in local time (or a fixed timezone) to guarantee the intended YYYY-MM-DD output.• Location:
- workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (around lines 211–225)
• Suggested helper (e.g., at the top of the file or in a shared util):
/** * Formats a Date as 'YYYY-MM-DD' in the specified timeZone (default: system local). */ function formatDate( date: Date, timeZone: string = Intl.DateTimeFormat().resolvedOptions().timeZone ): string { return new Intl.DateTimeFormat('en-CA', { timeZone }).format(date); }• Example diff in
composeWeeklyReportTitle:- const periodStart = new Date( - currentDate.getFullYear(), - quarter * 3, - 1 - ).toISOString() - .slice(0, 10); + const periodStart = formatDate( + new Date(currentDate.getFullYear(), quarter * 3, 1) + ); - const periodEnd = new Date( - currentDate.getFullYear(), - currentDate.getMonth(), - currentDate.getDate() - ((currentDate.getDay() + 6) % 7) - 1 - ) - .toISOString() - .slice(0, 10); + const periodEnd = formatDate( + new Date( + currentDate.getFullYear(), + currentDate.getMonth(), + currentDate.getDate() - ((currentDate.getDay() + 6) % 7) - 1 + ) + );This eliminates any unexpected one-day shifts by using a reliable formatting approach.
♻️ Duplicate comments (1)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (1)
95-115: Optional: refine within-level sort by effectiveMarginality, then nameIf you want higher-performing groups to surface within each marginality level, compare by effectiveMarginality (desc) before groupName. Coordinate with tests if you adopt this.
- // Sort by marginality level (High -> Medium -> Low), - // then within each level by groupName alphabetically + // Sort by marginality level (High -> Medium -> Low), + // then within each level by effectiveMarginality (desc), + // then by groupName alphabetically groupData.sort((a, b) => { const levelComparison = levelOrder[b.marginality.level] - levelOrder[a.marginality.level]; if (levelComparison !== 0) { return levelComparison; } - // Sort by groupName alphabetically within each level - return a.groupName.localeCompare(b.groupName); + if (b.effectiveMarginality !== a.effectiveMarginality) { + return b.effectiveMarginality - a.effectiveMarginality; + } + return a.groupName.localeCompare(b.groupName); });
🧹 Nitpick comments (2)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts (1)
89-90: Initialize repository per test to avoid accidental shared stateAlthough the repository appears stateless, constructing it once at describe-scope can hide state-related regressions later. Initialize in beforeEach for isolation.
Apply this diff:
-import { describe, expect, it } from 'vitest'; +import { describe, expect, it, beforeEach } from 'vitest'; @@ -describe('WeeklyFinancialReportRepository', () => { - const repo = new WeeklyFinancialReportRepository(); +describe('WeeklyFinancialReportRepository', () => { + let repo: WeeklyFinancialReportRepository; + beforeEach(() => { + repo = new WeeklyFinancialReportRepository(); + });workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (1)
51-53: Ensure visual separation between details and footerConcatenating without a newline risks footer sticking to the last group. Add a line break for readability.
- const reportDetails = - initialDetails + WeeklyFinancialReportFormatter.formatFooter(); + const reportDetails = `${initialDetails}\n${WeeklyFinancialReportFormatter.formatFooter()}`;
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts(3 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts(4 hunks)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportSorting.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportSorting.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.test.ts
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests are co-located with source files and should be named with the pattern *.test.ts
Files:
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Follow the function naming pattern: prefix? + action (A) + high context (HC) + low context? (LC), using action verbs such as get, fetch, send, create, validate, handle, calculate, and boolean prefixes is, has, should
Use descriptive, unabbreviated variable names; use singular for single values and plural for collections; ensure variable names are context-specific
Files:
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.tsworkers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts
🧬 Code Graph Analysis (2)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts (1)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (1)
WeeklyFinancialReportRepository(27-274)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (6)
workers/main/src/services/WeeklyFinancialReport/MarginalityCalculator.ts (2)
MarginalityResult(12-17)MarginalityCalculator(19-47)workers/main/src/services/WeeklyFinancialReport/IWeeklyFinancialReportRepository.ts (3)
IWeeklyFinancialReportRepository(10-14)GenerateReportInput(4-8)AggregateGroupDataInput(16-20)workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportFormatter.ts (3)
WeeklyFinancialReportFormatter(31-136)formatSummaryInput(7-12)WeeklyFinancialReportFormatter(27-84)workers/main/src/common/types.ts (1)
TargetUnit(3-14)workers/main/src/services/FinApp/types.ts (2)
Employee(5-12)Project(14-30)workers/main/src/services/WeeklyFinancialReport/GroupAggregator.ts (1)
GroupAggregator(3-15)
⏰ 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). (2)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (4)
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.test.ts (2)
5-87: Good builder: clear, reusable fixturesThe createBasicTestData helper is tidy and reads well. It covers target units, employees, and projects with required fields (including project.name) and keeps tests DRY.
153-228: Alphabetical ordering test is appropriate and focusedThe scenario cleanly isolates “within-level” ordering and uses consistent inputs to keep all groups at the same marginality level. Assertion via indexOf is straightforward and effective.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts (2)
54-56: Preserving order in summary looks goodcreateSortedGroups now respects the global order computed by sortGroupData. This aligns the summary with the detailed section ordering.
243-259: Correct: use QBO effectiveRevenue and dedupe per projectUsing project.effectiveRevenue (with per-project de-duping) aligns with the PR objective and avoids double-counting across multiple units for the same project.
workers/main/src/services/WeeklyFinancialReport/WeeklyFinancialReportRepository.ts
Outdated
Show resolved
Hide resolved
- Renamed `QBORepository` to `qboRepository` for consistency in naming conventions within `fetchFinancialAppData.test.ts`. - Updated mock implementations to reflect the new variable name, ensuring clarity and maintainability in the test structure. - Added assertions to verify the call counts for `qboRepository` and its method `getEffectiveRevenue`, enhancing test reliability. These changes improve the readability and accuracy of the financial app data tests, ensuring proper mocking and verification of the QBO repository interactions.
- Updated the handling of `quick_books_id` in `fetchFinancialAppData` to ensure it is converted to a string before accessing effective revenue data, preventing potential type-related issues. - Adjusted error handling in the test for `fetchFinancialAppData` to maintain proper disconnection of the mongo pool, ensuring resource management is correctly implemented. These changes enhance the reliability of financial app data fetching and improve the robustness of the associated tests.
- Added `groupTotalHours` to the `FormatDetailInput` interface and updated the `WeeklyFinancialReportFormatter` to include total hours in the formatted output. - Modified the `WeeklyFinancialReportRepository` to aggregate and return `groupTotalHours` alongside other financial metrics. These changes improve the detail and comprehensiveness of the weekly financial reports, providing better insights into group performance.
- Added a 'name' field to the project model in `FinAppSchemas.test.ts` to ensure valid project creation in tests. - Adjusted assertions in `WeeklyFinancialReportRepository.test.ts` to standardize the casing of output strings, improving test accuracy and consistency. These changes enhance the reliability of tests related to project data and weekly financial reports, ensuring that outputs are correctly formatted and validated.
|
QBORepositoryto fetch effective revenue data, enhancing financial report accuracy.WeeklyFinancialReportRepositoryto include effective revenue calculations and sorting by marginality levels.These changes improve the financial reporting capabilities, providing clearer insights into revenue and performance metrics.
Summary by CodeRabbit
New Features
Tests
Chores