Add unit tests for Redmine activities and weekly financial reports #34
Conversation
This commit removes the unused `workflowsPath` field from the worker configuration, simplifying the setup. Additionally, it introduces an environment validation utility to verify required environment variables, improving runtime robustness. Finally, updates to dependencies and `package-lock.json` reflect added packages like `uuid` and `dotenv` for functionality and testing.
This commit updates the variable name from `message` to `errorMessage` in the `validateEnv` function within the `utils.ts` file. This change improves clarity and consistency in error logging for missing or invalid environment variables, ensuring that the correct variable is referenced when logging errors. The overall functionality of the environment validation remains unchanged.
…al reports - Introduced unit tests for the `getProjectUnits` function in `redmine.test.ts`, ensuring it returns project units correctly and handles errors gracefully. - Enhanced `weeklyFinancialReports.test.ts` with mocked activities to validate the report generation, including project units and financial data. - Created new activity files for `getProjectUnits` and `fetchFinancialData`, establishing a clear structure for financial report generation. - Added type definitions for `ProjectUnit` and `FinancialData` to improve type safety and clarity in the codebase. These changes enhance test coverage and reliability of the Redmine integration and financial reporting functionalities.
📝 WalkthroughWalkthroughThis set of changes introduces a new financial reporting workflow and supporting infrastructure, including repositories, services, activities, and tests for project unit aggregation from Redmine data. It removes legacy workflow and utility code, updates exports, and refactors error handling. Environment variable handling and documentation are clarified, and test coverage is expanded and refactored. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as weeklyFinancialReportsWorkflow
participant Activities as getProjectUnits (activity)
participant Service as FinancialReportService
participant Repo as ProjectUnitRepository
participant DB as RedminePool/MySQL
Workflow->>Activities: getProjectUnits()
Activities->>Service: getWeeklyReport()
Service->>Repo: getProjectUnits()
Repo->>DB: execute SQL query
DB-->>Repo: project unit rows
Repo-->>Service: project unit array
Service-->>Activities: project unit array
Activities-->>Workflow: project unit array
Workflow->>Workflow: formatFinancialReport(units)
Workflow-->>Workflow: formatted report string
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🔍 Vulnerabilities of
|
| digest | sha256:90b02f3f89b5a3b555e75638fd1f89926088064efb2eb7d1227d391210191d68 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 243 MB |
| packages | 1628 |
📦 Base Image node:20-alpine
| also known as |
|
| digest | sha256:37a5a350292926f98d48de9af160b0a3f7fcb141566117ee452742739500a5bd |
| vulnerabilities |   |
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
- Introduced unit tests for the `fetchFinancialData` function in `redmine.test.ts`, validating its behavior for both default and custom periods. - Ensured that the function returns the expected mock data, enhancing test coverage for financial reporting functionalities. These changes improve the reliability of the Redmine integration tests.
- Adjusted the import order in `redmine.test.ts` to ensure consistency and clarity. - Added spacing in test cases for improved readability. These changes enhance the organization and maintainability of the test file.
- Deleted the `utils.ts` file as its functions are no longer utilized in the codebase. - Updated the `index.ts` file to directly implement error logging within the `handleRunError` function, enhancing clarity and reducing dependencies. - Adjusted the Vitest configuration to include all test files in the `src` directory for improved test coverage. These changes simplify the code structure and improve error handling consistency across the application.
…tency - Removed the unused `utils.ts` import from `index.test.ts` to simplify the code structure. - Ensured consistent error logging format in the `handleRunError` function. These changes enhance the clarity and maintainability of the test file.
- Deleted the `fetchFinancialData.ts` file, which contained the `FinancialData` interface and the associated function for fetching financial data. This change simplifies the codebase by removing unused code that is no longer needed. This update enhances maintainability by eliminating obsolete components related to financial data fetching.
- Introduced a new workflow for generating weekly financial reports, utilizing mocked activities for testing. - Created unit tests for the `weeklyFinancialReportsWorkflow` and `generateReport` functions to ensure correct report generation and formatting. - Updated the Vitest configuration to exclude specific files from coverage, enhancing test accuracy. These changes improve the functionality and test coverage of the financial reporting features.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
workers/main/src/activities/weeklyFinancialReports/redmine.test.ts (1)
15-21: Enhance test coverage by verifying data consistency.The second test case only checks the
periodfield but doesn't verify that other financial data fields remain consistent. This could miss potential bugs where the period parameter affects other fields unexpectedly.Consider expanding the assertions to verify key financial data fields:
it('returns expected mock data for custom period', async () => { const data = await fetchFinancialData('previous'); expect(data).toBeDefined(); expect(data.period).toBe('previous'); + expect(data.contractType).toBe('T&M'); + expect(data.revenue).toBe(120000); + expect(data.cogs).toBe(80000); + expect(data.margin).toBe(40000); });workers/main/src/common/utils.test.ts (2)
3-5: Fix the mock path to ensure proper module mocking.The mock path
'../common/../configs'is unnecessarily complex and could be simplified to'../configs'for better readability and maintainability.-vi.mock('../common/../configs', () => ({ +vi.mock('../configs', () => ({ validationResult: { success: true }, }));
24-26: Improve the process.exit mock implementation.The current implementation throws an error instead of preventing the actual exit. While this works for testing, it's better to use a more explicit mock that doesn't rely on throwing errors.
- exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => { - throw new Error('exit'); - }) as unknown as ReturnType<typeof vi.spyOn>; + exitSpy = vi.spyOn(process, 'exit').mockImplementation((() => {}) as any);Then update the test assertion to check for the spy call rather than expecting a throw:
- expect(() => validateEnv()).toThrow('exit'); + validateEnv();workers/main/src/common/Redmine.ts (2)
4-9: Consider centralizing the ProjectUnit interface.The
ProjectUnitinterface is duplicated in both this file andredmine.types.ts. Consider importing it from the types file to maintain a single source of truth.-export interface ProjectUnit { - group_id: number; - group_name: string; - project_id: number; - project_name: string; -} +import type { ProjectUnit } from '../activities/weeklyFinancialReports/redmine.types';
56-60: Add resource cleanup method.Consider adding a method to properly close the connection pool when the Redmine instance is no longer needed to prevent potential memory leaks.
+ async close(): Promise<void> { + if (this.pool) { + await this.pool.end(); + } + } + /** Expose the Pool Connection */ get connection() { return this.pool; }workers/main/src/activities/weeklyFinancialReports/redmine.ts (1)
11-25: Consider making mock data more realistic and configurable.The hardcoded financial data might not be suitable for all testing scenarios. Consider making it more configurable or realistic, especially the marginality calculations which should be consistent with the revenue and margin values.
export async function fetchFinancialData( period: string = 'current', ): Promise<FinancialData> { + // Calculate marginality to ensure consistency: margin / revenue * 100 + const revenue = 120000; + const margin = 40000; + const marginality = Number(((margin / revenue) * 100).toFixed(1)); + + const effectiveRevenue = 110000; + const effectiveMargin = 35000; + const effectiveMarginality = Number(((effectiveMargin / effectiveRevenue) * 100).toFixed(1)); + return { - period: period, + period, contractType: 'T&M', - revenue: 120000, - cogs: 80000, - margin: 40000, - marginality: 33.3, - effectiveRevenue: 110000, - effectiveMargin: 35000, - effectiveMarginality: 31.8, + revenue, + cogs: 80000, + margin, + marginality, + effectiveRevenue, + effectiveMargin, + effectiveMarginality, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
workers/common/utils.ts(0 hunks)workers/main/src/__tests__/weeklyFinancialReports.test.ts(0 hunks)workers/main/src/activities/fetchFinancialData.ts(0 hunks)workers/main/src/activities/index.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/index.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/redmine.test.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/redmine.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/redmine.types.ts(1 hunks)workers/main/src/common/Redmine.test.ts(1 hunks)workers/main/src/common/Redmine.ts(1 hunks)workers/main/src/common/utils.test.ts(1 hunks)workers/main/src/index.test.ts(2 hunks)workers/main/src/index.ts(2 hunks)workers/main/src/workflows/index.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/index.test.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/index.ts(1 hunks)workers/main/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (3)
- workers/main/src/tests/weeklyFinancialReports.test.ts
- workers/main/src/activities/fetchFinancialData.ts
- workers/common/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (8)
workers/main/src/activities/weeklyFinancialReports/redmine.test.ts (1)
workers/main/src/activities/weeklyFinancialReports/redmine.ts (1)
fetchFinancialData(11-25)
workers/main/src/index.test.ts (1)
workers/main/src/index.ts (2)
logger(7-7)handleRunError(40-46)
workers/main/src/activities/weeklyFinancialReports/redmine.ts (3)
workers/main/src/common/Redmine.ts (3)
getProjectUnits(24-54)ProjectUnit(4-9)Redmine(11-60)workers/main/src/activities/weeklyFinancialReports/redmine.types.ts (2)
ProjectUnit(1-6)FinancialData(8-18)workers/main/src/configs/redmineDatabase.ts (1)
redmineDatabaseConfig(3-8)
workers/main/src/common/Redmine.ts (1)
workers/main/src/activities/weeklyFinancialReports/redmine.types.ts (1)
ProjectUnit(1-6)
workers/main/src/common/Redmine.test.ts (1)
workers/main/src/common/Redmine.ts (1)
Redmine(11-60)
workers/main/src/common/utils.test.ts (1)
workers/main/src/common/utils.ts (1)
validateEnv(3-15)
workers/main/src/activities/weeklyFinancialReports/redmine.types.ts (1)
workers/main/src/common/Redmine.ts (1)
ProjectUnit(4-9)
workers/main/src/workflows/weeklyFinancialReports/index.ts (3)
workers/main/src/activities/weeklyFinancialReports/redmine.types.ts (1)
FinancialData(8-18)workers/main/src/activities/weeklyFinancialReports/redmine.ts (2)
getProjectUnits(5-9)fetchFinancialData(11-25)workers/main/src/common/Redmine.ts (1)
getProjectUnits(24-54)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (18)
workers/main/src/index.test.ts (2)
4-4: LGTM: Import statement updated correctlyThe import path change from '../index' to './index' is correct since both files are in the same directory. Adding
loggerto the imports aligns with the refactored error handling approach.
29-35: LGTM: Test updated to match refactored error handlingThe test correctly adapts to the new error handling approach:
- Spies on
logger.errorinstead of the removedutils.logWorkerError- Expects the formatted error message string that matches the implementation
- Uses
Errorinstance soerror.messageaccess in the implementation works correctlyworkers/main/src/index.ts (2)
3-3: LGTM: Import path updated for local utilitiesThe import change from shared utilities to local
./common/utilsaligns with the overall refactoring to remove dependencies on the sharedworkers/common/utils.tsmodule.
40-46: LGTM: Error handling refactored with improvementsThe
handleRunErrorfunction refactoring includes several good improvements:
- Parameter renamed from
errtoerrorfor better clarity- Direct use of
logger.errorinstead of external utility function- Safe error message formatting that handles both Error instances and other types
- Preserved original behavior (logging, scheduled exit, error throwing)
workers/main/src/activities/index.ts (1)
1-1: LGTM! Standard barrel export pattern.This follows the conventional approach for organizing module exports in TypeScript projects, providing a clean entry point for the weekly financial reports activities.
workers/main/vitest.config.ts (2)
10-10: Good improvement: Moving to co-located test pattern.The change from a centralized
src/__tests__directory to distributedsrc/**/*.test.tsfiles is a better practice that keeps tests closer to the code they're testing.
15-15: Updated exclusions align with new test structure.Removing the old
src/__tests__/**exclusion and adding specific config files is appropriate for the new test organization.workers/main/src/activities/weeklyFinancialReports/index.ts (1)
1-2: Well-organized module structure.The separation of implementation (
redmine) and types (redmine.types) with clean re-exports follows TypeScript best practices for module organization.workers/main/src/workflows/index.ts (1)
1-1: Excellent improvement: Using explicit named exports.Changing from wildcard export to named export is a best practice that:
- Makes the public API explicit and intentional
- Prevents accidental exposure of internal implementation details
- Improves IDE support and tree-shaking capabilities
workers/main/src/workflows/weeklyFinancialReports/index.test.ts (2)
55-88: Excellent integration test implementation.This test demonstrates best practices for Temporal workflow testing:
- Proper use of TestWorkflowEnvironment with time skipping
- Activity mocking for isolated testing
- Unique task queue and workflow IDs using UUIDs
- Comprehensive assertions validating report content
The test effectively verifies the workflow integration without external dependencies.
91-106: Well-structured unit test for generateReport function.The unit test properly isolates the
generateReportfunction and verifies all key financial metrics are included in the formatted output. Good separation of concerns between integration and unit testing.workers/main/src/activities/weeklyFinancialReports/redmine.types.ts (1)
8-18: Well-structured FinancialData interface.The interface comprehensively covers all financial metrics needed for reporting, including both regular and effective values. The field names are clear and the types are appropriate.
workers/main/src/common/utils.test.ts (2)
34-39: LGTM - Good test coverage for success scenario.The test correctly verifies that when validation succeeds, no errors are logged and the process doesn't exit.
41-57: LGTM - Comprehensive test for error scenario.The test properly validates the error message formatting and ensures the process exits with the correct code. The test data covers both cases where path exists and where it's empty.
workers/main/src/common/Redmine.ts (2)
15-18: LGTM - Proper connection pool initialization.The constructor correctly initializes the connection pool with the provided credentials.
24-54: LGTM - Well-implemented parameterized query with proper SQL injection protection.The method correctly uses parameterized queries to prevent SQL injection and handles optional filtering appropriately. The query structure and parameter binding are properly implemented.
workers/main/src/workflows/weeklyFinancialReports/index.ts (2)
6-10: LGTM - Proper activity proxy configuration.The activity proxy is correctly configured with an appropriate timeout for database operations.
26-38: LGTM - Well-structured workflow with proper error handling.The workflow correctly orchestrates the activities and handles errors appropriately. The combination of financial data and project units in the final output provides comprehensive reporting.
workers/main/src/activities/weeklyFinancialReports/redmine.types.ts
Outdated
Show resolved
Hide resolved
- Added an `env_file` entry in `docker-compose.yml` to streamline environment variable management. - Updated the Vitest configuration to exclude additional test files, enhancing test coverage accuracy. - Refactored `Redmine.test.ts` to include a mock for `mysql2/promise` and reorganized test cases for better clarity and maintainability. These changes improve the development environment setup and enhance the organization of test files.
…' into feature/64207_redmine_activities # Conflicts: # workers/main/vitest.config.ts
- Changed the exclusion pattern from `**/src/__tests__/**` to `**/*.test.ts` to better target test files for exclusion in Sonar analysis. - This update enhances the accuracy of code quality metrics by ensuring that only relevant files are included in the analysis.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
workers/main/src/common/Redmine.test.ts (1)
35-83: Past review comment still applies to commented-out tests.The previous review comment about improving test assertions for data structure verification is still relevant if these activity tests are activated.
The past feedback about enhancing assertions to verify returned data structure and content remains valid for the commented-out activity tests.
🧹 Nitpick comments (2)
sonar-project.properties (1)
4-4: Consider broadening test exclusion patterns
The current exclusion only matches*.test.tsfiles. If your codebase uses other conventions (e.g.,*.spec.ts), they won’t be excluded and may surface in Sonar analysis.- sonar.exclusions=**/*.test.ts,**/src/dist/** + sonar.exclusions=**/*.test.ts,**/*.spec.ts,**/src/dist/**workers/main/src/common/Redmine.test.ts (1)
19-33: Remove unused mock data.The
mockProjectUnitsvariable is unused since the activity tests are commented out.-// Mock data -const mockProjectUnits = [ - { - group_id: 1, - group_name: 'Engineering', - project_id: 101, - project_name: 'Project Alpha', - }, - { - group_id: 2, - group_name: 'QA', - project_id: 102, - project_name: 'Project Beta', - }, -];🧰 Tools
🪛 GitHub Actions: Code Quality
[warning] 20-20: ESLint: 'mockProjectUnits' is assigned a value but never used (@typescript-eslint/no-unused-vars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docker-compose.yml(1 hunks)sonar-project.properties(1 hunks)workers/main/src/common/Redmine.test.ts(1 hunks)workers/main/vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workers/main/vitest.config.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
workers/main/src/common/Redmine.test.ts (1)
workers/main/src/common/Redmine.ts (1)
Redmine(11-60)
🪛 GitHub Check: CodeQL
🪛 GitHub Actions: Code Quality
workers/main/src/common/Redmine.test.ts
[error] 1-1: ESLint: Run autofix to sort these imports! (simple-import-sort/imports)
[warning] 2-2: ESLint: 'MockActivityEnvironment' is defined but never used (@typescript-eslint/no-unused-vars)
[warning] 3-3: ESLint: 'TestWorkflowEnvironment' is defined but never used (@typescript-eslint/no-unused-vars)
[warning] 5-5: ESLint: 'DefaultLogger' is defined but never used (@typescript-eslint/no-unused-vars)
[warning] 5-5: ESLint: 'LogEntry' is defined but never used (@typescript-eslint/no-unused-vars)
[warning] 5-5: ESLint: 'Runtime' is defined but never used (@typescript-eslint/no-unused-vars)
[error] 6-6: Prettier: Replace imports formatting to multiple lines (prettier/prettier)
[warning] 6-6: ESLint: 'afterAll' is defined but never used (@typescript-eslint/no-unused-vars)
[warning] 6-6: ESLint: 'beforeAll' is defined but never used (@typescript-eslint/no-unused-vars)
[warning] 6-6: ESLint: 'afterEach' is defined but never used (@typescript-eslint/no-unused-vars)
[error] 15-15: ESLint: Run autofix to sort these imports! (simple-import-sort/imports)
[warning] 16-16: ESLint: 'getProjectUnits' is defined but never used (@typescript-eslint/no-unused-vars)
[warning] 20-20: ESLint: 'mockProjectUnits' is assigned a value but never used (@typescript-eslint/no-unused-vars)
[error] 87-87: Prettier: Replace object properties formatting to multiple lines (prettier/prettier)
[error] 88-88: Prettier: Replace 'typeof vi.fn' formatting to multiple lines (prettier/prettier)
[error] 89-89: ESLint: Expected blank line before this statement (padding-line-between-statements)
[error] 90-90: ESLint: Unexpected any. Specify a different type (@typescript-eslint/no-explicit-any)
[error] 93-93: ESLint: Expected blank line before this statement (padding-line-between-statements)
[error] 93-93: ESLint: Unexpected any. Specify a different type (@typescript-eslint/no-explicit-any)
[error] 93-93: ESLint: Unsafe member access .pool on an any value (@typescript-eslint/no-unsafe-member-access)
[error] 94-94: ESLint: Unexpected any. Specify a different type (@typescript-eslint/no-explicit-any)
[error] 94-94: ESLint: Unsafe member access .credentials on an any value (@typescript-eslint/no-unsafe-member-access)
[error] 95-95: ESLint: Unsafe call of a(n) any typed value (@typescript-eslint/no-unsafe-call)
[error] 95-95: ESLint: Unexpected any. Specify a different type (@typescript-eslint/no-explicit-any)
[error] 95-95: ESLint: Unsafe member access .ensureConnection on an any value (@typescript-eslint/no-unsafe-member-access)
[error] 100-100: Prettier: Replace object properties formatting to multiple lines (prettier/prettier)
[error] 105-105: Prettier: Replace 'typeof vi.fn' formatting to multiple lines (prettier/prettier)
[error] 106-106: ESLint: Expected blank line before this statement (padding-line-between-statements)
[error] 112-112: ESLint: Expected blank line before this statement (padding-line-between-statements)
[error] 119-119: Prettier: Insert missing comma (prettier/prettier)
[error] 126-126: Prettier: Insert missing comma (prettier/prettier)
[error] 131-131: Prettier: Replace object properties formatting to multiple lines (prettier/prettier)
[error] 132-132: Prettier: Replace 'execute: vi.fn().mockRejectedValue(new Error(...))' formatting to multiple lines with trailing comma (prettier/prettier)
[error] 133-133: Prettier: Replace 'typeof vi.fn' formatting to multiple lines (prettier/prettier)
[error] 134-134: ESLint: Expected blank line before this statement (padding-line-between-statements)
[error] 138-138: Prettier: Replace string formatting to multiple lines with trailing comma (prettier/prettier)
🔇 Additional comments (1)
workers/main/src/common/Redmine.test.ts (1)
35-83: Consider activating or removing commented-out activity tests.The commented-out tests provide valuable coverage for the activity layer and address integration testing. Consider whether these should be activated or completely removed.
These tests seem to provide value by testing the activity function
getProjectUnitsthat wraps the Redmine class. Should these be:
- Activated to provide activity-level test coverage
- Moved to a separate activity test file
- Removed entirely
If activated, the past review comment about improving assertions would still apply.
- Updated the Vitest configuration to format the exclusion patterns for better readability. - Refactored `Redmine.test.ts` by removing unused mocks and reorganizing test cases for clarity. - Enhanced the structure of the test suite for `Redmine` activities, ensuring better maintainability and readability. These changes streamline the testing setup and improve the overall organization of test files.
…file requirements - Revised the instructions for creating the `.env` file to emphasize its necessity for Docker Compose. - Added a note regarding the `.env.example` file, detailing its contents and the importance of editing the `.env` file for specific configurations. - Clarified the process for starting services after setting up the environment file. These changes enhance the documentation by providing clearer guidance on environment setup and ensuring users understand the importance of the `.env` file.
- Modified the exclusion patterns in `sonar-project.properties` to include `docker-compose.yml` alongside existing patterns. This change ensures that the specified files are excluded from Sonar analysis, improving the accuracy of code quality metrics. This update enhances the configuration for Sonar analysis by refining the exclusion criteria.
…ne class structure - Moved the `ProjectUnit` interface to a new `types.ts` file for better organization and separation of concerns. - Updated the `Redmine` class to import the `ProjectUnit` interface from the new location, enhancing code clarity. - Refactored the `getProjectUnits` function to streamline its implementation. - Improved error handling in tests for `getProjectUnits`, ensuring robust validation of results. These changes enhance the maintainability and readability of the Redmine-related code, promoting a cleaner architecture.
…verage - Refactored the `getProjectUnits` method in the `Redmine` class to separate query construction into a private method `getProjectUnitsQuery`, improving code clarity and maintainability. - Added comprehensive tests for the `getProjectUnitsQuery` method, ensuring correct query generation and parameter handling for various input scenarios. - Enhanced the test suite for the `Redmine` class, verifying pool initialization and connection management, which strengthens the overall reliability of the Redmine integration. These changes contribute to a more organized code structure and improved test coverage for the Redmine functionalities.
Pull request was converted to draft
…tegration - Introduced `ProjectUnitRepository` and `FinancialReportService` to encapsulate financial data handling and improve code organization. - Implemented `RedmineRepository` and `RedmineService` for better management of Redmine-related data retrieval. - Added new activities and workflows for generating weekly financial reports, enhancing the overall functionality of the financial module. - Removed obsolete test files and added new tests for the financial services, ensuring comprehensive coverage of the new implementations. These changes contribute to a more maintainable and efficient financial reporting system, improving the overall structure and reliability of the codebase.
- Updated the test for `getProjectUnits` in `redmine.test.ts` to use a spy on the service method, ensuring that the method is called correctly. - Refactored tests in `RedminePool.test.ts` to improve readability by formatting mock pool creation and ensuring consistent behavior across tests. - Enhanced assertions to verify the correct number of pool creations after the pool is ended, improving test reliability. These changes contribute to a more robust and maintainable test suite for Redmine functionalities, ensuring accurate validation of service interactions and connection pooling behavior.
…nd ProjectUnitRepository - Introduced new test files for `FinancialReportService` and `ProjectUnitRepository`, ensuring thorough coverage of their functionalities. - Added tests for `getWeeklyReport` in `FinancialReportService`, verifying correct data retrieval from the repository. - Implemented tests for `getProjectUnits` in `ProjectUnitRepository`, checking for expected behavior with both valid and empty responses. - Enhanced existing tests in `factory.test.ts` and `redmine.test.ts` to include additional cases for `getProjectUnits`, improving overall test reliability. These changes contribute to a more robust and maintainable test suite for financial services, ensuring accurate validation of data handling and service interactions.
…oject unit retrieval - Added `FinancialReportFormatter` to format weekly financial reports based on project units. - Introduced tests for `formatFinancialReport` to ensure correct output structure and content. - Updated `weeklyFinancialReportsWorkflow` to utilize the new formatter, improving code clarity and maintainability. - Created a new test file for `RedmineRepository` to validate the behavior of `getProjectUnits` and ensure accurate data retrieval. These changes enhance the financial reporting capabilities and improve the overall structure of the financial module.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
workers/main/src/activities/weeklyFinancialReports/redmine.ts (1)
1-10: 🛠️ Refactor suggestionAddress previous reviewer concerns about redundancy.
Based on past review comments, this file was identified as creating unnecessary complexity and redundancy. The reviewers suggested removing this wrapper and using a factory pattern instead, or moving the functionality directly to the Redmine class.
Consider one of these approaches:
- Remove this file and call the service directly where needed
- Move this logic to a factory function
- If keeping this file, add proper TypeScript typing:
-export const getProjectUnits = async () => service.getProjectUnits(); +export const getProjectUnits = async (): Promise<ProjectUnit[]> => service.getProjectUnits();However, given the previous feedback, I'd recommend removing this abstraction layer entirely.
workers/main/src/activities/weeklyFinancialReports/redmine.test.ts (1)
49-87: Comprehensive test coverage addressing past feedback.These tests effectively address the past review comment requesting tests for
getProjectUnits. The test cases cover:
- Data structure validation
- Empty result handling
- Error propagation
Good use of mock data that aligns with the expected
ProjectUnitinterface structure.Note on mock data structure: The mock data correctly includes only the four fields defined in the
ProjectUnitinterface (group_id,group_name,project_id,project_name), which highlights the type mismatch issue identified in the repository implementation.
🧹 Nitpick comments (9)
workers/main/src/services/redmine/RedmineService.ts (1)
6-8: Consider adding error handling and logging.While the delegation pattern is clean, consider adding error handling, logging, or validation logic to provide value beyond simple delegation to the repository.
async getProjectUnits() { + try { + console.log('Fetching project units from Redmine'); return this.repo.getProjectUnits(); + } catch (error) { + console.error('Failed to fetch project units:', error); + throw error; + } }workers/main/src/workflows/financial/FinancialReportFormatter.ts (1)
6-6: Consider improving report formatting for better readability.Using raw JSON.stringify for a financial report may not provide the best user experience. Consider implementing a more structured, human-readable format.
Example of a more readable format:
- return `${reportTitle}\n${JSON.stringify(units, null, 2)}`; + if (units.length === 0) { + return `${reportTitle}\nNo project units found for this period.`; + } + + const formattedUnits = units.map(unit => + `- ${unit.group_name} > ${unit.project_name} (Group ID: ${unit.group_id}, Project ID: ${unit.project_id})` + ).join('\n'); + + return `${reportTitle}\n\nProject Units (${units.length}):\n${formattedUnits}`;workers/main/src/services/financial/FinancialReportService.ts (1)
3-8: Consider if this thin service layer adds sufficient value.This service class is a very thin wrapper around the repository with no additional business logic, validation, or error handling. Consider whether this abstraction layer is necessary or if the repository could be used directly.
If keeping this service, consider adding business logic such as:
- Data validation
- Error handling and logging
- Data transformation or aggregation
- Caching logic
Otherwise, this might be over-engineering for a simple pass-through operation.
workers/main/src/workflows/financial/index.test.ts (1)
63-65: Consider more specific assertions.While string containment checks work for basic verification, consider adding more structured assertions about the report format or data integrity if the workflow becomes more complex.
workers/main/src/services/financial/FinancialReportService.test.ts (1)
1-41: Consider the value of this service layer.The
FinancialReportServiceappears to be a thin wrapper aroundIProjectUnitRepositorywithout adding business logic. Consider whether this additional layer provides sufficient value or if the activity could directly use the repository.If the service will grow to include business logic (data transformation, validation, caching, etc.), the current structure is good. If it remains a simple delegation, consider removing this layer for simplicity.
workers/main/src/services/redmine/RedmineRepository.test.ts (1)
25-33: Consider avoiding direct testing of private methods.While testing the private
getProjectUnitsQuerymethod ensures SQL correctness, accessing private methods with@ts-expect-errorsuggests the method might benefit from being extracted or the testing approach reconsidered.Consider these alternatives:
- Extract the SQL query to a separate utility function
- Test the SQL indirectly through the public
getProjectUnitsmethod by verifying the executed query- Make the method protected if it needs to be testable
// Alternative: Test indirectly through public method it('getProjectUnits executes correct SQL query', async () => { const mockRows = []; mockExecute.mockResolvedValueOnce([mockRows]); await repo.getProjectUnits(); const [executedQuery] = mockExecute.mock.calls[0]; expect(executedQuery).toContain('SELECT'); expect(executedQuery).toContain('SUM(total_hours) AS total_hours'); // ... other assertions });workers/main/src/common/RedminePool.test.ts (3)
17-22: Replace hard-coded credentials with non-sensitive test values.The static analysis correctly identifies hard-coded credentials. While these are test credentials, it's better practice to use obviously fake values to avoid confusion.
const credentials = { host: 'localhost', - user: 'user', - password: 'pass', - database: 'db', + user: 'test_user', + password: 'test_password', + database: 'test_database', };🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 19-19: Hard-coded credentials
The hard-coded value "user" is used as user name.
53-53: Replace Russian comment with English.For consistency and maintainability, all comments should be in English.
- // После завершения poolEnded = true, getPool создаёт новый пул + // After ending, poolEnded = true, getPool creates a new pool
28-73: Consider simplifying the type assertions for mocked functions.The repeated type casting
(mysql.createPool as unknown as ReturnType<typeof vi.fn>)is verbose and could be simplified.Consider creating a helper variable at the top of the describe block:
describe('RedminePool', () => { const mockCreatePool = mysql.createPool as unknown as ReturnType<typeof vi.fn>; // Then use mockCreatePool.mockReturnValue(mockPool as Pool) throughout });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
workers/main/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
workers/main/package.json(2 hunks)workers/main/src/activities/financial/factory.test.ts(1 hunks)workers/main/src/activities/financial/factory.ts(1 hunks)workers/main/src/activities/financial/getProjectUnits.activity.test.ts(1 hunks)workers/main/src/activities/financial/getProjectUnits.activity.ts(1 hunks)workers/main/src/activities/financial/index.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/redmine.test.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/redmine.ts(1 hunks)workers/main/src/common/RedminePool.test.ts(1 hunks)workers/main/src/index.test.ts(2 hunks)workers/main/src/repositories/financial/IProjectUnitRepository.ts(1 hunks)workers/main/src/repositories/financial/ProjectUnitRepository.test.ts(1 hunks)workers/main/src/repositories/financial/ProjectUnitRepository.ts(1 hunks)workers/main/src/services/financial/FinancialReportService.test.ts(1 hunks)workers/main/src/services/financial/FinancialReportService.ts(1 hunks)workers/main/src/services/redmine/IRedmineRepository.ts(1 hunks)workers/main/src/services/redmine/RedmineRepository.test.ts(1 hunks)workers/main/src/services/redmine/RedmineRepository.ts(1 hunks)workers/main/src/services/redmine/RedmineService.ts(1 hunks)workers/main/src/workflows/financial/FinancialReportFormatter.ts(1 hunks)workers/main/src/workflows/financial/index.test.ts(1 hunks)workers/main/src/workflows/financial/index.ts(1 hunks)workers/main/src/workflows/financial/weeklyFinancialReports.workflow.test.ts(1 hunks)workers/main/src/workflows/financial/weeklyFinancialReports.workflow.ts(1 hunks)workers/main/src/workflows/index.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- workers/main/src/workflows/weeklyFinancialReports/index.ts
✅ Files skipped from review due to trivial changes (6)
- workers/main/src/activities/financial/index.ts
- workers/main/src/services/redmine/IRedmineRepository.ts
- workers/main/src/workflows/financial/index.ts
- workers/main/src/activities/financial/getProjectUnits.activity.ts
- workers/main/src/activities/financial/factory.ts
- workers/main/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- workers/main/src/index.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (15)
workers/main/src/workflows/financial/weeklyFinancialReports.workflow.ts (3)
workers/main/src/activities/financial/getProjectUnits.activity.ts (1)
getProjectUnits(3-4)workers/main/src/repositories/financial/ProjectUnitRepository.ts (1)
getProjectUnits(43-48)workers/main/src/workflows/financial/FinancialReportFormatter.ts (1)
formatFinancialReport(3-7)
workers/main/src/services/redmine/RedmineService.ts (1)
workers/main/src/services/redmine/IRedmineRepository.ts (1)
IRedmineRepository(3-5)
workers/main/src/repositories/financial/IProjectUnitRepository.ts (1)
workers/main/src/common/types.ts (1)
ProjectUnit(1-6)
workers/main/src/services/financial/FinancialReportService.test.ts (2)
workers/main/src/repositories/financial/IProjectUnitRepository.ts (1)
IProjectUnitRepository(3-5)workers/main/src/services/financial/FinancialReportService.ts (1)
FinancialReportService(3-8)
workers/main/src/services/financial/FinancialReportService.ts (1)
workers/main/src/repositories/financial/IProjectUnitRepository.ts (1)
IProjectUnitRepository(3-5)
workers/main/src/workflows/financial/index.test.ts (2)
workers/main/src/workflows/financial/weeklyFinancialReports.workflow.ts (1)
weeklyFinancialReportsWorkflow(10-14)workers/main/src/workflows/index.ts (1)
weeklyFinancialReportsWorkflow(1-1)
workers/main/src/workflows/financial/FinancialReportFormatter.ts (1)
workers/main/src/common/types.ts (1)
ProjectUnit(1-6)
workers/main/src/workflows/financial/weeklyFinancialReports.workflow.test.ts (2)
workers/main/src/workflows/financial/FinancialReportFormatter.ts (1)
formatFinancialReport(3-7)workers/main/src/workflows/financial/weeklyFinancialReports.workflow.ts (1)
weeklyFinancialReportsWorkflow(10-14)
workers/main/src/activities/financial/factory.test.ts (3)
workers/main/src/activities/financial/factory.ts (2)
createProjectUnitRepository(4-6)createFinancialReportService(8-10)workers/main/src/repositories/financial/ProjectUnitRepository.ts (1)
ProjectUnitRepository(8-49)workers/main/src/services/financial/FinancialReportService.ts (1)
FinancialReportService(3-8)
workers/main/src/repositories/financial/ProjectUnitRepository.test.ts (3)
workers/main/src/common/RedminePool.ts (1)
RedminePool(4-33)workers/main/src/common/types.ts (1)
ProjectUnit(1-6)workers/main/src/repositories/financial/ProjectUnitRepository.ts (1)
ProjectUnitRepository(8-49)
workers/main/src/activities/financial/getProjectUnits.activity.test.ts (1)
workers/main/src/services/financial/FinancialReportService.ts (1)
FinancialReportService(3-8)
workers/main/src/services/redmine/RedmineRepository.test.ts (2)
workers/main/src/common/RedminePool.ts (1)
RedminePool(4-33)workers/main/src/services/redmine/RedmineRepository.ts (1)
RedmineRepository(7-51)
workers/main/src/repositories/financial/ProjectUnitRepository.ts (4)
workers/main/src/repositories/financial/IProjectUnitRepository.ts (1)
IProjectUnitRepository(3-5)workers/main/src/common/RedminePool.ts (1)
RedminePool(4-33)workers/main/src/configs/redmineDatabase.ts (1)
redmineDatabaseConfig(3-8)workers/main/src/common/types.ts (1)
ProjectUnit(1-6)
workers/main/src/common/RedminePool.test.ts (1)
workers/main/src/common/RedminePool.ts (1)
RedminePool(4-33)
workers/main/src/activities/weeklyFinancialReports/redmine.test.ts (2)
workers/main/src/services/redmine/RedmineService.ts (1)
RedmineService(3-9)workers/main/src/services/redmine/RedmineRepository.ts (1)
RedmineRepository(7-51)
🪛 GitHub Check: CodeQL
workers/main/src/common/RedminePool.test.ts
[failure] 19-19: Hard-coded credentials
The hard-coded value "user" is used as user name.
🔇 Additional comments (33)
workers/main/src/workflows/index.ts (1)
1-1: LGTM! Export restructuring aligns with new architecture.The change from wildcard export to named export improves clarity and follows best practices by making dependencies explicit.
workers/main/src/services/redmine/RedmineService.ts (1)
3-4: Clean dependency injection implementation.The constructor properly accepts an interface dependency, following dependency inversion principle.
workers/main/src/workflows/financial/weeklyFinancialReports.workflow.ts (2)
6-8: Proper Temporal activity configuration.The proxyActivities setup with 10-minute timeout is appropriate for database operations and follows Temporal best practices.
10-14: Clean workflow implementation with good separation of concerns.The workflow correctly separates data fetching from formatting, making it easy to test and maintain. The async/await pattern is properly used.
workers/main/src/repositories/financial/IProjectUnitRepository.ts (1)
3-5: Well-designed interface following SOLID principles.The interface is focused, type-safe, and follows the dependency inversion principle. The method signature clearly communicates its purpose and return type.
workers/main/src/repositories/financial/ProjectUnitRepository.test.ts (4)
8-17: Well-structured mock setup for database testing.The mock configuration properly isolates the repository from actual database connections by mocking
RedminePooland itsgetPool().execute()method. ThebeforeEachhook ensures clean test state.
20-36: Comprehensive success case testing.This test properly verifies both the return value and that the database execute method is called, ensuring the repository correctly delegates to the underlying pool.
38-44: Good edge case coverage for empty results.Testing the empty array scenario ensures the repository handles the case where no project units are found gracefully.
46-51: Proper error propagation testing.The test correctly verifies that database errors are propagated up from the repository layer without being swallowed or transformed.
workers/main/src/workflows/financial/index.test.ts (3)
26-35: Proper Temporal test environment setup.The configuration correctly sets up a time-skipping test environment with appropriate logging level for test execution.
41-61: Well-structured workflow integration test.The test properly:
- Creates isolated test environment with unique task queue
- Mocks activities at the worker level
- Executes workflow end-to-end
- Uses
runUntilfor proper test execution
52-52:Details
✅ Verification successful
Verify workflow export path.
Ensure that
./index.tscorrectly exports theweeklyFinancialReportsWorkflowfunction, as this path is used by the Temporal worker to load workflows.
🏁 Script executed:
#!/bin/bash # Description: Verify that the workflow is properly exported from the index file # Expected: The index.ts file should export weeklyFinancialReportsWorkflow cat workers/main/src/workflows/financial/index.tsLength of output: 102
🏁 Script executed:
#!/bin/bash # Verify that the workflow file exports the correct function echo "Directory contents:" ls -1 workers/main/src/workflows/financial echo "" echo "Checking export in weeklyFinancialReports.workflow.ts:" grep -R "export.*weeklyFinancialReportsWorkflow" -n workers/main/src/workflows/financial/weeklyFinancialReports.workflow.ts || trueLength of output: 552
Verified workflow export path
The
workers/main/src/workflows/financial/index.tsfile usesexport * from './weeklyFinancialReports.workflow', and theweeklyFinancialReports.workflow.tsfile declares and exports theweeklyFinancialReportsWorkflowfunction. No changes are needed here.workers/main/src/workflows/financial/weeklyFinancialReports.workflow.test.ts (4)
6-21: Clean unit test for formatting function.The test properly isolates the formatting logic and verifies the expected content is present in the output.
23-34: Proper Temporal workflow mocking.The mock correctly simulates the
proxyActivitiesfunction returning the mockedgetProjectUnitsactivity.
36-36: Import after mock is correct.Placing the workflow import after the mock ensures that the mocked dependencies are in place when the workflow module is loaded.
38-46:Details
✅ Verification successful
Verify mock data structure consistency.
The mock data should match the actual structure returned by the real
getProjectUnitsactivity. Based on the repository implementation, the actualProjectUnitinterface may include additional fields.
🏁 Script executed:
#!/bin/bash # Description: Check if mock data matches the complete ProjectUnit interface # Expected: ProjectUnit should include all required fields echo "=== ProjectUnit interface definition ===" cat workers/main/src/common/types.ts echo -e "\n=== Repository query fields ===" ast-grep --pattern 'SELECT $$$' workers/main/src/repositories/financial/ProjectUnitRepository.tsLength of output: 428
🏁 Script executed:
#!/bin/bash echo "=== Test file: weeklyFinancialReports.workflow.test.ts ===" cat workers/main/src/workflows/financial/weeklyFinancialReports.workflow.test.tsLength of output: 1505
Mock data matches
ProjectUnitinterface – no changes neededI’ve verified that the
ProjectUnitinterface inworkers/main/src/common/types.tsonly containsgroup_id,group_name,project_id, andproject_name, and the test’s mock forgetProjectUnitsincludes exactly those fields. The mock data aligns with the actual activity return type, so no update is required here.workers/main/src/services/financial/FinancialReportService.test.ts (3)
7-21: Well-structured service test with proper mocking.The test correctly uses
Partial<IProjectUnitRepository>to mock only the needed methods and verifies the service delegates to the repository correctly.
23-31: Good coverage of empty result scenario.Testing the empty array case ensures the service handles no-data scenarios properly.
33-40: Proper error propagation testing.The test verifies that repository errors are correctly propagated through the service layer.
workers/main/src/services/redmine/RedmineRepository.test.ts (3)
8-15: Well-structured mocking setup for database testing.The partial mocking approach for both
PoolandRedminePoolis clean and appropriate for unit testing database interactions.
35-45: Good test coverage for the happy path.The test properly verifies that the method returns the expected data structure and calls the database execute method.
47-52: Excellent edge case coverage.Testing the empty result scenario ensures the method handles cases where no data is found gracefully.
workers/main/src/common/RedminePool.test.ts (1)
28-34: Good test coverage for pool creation.The test properly verifies that the constructor calls
createPoolwith the correct credentials.workers/main/src/activities/financial/getProjectUnits.activity.test.ts (3)
8-23: Excellent test structure for activity testing.The test properly mocks the factory function and verifies the activity returns the expected data from the service. The mocking strategy appropriately isolates the activity logic from service implementation.
25-36: Good coverage of empty data scenario.Testing the empty array case ensures the activity handles no-data scenarios gracefully.
38-47: Comprehensive error handling test.The test properly verifies that errors from the service are propagated through the activity, which is important for proper error handling in the workflow.
workers/main/src/services/redmine/RedmineRepository.ts (2)
10-12: Good dependency injection pattern.The constructor properly accepts the RedminePool dependency and extracts the pool instance, following good dependency injection practices.
14-43:Details
❓ Verification inconclusive
Review SQL query logic and potential issues.
The query has several concerns that should be addressed:
Ambiguous table alias: Line 34 uses
users AS gbut treats it as "group" data, which is confusing sinceg.lastnameis used asgroup_name. This suggests users table might contain both individual users and groups.Data consistency issue: The query joins
time_entriesonproject_idbut also joinsusersonuser_id, which could create Cartesian products if not properly constrained.Performance consideration: The subquery and multiple JOINs without visible indexes could be slow on large datasets.
🏁 Script executed:
#!/bin/bash # Verify the database schema to understand the relationships echo "Checking for related schema files or documentation..." fd -t f -e sql -e md | head -10 rg -l "CREATE TABLE.*users" rg -l "CREATE TABLE.*time_entries" rg -l "CREATE TABLE.*projects" rg -l "CREATE TABLE.*members"Length of output: 570
Verify SQL query logic and alias usage
Please double-check the database schema and indexing to ensure this query is both correct and performant:
- Confirm that the users table really holds “group” records (alias g) and that using g.lastname for group_name is intentional.
- Ensure the JOINs between members, projects, time_entries and users won’t produce unintended duplicates. In particular, verify there are no missing join predicates or many-to-many relationships that could inflate the result set.
- Review indexes on the columns used in your WHERE and JOIN clauses (e.g. time_entries.spent_on, project_id, user_id) to avoid full table scans.
- If you see performance issues, consider collapsing the subquery into a single grouped SELECT or adding appropriate composite indexes.
workers/main/src/repositories/financial/ProjectUnitRepository.ts (2)
1-6: LGTM - Clean imports and dependencies.The imports are well-organized and include all necessary dependencies for the repository implementation.
8-12: LGTM - Proper repository pattern implementation.The class correctly implements the interface and initializes the database connection pool in the constructor.
workers/main/src/activities/weeklyFinancialReports/redmine.test.ts (3)
1-5: LGTM - Proper test dependencies imported.Clean import structure with necessary testing utilities and modules under test.
19-29: Good integration test pattern.This test properly verifies that the module function delegates to the service layer, which is the intended behavior.
31-47: Excellent service layer testing.Well-structured test that properly mocks the repository dependency and verifies the service delegation pattern. The mock repository setup is clean and the assertions are comprehensive.
workers/main/src/activities/weeklyFinancialReports/redmine.test.ts
Outdated
Show resolved
Hide resolved
…FinancialReportService - Added comprehensive tests for `ProjectUnitRepository.getProjectUnits` to verify correct data retrieval, handling of empty responses, and error scenarios. - Introduced tests for `FinancialReportService.getWeeklyReport` to ensure accurate reporting of project units under various conditions. - Utilized mocking to simulate database interactions, improving test reliability and isolation. These changes strengthen the test coverage for financial services, ensuring robust validation of data handling and service interactions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Added error handling in the `getProjectUnits` method of `RedmineRepository` to ensure that the method correctly throws an error if the query result is not an array. - Improved the overall reliability of the method by catching potential errors during database execution and providing a clear error message. These changes enhance the robustness of the data retrieval process in the Redmine functionalities, ensuring better error management and user feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
workers/main/src/services/redmine/RedmineRepository.ts (1)
45-61: Past review feedback has been properly addressed.The error handling and type safety concerns from the previous review have been well implemented. The method now includes:
- Proper try-catch error handling
- Array validation before type assertion
- Descriptive error messages with context
This is a significant improvement over the previous implementation.
🧹 Nitpick comments (1)
workers/main/src/services/redmine/RedmineRepository.ts (1)
14-43: Clarify the dual users table joins in the SQL query.The query joins the
userstable twice with different purposes, which could be confusing for future maintainers. The first join (users AS g) appears to represent group/organizational hierarchy, while the second (users AS u) represents the actual time entry user.Consider adding comments to clarify the purpose:
private getProjectUnitsQuery() { return `SELECT + -- Main query aggregates time entries by group, project, user, and date group_id, group_name, project_id, project_name, user_id, username, spent_on, SUM(total_hours) AS total_hours FROM ( SELECT + -- g: group/manager user, u: actual time entry user g.id AS group_id, g.lastname AS group_name,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workers/main/src/services/financial/FinancialReportService.ts(1 hunks)workers/main/src/services/redmine/RedmineRepository.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workers/main/src/services/financial/FinancialReportService.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (1)
workers/main/src/services/redmine/RedmineRepository.ts (1)
1-12: Class structure and imports look good.The dependency injection pattern with RedminePool and proper interface implementation follows good architectural practices.
- Updated the `getProjectUnits` method in `RedmineRepository` to enhance data mapping by converting database rows into a more structured format. - Each row is now transformed into an object with explicit types for `group_id`, `group_name`, `project_id`, and `project_name`, improving data integrity and usability. These changes contribute to a clearer and more reliable data retrieval process within the Redmine functionalities.
killev
left a comment
killev
left a comment
There was a problem hiding this comment.
Please split this pr into more focused smaller PRs
…y handling - Added new properties `user_id`, `username`, `spent_on`, and `total_hours` to the `ProjectUnit` interface in `types.ts` to improve data representation. - Refactored the `RedmineRepository` to utilize a new `IPoolProvider` interface, enhancing the flexibility of database interactions. - Updated the SQL query in `getProjectUnits` to include the new fields, ensuring comprehensive data retrieval from the database. - Modified tests in `RedmineRepository.test.ts` to validate the new structure and ensure correct handling of the extended data model. These changes improve the data structure and retrieval process within the Redmine functionalities, enhancing overall usability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
workers/main/src/common/types.ts (1)
1-10: Good interface design with a minor type consideration.The
ProjectUnitinterface is well-structured with clear field names and appropriate types. However, consider usingDatetype for thespent_onfield instead ofstringto ensure type safety and consistency in date operations across the application.- spent_on: string; + spent_on: Date;If you need to keep it as a string for serialization purposes, consider adding a comment to document the expected format (e.g., ISO 8601).
workers/main/src/services/redmine/RedmineRepository.ts (2)
41-41: Consider making the date interval configurable.The 7-day interval is hardcoded in the query. Consider making this configurable to improve flexibility for different reporting periods.
- WHERE te.spent_on >= CURDATE() - INTERVAL 7 DAY + WHERE te.spent_on >= CURDATE() - INTERVAL ? DAYThen modify the method signature to accept a
daysparameter:- async getProjectUnits(): Promise<ProjectUnit[]> { + async getProjectUnits(days: number = 7): Promise<ProjectUnit[]> {
17-44: Add documentation for the complex SQL query.The SQL query is quite complex with multiple joins and subqueries. Consider adding comments to explain the business logic and table relationships for better maintainability.
+// Query to aggregate time entries by group, project, user, and date +// Joins users table twice: once for groups (g) and once for actual users (u) +// Filters to last 7 days and groups by all dimensions except hours const PROJECT_UNITS_QUERY = `SELECT group_id, group_name,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
workers/main/src/common/types.ts(1 hunks)workers/main/src/services/redmine/RedmineRepository.test.ts(1 hunks)workers/main/src/services/redmine/RedmineRepository.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
workers/main/src/services/redmine/RedmineRepository.test.ts (1)
workers/main/src/services/redmine/RedmineRepository.ts (2)
IPoolProvider(46-48)RedmineRepository(50-82)
workers/main/src/services/redmine/RedmineRepository.ts (2)
workers/main/src/services/redmine/IRedmineRepository.ts (1)
IRedmineRepository(3-5)workers/main/src/common/types.ts (1)
ProjectUnit(1-10)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (3)
workers/main/src/services/redmine/RedmineRepository.test.ts (1)
1-69: Excellent test coverage and structure.The test file demonstrates excellent testing practices:
- Comprehensive coverage of success, empty results, and error scenarios
- Proper mocking with Vitest for database dependencies
- Good test isolation with
beforeEachcleanup- Specific error message validation ensures proper error wrapping
The test structure is clean and maintainable.
workers/main/src/services/redmine/RedmineRepository.ts (2)
57-81: Excellent error handling implementation.The error handling has been significantly improved compared to previous versions. The implementation now includes:
- Proper try-catch wrapping
- Array validation before processing
- Explicit type conversions for safety
- Descriptive error messages with context
This addresses the previous concerns about error handling and type safety.
28-29:Details
❌ Incorrect review comment
Verify the group name field mapping.
The query maps
g.lastname AS group_namewhich seems semantically incorrect. Groups typically have anameorgroup_namefield rather thanlastname. Please verify this is the correct field for representing group names in your database schema.
🏁 Script executed:
#!/bin/bash # Verify the database schema for the users table when used as groups # Check what fields are available for group identification # Search for database schema or migration files that define the users table structure fd -e sql -e js -e ts | xargs rg -l "users.*table\|CREATE TABLE.*users\|users.*schema" | head -5 # Look for any documentation about using users table for groups rg -A 10 -B 2 "users.*group\|group.*users" --type ts --type js --type mdLength of output: 173
To verify which table and column
g.lastnameis coming from, let’s pull in the full SQL snippet and alias definitions around those lines:
🏁 Script executed:
#!/bin/bash # Locate the repository file file=$(fd -e ts RedmineRepository.ts) echo "---- File: $file ----" # Show the first 200 lines to capture imports, connection setup, and the full query sed -n '1,200p' "$file" echo echo "---- All occurrences of the `g.` alias ----" rg -n -C3 'g\.' "$file"Length of output: 1177
Let’s pull the full SQL query and see exactly what table
gis aliasing, then inspect thelastnamecolumn in context:
🏁 Script executed:
#!/bin/bash # Locate the RedmineRepository implementation (exclude the interface) file=$(fd -e ts 'RedmineRepository.ts' | grep -v 'IRedmineRepository' | head -n1) echo "---- File: $file ----" # Dump the start of the file to capture imports and the full SQL block sed -n '1,200p' "$file" echo echo "---- Context around alias 'g' and its column mappings ----" rg -n -C3 " g\\.|FROM|JOIN" "$file"Length of output: 3522
No change needed for group_name mapping
In Redmine’s schema, groups are stored in the single-table‐inheritanceuserstable (aliasg) with their name in thelastnamecolumn. Mappingg.lastname AS group_nameis therefore correct—no update required.Likely an incorrect or invalid review comment.
…erties - Updated the test cases in `redmine.test.ts` to include new properties `user_id`, `username`, `spent_on`, and `total_hours` for the `getProjectUnits` method. - Added assertions to verify the presence of these properties in the returned results, ensuring comprehensive validation of the extended data model. These changes improve the test coverage for the `getProjectUnits` method, ensuring accurate validation of the new data structure and enhancing overall test reliability.
- Refactored the test for `getProjectUnits` in `redmine.test.ts` to use a spy on the `RedmineService` method, ensuring the method is called correctly. - Added an assertion to verify the returned result is an empty array, enhancing the test's reliability and coverage. These changes improve the accuracy of the test for the `getProjectUnits` method, ensuring it correctly validates the expected behavior of the service interaction.
- Removed the `redmine.types.ts` file as it is no longer needed, simplifying the codebase. - Refactored the `getProjectUnits` function to accept a `RedmineService` instance, enhancing flexibility in testing and usage. - Introduced a new `mocks/redmine.ts` file to provide mock implementations for testing, improving test isolation and reliability. - Updated tests in `redmine.test.ts` to utilize the new mocking structure, ensuring accurate validation of the `getProjectUnits` method. These changes enhance the maintainability and clarity of the Redmine functionalities, while also improving the testing framework.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
workers/main/src/activities/weeklyFinancialReports/mocks/redmine.ts (2)
6-20: Well-structured mock factory with good defaults.The
createMockProjectUnitfunction provides sensible default values that align with theProjectUnitinterface fromworkers/main/src/common/types.ts. The use ofPartial<ProjectUnit>for overrides allows flexible test data creation.Consider adding JSDoc documentation to clarify the purpose and usage:
+/** + * Creates a mock ProjectUnit object with sensible defaults. + * @param overrides - Partial ProjectUnit to override default values + * @returns A complete ProjectUnit object for testing + */ export function createMockProjectUnit( overrides: Partial<ProjectUnit> = {}, ): ProjectUnit {
22-31: Improve type safety for the mock repository.The mock implementation correctly handles both success and error scenarios. However, the type assertion could be improved for better type safety and clarity.
Consider using a more explicit typing approach:
export function createMockRedmineRepository( units: ProjectUnit[] = [], error?: Error, -): RedmineRepository { - return { +): Pick<RedmineRepository, 'getProjectUnits'> { + const mockRepository = { getProjectUnits: error ? vi.fn().mockRejectedValue(error) : vi.fn().mockResolvedValue(units), - } as unknown as RedmineRepository; + }; + + return mockRepository as RedmineRepository; }This approach makes it clearer which methods are being mocked and provides better type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
workers/main/src/activities/weeklyFinancialReports/index.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/mocks/redmine.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/redmine.test.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/redmine.ts(1 hunks)workers/main/src/services/redmine/RedmineService.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- workers/main/src/services/redmine/RedmineService.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- workers/main/src/activities/weeklyFinancialReports/index.ts
- workers/main/src/activities/weeklyFinancialReports/redmine.test.ts
- workers/main/src/activities/weeklyFinancialReports/redmine.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
workers/main/src/activities/weeklyFinancialReports/mocks/redmine.ts (2)
workers/main/src/common/types.ts (1)
ProjectUnit(1-10)workers/main/src/services/redmine/RedmineRepository.ts (1)
RedmineRepository(50-82)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (1)
workers/main/src/activities/weeklyFinancialReports/mocks/redmine.ts (1)
1-4: LGTM! Clean and necessary imports.The imports are well-organized and all necessary for the mock implementations.
Pull request was closed
feat(tests): add unit tests for Redmine activities and weekly financial reports
getProjectUnitsfunction inredmine.test.ts, ensuring it returns project units correctly and handles errors gracefully.weeklyFinancialReports.test.tswith mocked activities to validate the report generation, including project units and financial data.getProjectUnitsandfetchFinancialData, establishing a clear structure for financial report generation.ProjectUnitandFinancialDatato improve type safety and clarity in the codebase.These changes enhance test coverage and reliability of the Redmine integration and financial reporting functionalities.