Conversation
WalkthroughThe pull request introduces modifications to the 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: 3
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
1183-1192: Improve the consistency of documentation commentsThe method's documentation has misaligned parameter descriptions and lacks a clear summary of the method's purpose. Consistent and clear documentation enhances code readability and maintainability.
Update the documentation block to align parameters and provide a brief description:
/** - * This hook is called after a db write on entities. + * Event handler to update the camp status when the outcome is filled. * * @param string $op * The type of operation being performed. * @param string $objectName - * The name of the object. + * The name of the entity being processed. * @param int $objectId - * The unique identifier for the object. + * The unique identifier for the entity. * @param object $objectRef - * The reference to the object. + * A reference to the entity object. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2 hunks)
🧰 Additional context used
| public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) { | ||
| if ($objectName !== 'Eck_Collection_Camp' || !$objectRef->id) { | ||
| return; | ||
| } | ||
|
|
||
| if (!($goonjField = self::findOfficeId($params))) { | ||
| return; | ||
| } | ||
| static $processedCampId = NULL; | ||
|
|
||
| $goonjFieldId = $goonjField['value']; | ||
| $vehicleDispatchId = $goonjField['entity_id']; | ||
| $collectionCampId = $objectRef->id; | ||
|
|
||
| $collectionSourceVehicleDispatch = EckEntity::get('Collection_Source_Vehicle_Dispatch', FALSE) | ||
| ->addSelect('Camp_Vehicle_Dispatch.Collection_Camp') | ||
| ->addWhere('id', '=', $vehicleDispatchId) | ||
| ->execute()->first(); | ||
| // If the camp ID has already been processed, return to avoid repeated execution. | ||
| if ($collectionCampId === $processedCampId) { | ||
| return; | ||
| } | ||
|
|
||
| $collectionCampId = $collectionSourceVehicleDispatch['Camp_Vehicle_Dispatch.Collection_Camp']; | ||
| // Mark this camp ID as processed to prevent future executions for the same ID. | ||
| $processedCampId = $collectionCampId; | ||
|
|
||
| $results = EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | ||
| ->addWhere('id', '=', $collectionCampId) | ||
| ->execute(); | ||
| try { | ||
| EckEntity::update('Collection_Camp', FALSE) | ||
| ->addWhere('id', '=', $collectionCampId) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | ||
| ->execute(); | ||
|
|
||
| } | ||
| catch (\Exception $e) { | ||
| error_log("Exception occurred while updating camp status for campId: $collectionCampId"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication with updateCampStatusAfterAuth
The updateCampStatusOnOutcomeFilled method shares similar responsibilities with the existing updateCampStatusAfterAuth method. Both methods update the camp status based on certain conditions, which may lead to duplicated code and increased maintenance efforts.
Consider extracting the common logic into a private method or consolidating the two methods if appropriate. This adheres to the DRY (Don't Repeat Yourself) principle and promotes single responsibility, making the codebase more maintainable.
Avoid using static variables for state management in event handlers
The use of a static variable $processedCampId within the updateCampStatusOnOutcomeFilled method may lead to unexpected behavior in a web application context, especially under concurrent requests or when the method is called multiple times within the same request. Relying on static variables for state management can cause issues with thread safety and data consistency.
Consider refactoring to manage the processed camp IDs in a way that is safe for concurrent access. One approach is to maintain a class property to track processed IDs:
+private static $processedCampIds = [];
public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) {
- static $processedCampId = NULL;
$collectionCampId = $objectRef->id;
- if ($collectionCampId === $processedCampId) {
+ if (in_array($collectionCampId, self::$processedCampIds)) {
return;
}
- $processedCampId = $collectionCampId;
+ self::$processedCampIds[] = $collectionCampId;
try {
EckEntity::update('Collection_Camp', FALSE)
->addWhere('id', '=', $collectionCampId)
->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed')
->execute();
}
catch (\Exception $e) {
error_log("Exception occurred while updating camp status for campId: $collectionCampId");
}
}Alternatively, consider redesigning the logic to eliminate the need for tracking processed IDs within the method.
📝 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 static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) { | |
| if ($objectName !== 'Eck_Collection_Camp' || !$objectRef->id) { | |
| return; | |
| } | |
| if (!($goonjField = self::findOfficeId($params))) { | |
| return; | |
| } | |
| static $processedCampId = NULL; | |
| $goonjFieldId = $goonjField['value']; | |
| $vehicleDispatchId = $goonjField['entity_id']; | |
| $collectionCampId = $objectRef->id; | |
| $collectionSourceVehicleDispatch = EckEntity::get('Collection_Source_Vehicle_Dispatch', FALSE) | |
| ->addSelect('Camp_Vehicle_Dispatch.Collection_Camp') | |
| ->addWhere('id', '=', $vehicleDispatchId) | |
| ->execute()->first(); | |
| // If the camp ID has already been processed, return to avoid repeated execution. | |
| if ($collectionCampId === $processedCampId) { | |
| return; | |
| } | |
| $collectionCampId = $collectionSourceVehicleDispatch['Camp_Vehicle_Dispatch.Collection_Camp']; | |
| // Mark this camp ID as processed to prevent future executions for the same ID. | |
| $processedCampId = $collectionCampId; | |
| $results = EckEntity::update('Collection_Camp', FALSE) | |
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | |
| ->addWhere('id', '=', $collectionCampId) | |
| ->execute(); | |
| try { | |
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addWhere('id', '=', $collectionCampId) | |
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | |
| ->execute(); | |
| } | |
| catch (\Exception $e) { | |
| error_log("Exception occurred while updating camp status for campId: $collectionCampId"); | |
| } | |
| private static $processedCampIds = []; | |
| public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) { | |
| if ($objectName !== 'Eck_Collection_Camp' || !$objectRef->id) { | |
| return; | |
| } | |
| $collectionCampId = $objectRef->id; | |
| // If the camp ID has already been processed, return to avoid repeated execution. | |
| if (in_array($collectionCampId, self::$processedCampIds)) { | |
| return; | |
| } | |
| // Mark this camp ID as processed to prevent future executions for the same ID. | |
| self::$processedCampIds[] = $collectionCampId; | |
| try { | |
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addWhere('id', '=', $collectionCampId) | |
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | |
| ->execute(); | |
| } | |
| catch (\Exception $e) { | |
| error_log("Exception occurred while updating camp status for campId: $collectionCampId"); | |
| } | |
| } |
| catch (\Exception $e) { | ||
| error_log("Exception occurred while updating camp status for campId: $collectionCampId"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use CiviCRM logging facilities for exception handling
The current exception handling uses error_log, which may not integrate well with CiviCRM's logging system. Using CiviCRM's logging facilities provides better control over log levels and outputs.
Replace error_log with CiviCRM's logger:
catch (\Exception $e) {
- error_log("Exception occurred while updating camp status for campId: $collectionCampId");
+ \Civi::log()->error("Exception occurred while updating camp status for campId: $collectionCampId. Error: " . $e->getMessage());
}This ensures that exceptions are logged consistently with the rest of the application.
📝 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.
| catch (\Exception $e) { | |
| error_log("Exception occurred while updating camp status for campId: $collectionCampId"); | |
| } | |
| catch (\Exception $e) { | |
| \Civi::log()->error("Exception occurred while updating camp status for campId: $collectionCampId. Error: " . $e->getMessage()); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
1178-1187: Update method documentation to reflect parameter changesThe method
updateCampStatusOnOutcomeFilledhas had its parameters updated, but the documentation block still references the old parameters. This may cause confusion for future maintenance.Please update the documentation to match the new method signature:
/** - * This hook is called after a db write on entities. + * Hook function to update the camp status to 'completed' when the outcome form is submitted. * * @param string $op - * The type of operation being performed. - * @param string $objectName - * The name of the object. + * Operation being performed (e.g., 'create', 'edit'). + * @param string $objectName + * Name of the object being operated on. * @param int $objectId - * The unique identifier for the object. + * The ID of the object. * @param object $objectRef - * The reference to the object. + * Reference to the object. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (3)
1194-1204: Avoid using static variables for state management in event handlersUsing a static variable
$processedCampIdto track processed camp IDs may lead to issues when multiple camp IDs are processed in the same request. This approach is not thread-safe and can cause unexpected behavior in concurrent environments.Consider using a class property array to track processed IDs:
-class CollectionCampService extends AutoSubscriber { +class CollectionCampService extends AutoSubscriber { + private static $processedCampIds = []; // ... public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) { - static $processedCampId = NULL; $collectionCampId = $objectRef->id; - if ($collectionCampId === $processedCampId) { + if (in_array($collectionCampId, self::$processedCampIds, true)) { return; } - $processedCampId = $collectionCampId; + self::$processedCampIds[] = $collectionCampId;
1189-1215: Refactor to eliminate code duplication withupdateCampStatusAfterAuthThe
updateCampStatusOnOutcomeFilledmethod shares similar functionality withupdateCampStatusAfterAuth. Both methods are updating the camp status based on certain conditions, which introduces duplicated code.Consider extracting the common logic into a private method or consolidating these methods to adhere to the DRY (Don't Repeat Yourself) principle. This will improve maintainability and readability.
1213-1215: Ensure exception messages provide sufficient contextThe exception handling logs the camp ID and the error message. Consider including more context if available, such as the operation or object name, to aid in debugging.
| if ($objectName !== 'Eck_Collection_Camp' || !$objectRef->id) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Improve null check for objectRef->id
The current null check for $objectRef->id may cause a notice if objectRef is not an object or if id is not set. To make the code more robust, consider checking if objectRef is an object and if id is set:
- if ($objectName !== 'Eck_Collection_Camp' || !$objectRef->id) {
+ if ($objectName !== 'Eck_Collection_Camp' || !is_object($objectRef) || !isset($objectRef->id)) {
return;
}📝 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.
| if ($objectName !== 'Eck_Collection_Camp' || !$objectRef->id) { | |
| return; | |
| } | |
| if ($objectName !== 'Eck_Collection_Camp' || !is_object($objectRef) || !isset($objectRef->id)) { | |
| return; | |
| } | |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
1214-1214: Remove debug logging statementThe
error_logstatement appears to be used for debugging purposes and should be removed or replaced with proper logging mechanisms before deploying to production.Apply this diff to remove the debug statement:
-error_log("collectionCampId: " . print_r($collectionCampId, TRUE));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (3 hunks)
🧰 Additional context used
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
Line range hint
1229-1257: LGTM: Good refactoring for camp status checkingThe addition of the
checkCampStatusAndIdsmethod is a positive change that centralizes the logic for checking camp status and IDs. This refactoring improves code reusability and maintainability.One minor suggestion to further improve this method:
Consider using named constants for the status field names to improve readability and maintainability. For example:
class CollectionCampService extends AutoSubscriber { // ... other code ... const CAMP_STATUS_FIELD = 'Collection_Camp_Core_Details.Status'; public static function checkCampStatusAndIds(string $objectName, $objectId, &$objectRef) { if ($objectName != 'Eck_Collection_Camp') { return NULL; } $newStatus = $objectRef[self::CAMP_STATUS_FIELD] ?? ''; if (!$newStatus || !$objectId) { return NULL; } $collectionCamp = EckEntity::get('Collection_Camp', FALSE) ->addSelect(self::CAMP_STATUS_FIELD) ->addWhere('id', '=', $objectId) ->execute()->single(); $currentStatus = $collectionCamp[self::CAMP_STATUS_FIELD] ?? ''; return [ 'newStatus' => $newStatus, 'currentStatus' => $currentStatus, ]; } }This change makes the code more maintainable by centralizing the field name definition, reducing the risk of typos and making future changes easier.
Line range hint
1-1257: Consider broader refactoring for improved structure and maintainabilityWhile the individual changes in this PR improve certain aspects of the code, the
CollectionCampServiceclass has grown quite large and seems to handle multiple responsibilities. This could potentially violate the Single Responsibility Principle and make the code harder to maintain in the long run.Consider the following suggestions for a broader refactoring:
Split the class into smaller, more focused classes. For example:
CollectionCampStatusManagerCollectionCampEventHandlerCollectionCampNotificationServiceUse dependency injection to manage relationships between these classes, rather than relying on static methods.
Consider implementing the Command pattern for status update operations, which could improve testability and separation of concerns.
Create a separate configuration class to manage constants and configuration values, rather than defining them within the service class.
Here's a high-level example of how this refactoring might look:
class CollectionCampStatusManager { public function updateStatus(int $campId, string $newStatus): void { // Implementation } public function canCompleteCamp(int $campId): bool { // Implementation } } class CollectionCampEventHandler { private $statusManager; private $notificationService; public function __construct(CollectionCampStatusManager $statusManager, CollectionCampNotificationService $notificationService) { $this->statusManager = $statusManager; $this->notificationService = $notificationService; } public function handleOutcomeFormSubmission(int $campId): void { if ($this->statusManager->canCompleteCamp($campId)) { $this->statusManager->updateStatus($campId, 'completed'); $this->notificationService->sendCompletionNotification($campId); } } } class CollectionCampNotificationService { public function sendCompletionNotification(int $campId): void { // Implementation } }This refactoring would make the code more modular, easier to test, and more maintainable in the long run. It would also make it easier to add new features or modify existing ones without affecting other parts of the system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
51-51: LGTM: Appropriate event subscription for updateCampStatusOnOutcomeFilledThe addition of
updateCampStatusOnOutcomeFilledto the&hook_civicrm_postevent is a logical change. This ensures that the camp status is updated after the relevant database write operations have been completed, which is the correct sequence for this type of operation.
1192-1192: LGTM: Improved method signature with type hintsThe addition of type hints to the method parameters enhances code readability and type safety. This is a good practice that helps catch potential type-related errors early and improves code documentation.
| private static $individualId = NULL; | ||
| private static $collectionCampAddress = NULL; | ||
| private static $fromAddress = NULL; | ||
| private static $processedCampIds = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a non-static approach for tracking processed camp IDs
The addition of a static property $processedCampIds to track processed camp IDs might lead to unexpected behavior in a web application context, especially under concurrent requests. Static properties maintain state across all instances of a class and can cause issues with thread safety and data consistency.
Instead, consider using a session-based or database-driven approach to track processed camp IDs. This would ensure that the state is managed properly for each user session or request.
Here's a potential alternative approach using session storage:
public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) {
// ... existing code ...
$session = \CRM_Core_Session::singleton();
$processedCampIds = $session->get('processedCampIds', []);
if (in_array($collectionCampId, $processedCampIds)) {
return;
}
$processedCampIds[] = $collectionCampId;
$session->set('processedCampIds', $processedCampIds);
// ... rest of the method ...
}This approach ensures that processed camp IDs are tracked on a per-session basis, avoiding potential conflicts between different users or requests.
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'planned') | ||
| ->addWhere('id', '=', $campId) | ||
| ->execute(); | ||
| error_log("results: " . print_r($results, TRUE)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance logging for better debugging and consistency
While adding logging is beneficial, using error_log directly may not align with CiviCRM's logging best practices. Additionally, the current log message lacks context, which could make debugging more challenging.
Consider the following improvements:
- Use CiviCRM's logging system instead of
error_log. - Provide more context in the log message, including the method name and relevant variables.
Here's a suggested improvement:
\Civi::log()->info("updateCampStatusAfterAuth: Camp status updated for camp ID {$campId}. Results: " . json_encode($results));This approach uses CiviCRM's logging system, provides method context, includes the camp ID, and properly formats the results for logging.
| if ($objectRef instanceof CRM_Afform_BAO_AfformSubmission) { | ||
| $afformName = $objectRef->afform_name; | ||
|
|
||
| if ($afformName !== 'afformCampOutcomeForm') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting AfformSubmission handling logic
The addition of AfformSubmission handling logic increases the method's complexity and potentially violates the Single Responsibility Principle. Consider extracting this logic into a separate method for better maintainability and clarity.
Here's a suggested refactoring:
private static function isValidAfformSubmission($objectRef): bool {
if ($objectRef instanceof CRM_Afform_BAO_AfformSubmission) {
return $objectRef->afform_name === 'afformCampOutcomeForm';
}
return true;
}
public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) {
if (!self::isValidAfformSubmission($objectRef)) {
return;
}
// ... rest of the method ...
}This refactoring improves readability and keeps the main method focused on its primary responsibility.
| ->execute(); | ||
| // Mark this camp ID as processed to prevent future executions for the same ID. | ||
| self::$processedCampIds[] = $collectionCampId; | ||
| error_log("collectionCampId: " . print_r($collectionCampId, TRUE)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error logging for consistency and clarity
The current error logging uses error_log and \Civi::log()->error, which is inconsistent. Additionally, the first log message lacks context. For better debugging and consistency with CiviCRM practices, consider using \Civi::log() for both log messages and provide more context.
Replace the current logging statements with:
\Civi::log()->info("updateCampStatusOnOutcomeFilled: Processing collection camp ID: {$collectionCampId}");
// ... and later in the catch block ...
\Civi::log()->error("updateCampStatusOnOutcomeFilled: Exception occurred while updating camp status for campId: {$collectionCampId}. Error: " . $e->getMessage());This approach provides consistent logging, better context, and adheres to CiviCRM's logging practices.
Also applies to: 1226-1226
| if (in_array($collectionCampId, self::$processedCampIds)) { | ||
| return; | ||
| } | ||
|
|
||
| $results = EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | ||
| ->addWhere('id', '=', $collectionCampId) | ||
| ->execute(); | ||
| // Mark this camp ID as processed to prevent future executions for the same ID. | ||
| self::$processedCampIds[] = $collectionCampId; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the approach for preventing repeated executions
The current implementation uses a static array $processedCampIds to prevent repeated executions for the same camp ID. While this approach works for preventing duplicate processing within a single request, it may not be effective across multiple requests or in a concurrent environment.
Consider using a more robust solution, such as:
- Implementing a database-level lock.
- Using a distributed locking mechanism if dealing with multiple servers.
- Adding a status flag in the database to indicate processing state.
Here's a potential implementation using a database flag:
public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) {
// ... existing code ...
$camp = EckEntity::get('Collection_Camp', FALSE)
->addSelect('Collection_Camp_Intent_Details.Camp_Status')
->addWhere('id', '=', $collectionCampId)
->execute()
->first();
if ($camp['Collection_Camp_Intent_Details.Camp_Status'] === 'completed') {
return;
}
// Attempt to update the status atomically
$result = EckEntity::update('Collection_Camp', FALSE)
->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed')
->addWhere('id', '=', $collectionCampId)
->addWhere('Collection_Camp_Intent_Details.Camp_Status', '!=', 'completed')
->execute();
if ($result->count() === 0) {
// Another process has already updated the status
return;
}
// ... rest of the method ...
}This approach ensures that the status is only updated once, even in a concurrent environment.
|
|
||
| $goonjFieldId = $goonjField['value']; | ||
| $vehicleDispatchId = $goonjField['entity_id']; | ||
| static $processedCampId = NULL; |
There was a problem hiding this comment.
Remove unused static variable
The static variable $processedCampId is declared but never used in the method. It should be removed to avoid confusion and maintain clean code.
Apply this diff to remove the unused variable:
- static $processedCampId = NULL;📝 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.
| static $processedCampId = NULL; |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
1217-1219: Enhance exception logging with method contextIncluding the method name in the exception log provides clearer context for debugging purposes.
Apply this diff to improve the log message:
\Civi::log()->error( - "Exception occurred while updating camp status for campId: $collectionCampId. Error: " . $e->getMessage() + "updateCampStatusOnOutcomeFilled: Exception occurred while updating camp status for campId: $collectionCampId. Error: " . $e->getMessage() );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
50-50: Ensure correct hook registrationThe method
updateCampStatusOnOutcomeFilledis added to the&hook_civicrm_postevent in thegetSubscribedEventsmethod. Verify that this hook is appropriate for the intended functionality and that it aligns with CiviCRM's event handling best practices.
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'planned') | ||
| ->addWhere('id', '=', $campId) | ||
| ->execute(); | ||
| error_log("results: " . print_r($results, TRUE)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use CiviCRM logging instead of error_log
Replacing error_log with CiviCRM's logging facilities improves consistency and leverages the application's logging configurations.
Apply this diff to update the logging method:
- error_log("results: " . print_r($results, TRUE));
+ \Civi::log()->info("updateCampStatusAfterAuth: Results: " . print_r($results, TRUE));📝 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.
| error_log("results: " . print_r($results, TRUE)); | |
| \Civi::log()->info("updateCampStatusAfterAuth: Results: " . print_r($results, TRUE)); |
| EckEntity::update('Collection_Camp', FALSE) | ||
| ->addWhere('id', '=', $collectionCampId) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | ||
| ->execute(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure idempotent update to prevent race conditions
When updating the camp status to 'completed', adding a condition to check the current status prevents unnecessary database operations and potential race conditions.
Modify the update query as follows:
EckEntity::update('Collection_Camp', FALSE)
->addWhere('id', '=', $collectionCampId)
+ ->addWhere('Collection_Camp_Intent_Details.Camp_Status', '!=', 'completed')
->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed')
->execute();📝 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.
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addWhere('id', '=', $collectionCampId) | |
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | |
| ->execute(); | |
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addWhere('id', '=', $collectionCampId) | |
| ->addWhere('Collection_Camp_Intent_Details.Camp_Status', '!=', 'completed') | |
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | |
| ->execute(); |
| public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) { | ||
| if ($objectName !== 'AfformSubmission') { | ||
| return; | ||
| } | ||
|
|
||
| if (!($goonjField = self::findOfficeId($params))) { | ||
| $afformName = $objectRef->afform_name; | ||
|
|
||
| if ($afformName !== 'afformCampOutcomeForm') { | ||
| return; | ||
| } | ||
|
|
||
| $goonjFieldId = $goonjField['value']; | ||
| $vehicleDispatchId = $goonjField['entity_id']; | ||
| $jsonData = $objectRef->data; | ||
| $dataArray = json_decode($jsonData, TRUE); | ||
|
|
||
| $collectionSourceVehicleDispatch = EckEntity::get('Collection_Source_Vehicle_Dispatch', FALSE) | ||
| ->addSelect('Camp_Vehicle_Dispatch.Collection_Camp') | ||
| ->addWhere('id', '=', $vehicleDispatchId) | ||
| ->execute()->first(); | ||
| $collectionCampId = $dataArray['Eck_Collection_Camp1'][0]['fields']['id']; | ||
|
|
||
| $collectionCampId = $collectionSourceVehicleDispatch['Camp_Vehicle_Dispatch.Collection_Camp']; | ||
| if (!$collectionCampId) { | ||
| return; | ||
| } | ||
|
|
||
| $results = EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | ||
| ->addWhere('id', '=', $collectionCampId) | ||
| ->execute(); | ||
| try { | ||
| EckEntity::update('Collection_Camp', FALSE) | ||
| ->addWhere('id', '=', $collectionCampId) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | ||
| ->execute(); | ||
|
|
||
| } | ||
| catch (\Exception $e) { | ||
| \Civi::log()->error("Exception occurred while updating camp status for campId: $collectionCampId. Error: " . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Add checks for JSON decoding errors and array keys
In the updateCampStatusOnOutcomeFilled method, there are no checks for JSON decoding errors or the existence of expected keys in the decoded array. This could lead to undefined index notices or runtime errors if the data structure is not as anticipated.
Apply this diff to add necessary validations:
$jsonData = $objectRef->data;
$dataArray = json_decode($jsonData, TRUE);
+ if (json_last_error() !== JSON_ERROR_NONE) {
+ \Civi::log()->error("JSON decode error: " . json_last_error_msg());
+ return;
+ }
+
+ if (!isset($dataArray['Eck_Collection_Camp1'][0]['fields']['id'])) {
+ \Civi::log()->error("Collection Camp ID not found in submission data.");
+ return;
+ }
$collectionCampId = $dataArray['Eck_Collection_Camp1'][0]['fields']['id'];📝 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 static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) { | |
| if ($objectName !== 'AfformSubmission') { | |
| return; | |
| } | |
| if (!($goonjField = self::findOfficeId($params))) { | |
| $afformName = $objectRef->afform_name; | |
| if ($afformName !== 'afformCampOutcomeForm') { | |
| return; | |
| } | |
| $goonjFieldId = $goonjField['value']; | |
| $vehicleDispatchId = $goonjField['entity_id']; | |
| $jsonData = $objectRef->data; | |
| $dataArray = json_decode($jsonData, TRUE); | |
| $collectionSourceVehicleDispatch = EckEntity::get('Collection_Source_Vehicle_Dispatch', FALSE) | |
| ->addSelect('Camp_Vehicle_Dispatch.Collection_Camp') | |
| ->addWhere('id', '=', $vehicleDispatchId) | |
| ->execute()->first(); | |
| $collectionCampId = $dataArray['Eck_Collection_Camp1'][0]['fields']['id']; | |
| $collectionCampId = $collectionSourceVehicleDispatch['Camp_Vehicle_Dispatch.Collection_Camp']; | |
| if (!$collectionCampId) { | |
| return; | |
| } | |
| $results = EckEntity::update('Collection_Camp', FALSE) | |
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | |
| ->addWhere('id', '=', $collectionCampId) | |
| ->execute(); | |
| try { | |
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addWhere('id', '=', $collectionCampId) | |
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | |
| ->execute(); | |
| } | |
| catch (\Exception $e) { | |
| \Civi::log()->error("Exception occurred while updating camp status for campId: $collectionCampId. Error: " . $e->getMessage()); | |
| } | |
| public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) { | |
| if ($objectName !== 'AfformSubmission') { | |
| return; | |
| } | |
| $afformName = $objectRef->afform_name; | |
| if ($afformName !== 'afformCampOutcomeForm') { | |
| return; | |
| } | |
| $jsonData = $objectRef->data; | |
| $dataArray = json_decode($jsonData, TRUE); | |
| if (json_last_error() !== JSON_ERROR_NONE) { | |
| \Civi::log()->error("JSON decode error: " . json_last_error_msg()); | |
| return; | |
| } | |
| if (!isset($dataArray['Eck_Collection_Camp1'][0]['fields']['id'])) { | |
| \Civi::log()->error("Collection Camp ID not found in submission data."); | |
| return; | |
| } | |
| $collectionCampId = $dataArray['Eck_Collection_Camp1'][0]['fields']['id']; | |
| if (!$collectionCampId) { | |
| return; | |
| } | |
| try { | |
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addWhere('id', '=', $collectionCampId) | |
| ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') | |
| ->execute(); | |
| } | |
| catch (\Exception $e) { | |
| \Civi::log()->error("Exception occurred while updating camp status for campId: $collectionCampId. Error: " . $e->getMessage()); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
1189-1218: Approve changes with suggestions for improvementThe updates to the
updateCampStatusOnOutcomeFilledmethod are generally good, improving type safety and error handling. However, there are a few areas that could be further enhanced:
- Add JSON decoding error checking.
- Validate the array structure before accessing the collection camp ID.
- Improve error logging with more context.
Here's a suggested improvement:
public static function updateCampStatusOnOutcomeFilled(string $op, string $objectName, int $objectId, &$objectRef) { if ($objectName !== 'AfformSubmission') { return; } $afformName = $objectRef->afform_name; if ($afformName !== 'afformCampOutcomeForm') { return; } $jsonData = $objectRef->data; $dataArray = json_decode($jsonData, TRUE); + if (json_last_error() !== JSON_ERROR_NONE) { + \Civi::log()->error("JSON decode error in updateCampStatusOnOutcomeFilled: " . json_last_error_msg()); + return; + } + if (!isset($dataArray['Eck_Collection_Camp1'][0]['fields']['id'])) { + \Civi::log()->error("Invalid data structure in updateCampStatusOnOutcomeFilled"); + return; + } $collectionCampId = $dataArray['Eck_Collection_Camp1'][0]['fields']['id']; if (!$collectionCampId) { return; } try { EckEntity::update('Collection_Camp', FALSE) ->addWhere('id', '=', $collectionCampId) ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'completed') ->execute(); } catch (\Exception $e) { - \Civi::log()->error("Exception occurred while updating camp status for campId: $collectionCampId. Error: " . $e->getMessage()); + \Civi::log()->error("Exception in updateCampStatusOnOutcomeFilled for campId: $collectionCampId. Error: " . $e->getMessage(), [ + 'exception' => $e, + 'objectId' => $objectId, + 'afformName' => $afformName, + ]); } }These changes will make the method more robust and easier to debug if issues arise.
1178-1218: Approve changes with a suggestion for improved loggingThe updates to the
updateCampStatusAfterAuthmethod are good. The use of thecheckCampStatusAndIdshelper function improves code reusability, and the simplification of the method enhances its focus. However, the error logging could be more informative.Consider enhancing the error logging as follows:
public static function updateCampStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) { $statusDetails = self::checkCampStatusAndIds($objectName, $objectId, $objectRef); if (!$statusDetails) { return; } $newStatus = $statusDetails['newStatus']; $currentStatus = $statusDetails['currentStatus']; if ($currentStatus !== $newStatus) { if ($newStatus === 'authorized') { $campId = $objectRef['id'] ?? NULL; if ($campId === NULL) { return; } $results = EckEntity::update('Collection_Camp', TRUE) ->addValue('Collection_Camp_Intent_Details.Camp_Status', 'planned') ->addWhere('id', '=', $campId) ->execute(); + \Civi::log()->info("Camp status updated in updateCampStatusAfterAuth", [ + 'campId' => $campId, + 'oldStatus' => $currentStatus, + 'newStatus' => 'planned', + 'results' => $results, + ]); } } }This change will provide more context in the logs, making it easier to track and debug status changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
Line range hint
1-1218: Approve additional changesThe new constants, private static variables, and the
checkCampStatusAndIdsmethod are good additions to the class. They improve code organization, reduce magic values, and enhance reusability.These changes contribute to making the code more maintainable and easier to understand. Good job on improving the overall structure of the class.
2 participants
Move the logic to post-type and change the status from anything to completed when the outcome form is submitted and added the check on based of form so that it wont run on other triggers
The second point from the below feedback PR
https://github.com/coloredcow-admin/goonj-crm/issues/146
Summary by CodeRabbit
New Features
Bug Fixes
Documentation