-
Notifications
You must be signed in to change notification settings - Fork 6
HTM-1438: Delete scheduled task by setting search index schedule to no schedule #1044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the task deletion and scheduling functionality in the search index management system. The changes convert TaskMonitoringService to a root-level singleton service and improve the handling of schedule deletions.
Key changes:
- Converted
TaskMonitoringServicefrom component-level to root-level singleton service - Implemented automatic task deletion when removing a search index schedule
- Enabled the "No schedule" option in the UI that was previously disabled
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/shared/src/lib/components/slider/slider.component.ts | Removed debug console statement |
| projects/admin-core/src/lib/tasks/task-details/task-details.component.ts | Removed service from component providers, fixed method name to follow naming convention |
| projects/admin-core/src/lib/tasks/services/task-monitoring.service.ts | Changed service to root-level singleton, renamed method to follow RxJS naming convention |
| projects/admin-core/src/lib/search-index/services/search-index.service.ts | Added logic to delete associated tasks when removing schedules, imported required dependencies |
| projects/admin-core/src/lib/search-index/search-index-scheduling/search-index-scheduling.component.ts | Simplified schedule object creation to always build schedule object |
| projects/admin-core/src/lib/search-index/search-index-scheduling/search-index-scheduling.component.html | Removed disabled state from "No schedule" option |
| projects/admin-core/src/lib/search-index/search-index-edit/search-index-edit.component.ts | Added task loading on component initialization |
Comments suppressed due to low confidence (1)
projects/admin-core/src/lib/tasks/services/task-monitoring.service.ts:78
- The method dispatches
deleteTaskSuccesswiththis.uuid$.valueinstead of theuuidparameter passed to the method. SinceTaskMonitoringServiceis now a root-level singleton used by multiple components,this.uuid$.valuemay contain a different task UUID than the one being deleted. Use theuuidparameter instead:deleteTaskSuccess({ taskUuid: uuid }).
this.store$.dispatch(deleteTaskSuccess({ taskUuid: this.uuid$.value }));
projects/admin-core/src/lib/search-index/services/search-index.service.ts
Show resolved
Hide resolved
projects/admin-core/src/lib/search-index/services/search-index.service.ts
Outdated
Show resolved
Hide resolved
| const schedule = { | ||
| ...this.taskSchedule, | ||
| cronExpression: value.cronExpression || '', | ||
| description: value.description, | ||
| priority: value.priority, | ||
| }; |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schedule object is now always created even when cronExpression is empty. This changes the behavior from emitting undefined to emitting a schedule object with an empty cronExpression. This could cause issues in the parent component logic that checks for schedule existence. Consider preserving the original behavior where schedule is set to undefined when cronExpression is empty.
| const schedule = { | |
| ...this.taskSchedule, | |
| cronExpression: value.cronExpression || '', | |
| description: value.description, | |
| priority: value.priority, | |
| }; | |
| let schedule: TaskSchedule | undefined; | |
| if (value.cronExpression && value.cronExpression.trim() !== '') { | |
| schedule = { | |
| ...this.taskSchedule, | |
| cronExpression: value.cronExpression, | |
| description: value.description, | |
| priority: value.priority, | |
| }; | |
| } else { | |
| schedule = undefined; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steinkobben this seems a valid concern, see also: #1044 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a check that prevents creating new schedules if the cron expression is empty. When there is an existing schedule, the schedule should be emitted with an empty cron expression because the parent component needs the schedule uuid to delete the task.
...admin-core/src/lib/search-index/search-index-scheduling/search-index-scheduling.component.ts
Outdated
Show resolved
Hide resolved
projects/admin-core/src/lib/search-index/services/search-index.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some comments/questions; the logic as such (delete the task if the schedule is set to null / undefined) is valid for indexing tasks because these are always scheduled using a cron expression
Uh oh!
There was an error while loading. Please reload this page.