Add the Update Scheduled Time Job#320
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce the file 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: 2
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateScheduledTimeCron.php (3)
1-5: Improve file-level documentationThe file-level documentation block is currently empty. Consider adding a brief description of the file's purpose and its role within the Goonjcustom extension. This will help other developers understand the file's contents and functionality at a glance.
Example improvement:
<?php /** * @file * Implements the UpdateScheduledTimeCron API for the Goonjcustom extension. * This file contains functions to define and execute a scheduled cron job. */
7-18: Update function documentation for accuracyThe function documentation has some inconsistencies:
- The function name in the comment is incorrect. It mentions "CollectionCampCron" instead of "UpdateScheduledTimeCron".
- The @return annotation doesn't match the actual function behavior (void vs. no return statement).
Please update the documentation to accurately reflect the function:
/** * Goonjcustom.UpdateScheduledTimeCron API specification (optional) * This is used for documentation and validation. * * @param array $spec description of fields supported by this API call * * @return void */ function _civicrm_api3_goonjcustom_update_scheduled_cron_spec(&$spec) { // There are no parameters for the Goonjcustom cron. }
37-39: Improve error loggingThe current error logging can be improved for better readability and consistency with CiviCRM best practices.
Consider updating the error logging as follows:
catch (Exception $e) { \Civi::log()->error('Error in Goonjcustom.UpdateScheduledTimeCron job: {error}', [ 'error' => $e->getMessage(), 'exception' => $e, ]); }This approach uses placeholders and provides more context about where the error occurred.
| function civicrm_api3_goonjcustom_update_scheduled_cron($params) { | ||
| $returnValues = []; | ||
| try { | ||
|
|
There was a problem hiding this comment.
Implement the missing cron job logic
The try block is currently empty, which means the cron job is not performing any actions. Implement the necessary logic for updating the scheduled time as per the requirements of your project.
Would you like assistance in outlining the steps for implementing the cron job logic?
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'update_scheduled_cron'); | ||
| } |
There was a problem hiding this comment.
Reconsider the unconditional success response
The function currently returns a success response regardless of whether any work was done or if an exception was caught. This may lead to misleading results if the cron job fails.
Consider the following improvements:
- Return an error response if an exception is caught.
- Only return a success response if the cron job actually performed some work.
Example:
function civicrm_api3_goonjcustom_update_scheduled_cron($params) {
try {
$returnValues = []; // Initialize with default value
// Implement cron job logic here
// If work is done, populate $returnValues with relevant data
if (empty($returnValues)) {
return civicrm_api3_create_success([], $params, 'Goonjcustom', 'update_scheduled_cron', ['message' => 'No updates were necessary.']);
}
return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'update_scheduled_cron');
}
catch (Exception $e) {
\Civi::log()->error('Error in Goonjcustom.UpdateScheduledTimeCron job: {error}', [
'error' => $e->getMessage(),
'exception' => $e,
]);
return civicrm_api3_create_error('Failed to update scheduled time. Check logs for details.');
}
}This approach provides more accurate feedback about the cron job's execution.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateScheduledTimeCron.php (1)
1-74: Summary of review and offer of assistanceThe implementation of the scheduled time update cron job is mostly correct, but there are several areas that need improvement:
- Clarify the mismatch between the function name and the job being updated.
- Improve logging and return values when no update is needed.
- Enhance error handling and logging.
- Refine the return value to accurately reflect the function's outcome.
These improvements will make the cron job more robust and easier to maintain.
Would you like assistance in implementing these improvements or clarifying any of the suggested changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateScheduledTimeCron.php (1 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateScheduledTimeCron.php (3)
18-20: LGTM: Spec function is correctly implementedThe specification function follows CiviCRM API conventions and correctly indicates that no parameters are required for this cron job.
57-66: LGTM: Update logic is correctly implementedThe logic for updating the scheduled run date when the current date is past it is implemented correctly. It sets the next scheduled date to 10:00 AM of the next day and updates the job record in the database.
73-73:⚠️ Potential issueImprove the return value to reflect the function's outcome
The current implementation always returns a success response, which doesn't accurately reflect the function's outcome. This issue was pointed out in a previous review. To address this:
- Return an error response if an exception is caught (as suggested in the error handling comment).
- Return a success response with appropriate messages based on the action taken:
- No update needed
- Update performed successfully
Example:
if ($updated) { return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'update_scheduled_time_cron', ['message' => 'Scheduled time updated successfully.']); } else { return civicrm_api3_create_success([], $params, 'Goonjcustom', 'update_scheduled_time_cron', ['message' => 'No update was necessary.']); }
| $jobs = Job::get(TRUE) | ||
| ->addSelect('scheduled_run_date') | ||
| ->addWhere('api_action', '=', 'collection_camp_cron') | ||
| ->execute()->single(); |
There was a problem hiding this comment.
Clarify the mismatch between function name and queried job
The function name suggests it's updating the scheduled time, but the query is filtering for a job with api_action = 'collection_camp_cron'. This mismatch could lead to confusion. Consider either:
- Renaming the function to match the job it's updating, or
- Updating the query to match the intended job for this function.
Please clarify which job this function is intended to update.
| if ($todayDate == $scheduledDate) { | ||
| error_log("Hello"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Improve logging and return value when dates match
When today's date matches the scheduled date, the function logs "Hello" and returns early. Consider the following improvements:
- Use a more descriptive log message.
- Return a success response indicating no action was needed.
Example:
if ($todayDate == $scheduledDate) {
\Civi::log()->info("Scheduled date matches current date. No update needed.");
return civicrm_api3_create_success([], $params, 'Goonjcustom', 'update_scheduled_time_cron', ['message' => 'No update needed.']);
}| catch (Exception $e) { | ||
| \Civi::log()->info("Error is there: " . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Improve error handling and logging
The current error handling can be improved:
- Use
\Civi::log()->error()instead ofinfo()for logging exceptions. - Include more details in the log message, such as the exception trace.
- Reflect the error in the function's return value.
Example:
catch (Exception $e) {
\Civi::log()->error('Error in Goonjcustom.UpdateScheduledTimeCron job: {error}', [
'error' => $e->getMessage(),
'trace' => $e->getTraceAsString(),
]);
return civicrm_api3_create_error('Failed to update scheduled time. Check logs for details.');
}
tarunnjoshi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateScheduledTimeCron.php (2)
34-36: Consider improving return value initializationThe
$returnValuesarray is initialized but never populated. Consider removing it if it's not needed, or populate it with relevant data about the updates performed.
1-87: Overall assessment: Good foundation with room for improvementThe
UpdateScheduledTimeCron.phpfile provides a good foundation for updating scheduled cron job times in CiviCRM. It follows the API3 conventions and implements the basic functionality correctly. However, there are several areas where the code can be improved:
- Error handling and logging can be enhanced to provide better feedback and traceability.
- Code duplication in job update logic can be reduced through refactoring.
- The return value can be improved to provide more meaningful information about the cron job execution.
- Input validation and edge case handling could be added for more robustness.
Implementing the suggested improvements will result in a more maintainable, robust, and informative cron job implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateScheduledTimeCron.php (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateScheduledTimeCron.php (2)
18-20: LGTM: API specification function is correctly implementedThe
_civicrm_api3_goonjcustom_update_scheduled_time_cron_specfunction follows the CiviCRM API3 naming convention and correctly indicates that there are no parameters for this API call. The documentation block is present and follows the standard format.
37-45: LGTM: Date and time setup is well-implementedThe code correctly sets up two different times (10:00 AM and 2:00 PM) for the two jobs. Using
cloneto create a separate date object is a good practice to avoid unintended modifications.
| $logisticJob = Job::get(TRUE) | ||
| ->addSelect('scheduled_run_date') | ||
| ->addWhere('api_action', '=', 'collection_camp_cron') | ||
| ->execute()->single(); | ||
|
|
||
| $logisticScheduledRunDate = $logisticJob['scheduled_run_date']; | ||
|
|
||
| $feedbackJob = Job::get(TRUE) | ||
| ->addSelect('scheduled_run_date') | ||
| ->addWhere('api_action', '=', 'volunteer_feedback_collection_camp_cron') | ||
| ->execute()->single(); | ||
|
|
||
| $volunteerScheduledRunDate = $feedbackJob['scheduled_run_date']; | ||
|
|
||
| // Update the scheduled run time for logistics mail. | ||
| if ($logisticScheduledRunDate != $todayDateTimeForLogistics) { | ||
| $results = Job::update(TRUE) | ||
| ->addValue('scheduled_run_date', $todayDateTimeForLogistics) | ||
| ->addWhere('api_action', '=', 'collection_camp_cron') | ||
| ->execute(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging for job updates
While the code correctly fetches and updates the logistics job, consider the following improvements:
- Add a check to ensure the job exists before attempting to update it.
- Log the results of the update operation for better traceability.
- Update the
$returnValuesarray with information about the changes made.
Example:
if (empty($logisticJob)) {
\Civi::log()->error('Logistics job not found');
return civicrm_api3_create_error('Logistics job not found');
}
if ($logisticScheduledRunDate != $todayDateTimeForLogistics) {
$results = Job::update(TRUE)
->addValue('scheduled_run_date', $todayDateTimeForLogistics)
->addWhere('api_action', '=', 'collection_camp_cron')
->execute();
\Civi::log()->info('Updated logistics job scheduled time', ['results' => $results]);
$returnValues['logistics_job_updated'] = TRUE;
}This will provide better error handling and logging, making it easier to track the changes made by this cron job.
| $feedbackJob = Job::get(TRUE) | ||
| ->addSelect('scheduled_run_date') | ||
| ->addWhere('api_action', '=', 'volunteer_feedback_collection_camp_cron') | ||
| ->execute()->single(); | ||
|
|
||
| $volunteerScheduledRunDate = $feedbackJob['scheduled_run_date']; | ||
|
|
||
| // Update the scheduled run time for logistics mail. | ||
| if ($logisticScheduledRunDate != $todayDateTimeForLogistics) { | ||
| $results = Job::update(TRUE) | ||
| ->addValue('scheduled_run_date', $todayDateTimeForLogistics) | ||
| ->addWhere('api_action', '=', 'collection_camp_cron') | ||
| ->execute(); | ||
| } | ||
|
|
||
| // Update the scheduled run time for volunteer feedback mail. | ||
| if ($volunteerScheduledRunDate != $todayDateTimeForFeedback) { | ||
| $results = Job::update(TRUE) | ||
| ->addValue('scheduled_run_date', $todayDateTimeForFeedback) | ||
| ->addWhere('api_action', '=', 'volunteer_feedback_collection_camp_cron') | ||
| ->execute(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor job update logic to reduce duplication
The code for updating the volunteer feedback job is very similar to the logistics job update. Consider refactoring this logic into a separate function to reduce duplication and improve maintainability.
Example:
function updateJobSchedule($jobAction, $scheduledTime, &$returnValues) {
$job = Job::get(TRUE)
->addSelect('scheduled_run_date')
->addWhere('api_action', '=', $jobAction)
->execute()->first();
if (empty($job)) {
\Civi::log()->error("Job not found: {$jobAction}");
return FALSE;
}
if ($job['scheduled_run_date'] != $scheduledTime) {
$results = Job::update(TRUE)
->addValue('scheduled_run_date', $scheduledTime)
->addWhere('api_action', '=', $jobAction)
->execute();
\Civi::log()->info("Updated job scheduled time: {$jobAction}", ['results' => $results]);
$returnValues["{$jobAction}_updated"] = TRUE;
return TRUE;
}
return FALSE;
}
// Usage:
updateJobSchedule('collection_camp_cron', $todayDateTimeForLogistics, $returnValues);
updateJobSchedule('volunteer_feedback_collection_camp_cron', $todayDateTimeForFeedback, $returnValues);This refactoring will make the code more maintainable and easier to extend in the future.
| catch (Exception $e) { | ||
| \Civi::log()->error('Error in Goonjcustom.UpdateScheduledTimeCron job: {error}', [ | ||
| 'error' => $e->getMessage(), | ||
| 'trace' => $e->getTraceAsString(), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Improve error handling to return appropriate response
While the error logging is well-implemented, the function currently returns a success response even if an exception occurs. Consider returning an error response when an exception is caught:
catch (Exception $e) {
\Civi::log()->error('Error in Goonjcustom.UpdateScheduledTimeCron job: {error}', [
'error' => $e->getMessage(),
'trace' => $e->getTraceAsString(),
]);
return civicrm_api3_create_error('Failed to update scheduled time. Check logs for details.');
}This will ensure that the API caller is aware of any errors that occurred during the execution of the cron job.
| ]); | ||
| } | ||
|
|
||
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'update_scheduled_time_cron'); |
There was a problem hiding this comment.
Improve return value to provide meaningful feedback
The current implementation always returns a success response without providing any information about the actions performed. Consider the following improvements:
- Populate the
$returnValuesarray with information about the updates made. - Return a success response only if updates were made or a specific condition was met.
- Include a message in the response to provide more context.
Example:
if (empty($returnValues)) {
return civicrm_api3_create_success([], $params, 'Goonjcustom', 'update_scheduled_time_cron', ['message' => 'No updates were necessary.']);
}
return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'update_scheduled_time_cron', ['message' => 'Scheduled times updated successfully.']);This will provide more meaningful feedback about the execution of the cron job.
tarunnjoshi
force-pushed
the
update-the-scheduled-time-for-logistics-and-feedback-cron
branch
from
f0677a9 to
0dc7f35
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateScheduledTimeCron.php (1 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateScheduledTimeCron.php (1)
18-20: LGTM: API specification function is correct and well-documented.The specification function correctly indicates that there are no parameters for this API call. The documentation comments follow CiviCRM conventions.
| function civicrm_api3_goonjcustom_update_scheduled_time_cron($params) { | ||
| $returnValues = []; | ||
| try { | ||
| $currentDate = new DateTime(); | ||
| // Set time to 10:00 AM. | ||
| $currentDate->setTime(10, 0, 0); | ||
| $todayDateTimeForLogistics = $currentDate->format('Y-m-d H:i:s'); | ||
|
|
||
| $twoPmDateTime = clone $currentDate; | ||
| // Set time to 2:00 PM. | ||
| $twoPmDateTime->setTime(14, 0, 0); | ||
| $todayDateTimeForFeedback = $twoPmDateTime->format('Y-m-d H:i:s'); | ||
|
|
||
| // Update scheduled run time for logistics and volunteer feedback. | ||
| updateJobScheduledTime('collection_camp_cron', $todayDateTimeForLogistics); | ||
| updateJobScheduledTime('volunteer_feedback_collection_camp_cron', $todayDateTimeForFeedback); | ||
|
|
||
| } catch (Exception $e) { | ||
| \Civi::log()->error('Error in Goonjcustom.UpdateScheduledTimeCron job: {error}', [ | ||
| 'error' => $e->getMessage(), | ||
| 'trace' => $e->getTraceAsString(), | ||
| ]); | ||
| } | ||
|
|
||
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'update_scheduled_time_cron'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve return value and exception handling
While the overall structure of the function has improved, there are two areas that could be enhanced:
-
The return value doesn't provide meaningful feedback about the actions performed. Consider populating the
$returnValuesarray with information about the updates made and returning a more informative response. -
The exception handling could be improved to return an error response when an exception occurs, rather than a success response.
Here's a suggested improvement:
function civicrm_api3_goonjcustom_update_scheduled_time_cron($params) {
$returnValues = [];
try {
// ... (existing code) ...
$returnValues['logistics_updated'] = updateJobScheduledTime('collection_camp_cron', $todayDateTimeForLogistics);
$returnValues['feedback_updated'] = updateJobScheduledTime('volunteer_feedback_collection_camp_cron', $todayDateTimeForFeedback);
if (empty(array_filter($returnValues))) {
return civicrm_api3_create_success([], $params, 'Goonjcustom', 'update_scheduled_time_cron', ['message' => 'No updates were necessary.']);
}
return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'update_scheduled_time_cron', ['message' => 'Scheduled times updated successfully.']);
} catch (Exception $e) {
\Civi::log()->error('Error in Goonjcustom.UpdateScheduledTimeCron job: {error}', [
'error' => $e->getMessage(),
'trace' => $e->getTraceAsString(),
]);
return civicrm_api3_create_error('Failed to update scheduled time. Check logs for details.');
}
}This modification provides more meaningful feedback about the execution of the cron job and ensures that API callers are aware of any errors that occurred during execution.
| function updateJobScheduledTime($apiAction, $scheduledRunDate) { | ||
| // Fetch the scheduled run date. | ||
| $job = Job::get(TRUE) | ||
| ->addSelect('scheduled_run_date') | ||
| ->addWhere('api_action', '=', $apiAction) | ||
| ->execute()->single(); | ||
|
|
||
| $scheduledRunDateFromDb = $job['scheduled_run_date']; | ||
|
|
||
| // Update the scheduled run time if it differs from the current value. | ||
| if ($scheduledRunDateFromDb != $scheduledRunDate) { | ||
| Job::update(TRUE) | ||
| ->addValue('scheduled_run_date', $scheduledRunDate) | ||
| ->addWhere('api_action', '=', $apiAction) | ||
| ->execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance helper function with return value and error handling
The updateJobScheduledTime function effectively reduces code duplication, but it could be improved in the following ways:
- Return a boolean value to indicate whether an update was performed.
- Add error handling and logging within the function.
- Use a more descriptive variable name for the job data.
Here's a suggested improvement:
function updateJobScheduledTime($apiAction, $scheduledRunDate) {
try {
// Fetch the scheduled run date.
$jobData = Job::get(TRUE)
->addSelect('scheduled_run_date')
->addWhere('api_action', '=', $apiAction)
->execute()->single();
$scheduledRunDateFromDb = $jobData['scheduled_run_date'];
// Update the scheduled run time if it differs from the current value.
if ($scheduledRunDateFromDb != $scheduledRunDate) {
Job::update(TRUE)
->addValue('scheduled_run_date', $scheduledRunDate)
->addWhere('api_action', '=', $apiAction)
->execute();
\Civi::log()->info("Updated scheduled time for job: {$apiAction}", ['old' => $scheduledRunDateFromDb, 'new' => $scheduledRunDate]);
return true;
}
return false;
} catch (Exception $e) {
\Civi::log()->error("Error updating scheduled time for job: {$apiAction}", [
'error' => $e->getMessage(),
'trace' => $e->getTraceAsString(),
]);
throw $e; // Re-throw the exception to be caught by the main function
}
}These changes will make the helper function more robust and informative, allowing the main function to better track which updates were performed and handle any errors that occur during the update process.
| $scheduledRunDateFromDb = $job['scheduled_run_date']; | ||
|
|
||
| // Update the scheduled run time if it differs from the current value. | ||
| if ($scheduledRunDateFromDb != $scheduledRunDate) { |
There was a problem hiding this comment.
| if ($scheduledRunDateFromDb != $scheduledRunDate) { | |
| if ($scheduledRunDateFromDb !== $scheduledRunDate) { |
tarunnjoshi
deleted the
update-the-scheduled-time-for-logistics-and-feedback-cron
branch
2 participants
Add the update schd time job
Summary by CodeRabbit