Refactor getTargetUnits to accept group name parameter #61
Conversation
- Introduced `GroupNameEnum` in `weeklyFinancialReport.ts` to define group names for financial reports. - Updated `types.ts` to import `GroupNameEnum` and added a new type `GroupName` based on the enum. These changes enhance type safety and improve the organization of group name constants within the application.
- Updated `weeklyFinancialReportsWorkflow` to accept a `groupName` parameter, ensuring it is validated against the `GroupNameEnum`. - Added error handling to throw an `AppError` for invalid group names, improving robustness and clarity in error reporting. - Modified the corresponding test to reflect the new parameter usage. These changes enhance the workflow's functionality and ensure that only valid group names are processed.
- Updated the `getTargetUnits` function to accept a `groupName` parameter, enhancing its flexibility and allowing for targeted data retrieval. - Modified the `TargetUnitRepository` and its interface to reflect the new parameter requirement. - Adjusted related tests to ensure proper functionality with the new parameter, including validation for group names. - Enhanced SQL query to utilize the group name for fetching relevant target units. These changes improve the overall functionality and maintainability of the weekly financial reports workflow.
📝 WalkthroughWalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant getTargetUnits Activity
participant TargetUnitRepository
participant Database
Workflow->>getTargetUnits Activity: getTargetUnits(groupName)
getTargetUnits Activity->>TargetUnitRepository: getTargetUnits(groupName)
TargetUnitRepository->>Database: Execute SQL with groupName
Database-->>TargetUnitRepository: Target units data
TargetUnitRepository-->>getTargetUnits Activity: Target units
getTargetUnits Activity-->>Workflow: File link
Workflow->>fetchFinancialAppData Activity: fetchFinancialAppData(file link)
fetchFinancialAppData Activity-->>Workflow: Final file link
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:e82621bc8773e2e03695355ef539d9f221f9368903c63d44ea51284569994550 |
| 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
|
- Renamed `getTargetUnitsQuery` to `TARGET_UNITS_QUERY` for improved clarity and consistency in naming conventions. - Updated the import statement in `TargetUnitRepository` to reflect the new query name. These changes enhance code readability and maintainability within the TargetUnit service.
- Introduced `SlackService` class to facilitate posting messages to a specified Slack channel. - Implemented error handling to throw an `AppError` if the Slack channel ID is not set, ensuring robust error reporting. - The service utilizes the Slack Web API for message posting, enhancing integration capabilities with Slack. These changes improve the application's ability to communicate via Slack and provide a structured approach to message handling.
- Introduced `SlackService` class to facilitate posting messages to a specified Slack channel. - Implemented error handling to throw an `AppError` if the Slack channel ID is not set, ensuring robust error reporting. - The service utilizes the Slack Web API for message posting, enhancing integration capabilities with Slack. These changes improve the application's ability to communicate via Slack and provide a structured approach to message handling.
- Added a mock for `fetchFinancialAppData` in the test suite to validate its integration with the `weeklyFinancialReportsWorkflow`. - Implemented a new test case to ensure that errors from `fetchFinancialAppData` are properly propagated. - Updated the error message for invalid group names to correct a typo in the parameter name. These changes improve the robustness of the workflow tests and ensure accurate error reporting.
![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 (8)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts (1)
21-24: EmbedgroupNameinto the generated file name for easier traceabilityWhen multiple groups are processed in quick succession, the current naming scheme (
target-units-<timestamp>.json) makes it difficult to see which JSON belongs to which group. Appending thegroupNamekeeps artifacts self-describing with zero cost.- const filename = `data/weeklyFinancialReportsWorkflow/getTargetUnits/target-units-${Date.now()}.json`; + const filename = `data/weeklyFinancialReportsWorkflow/getTargetUnits/${groupName}-target-units-${Date.now()}.json`;workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (1)
17-20: Avoid recomputingObject.values(GroupNameEnum)on every invocation
Object.valuesallocates a new array each call. For a hot workflow this is negligible but still unnecessary. Caching in aconst(or using aSet) gives O(1) lookups and cleaner intent.- if (!(Object.values(GroupNameEnum) as GroupName[]).includes(groupName)) { + const allowedGroups: ReadonlySet<GroupName> = new Set(Object.values(GroupNameEnum)); + if (!allowedGroups.has(groupName)) {workers/main/src/services/TargetUnit/TargetUnitRepository.ts (1)
48-52: Preserve the original stack by using thecauseoptionWrapping the error is great, but the original stack trace is lost, making debugging harder.
Node ≥16 supports thecauseproperty.- throw new TargetUnitRepositoryError( - `TargetUnitRepository.getTargetUnits failed: ${(error as Error).message}`, - ); + throw new TargetUnitRepositoryError( + `TargetUnitRepository.getTargetUnits failed: ${(error as Error).message}`, + { cause: error as Error }, + );workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts (3)
117-123: Update the test description to match the new enum-based callThe test name still mentions “default group”, but you are explicitly passing
GroupNameEnum.SD_REPORT.
Renaming keeps the spec self-documenting and avoids confusion when additional groups are added later.-it('returns fileLink when successful (default group)', async () => { +it('returns fileLink when successful (SD_REPORT group)', async () => {
126-135: Avoid executing the same async path twice in the same assertion blockCalling
getTargetUnitstwice performs redundant I/O and lengthens test time.
Capture the promise once and run multiple matchers against it.- await expect(getTargetUnits(GroupNameEnum.SD_REPORT)).rejects.toThrow( - AppError, - ); - await expect(getTargetUnits(GroupNameEnum.SD_REPORT)).rejects.toThrow( - 'Failed to get Target Units', - ); + const action = getTargetUnits(GroupNameEnum.SD_REPORT); + await expect(action).rejects.toThrow(AppError); + await expect(action).rejects.toThrow('Failed to get Target Units');Apply the same pattern to the similar block below to keep tests fast and DRY.
138-145: Duplicate execution – same remark as aboveThe block duplicates the two-call pattern; refactor identically to keep consistency and reduce runtime.
workers/main/src/services/TargetUnit/ITargetUnitRepository.ts (1)
1-5: Minor layering concern:commontypes depend on a feature enum
GroupNamein../../common/typesis derived fromGroupNameEnumthat lives in the weekly-report config layer.
This creates an upward dependency from “common” to a feature package, which can snowball into circular-import pain later.Consider either:
- Moving
GroupNameEnumintocommon(preferred), or- Defining
GroupNamelocally here and avoiding the enum indirection.No functional break, just an architectural heads-up.
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (1)
8-19: Mock ignoresproxyActivitiesarguments & genericsThe workflow passes options to
proxyActivities<ActivityTypes>(options).
Your mock discards the incomingoptionsparameter and the generic, which is fine for today but can hide future errors (e.g., timeouts/options mismatches).Minimal improvement:
-proxyActivities: () => ({ +proxyActivities: (_opts?: unknown) => ({…so unexpected argument shapes surface in test failures instead of production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts(4 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/weeklyFinancialReports.workflow.test.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
workers/main/src/services/TargetUnit/TargetUnitRepository.ts (3)
workers/main/src/common/types.ts (2)
GroupName(16-16)TargetUnit(3-14)workers/main/src/services/TargetUnit/types.ts (1)
TargetUnitRow(3-12)workers/main/src/services/TargetUnit/queries.ts (1)
TARGET_UNITS_QUERY(1-43)
workers/main/src/services/TargetUnit/ITargetUnitRepository.ts (1)
workers/main/src/common/types.ts (2)
GroupName(16-16)TargetUnit(3-14)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (2)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (1)
weeklyFinancialReportsWorkflow(14-27)workers/main/src/common/errors/AppError.ts (1)
AppError(1-9)
⏰ 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 (3)
workers/main/src/services/TargetUnit/queries.ts (1)
21-28: Potential full-table scan oncustom_values; consider pinning the custom field id
custom_valuescan grow quickly.
Filtering only bycustomized_type = 'Principal'andvalue = ?may bypass indexes, causing a scan.
If the group name is stored in a dedicated custom field, also constrain bycv.custom_field_id = <id>so the composite(custom_field_id, value)index (common in Redmine) can be used.Please verify the schema and add the extra predicate if applicable.
workers/main/src/services/TargetUnit/TargetUnitRepository.test.ts (1)
58-60: 👍 Tests correctly exercise the new parameterThe updated assertions keep the focus on repository behaviour while validating the new
groupNameargument. Looks good.workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (1)
1-3: Import executed before mock – double-check Vitest hoisting
import * as workflowModule from '@temporalio/workflow';is declared before thevi.mock()for that module.
Vitest usually hoistsvi.mockcalls, but the behaviour is opt-in ("hoisted": truetransform).
If hoisting is disabled anywhere (e.g., viaunhoisted: true), the real module will load, and your stub won’t apply.Safer ordering:
vi.mock('@temporalio/workflow', /* factory */); import * as workflowModule from '@temporalio/workflow';Not a blocker if your project keeps default hoisting, but worth verifying.
|
getTargetUnitsfunction to accept agroupNameparameter, enhancing its flexibility and allowing for targeted data retrieval.TargetUnitRepositoryand its interface to reflect the new parameter requirement.These changes improve the overall functionality and maintainability of the weekly financial reports workflow.