[Feat] Find tasks by start and due dates filters #8542
Conversation
WalkthroughThe changes introduce a new interface Changes
Possibly related PRs
Suggested labels
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: 5
🧹 Outside diff range and nitpick comments (2)
packages/core/src/tasks/dto/index.ts (1)
5-5: LGTM! Consider sorting exports alphabetically.The new export aligns with the PR's objective of adding date filtering functionality and follows the established patterns. As a minor enhancement, consider sorting the exports alphabetically for better maintainability.
export * from './create-task.dto'; +export * from './get-task-by-date-filter.dto'; +export * from './get-task-by-id.dto'; export * from './task-max-number-query.dto'; export * from './update-task.dto'; -export * from './get-task-by-id.dto'; -export * from './get-task-by-date-filter.dto';packages/contracts/src/task.model.ts (1)
108-115: LGTM! Consider adding JSDoc documentation.The interface is well-structured and properly extends the base models. Consider adding JSDoc documentation to describe the purpose and usage of this interface and its properties.
+/** + * Interface for filtering tasks by start and due dates + * @extends IBasePerTenantAndOrganizationEntityModel + */ export interface ITaskDateFilterInput extends IBasePerTenantAndOrganizationEntityModel, Pick<IGetTasksByViewFilters, 'relations'> { + /** Start date lower bound for filtering tasks */ startDateFrom?: Date; + /** Start date upper bound for filtering tasks */ startDateTo?: Date; + /** Due date lower bound for filtering tasks */ dueDateFrom?: Date; + /** Due date upper bound for filtering tasks */ dueDateTo?: Date; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/common/src/utils/find-by-date-between.ts(1 hunks)packages/common/src/utils/index.ts(1 hunks)packages/contracts/src/task.model.ts(1 hunks)packages/core/src/tasks/dto/get-task-by-date-filter.dto.ts(1 hunks)packages/core/src/tasks/dto/index.ts(1 hunks)packages/core/src/tasks/task.controller.ts(2 hunks)packages/core/src/tasks/task.service.ts(2 hunks)
🔇 Additional comments (9)
packages/common/src/utils/index.ts (1)
6-6: LGTM! Verify the module existence and usage.
The export statement follows the established pattern and is correctly placed.
Let's verify the module existence and its usage:
✅ Verification successful
Verify that no code was removed or replaced in index.ts.
No code was removed or replaced in index.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the exported module exists and is used in TaskService
# Check if the module exists
echo "Checking if find-by-date-between.ts exists..."
fd --type f "find-by-date-between.ts" packages/common/src/utils
# Check for imports in TaskService
echo "Checking usage in TaskService..."
rg -l "import.*find-by-date-between" "packages/core/src/tasks/task.service.ts"
# Check for actual usage of addBetween function
echo "Checking for addBetween usage..."
rg "addBetween" "packages/core/src/tasks/task.service.ts"
Length of output: 706
Script:
#!/bin/bash
# Description: Check for removed or replaced code in packages/common/src/utils/index.ts
echo "Checking for any removed or replaced code in index.ts..."
git diff --unified=0 packages/common/src/utils/index.ts | grep '^[-]'
Length of output: 194
packages/common/src/utils/find-by-date-between.ts (2)
1-1: LGTM!
Clean and focused import of the required TypeORM component.
1-14: Verify usage patterns across the codebase
Let's ensure this utility is used consistently across the codebase and that all callers handle the potential error cases appropriately.
✅ Verification successful
addBetween Utility is Consistently Used
The addBetween function is consistently used in packages/core/src/tasks/task.service.ts. No other usage patterns require attention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of addBetween function
echo "Searching for direct usage of addBetween..."
rg -l "addBetween\("
# Search for similar date filtering patterns that might need to use this utility
echo "\nSearching for potential BETWEEN queries that could use this utility..."
rg "BETWEEN.*AND.*date" --type typescript
# Search for TaskService implementations that might use this
echo "\nSearching for TaskService implementations..."
rg "class TaskService" --type typescript -A 10
Length of output: 580
Script:
#!/bin/bash
# Search for usages of addBetween function
echo "Searching for direct usage of addBetween..."
rg -l "addBetween\(" --type ts
# Search for similar date filtering patterns that might need to use this utility
echo "\nSearching for potential BETWEEN queries that could use this utility..."
rg "BETWEEN.*AND.*date" --type ts
# Search for TaskService implementations that might use this
echo "\nSearching for TaskService implementations..."
rg "class TaskService" --type ts -A 10
Length of output: 2432
packages/core/src/tasks/dto/get-task-by-date-filter.dto.ts (1)
1-30: LGTM! Clean and well-structured DTO implementation.
The implementation follows best practices with proper decorators for validation, transformation, and API documentation. The code is well-organized and maintains consistency across all properties.
packages/contracts/src/task.model.ts (1)
111-114: Consider adding runtime validation constraints.
Since this interface will likely be used with DTOs, consider adding validation constraints to ensure date ranges are valid (e.g., 'from' date should not be after 'to' date).
Let's check if validation decorators are being used in the DTO:
packages/core/src/tasks/task.controller.ts (1)
26-26: LGTM! Import follows conventions
The import of TaskDateFilterInputDTO is properly placed and follows the established naming conventions.
packages/core/src/tasks/task.service.ts (3)
25-28: LGTM! Import changes are appropriate.
The new imports are correctly placed and necessary for the new date filtering functionality.
842-851: Documentation is well-structured and comprehensive.
The JSDoc comments clearly describe the method's purpose, parameters, return type, and possible errors.
859-860: Verify the addBetween utility implementation.
Let's ensure the addBetween utility correctly handles date filtering across different database types.
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 7bc5e29. 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: 1
🧹 Outside diff range and nitpick comments (1)
packages/core/src/tasks/task.service.ts (1)
841-882: Implementation looks good but needs input validation.The method correctly implements date-based filtering with proper security context and error handling. However, there are some important improvements needed:
Consider these essential improvements:
- Add input validation for date ranges:
async getTasksByDateFilters(params: ITaskDateFilterInput): Promise<IPagination<ITask>> { const tenantId = RequestContext.currentTenantId() || params.tenantId; + // Validate date ranges + if (params.startDateFrom && params.startDateTo && params.startDateFrom > params.startDateTo) { + throw new BadRequestException('Start date range is invalid'); + } + if (params.dueDateFrom && params.dueDateTo && params.dueDateFrom > params.dueDateTo) { + throw new BadRequestException('Due date range is invalid'); + } + // Validate organization + if (!params.organizationId) { + throw new BadRequestException('Organization ID is required'); + } try {
- Add pagination support:
-async getTasksByDateFilters(params: ITaskDateFilterInput): Promise<IPagination<ITask>> { +async getTasksByDateFilters( + params: ITaskDateFilterInput & { + take?: number; + skip?: number + } +): Promise<IPagination<ITask>> { // ... existing code ... query.setFindOptions({ - ...(relations ? { relations } : {}) + ...(relations ? { relations } : {}), + ...(params.take ? { take: params.take } : {}), + ...(params.skip ? { skip: params.skip } : {}) });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/common/src/utils/find-by-date-between.ts(1 hunks)packages/core/src/tasks/dto/get-task-by-date-filter.dto.ts(1 hunks)packages/core/src/tasks/task.controller.ts(2 hunks)packages/core/src/tasks/task.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/tasks/dto/get-task-by-date-filter.dto.ts
🔇 Additional comments (6)
packages/common/src/utils/find-by-date-between.ts (3)
1-4: LGTM! Clean imports and type definitions.
The imports are appropriate and the AllowedFields type provides good type safety.
10-19: LGTM! Well-documented function.
The JSDoc documentation is comprehensive and clearly describes the function's behavior, parameters, return value, and potential exceptions.
6-8:
Fix type-runtime mismatch in field validation
There's a critical inconsistency between the AllowedFields type and the validation array:
- Type allows: 'startDate', 'dueDate', 'createdAt'
- Runtime validates: 'startDate', 'dueDate', 'endDate', 'createdAt'
This mismatch could lead to runtime errors where TypeScript allows usage of 'endDate' but the validation fails.
Apply this fix:
-type AllowedFields = 'startDate' | 'dueDate' | 'createdAt';
+type AllowedFields = 'startDate' | 'dueDate' | 'endDate' | 'createdAt';Likely invalid or redundant comment.
packages/core/src/tasks/task.controller.ts (2)
26-26: LGTM!
The import is properly grouped with other DTOs and follows the existing pattern.
149-163: 🛠️ Refactor suggestion
Enhance endpoint implementation for consistency
The implementation needs improvements to align with the controller's existing patterns:
- The endpoint should accept pagination parameters like other list endpoints
- Swagger documentation should be enhanced with query parameter descriptions
The previous review comment about pagination, documentation, and error handling is still applicable. Please refer to the earlier feedback for detailed suggestions.
Update the method signature and documentation:
@ApiOperation({ summary: 'Get tasks by start and due dates filters.' })
+@ApiQuery({ name: 'startDateFrom', type: Date, required: false, description: 'Filter tasks starting from this date' })
+@ApiQuery({ name: 'startDateTo', type: Date, required: false, description: 'Filter tasks starting before this date' })
+@ApiQuery({ name: 'dueDateFrom', type: Date, required: false, description: 'Filter tasks due from this date' })
+@ApiQuery({ name: 'dueDateTo', type: Date, required: false, description: 'Filter tasks due before this date' })
+@ApiQuery({ name: 'skip', type: Number, required: false, description: 'Number of records to skip for pagination' })
+@ApiQuery({ name: 'take', type: Number, required: false, description: 'Number of records to take for pagination' })
@ApiResponse({ status: HttpStatus.OK, description: 'Tasks retrieved successfully.' })
@ApiResponse({ status: HttpStatus.NOT_FOUND, description: 'No records found.' })
+@ApiResponse({ status: HttpStatus.BAD_REQUEST, description: 'Invalid date format or validation errors.' })
@Permissions(PermissionsEnum.ALL_ORG_VIEW, PermissionsEnum.ORG_TASK_VIEW)
@Get('/filter-by-date')
@UseValidationPipe({ transform: true })
-async getTasksByDateFilters(@Query() params: TaskDateFilterInputDTO): Promise<IPagination<ITask>> {
+async getTasksByDateFilters(
+ @Query() params: TaskDateFilterInputDTO & PaginationParams<Task>
+): Promise<IPagination<ITask>> {
return this.taskService.getTasksByDateFilters(params);
}packages/core/src/tasks/task.service.ts (1)
25-26: LGTM: Import statements are properly organized.
The new imports are correctly added and necessary for the new date filtering functionality.
Also applies to: 28-28
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/core/src/tasks/task.service.ts (1)
924-926: Enhance error handling with specific error types.The current implementation catches all errors and wraps them in BadRequestException, which might not be appropriate for all error cases.
Consider implementing more specific error handling:
} catch (error) { - throw new BadRequestException(error); + // Log the error for debugging + console.error('Error in getTasksByDateFilters:', error); + + if (error instanceof QueryFailedError) { + throw new BadRequestException('Invalid query parameters'); + } else if (error instanceof EntityNotFoundError) { + throw new NotFoundException('Referenced entity not found'); + } else if (error instanceof BadRequestException) { + throw error; + } else { + throw new InternalServerErrorException('An unexpected error occurred'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/contracts/src/task.model.ts(2 hunks)packages/core/src/tasks/dto/get-task-by-date-filter.dto.ts(1 hunks)packages/core/src/tasks/task.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts/src/task.model.ts
- packages/core/src/tasks/dto/get-task-by-date-filter.dto.ts
🔇 Additional comments (3)
packages/core/src/tasks/task.service.ts (3)
25-26: LGTM: Required imports added correctly.
The new imports support the date filtering functionality.
Also applies to: 28-28
869-876: LGTM: Security filters properly implemented.
The query correctly includes tenant and organization context filters for data isolation.
879-880: LGTM: Date filtering implementation.
The use of the addBetween utility function provides a clean and reusable approach for date range filtering.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/core/src/tasks/task.service.ts (1)
928-930: Enhance error handling with specific error types.Instead of wrapping all errors with BadRequestException, consider handling specific error cases:
} catch (error) { - throw new BadRequestException(error); + if (error instanceof BadRequestException) { + throw error; + } + console.error('Error in getTasksByDateFilters:', error); + throw new HttpException( + 'Failed to retrieve tasks by date filters', + HttpStatus.INTERNAL_SERVER_ERROR + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/contracts/src/task.model.ts(2 hunks)packages/core/src/tasks/task.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts/src/task.model.ts
🔇 Additional comments (4)
packages/core/src/tasks/task.service.ts (4)
25-28: LGTM: Import statements are correctly added.
The new imports are necessary for the date filtering functionality and are properly organized.
852-854: LGTM: Proper tenant context handling.
The method correctly retrieves the tenant context and falls back to the provided tenantId, ensuring proper data isolation.
883-918: LGTM: Well-structured query filters.
The implementation of optional filters is clean and efficient:
- Proper use of Brackets for AND/OR conditions
- Efficient subqueries for employee and team filtering
- Clear separation of concerns for different filter types
879-881: Consider adding database indexes for date columns.
The query filters on startDate and dueDate columns. Consider adding indexes to improve query performance.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
packages/core/src/tasks/dto/get-task-by-date-filter.dto.ts (2)
9-24: Consider enhancing date range validation.While the current validation is good, consider ensuring that both start dates are provided together to prevent partial date ranges.
@ApiPropertyOptional({ type: () => Date }) @Type(() => Date) @IsOptional() @IsDate() +@ValidateIf((o) => o.startDateTo != null) @IsBeforeDate(TaskDateFilterInputDTO, (it) => it.startDateTo, { message: 'Start date from must be before the start date to' }) startDateFrom?: Date;
25-40: Consider adding cross-field validation between start and due dates.While individual date ranges are validated, there's no validation ensuring that the due dates are after the start dates when both ranges are provided.
export class TaskDateFilterInputDTO extends TenantOrganizationBaseDTO implements ITaskDateFilterInput { // ... existing properties ... + @ValidateIf((o) => o.startDateFrom != null && o.dueDateFrom != null) + @IsBeforeDate(TaskDateFilterInputDTO, (it) => it.dueDateFrom, { + message: 'Start date must be before due date' + }) + getStartDate(): Date { + return this.startDateFrom; + } }packages/core/src/core/util/find-by-date-between.ts (2)
6-8: Optimize field validation with SetConsider using a
Setfor O(1) lookup performance instead of array inclusion check.+const VALID_FIELDS = new Set(['startDate', 'dueDate', 'endDate', 'createdAt']); + function isValidField(field: string): field is AllowedFields { - return ['startDate', 'dueDate', 'endDate', 'createdAt'].includes(field); + return VALID_FIELDS.has(field); }
33-33: Fix capitalization inconsistency in error messageThe error message has inconsistent capitalization: "From" vs "to".
- throw new BadRequestException('"From" date must not be after "to" date'); + throw new BadRequestException('"from" date must not be after "to" date');packages/core/src/tasks/task.service.ts (3)
884-919: Enhance error handling with specific error messages.The current error handling is basic and doesn't provide specific error messages for different failure scenarios.
Consider adding specific error handling:
try { // ... existing code ... } catch (error) { - throw new BadRequestException(error); + const message = error.message || 'Failed to filter tasks by date'; + console.error(`Error in getTasksByDateFilters: ${message}`, error); + throw new BadRequestException({ + message, + error: error.stack, + params // Include filtered params for debugging + }); }
920-929: Consider adding index recommendation for date columns.Since the method heavily relies on date-based filtering, having proper indexes on startDate and dueDate columns would improve query performance.
Consider adding the following indexes:
- Index on (tenantId, organizationId, startDate)
- Index on (tenantId, organizationId, dueDate)
- Composite index on (tenantId, organizationId, startDate, dueDate) if both filters are frequently used together
930-932: Improve error handling in catch block.The current error handling simply wraps the error in a BadRequestException without any additional context or logging.
Consider enhancing the error handling:
- } catch (error) { - throw new BadRequestException(error); - } + } catch (error) { + console.error('Failed to filter tasks by date:', { + error: error.message, + stack: error.stack, + params: { + ...params, + tenantId, + organizationId: params.organizationId + } + }); + throw new BadRequestException({ + message: 'Failed to filter tasks by date', + error: error.message + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/core/src/core/index.ts(1 hunks)packages/core/src/core/util/find-by-date-between.ts(1 hunks)packages/core/src/core/util/index.ts(1 hunks)packages/core/src/tasks/dto/get-task-by-date-filter.dto.ts(1 hunks)packages/core/src/tasks/task.service.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/core/index.ts
- packages/core/src/core/util/index.ts
🔇 Additional comments (5)
packages/core/src/tasks/dto/get-task-by-date-filter.dto.ts (1)
1-8: LGTM! Well-structured class declaration with appropriate imports.
The class properly extends TenantOrganizationBaseDTO and implements ITaskDateFilterInput, with all necessary decorators imported.
packages/core/src/core/util/find-by-date-between.ts (1)
36-38: Verify UTC conversion necessity
The comment suggests dates are being converted to UTC, but toISOString() is being called on potentially already UTC dates. Consider adding validation or documentation about expected date input format.
✅ Verification successful
Based on the search results, I can now generate the final response:
UTC conversion is necessary and correctly implemented
The codebase shows consistent usage of UTC date handling across multiple components, particularly in time tracking and scheduling features. The toISOString() conversion in this context is necessary because:
- Time logs and scheduling components consistently use UTC timestamps for database storage and comparisons
- Multiple services (e.g., TimeTrackerService, ActivityWatchEventAdapter) explicitly convert dates to UTC before using
toISOString() - The conversion ensures consistent date comparison across different timezones, which is crucial for a time tracking system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for date handling patterns in the codebase to understand the date format consistency
# Look for date conversion patterns
rg -A 2 'toISOString|toUTCString|setUTCHours|getUTCHours'
# Look for date-related types or interfaces
ast-grep --pattern 'interface $_ {
$$$
date: $_
$$$
}'
Length of output: 17873
packages/core/src/tasks/task.service.ts (3)
25-26: LGTM! Required imports are correctly added.
The new imports support the date filtering functionality and are properly organized.
Also applies to: 31-31
843-852: Documentation is well-structured and comprehensive.
The JSDoc comments clearly describe the method's purpose, parameters, and return type.
853-869:
Add input validation for date ranges.
The method should validate that:
- startDateFrom <= startDateTo
- dueDateFrom <= dueDateTo
- startDate range doesn't exceed dueDate range when both are provided
Apply this diff at the beginning of the try block:
try {
+ // Validate date ranges
+ if (startDateFrom && startDateTo && startDateFrom > startDateTo) {
+ throw new BadRequestException('Start date range is invalid');
+ }
+ if (dueDateFrom && dueDateTo && dueDateFrom > dueDateTo) {
+ throw new BadRequestException('Due date range is invalid');
+ }
+ if (startDateTo && dueDateFrom && startDateTo > dueDateFrom) {
+ throw new BadRequestException('Start date cannot exceed due date');
+ }
const {
startDateFrom,
startDateTo,Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/core/src/core/util/find-by-date-between.ts (2)
6-8: Optimize field validation using SetConsider using a Set for more efficient field name validation, especially if this function is called frequently.
+const VALID_FIELDS = new Set(['startDate', 'dueDate', 'endDate', 'createdAt']); + function isValidField(field: string): field is AllowedFields { - return ['startDate', 'dueDate', 'endDate', 'createdAt'].includes(field); + return VALID_FIELDS.has(field); }
10-19: Enhance function documentationThe documentation could be more helpful with:
- Explanation of the transform function's purpose and usage
- Example usage of the function
- Note about date conversion to ISO string format
/** * Adds a date range condition to a TypeORM query builder * @param query - The TypeORM SelectQueryBuilder instance * @param field - The date field to filter on * @param from - Start date (inclusive, UTC) * @param to - End date (inclusive, UTC) - * @param p - Optional transform function for the query string + * @param p - Optional transform function for customizing the generated SQL query string * @returns Modified query builder instance * @throws a BadRequestException if from date is after to date + * @throws a BadRequestException if field name is invalid + * @example + * // Basic usage + * addBetween(query, 'startDate', new Date('2023-01-01'), new Date('2023-12-31')) + * + * // With transform function + * addBetween(query, 'dueDate', from, to, (sql) => `COALESCE(${sql}, NOW())`) + * + * @remarks + * Dates are converted to ISO string format for consistent UTC comparison */
rahul-rocket
deleted the
feat/find-tasks-by-start-and-due-dates-filters
branch
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
Release Notes
New Features
Bug Fixes
Documentation
Chores