[Enhancement] Add Timesheet status filter #8547
Conversation
WalkthroughThe changes in this pull request involve updates to various interfaces and classes related to timesheet management. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
🧹 Outside diff range and nitpick comments (3)
packages/core/src/shared/dto/selectors-query.dto.ts (1)
2-2: Remove unused importIsStringThe
IsStringvalidator is imported but not used in this file.-import { IsOptional, IsArray, IsString } from 'class-validator'; +import { IsOptional, IsArray } from 'class-validator';packages/core/src/time-tracking/timesheet/timesheet.service.ts (2)
119-119: Consider adding status value validation.The status filter accepts any values present in the array. Consider validating that the values match the
TimesheetStatusenum to prevent invalid queries.- ...(status.length > 0 ? { status: In(status) } : {}), + ...(status.length > 0 ? { + status: In( + status.filter((s) => Object.values(TimesheetStatus).includes(s)) + ) + } : {}),
119-120: Consider adding database indexes for performance.The new filtering conditions on
statusandtaskIdscolumns might benefit from database indexes, especially if these filters are frequently used in queries.Consider adding indexes in your database migration:
CREATE INDEX idx_timesheet_status ON "timesheet" ("status"); CREATE INDEX idx_timesheet_task ON "timesheet" ("taskIds");Also applies to: 129-129
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/contracts/src/timesheet.model.ts(2 hunks)packages/core/src/shared/dto/selectors-query.dto.ts(2 hunks)packages/core/src/time-tracking/timesheet/timesheet.service.ts(3 hunks)
🔇 Additional comments (5)
packages/core/src/shared/dto/selectors-query.dto.ts (1)
46-54: LGTM! Well-structured status filter implementation
The new status property is:
- Properly typed as
TimesheetStatus[] - Well-documented with JSDoc comments
- Correctly decorated with necessary validators
- Consistent with the existing filtering pattern
packages/core/src/time-tracking/timesheet/timesheet.service.ts (2)
87-95: LGTM! Clean parameter initialization.
The new parameters status and taskIds are properly initialized as empty arrays, following the existing pattern and preventing potential null/undefined issues.
119-120: Verify the taskIds column name in the database schema.
The condition taskIds: In(taskIds) might be using an incorrect column name. The typical pattern would be to use a relation or a specific column name like taskId.
Let's verify the entity structure:
packages/contracts/src/timesheet.model.ts (2)
92-93: LGTM! Well-structured filter properties added
The new optional properties status and taskIds are well-designed:
- They follow the existing pattern of array-based filters
- They maintain backward compatibility by being optional
- The
statusproperty correctly uses theTimesheetStatusenum type
196-196: Verify the business logic for filtering time logs by timesheet status
While the implementation is technically correct, please verify if filtering time logs by timesheet status aligns with the business requirements. Time logs are typically filtered by their own properties rather than their parent timesheet's status.
Consider these questions:
- Should time logs from a rejected timesheet be filtered out?
- How should the status filter handle time logs that aren't associated with any timesheet?
Let's verify the relationship between time logs and timesheets:
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 9622255. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
Sent with 💌 from NxCloud. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/core/src/time-tracking/timesheet/dto/query/timesheet-query.dto.ts (1)
19-22: Consider enhancing enum validation.While the current validation ensures the property is an array, it doesn't validate that the array elements are valid TimesheetStatus enum values.
Consider adding enum validation:
@ApiPropertyOptional({ type: () => Array, isArray: true }) @IsOptional() @IsArray() +@IsEnum(TimesheetStatus, { each: true }) status: TimesheetStatus[];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/time-tracking/timesheet/dto/query/timesheet-query.dto.ts(1 hunks)
🔇 Additional comments (1)
packages/core/src/time-tracking/timesheet/dto/query/timesheet-query.dto.ts (1)
1-23: LGTM! Clean and well-structured implementation.
The implementation follows DTO best practices with proper typing, validation, and API documentation. The code is well-organized and maintains consistency with the existing codebase structure.
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
New Features
statusproperty added to the timesheet query, enabling more granular control over retrieved data.Bug Fixes