Add FinApp repository with employee and project data access methods #52
Conversation
- Introduced `FinAppRepository` class for managing employee and project data retrieval. - Implemented methods `getEmployees` and `getProjects` to fetch data by Redmine IDs, with error handling using `FinAppRepositoryError`. - Created corresponding unit tests in `FinAppRepository.test.ts` to validate functionality and error handling. - Added Mongoose schemas for `Employee` and `Project` models in `FinAppSchemas.ts`. These additions enhance data management capabilities for the financial application and improve test coverage for repository operations.
📝 Walkthrough""" WalkthroughThis change introduces a new FinApp data access layer, including Mongoose schemas and models for employees and projects, TypeScript interfaces for repository operations and data types, a repository class implementing data fetching with error handling, and comprehensive unit tests for both the repository and schema validation. An index file aggregates exports for easier module consumption. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FinAppRepository
participant EmployeeModel
participant ProjectModel
Client->>FinAppRepository: getEmployees(userIds)
FinAppRepository->>EmployeeModel: find({ redmine_id: { $in: userIds } }, projection)
EmployeeModel-->>FinAppRepository: employees[]
FinAppRepository-->>Client: employees[]
Client->>FinAppRepository: getProjects(projectIds)
FinAppRepository->>ProjectModel: find({ redmine_id: { $in: projectIds } }, projection)
ProjectModel-->>FinAppRepository: projects[]
FinAppRepository-->>Client: projects[]
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ 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:8452cf853bbf92e7db2baa0b00d7791dedee068d6c15bc959d3c4aad2d21f99e |
| 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
| ||||||||||||
Description
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
workers/main/src/services/FinApp/IFinAppRepository.ts (1)
6-15: Well-designed interface with clear method signatures.The interface follows good TypeScript practices with proper documentation and type safety. The batch operation design (accepting arrays) is efficient for database operations.
Consider adding JSDoc parameter documentation to clarify behavior with empty arrays:
/** * Fetch employees by Redmine user IDs. + * @param userIds Array of Redmine user IDs (empty array returns empty result) */ getEmployees(userIds: number[]): Promise<Employee[]>; /** * Fetch projects by Redmine project IDs. + * @param projectIds Array of Redmine project IDs (empty array returns empty result) */ getProjects(projectIds: number[]): Promise<Project[]>;workers/main/src/services/FinApp/FinAppRepository.test.ts (1)
42-47: Enhance error assertion specificity.Consider asserting the specific error message or properties to ensure the correct error handling flow:
- await expect(repo.getEmployees([1])).rejects.toThrow(FinAppRepositoryError); + await expect(repo.getEmployees([1])).rejects.toThrow( + expect.objectContaining({ + name: 'FinAppRepositoryError', + message: expect.stringContaining('FinAppRepository._findByIds failed') + }) + );Also applies to: 66-71
workers/main/src/services/FinApp/types.ts (1)
1-3: Consider stricter typing for date strings in History interface.The current implementation allows any string as a date key. Consider if a more specific type would be beneficial:
export interface History { - rate: { [date: string]: number }; + rate: { [date: string]: number }; // Consider: Record<string, number> or with ISO date constraint }The current flexible approach may be intentional for various date formats, so this change should align with your data requirements.
workers/main/src/services/FinApp/FinAppRepository.ts (1)
18-29: Improve type safety and add input validation.The implementation is solid with good error handling and efficient querying using
.lean()for performance. However, consider these improvements:
- Input validation: Add a guard clause for empty arrays to make behavior explicit
- Type safety: Use a more specific projection type
- Error handling: Improve error type checking
- private async _findByIds<T>(model: Model<T & Document>, ids: number[], projection: Record<string, any>): Promise<T[]> { + private async _findByIds<T>( + model: Model<T & Document>, + ids: number[], + projection: Record<string, 0 | 1> + ): Promise<T[]> { + if (ids.length === 0) { + return []; + } + try { return await model.find( { redmine_id: { $in: ids } }, projection, ).lean<T[]>(); } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); throw new FinAppRepositoryError( - `FinAppRepository._findByIds failed: ${(error as Error).message} (ids: ${ids})`, + `FinAppRepository._findByIds failed: ${errorMessage} (ids: ${ids})`, ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
workers/main/src/services/FinApp/FinAppRepository.test.ts(1 hunks)workers/main/src/services/FinApp/FinAppRepository.ts(1 hunks)workers/main/src/services/FinApp/FinAppSchemas.ts(1 hunks)workers/main/src/services/FinApp/IFinAppRepository.ts(1 hunks)workers/main/src/services/FinApp/index.ts(1 hunks)workers/main/src/services/FinApp/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
workers/main/src/services/FinApp/FinAppRepository.test.ts (4)
workers/main/src/services/FinApp/FinAppRepository.ts (1)
FinAppRepository(10-49)workers/main/src/services/FinApp/types.ts (2)
Employee(5-8)Project(10-14)workers/main/src/services/FinApp/FinAppSchemas.ts (2)
EmployeeModel(15-16)ProjectModel(24-25)workers/main/src/common/errors/FinAppRepositoryError.ts (1)
FinAppRepositoryError(3-7)
workers/main/src/services/FinApp/IFinAppRepository.ts (1)
workers/main/src/services/FinApp/types.ts (2)
Employee(5-8)Project(10-14)
workers/main/src/services/FinApp/FinAppRepository.ts (4)
workers/main/src/services/FinApp/IFinAppRepository.ts (1)
IFinAppRepository(6-15)workers/main/src/common/errors/FinAppRepositoryError.ts (1)
FinAppRepositoryError(3-7)workers/main/src/services/FinApp/types.ts (2)
Employee(5-8)Project(10-14)workers/main/src/services/FinApp/FinAppSchemas.ts (2)
EmployeeModel(15-16)ProjectModel(24-25)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Docker Security Scanning (temporal, Dockerfile.temporal, temporal-test:latest)
- GitHub Check: Service Availability Check
- GitHub Check: SonarQube
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
workers/main/src/services/FinApp/index.ts (1)
1-4: Clean barrel export pattern implemented correctly.The index file follows standard TypeScript conventions for creating a single entry point. This enables clean imports and proper module encapsulation.
workers/main/src/services/FinApp/FinAppRepository.test.ts (4)
7-16: Proper mocking setup for Mongoose models.The mock correctly simulates the Mongoose API structure with the chained
.find().lean()pattern.
21-24: Good test setup with proper mock clearing.The
beforeEachsetup ensures test isolation by clearing mocks between test runs.
27-40: Comprehensive success case testing.The test validates both the return value and the correct method invocation with proper query parameters and projections.
51-64: Thorough project testing with realistic data.The test includes
quick_books_idwhich properly validates the Project interface structure and correct projection fields.workers/main/src/services/FinApp/types.ts (2)
5-8: Clean Employee interface design.The interface properly defines the required
redmine_idand optionalhistoryfield, matching the expected data structure.
10-14: Well-structured Project interface.The interface appropriately includes both Redmine and QuickBooks integration points with optional fields for incomplete data scenarios.
workers/main/src/services/FinApp/FinAppSchemas.ts (5)
3-8: LGTM! Well-structured embedded schema.The
historySchemacorrectly defines the rate field as a Map of numbers and properly disables_idgeneration for the embedded subdocument. This aligns well with the History interface structure.
10-13: LGTM! Proper indexing and schema structure.The
employeeSchemacorrectly defines the requiredredmine_idfield with indexing for efficient queries and properly embeds the history schema. This matches the Employee interface requirements.
15-16: LGTM! Good pattern for development environments.The conditional model creation pattern prevents "Cannot overwrite model" errors during development when modules are reloaded.
18-22: LGTM! Schema structure matches interface requirements.The
projectSchemacorrectly includes the optionalquick_books_idfield and maintains consistency with the employee schema pattern. The indexedredmine_idfield will ensure efficient queries.
24-25: LGTM! Consistent model creation pattern.The ProjectModel follows the same safe creation pattern as EmployeeModel, maintaining consistency across the codebase.
workers/main/src/services/FinApp/FinAppRepository.ts (3)
1-5: LGTM! All necessary imports are present.The imports correctly reference the custom error class, Mongoose models, interface, types, and required Mongoose types.
35-37: LGTM! Correct projection for Employee interface.The projection correctly selects only the
redmine_idandhistory.ratefields, which aligns with the Employee interface requirements and optimizes query performance.
44-46: LGTM! Correct projection for Project interface.The projection appropriately includes
redmine_id,quick_books_id, andhistory.ratefields, matching the Project interface structure and maintaining efficient data retrieval.
- Updated unit tests in `FinAppRepository.test.ts` for better clarity and consistency, including renaming test cases for improved readability. - Introduced mock functions for employee and project retrieval to streamline test setup and enhance error handling. - Refactored `_findByIds` method in `FinAppRepository.ts` to improve error message clarity by including the list of IDs in the error message. - Enhanced Mongoose model definitions in `FinAppSchemas.ts` to ensure proper type handling for `Employee` and `Project`. These changes improve the robustness of the FinApp repository tests and enhance error reporting for data retrieval operations.
- Introduced tests for `EmployeeModel` and `ProjectModel` to validate required fields and data types, ensuring proper error handling for invalid inputs. - Added tests for `historySchema` to verify acceptance of valid history objects and rejection of non-numeric rate values. - Implemented a helper function to validate Mongoose document instances without saving to the database. These tests enhance the reliability of the FinAppSchemas and improve overall test coverage for model validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
workers/main/src/services/FinApp/FinAppSchemas.test.ts (3)
22-49: Enhance test coverage for EmployeeModel validation.The basic validation tests are good, but consider adding more comprehensive test cases to improve coverage.
Consider adding these additional test cases:
+ it('should reject negative redmine_id', async () => { + const doc = new EmployeeModel({ redmine_id: -1 }); + const err = await validateDoc(doc); + + expect(err).toBeTruthy(); + expect(err?.errors.redmine_id).toBeDefined(); + }); + + it('should accept employee without history', async () => { + const doc = new EmployeeModel({ redmine_id: 123 }); + const err = await validateDoc(doc); + + expect(err).toBeNull(); + });
51-78: Enhance test coverage for ProjectModel validation.Similar to EmployeeModel, the basic tests are solid but could benefit from additional edge case coverage.
Consider adding these test cases for consistency:
+ it('should reject negative redmine_id', async () => { + const doc = new ProjectModel({ redmine_id: -1 }); + const err = await validateDoc(doc); + + expect(err).toBeTruthy(); + expect(err?.errors.redmine_id).toBeDefined(); + }); + + it('should accept project without optional fields', async () => { + const doc = new ProjectModel({ redmine_id: 456 }); + const err = await validateDoc(doc); + + expect(err).toBeNull(); + });
80-106: Improve test organization with systematic cleanup.The historySchema tests are well-implemented, but the manual cleanup could be systematized for better maintainability.
Consider refactoring to use
afterEachfor more systematic cleanup:describe('historySchema', () => { + const createdModels: string[] = []; + + afterEach(() => { + createdModels.forEach(modelName => { + if (mongoose.models[modelName]) { + mongoose.deleteModel(modelName); + } + }); + createdModels.length = 0; + }); + it('should accept a valid history object', async () => { + const modelName = 'TestHistory'; const TestModel = mongoose.model( - 'TestHistory', + modelName, new mongoose.Schema({ history: historySchema }), ); + createdModels.push(modelName); const doc = new TestModel({ history: { rate: { '2024-01-01': 123 } } }); const err = await validateDoc(doc); expect(err).toBeNull(); - mongoose.deleteModel('TestHistory'); }); it('should reject non-number rate values', async () => { + const modelName = 'TestHistory2'; const TestModel = mongoose.model( - 'TestHistory2', + modelName, new mongoose.Schema({ history: historySchema }), ); + createdModels.push(modelName); const doc = new TestModel({ history: { rate: { '2024-01-01': 'not-a-number' } }, }); const err = await validateDoc(doc); expect(err).toBeTruthy(); - mongoose.deleteModel('TestHistory2'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workers/main/src/services/FinApp/FinAppSchemas.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
workers/main/src/services/FinApp/FinAppSchemas.test.ts (1)
workers/main/src/services/FinApp/FinAppSchemas.ts (3)
EmployeeModel(17-20)ProjectModel(28-31)historySchema(5-10)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Docker Security Scanning (temporal, Dockerfile.temporal, temporal-test:latest)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: SonarQube
- GitHub Check: Service Availability Check
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
workers/main/src/services/FinApp/FinAppSchemas.test.ts (1)
6-20: LGTM! Well-implemented validation helper.The
validateDochelper function is well-designed with proper error handling and type safety. It correctly distinguishes between validation errors and unexpected errors.
- Introduced `getFinAppData` function to fetch financial application data from a specified JSON file, process it, and save the results to a new file. - Implemented error handling using `AppError` for various failure scenarios, ensuring robust error reporting. - Added unit tests for `getFinAppData` to validate successful data retrieval and error handling, including mock implementations for dependencies. - Updated the `index.ts` file to export the new `getFinAppData` function for broader access. These changes enhance the financial reporting capabilities and improve test coverage for data processing operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (1)
17-17: 🛠️ Refactor suggestionUpdate the test description to reflect actual workflow behavior.
The test description indicates it returns the fileLink from
getTargetUnits, but the workflow now calls bothgetTargetUnitsandgetFinAppData, returning the result fromgetFinAppData.- it('returns the fileLink from getTargetUnits', async () => { + it('returns the fileLink from getFinAppData', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
workers/main/src/activities/weeklyFinancialReports/getFinAppData.test.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/getFinAppData.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/index.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- workers/main/src/activities/weeklyFinancialReports/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (2)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts (1)
getTargetUnits(11-31)workers/main/src/activities/weeklyFinancialReports/getFinAppData.ts (1)
getFinAppData(15-44)
workers/main/src/activities/weeklyFinancialReports/getFinAppData.ts (5)
workers/main/src/common/MongoPool.ts (1)
MongoPool(5-65)workers/main/src/services/FinApp/FinAppRepository.ts (1)
FinAppRepository(11-58)workers/main/src/common/fileUtils.ts (2)
readJsonFile(6-17)writeJsonFile(19-32)workers/main/src/common/types.ts (1)
TargetUnit(1-12)workers/main/src/common/errors/AppError.ts (1)
AppError(1-9)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Docker Security Scanning (temporal, Dockerfile.temporal, temporal-test:latest)
- GitHub Check: Service Availability Check
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (1)
5-14: LGTM! Well-structured workflow implementation.The workflow correctly chains the activities, using the output from
getTargetUnitsas input togetFinAppData, and follows the established pattern for activity orchestration.workers/main/src/activities/weeklyFinancialReports/getFinAppData.ts (2)
11-13: LGTM! Efficient unique ID extraction.The
getUniqueIdsutility function correctly uses Set for deduplication and provides good type safety with generics.
15-44: LGTM! Robust implementation with proper resource management.The function demonstrates excellent practices:
- Proper MongoDB connection management with singleton pattern
- Concurrent data fetching with Promise.all for better performance
- Comprehensive error handling with custom AppError
- Resource cleanup in finally block ensures connection is always closed
- Consistent file naming pattern with timestamps
workers/main/src/activities/weeklyFinancialReports/getFinAppData.test.ts (3)
61-82: LGTM! Excellent helper functions for test setup.The helper functions
createRepoInstanceandcreateMongoPoolInstanceprovide clean abstractions for creating mocked dependencies with sensible defaults and override capabilities.
108-112: LGTM! Smart error testing helper.The
expectAppErrorhelper function efficiently tests error scenarios while ensuring proper resource cleanup behavior.
136-198: LGTM! Comprehensive test coverage.The test suite excellently covers:
- Success scenarios with proper data flow verification
- Multiple error scenarios (file read, employee fetch, project fetch, file write)
- Resource cleanup verification in all cases
- Deterministic testing with Date.now() mocking
This provides robust confidence in the function's reliability.
workers/main/src/activities/weeklyFinancialReports/getFinAppData.ts
Outdated
Show resolved
Hide resolved
- Added comments to `FinAppSchemas.ts` to clarify the purpose of the `historySchema`, `employeeSchema`, and `projectSchema`. - Improved documentation for the `EmployeeModel` and `ProjectModel` to provide better context for their usage. These changes enhance code readability and maintainability by providing clear explanations of the schemas used in the financial application.
…eature/add-fin-app-repo
This reverts commit 14a1f86.
- Removed the error parameter from the TargetUnitRepositoryError instantiation in the getTargetUnits method to streamline error reporting. - This change enhances the clarity of error messages by focusing on the primary error message without additional context. These modifications improve the error handling mechanism within the TargetUnitRepository, ensuring more concise error reporting.
- Simplified the `getEmployees` and `getProjects` methods by removing the generic `_findByIds` method and directly implementing the data fetching logic. - Enhanced error handling in both methods to provide clearer error messages. - Updated the projection fields in the queries to ensure only necessary data is retrieved. These changes improve code clarity and maintainability while ensuring robust error reporting for employee and project data retrieval.
…eature/add-fin-app-repo
- Updated method names in FinAppRepository and IFinAppRepository to better reflect their functionality by including 'ByRedmineIds'. - Adjusted corresponding test cases to ensure consistency with the new method names. - Enhanced error messages in the repository methods to provide clearer context during failures. These changes improve code readability and maintainability by making method purposes more explicit.
…adjustments - Expanded mock data for Employee and Project interfaces to include additional fields such as name, sorting, and timestamps. - Updated test cases in FinAppRepository to reflect changes in project IDs from 2 to 550, ensuring consistency with the new mock data. - These modifications improve the accuracy of tests and better simulate real-world scenarios for the FinAppRepository functionality.
- Introduced `redmine_id` to the Employee interface to link users to their corresponding Redmine accounts. - Added `redmine_id` and `quick_books_id` to the Project interface for better integration with Redmine and QuickBooks Online. - These enhancements improve the data structure by providing necessary identifiers for external system integration.
|
FinAppRepositoryclass for managing employee and project data retrieval.getEmployeesandgetProjectsto fetch data by Redmine IDs, with error handling usingFinAppRepositoryError.FinAppRepository.test.tsto validate functionality and error handling.EmployeeandProjectmodels inFinAppSchemas.ts.These additions enhance data management capabilities for the financial application and improve test coverage for repository operations.