delete temporal schedule for discourse. #412
Conversation
WalkthroughThe changes introduce a new function, Changes
Possibly related PRs
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (1)
src/services/discourse/core.service.ts (1)
28-30: Consider using a standard HTTP status codeThe error code 590 is outside the standard HTTP status code range (100-599). Consider using a more standard error code, such as 500 for internal server errors or 503 for service unavailable.
- throw new ApiError(590, 'Failed to create discourse schedule.'); + throw new ApiError(503, 'Failed to create discourse schedule.');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/services/discourse/core.service.ts(1 hunks)src/services/platform.service.ts(2 hunks)src/services/temporal/discourse.service.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/services/platform.service.ts
[error] 148-152: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
🔇 Additional comments (5)
src/services/temporal/discourse.service.ts (1)
30-40: Consider architectural implications of schedule management.
A few architectural considerations for the schedule management implementation:
- Should we ensure cleanup of any in-progress workflows when deleting a schedule?
- Consider implementing a status check before operations to prevent race conditions between pause/delete operations.
- Consider adding a method to list active schedules for better management.
Let's verify the usage pattern:
✅ Verification successful
Schedule management implementation appears sufficient with existing safeguards
Based on the verification results:
- The schedule operations are part of a larger system with proper transaction management, as seen in the announcement service.
- Schedule operations (create/pause/delete) are atomic operations provided by the Temporal client, which handles race conditions internally.
- The system already implements proper error handling and rollback mechanisms through database transactions.
The initial architectural concerns are addressed:
- Cleanup: Temporal handles workflow cleanup internally when a schedule is deleted
- Race conditions: Temporal's schedule handle operations are atomic
- Management: The system has comprehensive schedule lifecycle management through create, pause, and delete operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions or cleanup patterns in schedule management
# Look for places where pause and delete are called in sequence
rg -A 5 "pauseSchedule.*deleteSchedule"
# Check for any cleanup patterns in workflow definitions
ast-grep --pattern 'class $_ implements WorkflowInterface {
$$$
cleanup() {
$$$
}
$$$
}'
Length of output: 144
Script:
#!/bin/bash
# Let's try a broader search to understand the schedule management patterns
# Search for schedule-related method calls
rg -B 2 -A 2 "schedule\.(pause|delete|getHandle)"
# Look for workflow implementations
rg -l "implements.*Workflow"
# Search for any error handling or cleanup patterns around schedule operations
rg -B 2 -A 2 "(try|catch).*schedule"
# Look for schedule creation patterns to understand lifecycle
rg -B 2 -A 2 "createSchedule"
Length of output: 3096
Script:
#!/bin/bash
# Let's examine the error handling and transaction patterns in schedule-related operations
# Check the implementation of createSchedule for error handling
rg -B 5 -A 10 "createSchedule.*Promise.*{"
# Look for transaction patterns around schedule operations
rg -B 3 -A 3 "startTransaction|commitTransaction|abortTransaction"
# Check for any schedule-related error handling in the discourse service
rg -B 3 -A 3 "catch.*schedule" src/services/temporal/discourse.service.ts
# Look for any schedule cleanup or status check implementations
rg -B 3 -A 3 "schedule.*status|schedule.*check"
Length of output: 5202
src/services/discourse/core.service.ts (2)
45-45: LGTM!
The export addition follows the existing pattern and correctly exposes the new functionality.
33-38: Verify temporal workflow integration
Let's verify that the temporal workflow properly handles the schedule deletion.
✅ Verification successful
Let me gather more information about the temporal workflow integration.
Let me do one final check to verify the client initialization and error handling.
Temporal workflow integration is properly implemented
The deletion functionality is correctly integrated with the temporal workflow:
- The
deleteDiscourseSchedulefunction properly calls the temporal service - The temporal service correctly obtains the client, gets the schedule handle, and calls the delete method
- Error handling is implemented at the service level with appropriate logging
- The implementation follows the same pattern as other schedule operations (e.g., pause)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to deleteSchedule to ensure proper integration
rg -A 5 "deleteSchedule" --type ts
# Look for any potential error handling in temporal workflow
ast-grep --pattern 'catch (error) {
$$$
deleteSchedule($$$)
$$$
}'
Length of output: 1029
Script:
#!/bin/bash
# Check the temporal client initialization and error handling
ast-grep --pattern 'class TemporalDiscourseService {
$$$
getClient() {
$$$
}
$$$
}'
# Look for schedule handle usage patterns
rg -A 5 "schedule.getHandle" --type ts
# Check for any tests related to schedule deletion
rg -A 5 "deleteSchedule.*test" --type ts
Length of output: 1041
Script:
#!/bin/bash
# Check temporal client initialization
rg -A 10 "class TemporalDiscourseService" --type ts
# Look for any workflow definitions that might be related
rg -A 5 "workflow.ts" --type ts
# Check for any schedule-related interfaces or types
rg -A 5 "interface.*Schedule" --type ts
Length of output: 1064
src/services/platform.service.ts (2)
169-169: LGTM!
Good change to reuse the deletePlatform function, ensuring consistent cleanup across all deletion paths.
Line range hint 147-169: Verify all Discourse platform deletion paths
Let's ensure all Discourse platform deletions go through this code path and no deletion bypasses the schedule cleanup.
✅ Verification successful
Let me gather more specific information about the deletion paths.
All Discourse platform deletions are properly handled
The verification shows that all platform deletions are centralized and properly handled:
- All platform deletions go through
platformService.deletePlatform()ordeletePlatformByFilter()which internally callsdeletePlatform() - The deletion logic in
platform.service.tsproperly handles Discourse schedule cleanup - There are no direct platform removals bypassing this logic - the only
platform.remove()call is within the verifieddeletePlatformmethod - The platform controller's delete endpoint routes through the same service methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all platform deletion code paths
# Search for direct platform removals
echo "Checking for direct platform removals:"
rg -l "platform.*remove\(\)" --type ts
# Search for other deletion methods
echo "Checking for other deletion methods:"
rg -l "delete.*[Pp]latform" --type ts
# Search for Discourse-specific operations
echo "Checking for Discourse-specific operations:"
rg -l "PlatformNames\.Discourse" --type ts
Length of output: 849
Script:
#!/bin/bash
# Check platform deletion implementations and routes
echo "=== Platform Service Deletion Methods ==="
rg "delete.*[Pp]latform" src/services/platform.service.ts -B 2 -A 2
echo -e "\n=== Platform Controller Deletion Methods ==="
rg "delete.*[Pp]latform" src/controllers/platform.controller.ts -B 2 -A 2
echo -e "\n=== Platform Routes ==="
rg "delete.*[Pp]latform" src/routes/v1/platform.route.ts -B 2 -A 2
# Check if there are any other methods that might remove platforms
echo -e "\n=== Other Platform Removal Methods ==="
rg "platform.*remove" --type ts -B 2 -A 2
Length of output: 2780
🧰 Tools
🪛 Biome (1.9.4)
[error] 148-152: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
| public async pauseSchedule(scheduleId: string): Promise<void> { | ||
| const client: Client = await this.getClient(); | ||
| const handle = client.schedule.getHandle(scheduleId); | ||
| await handle.pause(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation.
The method needs additional safeguards for production reliability:
- Error handling for invalid scheduleId or network failures
- Input validation
- Method documentation
Consider this improved implementation:
+ /**
+ * Pauses a Discourse schedule by its ID
+ * @param scheduleId - The ID of the schedule to pause
+ * @throws Error if the schedule cannot be paused or doesn't exist
+ */
public async pauseSchedule(scheduleId: string): Promise<void> {
+ if (!scheduleId) {
+ throw new Error('Schedule ID is required');
+ }
const client: Client = await this.getClient();
- const handle = client.schedule.getHandle(scheduleId);
- await handle.pause();
+ try {
+ const handle = client.schedule.getHandle(scheduleId);
+ await handle.pause();
+ } catch (error) {
+ throw new Error(`Failed to pause schedule ${scheduleId}: ${(error as Error).message}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async pauseSchedule(scheduleId: string): Promise<void> { | |
| const client: Client = await this.getClient(); | |
| const handle = client.schedule.getHandle(scheduleId); | |
| await handle.pause(); | |
| } | |
| /** | |
| * Pauses a Discourse schedule by its ID | |
| * @param scheduleId - The ID of the schedule to pause | |
| * @throws Error if the schedule cannot be paused or doesn't exist | |
| */ | |
| public async pauseSchedule(scheduleId: string): Promise<void> { | |
| if (!scheduleId) { | |
| throw new Error('Schedule ID is required'); | |
| } | |
| const client: Client = await this.getClient(); | |
| try { | |
| const handle = client.schedule.getHandle(scheduleId); | |
| await handle.pause(); | |
| } catch (error) { | |
| throw new Error(`Failed to pause schedule ${scheduleId}: ${(error as Error).message}`); | |
| } | |
| } |
| public async deleteSchedule(scheduleId: string): Promise<void> { | ||
| const client: Client = await this.getClient(); | ||
| const handle = client.schedule.getHandle(scheduleId); | ||
| await handle.delete(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance deletion method with error handling, validation, and logging.
As this is the core functionality for the PR's objective of deleting temporal schedules, it needs to be robust and auditable.
Consider this enhanced implementation:
+ /**
+ * Deletes a Discourse schedule by its ID
+ * @param scheduleId - The ID of the schedule to delete
+ * @throws Error if the schedule cannot be deleted or doesn't exist
+ */
public async deleteSchedule(scheduleId: string): Promise<void> {
+ if (!scheduleId) {
+ throw new Error('Schedule ID is required');
+ }
const client: Client = await this.getClient();
- const handle = client.schedule.getHandle(scheduleId);
- await handle.delete();
+ try {
+ const handle = client.schedule.getHandle(scheduleId);
+ // Verify schedule exists before deletion
+ await handle.describe();
+ await handle.delete();
+ console.info(`Successfully deleted Discourse schedule: ${scheduleId}`);
+ } catch (error) {
+ const message = `Failed to delete schedule ${scheduleId}: ${(error as Error).message}`;
+ console.error(message);
+ throw new Error(message);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async deleteSchedule(scheduleId: string): Promise<void> { | |
| const client: Client = await this.getClient(); | |
| const handle = client.schedule.getHandle(scheduleId); | |
| await handle.delete(); | |
| } | |
| /** | |
| * Deletes a Discourse schedule by its ID | |
| * @param scheduleId - The ID of the schedule to delete | |
| * @throws Error if the schedule cannot be deleted or doesn't exist | |
| */ | |
| public async deleteSchedule(scheduleId: string): Promise<void> { | |
| if (!scheduleId) { | |
| throw new Error('Schedule ID is required'); | |
| } | |
| const client: Client = await this.getClient(); | |
| try { | |
| const handle = client.schedule.getHandle(scheduleId); | |
| // Verify schedule exists before deletion | |
| await handle.describe(); | |
| await handle.delete(); | |
| console.info(`Successfully deleted Discourse schedule: ${scheduleId}`); | |
| } catch (error) { | |
| const message = `Failed to delete schedule ${scheduleId}: ${(error as Error).message}`; | |
| console.error(message); | |
| throw new Error(message); | |
| } | |
| } |
| async function deleteDiscourseSchedule(scheduleId: string): Promise<void> { | ||
| try { | ||
| await temporalDiscourse.deleteSchedule(scheduleId); | ||
| } catch (error) { | ||
| logger.error(error, 'Failed to delete discourse schedule.'); | ||
| throw new ApiError(590, 'Failed to delete discourse schedule.'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and standardize error code
The function implementation looks good but could benefit from these improvements:
- Add input validation for scheduleId
- Use a standard HTTP status code for consistency
async function deleteDiscourseSchedule(scheduleId: string): Promise<void> {
+ if (!scheduleId) {
+ throw new ApiError(400, 'Schedule ID is required');
+ }
try {
await temporalDiscourse.deleteSchedule(scheduleId);
} catch (error) {
logger.error(error, 'Failed to delete discourse schedule.');
- throw new ApiError(590, 'Failed to delete discourse schedule.');
+ throw new ApiError(503, 'Failed to delete discourse schedule.');
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function deleteDiscourseSchedule(scheduleId: string): Promise<void> { | |
| try { | |
| await temporalDiscourse.deleteSchedule(scheduleId); | |
| } catch (error) { | |
| logger.error(error, 'Failed to delete discourse schedule.'); | |
| throw new ApiError(590, 'Failed to delete discourse schedule.'); | |
| async function deleteDiscourseSchedule(scheduleId: string): Promise<void> { | |
| if (!scheduleId) { | |
| throw new ApiError(400, 'Schedule ID is required'); | |
| } | |
| try { | |
| await temporalDiscourse.deleteSchedule(scheduleId); | |
| } catch (error) { | |
| logger.error(error, 'Failed to delete discourse schedule.'); | |
| throw new ApiError(503, 'Failed to delete discourse schedule.'); | |
| } | |
| } |
| switch (platform.name) { | ||
| case PlatformNames.Discourse: { | ||
| if (platform.metadata?.scheduleId) { | ||
| await discourseService.coreService.deleteDiscourseSchedule(platform.metadata.scheduleId); | ||
| } | ||
| } | ||
| default: { | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix case fallthrough and add error handling
There are several issues in the platform deletion logic:
- The Discourse case falls through to the default case due to missing
breakstatement - No error handling for the schedule deletion operation
- The switch statement structure might not scale well as more platforms are added
Apply this diff to fix the immediate issues:
switch (platform.name) {
case PlatformNames.Discourse: {
if (platform.metadata?.scheduleId) {
- await discourseService.coreService.deleteDiscourseSchedule(platform.metadata.scheduleId);
+ try {
+ await discourseService.coreService.deleteDiscourseSchedule(platform.metadata.scheduleId);
+ } catch (error) {
+ throw new ApiError(
+ httpStatus.INTERNAL_SERVER_ERROR,
+ `Failed to delete Discourse schedule: ${error.message}`
+ );
+ }
}
+ break;
}
default: {
+ break;
}
}Consider refactoring to use a strategy pattern for platform-specific deletion logic as the number of platforms grows. This would make the code more maintainable and easier to test. Would you like me to provide an example implementation?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (platform.name) { | |
| case PlatformNames.Discourse: { | |
| if (platform.metadata?.scheduleId) { | |
| await discourseService.coreService.deleteDiscourseSchedule(platform.metadata.scheduleId); | |
| } | |
| } | |
| default: { | |
| } | |
| } | |
| switch (platform.name) { | |
| case PlatformNames.Discourse: { | |
| if (platform.metadata?.scheduleId) { | |
| try { | |
| await discourseService.coreService.deleteDiscourseSchedule(platform.metadata.scheduleId); | |
| } catch (error) { | |
| throw new ApiError( | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| `Failed to delete Discourse schedule: ${error.message}` | |
| ); | |
| } | |
| } | |
| break; | |
| } | |
| default: { | |
| break; | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 148-152: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
Adds the function of deleting the temporal schedule associated with the discourse extraction.
Summary by CodeRabbit
New Features
Bug Fixes
These changes enhance the management of discourse schedules and streamline platform deletions, improving overall user experience.