Add filtering for Target Units in SQL query (#57) #58
Conversation
- Introduced a constant `TARGET_UNITS_GROUP` to specify the group for the weekly financial report. - Updated the SQL query in `queries.ts` to join with `custom_values` and filter results based on the `TARGET_UNITS_GROUP`. - This change enhances the ability to retrieve specific target units relevant to the financial report. These modifications improve the accuracy of data retrieval for financial reporting purposes.
📝 WalkthroughWalkthroughThe codebase was refactored to introduce a strongly-typed Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as weeklyFinancialReportsWorkflow
participant Activity as getTargetUnits
participant Repo as TargetUnitRepository
participant DB as Database
Workflow->>Workflow: Validate groupName in GroupNameEnum
Workflow->>Activity: getTargetUnits(groupName)
Activity->>Repo: getTargetUnits(groupName)
Repo->>DB: Execute getTargetUnitsQuery with [groupName]
DB-->>Repo: Return filtered target units data
Repo-->>Activity: Return TargetUnit[]
Activity-->>Workflow: Return fileLink
✨ 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:725c44ee8ae683f539f9e5a5db2aee4fe04f51be3b4f3ca5856f06bdaeebf649 |
| 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: 1
🧹 Nitpick comments (1)
workers/main/src/services/TargetUnit/queries.ts (1)
29-29: Fix indentation inconsistency.The WHERE clause condition has inconsistent indentation compared to the previous line. Consider aligning the indentation for better readability.
WHERE g.type = 'Group' - AND cv.customized_type = 'Principal' and cv.value='${TARGET_UNITS_GROUP}' + AND cv.customized_type = 'Principal' AND cv.value = '${TARGET_UNITS_GROUP}'Also consider using consistent capitalization for SQL keywords (use
ANDinstead ofand).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workers/main/src/services/TargetUnit/queries.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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)
- GitHub Check: Analyze (actions)
🔇 Additional comments (1)
workers/main/src/services/TargetUnit/queries.ts (1)
1-1: LGTM! Well-defined constant for filtering criteria.The constant is properly named, typed as string, and provides good maintainability by centralizing the group name used for filtering.
- Updated the `getTargetUnits` function to accept a `groupName` parameter, allowing for dynamic group selection when retrieving target units. - Modified the SQL query in `queries.ts` to filter results based on the provided `groupName`. - Enhanced unit tests to cover scenarios for both default and custom group names. - Introduced a new constant `defaultGroupName` for the default group value. These changes improve the flexibility of the target units retrieval process and enhance test coverage for various group scenarios.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
workers/main/src/services/TargetUnit/queries.ts (1)
22-22: Note: Previous indexing recommendation still applies.There's an existing recommendation to add an index on
custom_values.customized_idfor performance optimization of this JOIN operation.
🧹 Nitpick comments (1)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (1)
10-13: Consider adding input validation for the groupName parameter.The workflow correctly maintains backward compatibility with an optional parameter and default value. However, consider adding validation similar to the activity function to ensure the parameter is valid before processing.
export async function weeklyFinancialReportsWorkflow( groupName: string = defaultGroupName, ): Promise<string> { + if (!groupName || groupName.trim().length === 0) { + throw new Error('Invalid groupName: parameter cannot be empty'); + } + const targetUnits = await getTargetUnits(groupName);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts(3 hunks)workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts(1 hunks)workers/main/src/services/TargetUnit/ITargetUnitRepository.ts(1 hunks)workers/main/src/services/TargetUnit/TargetUnitRepository.test.ts(3 hunks)workers/main/src/services/TargetUnit/TargetUnitRepository.ts(2 hunks)workers/main/src/services/TargetUnit/queries.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/const.ts(1 hunks)workers/main/src/workflows/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 (2)
- workers/main/src/workflows/weeklyFinancialReports/const.ts
- workers/main/src/workflows/weeklyFinancialReports/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (7)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts (3)
workers/main/src/services/TargetUnit/TargetUnitRepository.ts (2)
getTargetUnits(36-51)TargetUnitRepository(9-52)workers/main/src/common/RedminePool.ts (1)
RedminePool(4-46)workers/main/src/configs/redmineDatabase.ts (1)
redmineDatabaseConfig(3-8)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (1)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (1)
weeklyFinancialReportsWorkflow(10-16)
workers/main/src/services/TargetUnit/TargetUnitRepository.test.ts (2)
workers/main/src/workflows/weeklyFinancialReports/const.ts (1)
defaultGroupName(1-1)workers/main/src/common/errors/TargetUnitRepositoryError.ts (1)
TargetUnitRepositoryError(3-7)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (2)
workers/main/src/workflows/weeklyFinancialReports/const.ts (1)
defaultGroupName(1-1)workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts (1)
getTargetUnits(11-33)
workers/main/src/services/TargetUnit/ITargetUnitRepository.ts (1)
workers/main/src/common/types.ts (1)
TargetUnit(1-12)
workers/main/src/services/TargetUnit/TargetUnitRepository.ts (3)
workers/main/src/common/types.ts (1)
TargetUnit(1-12)workers/main/src/services/TargetUnit/queries.ts (1)
getTargetUnitsQuery(1-45)workers/main/src/services/TargetUnit/types.ts (1)
TargetUnitRow(3-12)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts (5)
workers/main/src/services/TargetUnit/TargetUnitRepository.ts (1)
getTargetUnits(36-51)workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts (1)
getTargetUnits(11-33)workers/main/src/workflows/weeklyFinancialReports/const.ts (1)
defaultGroupName(1-1)workers/main/src/common/fileUtils.ts (1)
writeJsonFile(19-32)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: Service Availability Check
- GitHub Check: SonarQube
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts (1)
20-20: LGTM!The parameter is correctly passed to the repository method, maintaining consistency with the updated interface.
workers/main/src/services/TargetUnit/TargetUnitRepository.test.ts (2)
6-6: LGTM!The import of
defaultGroupNameis correctly added to support the updated test calls.
41-41: LGTM!All test method calls are consistently updated to pass the
defaultGroupNameparameter, maintaining test coverage while adapting to the new interface.Also applies to: 59-59, 66-68
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (2)
14-14: LGTM!The test description is more specific and clearly indicates this tests the default group scenario.
20-24: LGTM!Good addition of a test case to verify the workflow works correctly with a custom group name parameter.
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (1)
4-4: LGTM!The import of
defaultGroupNameis correctly added for the default parameter value.workers/main/src/services/TargetUnit/TargetUnitRepository.ts (1)
36-39: LGTM! Repository method correctly updated to support parameterized queries.The method signature change properly accepts the
groupNameparameter and the implementation correctly uses the newgetTargetUnitsQueryfunction. Error handling and data mapping remain unchanged, which maintains consistency.workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts (3)
6-6: Good addition of defaultGroupName import.The import provides consistency with the workflow configuration and ensures tests use the same default value.
116-132: Excellent test coverage for both default and custom group scenarios.The tests properly cover both the default group name usage and custom group name scenarios. The separation into distinct test cases makes the test intentions clear and maintainable.
137-155: All error handling tests properly updated.The error scenarios and cleanup tests correctly use the
defaultGroupNameparameter, maintaining test coverage while adapting to the new function signature.
- Updated the `getTargetUnitsQuery` function to return an object containing the SQL query and parameters for improved query execution. - Modified the SQL query to use a placeholder for the `groupName` parameter, enhancing security and flexibility. - This change streamlines the process of executing the query with dynamic parameters. These modifications improve the maintainability and usability of the target units retrieval process.
…me type - Introduced a new JSON file containing target units data for various projects and users, enhancing the reporting capabilities. - Refactored the `getTargetUnits` function to accept a `GroupName` type instead of a string, improving type safety and clarity. - Updated related tests to utilize the new `GroupName` type and ensure consistent behavior across different group scenarios. - Removed the deprecated `defaultGroupName` constant to streamline the codebase. These changes enhance the maintainability and usability of the target units retrieval process while providing necessary data for financial reporting.
- Removed the target units JSON file as it is no longer needed. - Updated the `getTargetUnits` function to utilize `GroupNameEnum` for improved type safety and clarity. - Refactored related tests to replace string literals with `GroupNameEnum` values, ensuring consistent behavior across different group scenarios. - Enhanced error handling in the `weeklyFinancialReportsWorkflow` to validate group names against `GroupNameEnum`. These changes streamline the target units retrieval process and improve maintainability by leveraging enumerated types.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (1)
8-17: Consider simplifying the mock interface.While the custom interface works, it might be overly complex for this use case. Consider using Vitest's built-in mock types directly.
You could simplify this by removing the custom interface and using Vitest's
MockedFunctiontype:-// Define the type for the mocked getTargetUnits function -interface GetTargetUnitsMock extends Mock { - mockResolvedValueOnce: (value: { fileLink: string }) => void; - mockRejectedValueOnce: (error: Error) => void; - mockReset: () => void; -} - -declare module '@temporalio/workflow' { - export function __getTargetUnitsMock(): GetTargetUnitsMock; -}And use the standard Vitest mock approach in the mock declaration.
🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Do not export from a test file.
(lint/suspicious/noExportsInTest)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts(4 hunks)workers/main/src/common/types.ts(2 hunks)workers/main/src/configs/weeklyFinancialReport.ts(1 hunks)workers/main/src/services/TargetUnit/TargetUnitRepository.test.ts(3 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/configs/weeklyFinancialReport.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- workers/main/src/services/TargetUnit/TargetUnitRepository.test.ts
- workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts
- workers/main/src/common/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (2)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (1)
weeklyFinancialReportsWorkflow(12-24)workers/main/src/common/errors/AppError.ts (1)
AppError(1-9)
🪛 Biome (1.9.4)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts
[error] 16-16: Do not export from a test file.
(lint/suspicious/noExportsInTest)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Docker Security Scanning (temporal, Dockerfile.temporal, temporal-test:latest)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
🔇 Additional comments (7)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (3)
4-6: LGTM! Clean import additions.The imports are well-organized and include all necessary types and error handling classes for the new functionality.
12-20: Well-implemented validation with proper error handling.The validation logic correctly checks the
groupNameparameter against the enum values and throws a descriptive error. The type casting(Object.values(GroupNameEnum) as GroupName[])is necessary to align the enum values with theGroupNametype.
21-21: Correctly passes the validated groupName parameter.The call to
getTargetUnitsnow properly passes the validatedgroupNameparameter, enabling group-specific data filtering as intended.workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (4)
37-47: Excellent parameterized testing approach.The parameterized test using
it.eachprovides comprehensive coverage for all valid group names, ensuring the workflow behaves correctly for each enum value.
49-60: Thorough validation testing with proper error assertions.The test correctly validates that invalid group names throw the expected
AppErrorwith the appropriate message. The type casting is necessary for testing invalid inputs.
62-67: Good coverage for error propagation.This test ensures that errors from the
getTargetUnitsactivity properly propagate through the workflow, which is important for debugging and error handling.
15-17: Static analysis false positive can be ignored.The static analysis tool flagged the module declaration as an export from a test file, but this is a false positive. The declaration is just for TypeScript typing support and doesn't actually export anything from the test file.
🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Do not export from a test file.
(lint/suspicious/noExportsInTest)
TARGET_UNITS_GROUPto specify the group for the weekly financial report.queries.tsto join withcustom_valuesand filter results based on theTARGET_UNITS_GROUP.These modifications improve the accuracy of data retrieval for financial reporting purposes.