Fix(*) Added check to avoid error of not found#663
Conversation
WalkthroughThis pull request introduces modifications across several service classes in the CiviCRM extension, focusing on enhancing error handling and control flow. The changes primarily involve adding conditional checks using bitwise AND operations and improving error logging mechanisms. The modifications span multiple service classes including Changes
Sequence DiagramsequenceDiagram
participant Service
participant CiviCRM
participant Logger
Service->>Service: Validate Input Parameters
alt Parameters are Valid
Service->>CiviCRM: Perform Operation
CiviCRM-->>Service: Return Result
else Parameters Invalid
Service->>Logger: Log Error
Service-->>Service: Skip Operation
end
Possibly related PRs
Suggested labels
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: 10
🔭 Outside diff range comments (3)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
Line range hint
298-303: Replace bitwise AND with logical AND operatorThe code uses a bitwise AND operator (
&) where a logical AND (&&) should be used. This is incorrect as bitwise operations work on the binary representation of numbers, while you need to check if both values exist.Apply this fix:
- if ($groupId & self::$individualId) { + if ($groupId && self::$individualId) { GroupContact::create(FALSE) ->addValue('contact_id', self::$individualId) ->addValue('group_id', $groupId) ->addValue('status', 'Added') ->execute(); }wp-content/civi-extensions/goonjcustom/Civi/GoonjActivitiesService.php (2)
Line range hint
591-673: Refactor email handling to reduce code duplicationThe email sending logic in
sendActivityLogisticsEmailandsendActivityVolunteerFeedbackEmailcontains similar patterns. Consider extracting common email sending functionality into a reusable method.+ private static function sendEmail($params, $campId, $updateField) { + try { + $emailSendResult = \CRM_Utils_Mail::send($params); + if ($emailSendResult) { + EckEntity::update('Collection_Camp', FALSE) + ->addValue($updateField, 1) + ->addWhere('id', '=', $campId) + ->execute(); + } + return $emailSendResult; + } catch (\Exception $e) { + \Civi::log()->error(sprintf("Error sending email for camp %d: %s", $campId, $e->getMessage())); + return false; + } + }Then update the existing methods to use this helper:
public static function sendActivityLogisticsEmail($collectionCamp) { try { // ... existing validation and parameter setup ... - $emailSendResult = \CRM_Utils_Mail::send($mailParams); - if ($emailSendResult) { - EckEntity::update('Collection_Camp', FALSE) - ->addValue('Logistics_Coordination.Email_Sent', 1) - ->addWhere('id', '=', $campId) - ->execute(); - } + self::sendEmail($mailParams, $campId, 'Logistics_Coordination.Email_Sent'); } catch (\Exception $e) { \Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage()); } }Also applies to: 675-748
Line range hint
466-481: Improve error handling in activity creationThe method silently fails by only logging the error. Consider:
- Adding a return value to indicate success/failure
- Providing more context in the error message
- Potentially rethrowing the exception for critical failures
- private static function createGoonjActivitiesOrganizeActivity($contactId, $collectionCampTitle, $collectionCampId) { + private static function createGoonjActivitiesOrganizeActivity($contactId, $collectionCampTitle, $collectionCampId): bool { try { $results = Activity::create(FALSE) // ... existing code ... ->execute(); + return true; } catch (\CiviCRM_API4_Exception $ex) { - \Civi::log()->debug("Exception while creating Organize Collection Camp activity: " . $ex->getMessage()); + $message = sprintf( + "Failed to create Organize Collection Camp activity. Contact: %d, Camp: %d, Error: %s", + $contactId, + $collectionCampId, + $ex->getMessage() + ); + \Civi::log()->error($message); + return false; } }
🧹 Nitpick comments (13)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)
659-659: Remove debugging artifactThe debug message includes what appears to be a random number "232132". This should be removed or replaced with a meaningful message.
Apply this fix:
- \CRM_Core_Error::debug_log_message('No coordinator available to assign.232132'); + \CRM_Core_Error::debug_log_message('No coordinator available to assign.');
Line range hint
1-1000: Consider splitting this large service classThis service class has grown quite large and handles multiple responsibilities including:
- Contact group management
- Email notifications
- Coordinator assignment
- QR code generation
- Tab management
- Contribution tracking
This violates the Single Responsibility Principle and makes the code harder to maintain.
Consider splitting this into smaller, focused service classes:
InstitutionCollectionCampGroupService: Contact group managementInstitutionCollectionCampNotificationService: Email notificationsInstitutionCollectionCampCoordinatorService: Coordinator assignmentInstitutionCollectionCampQrService: QR code generationInstitutionCollectionCampTabService: Tab managementInstitutionCollectionCampContributionService: Contribution trackingwp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (2)
257-257: Remove debugging artifact from log messageThe appended "12312" appears to be a debugging artifact and should be removed from the production log message.
Apply this diff to clean up the log message:
- \CRM_Core_Error::debug_log_message('No coordinator available to assign.12312'); + \CRM_Core_Error::debug_log_message('No coordinator available to assign.');
Line range hint
1-1000: Consider breaking down the service classThe
InstitutionDroppingCenterServiceclass has grown quite large and handles multiple responsibilities including:
- Group management
- Coordinator assignment
- QR code generation
- Email notifications
- Tab management
- Vehicle dispatch
- Material management
This violates the Single Responsibility Principle and makes the code harder to maintain.
Consider splitting this into smaller, focused service classes:
InstitutionGroupService: Handle group-related operationsCoordinatorAssignmentService: Handle coordinator assignmentsNotificationService: Handle email notificationsQRCodeService: Handle QR code generationTabManagementService: Handle tab-related operationsVehicleDispatchService: Handle dispatch operationsMaterialManagementService: Handle material-related operationsThis would improve maintainability, testability, and make the code more modular.
wp-content/civi-extensions/goonjcustom/Civi/InstitutionGoonjActivitiesService.php (4)
204-204: Remove magic number from debug messageThe debug message contains a magic number (2321) that lacks context and meaning. Consider using a meaningful constant or removing the number if it's not necessary.
- \CRM_Core_Error::debug_log_message('No coordinator available to assign.2321'); + \CRM_Core_Error::debug_log_message('No coordinator available to assign.');
121-130: Consider adding more context to error loggingThe error handling could be improved by adding more context about the operation being performed.
catch (Exception $e) { - \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); + \Civi::log()->error(sprintf( + "Failed to add contact (ID: %d) to group (ID: %d). Error: %s", + $contactId, + $groupId, + $e->getMessage() + )); }
Line range hint
1-694: Consider splitting the service class to improve maintainabilityThe
InstitutionGoonjActivitiesServiceclass has grown quite large and handles multiple responsibilities including:
- Activity management
- Logistics coordination
- Email notifications
- QR code generation
- Group management
This violates the Single Responsibility Principle and makes the code harder to maintain.
Consider splitting this into smaller, focused service classes:
InstitutionActivityServiceInstitutionLogisticsServiceInstitutionNotificationServiceInstitutionQrCodeServiceInstitutionGroupService
Line range hint
32-694: Add return type declarations to methodsThe class methods lack PHP return type declarations, which would improve code clarity and enable better static analysis.
Example for a few methods:
- public static function getSubscribedEvents() + public static function getSubscribedEvents(): array - private static function getChapterGroupForState($stateId) + private static function getChapterGroupForState($stateId): ?int - private static function addContactToGroup($contactId, $groupId) + private static function addContactToGroup($contactId, $groupId): voidwp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (2)
247-247: Remove hardcoded number from debug messageThe debug message includes a hardcoded number "121" which seems arbitrary and reduces code maintainability.
Apply this fix:
- \CRM_Core_Error::debug_log_message('No coordinator available to assign.121'); + \CRM_Core_Error::debug_log_message('No coordinator available to assign.');
Line range hint
1-341: Standardize error handling patternsThe class uses inconsistent error handling patterns:
- Some methods use debug_log_message
- Some use Civi::log()->info
- Some have no error logging
- Exception handling is missing in critical API calls
Consider implementing a consistent error handling strategy across the class.
Would you like me to provide a detailed proposal for standardizing error handling across the class?
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (2)
427-432: Simplify the isEmailAlreadySent methodThe method can be made more concise while maintaining the same functionality.
Consider this refactor:
private static function isEmailAlreadySent($contactId) { - if($contactId){ - $contactDetails = Contact::get(FALSE) - ->addSelect('Individual_fields.Volunteer_Registration_Email_Sent') - ->addWhere('id', '=', $contactId) - ->execute()->single(); - } - $isEmailSent = $contactDetails['Individual_fields.Volunteer_Registration_Email_Sent'] ?? NULL; - - if (!empty($isEmailSent)) { - // Email already sent. - return TRUE; - } - - return FALSE; + if (!$contactId) { + return FALSE; + } + $contactDetails = Contact::get(FALSE) + ->addSelect('Individual_fields.Volunteer_Registration_Email_Sent') + ->addWhere('id', '=', $contactId) + ->execute()->single(); + return !empty($contactDetails['Individual_fields.Volunteer_Registration_Email_Sent']);
463-463: Improve logging messagesThe current debug logs ('check1' and 'check12') don't provide meaningful context about what's being checked.
Enhance the logging:
- \Civi::log()->info('check1'); + \Civi::log()->info('Checking contact subtypes for volunteer transition', ['contact_id' => $id]); $contacts = Contact::get(FALSE) ->addSelect('contact_sub_type') ->addWhere('id', '=', $id) ->execute()->single(); - \Civi::log()->info('check12'); + \Civi::log()->info('Retrieved contact details', ['contact' => $contacts]);Also applies to: 468-468
wp-content/civi-extensions/goonjcustom/Civi/GoonjActivitiesService.php (1)
Line range hint
535-564: Separate QR generation concernsThe
generateGoonjActivitiesQrmethod handles both status validation and QR code generation. Consider splitting these responsibilities to improve maintainability and testability.+ private static function shouldGenerateQr(string $newStatus, array $currentCollectionCamp): bool { + $currentStatus = $currentCollectionCamp['Collection_Camp_Core_Details.Status']; + return $currentStatus !== $newStatus && $newStatus === 'authorized'; + } public static function generateGoonjActivitiesQr(string $op, string $objectName, $objectId, &$objectRef) { if ($objectName !== 'Eck_Collection_Camp' || !$objectId || !self::isCurrentSubtype($objectRef)) { return; } $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; if (!$newStatus) { return; } $collectionCamps = EckEntity::get('Collection_Camp', TRUE) ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id') ->addWhere('id', '=', $objectId) ->execute(); $currentCollectionCamp = $collectionCamps->first(); - $currentStatus = $currentCollectionCamp['Collection_Camp_Core_Details.Status']; $collectionCampId = $currentCollectionCamp['id']; - if ($currentStatus !== $newStatus) { - if ($newStatus === 'authorized') { + if (self::shouldGenerateQr($newStatus, $currentCollectionCamp)) { self::generateGoonjActivitiesQrCode($collectionCampId); - } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/GoonjActivitiesService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(6 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionGoonjActivitiesService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
Line range hint 332-338: Replace bitwise AND with logical AND operator
Same issue as in the assignChapterGroupToIndividual method above.
| if($contactId & $groupID){ | ||
| try { | ||
| GroupContact::create(FALSE) | ||
| ->addValue('contact_id', $contactId) | ||
| ->addValue('group_id', $groupId) | ||
| ->addValue('status', 'Added') | ||
| ->execute(); | ||
| \Civi::log()->info("Successfully added contact_id: $contactId to group_id: $groupId."); | ||
| } | ||
| catch (Exception $e) { | ||
| \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Fix the incorrect bitwise operator and variable name
There are two issues in the condition:
- Using bitwise AND operator (
&) instead of logical AND operator (&&) - Variable name mismatch:
groupIDvsgroupId
This will cause incorrect behavior as the bitwise operation will produce unexpected results.
Apply this fix:
- if($contactId & $groupID){
+ if($contactId && $groupId){📝 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($contactId & $groupID){ | |
| try { | |
| GroupContact::create(FALSE) | |
| ->addValue('contact_id', $contactId) | |
| ->addValue('group_id', $groupId) | |
| ->addValue('status', 'Added') | |
| ->execute(); | |
| \Civi::log()->info("Successfully added contact_id: $contactId to group_id: $groupId."); | |
| } | |
| catch (Exception $e) { | |
| \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); | |
| } | |
| if($contactId && $groupId){ | |
| try { | |
| GroupContact::create(FALSE) | |
| ->addValue('contact_id', $contactId) | |
| ->addValue('group_id', $groupId) | |
| ->addValue('status', 'Added') | |
| ->execute(); | |
| \Civi::log()->info("Successfully added contact_id: $contactId to group_id: $groupId."); | |
| } | |
| catch (Exception $e) { | |
| \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); | |
| } |
| if($contactId & $groupId){ | ||
| try { | ||
| GroupContact::create(FALSE) | ||
| ->addValue('contact_id', $contactId) | ||
| ->addValue('group_id', $groupId) | ||
| ->addValue('status', 'Added') | ||
| ->execute(); | ||
| \Civi::log()->info("Successfully added contact_id: $contactId to group_id: $groupId."); | ||
| } | ||
| catch (Exception $e) { | ||
| \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Fix incorrect bitwise operator in condition
The condition uses a bitwise AND operator (&) instead of the logical AND operator (&&). This could lead to unexpected behavior as it performs a bitwise operation instead of a logical comparison.
Apply this diff to fix the operator:
- if($contactId & $groupId){
+ if($contactId && $groupId){📝 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($contactId & $groupId){ | |
| try { | |
| GroupContact::create(FALSE) | |
| ->addValue('contact_id', $contactId) | |
| ->addValue('group_id', $groupId) | |
| ->addValue('status', 'Added') | |
| ->execute(); | |
| \Civi::log()->info("Successfully added contact_id: $contactId to group_id: $groupId."); | |
| } | |
| catch (Exception $e) { | |
| \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); | |
| } | |
| if($contactId && $groupId){ | |
| try { | |
| GroupContact::create(FALSE) | |
| ->addValue('contact_id', $contactId) | |
| ->addValue('group_id', $groupId) | |
| ->addValue('status', 'Added') | |
| ->execute(); | |
| \Civi::log()->info("Successfully added contact_id: $contactId to group_id: $groupId."); | |
| } | |
| catch (Exception $e) { | |
| \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); | |
| } |
| } | ||
| catch (Exception $e) { | ||
| \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); | ||
| if($contactId & $groupID){ |
There was a problem hiding this comment.
Fix the logical operator in the condition
The condition uses a bitwise AND operator (&) instead of the logical AND operator (&&). This could lead to unexpected behavior as it performs a bitwise operation instead of a logical comparison.
- if($contactId & $groupID){
+ if($contactId && $groupId){Also, note the variable name inconsistency: groupID vs groupId.
📝 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($contactId & $groupID){ | |
| if($contactId && $groupId){ |
| if($coordinatorCount & self::$organizationId){ | ||
| Organization::update('Organization', FALSE) | ||
| ->addValue('Review.Coordinating_POC', $coordinatorId) | ||
| ->addWhere('id', '=', self::$organizationId) | ||
| ->execute(); | ||
| } |
There was a problem hiding this comment.
Critical: Fix operator and API call syntax
Two issues in this code segment:
- Incorrect use of bitwise AND operator (
&) instead of logical AND (&&) - Incorrect API call syntax with redundant 'Organization' parameter
Apply these fixes:
- if($coordinatorCount & self::$organizationId){
- Organization::update('Organization', FALSE)
+ if($coordinatorCount && self::$organizationId) {
+ Organization::update(FALSE)
->addValue('Review.Coordinating_POC', $coordinatorId)
->addWhere('id', '=', self::$organizationId)
->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.
| if($coordinatorCount & self::$organizationId){ | |
| Organization::update('Organization', FALSE) | |
| ->addValue('Review.Coordinating_POC', $coordinatorId) | |
| ->addWhere('id', '=', self::$organizationId) | |
| ->execute(); | |
| } | |
| if($coordinatorCount && self::$organizationId) { | |
| Organization::update(FALSE) | |
| ->addValue('Review.Coordinating_POC', $coordinatorId) | |
| ->addWhere('id', '=', self::$organizationId) | |
| ->execute(); | |
| } |
| if($contactId){ | ||
| $organization = Organization::get(FALSE) | ||
| ->addSelect('Institute_Registration.Type_of_Institution:label') | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute()->single(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for API call
While the null check for $contactId is good, the single() method could throw an exception if no results are found. Consider adding proper error handling.
Apply this improvement:
if($contactId){
- $organization = Organization::get(FALSE)
- ->addSelect('Institute_Registration.Type_of_Institution:label')
- ->addWhere('id', '=', $contactId)
- ->execute()->single();
+ try {
+ $organization = Organization::get(FALSE)
+ ->addSelect('Institute_Registration.Type_of_Institution:label')
+ ->addWhere('id', '=', $contactId)
+ ->execute()
+ ->first();
+ } catch (\Exception $e) {
+ \CRM_Core_Error::debug_log_message('Failed to retrieve organization: ' . $e->getMessage());
+ return 'Default Coordinator of';
+ }
}📝 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($contactId){ | |
| $organization = Organization::get(FALSE) | |
| ->addSelect('Institute_Registration.Type_of_Institution:label') | |
| ->addWhere('id', '=', $contactId) | |
| ->execute()->single(); | |
| } | |
| if($contactId){ | |
| try { | |
| $organization = Organization::get(FALSE) | |
| ->addSelect('Institute_Registration.Type_of_Institution:label') | |
| ->addWhere('id', '=', $contactId) | |
| ->execute() | |
| ->first(); | |
| } catch (\Exception $e) { | |
| \CRM_Core_Error::debug_log_message('Failed to retrieve organization: ' . $e->getMessage()); | |
| return 'Default Coordinator of'; | |
| } | |
| } |
| if($stateOfficeId & self::$organizationId){ | ||
| Organization::update(FALSE) | ||
| ->addValue('Review.Goonj_Office', $stateOfficeId) | ||
| ->addWhere('id', '=', self::$organizationId) | ||
| ->execute(); | ||
| } |
There was a problem hiding this comment.
Critical: Replace bitwise AND with logical AND operator
The condition uses a bitwise AND operator (&) instead of the logical AND operator (&&). This is a critical issue as it performs binary operations instead of logical comparison, potentially leading to incorrect validation of the IDs.
Apply this fix:
- if($stateOfficeId & self::$organizationId){
+ if($stateOfficeId && self::$organizationId) {
Organization::update(FALSE)
->addValue('Review.Goonj_Office', $stateOfficeId)
->addWhere('id', '=', self::$organizationId)
->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.
| if($stateOfficeId & self::$organizationId){ | |
| Organization::update(FALSE) | |
| ->addValue('Review.Goonj_Office', $stateOfficeId) | |
| ->addWhere('id', '=', self::$organizationId) | |
| ->execute(); | |
| } | |
| if($stateOfficeId && self::$organizationId) { | |
| Organization::update(FALSE) | |
| ->addValue('Review.Goonj_Office', $stateOfficeId) | |
| ->addWhere('id', '=', self::$organizationId) | |
| ->execute(); | |
| } |
| if(self::$volunteerId & $stateId){ | ||
| self::createInduction(self::$volunteerId, $stateId); | ||
| } |
There was a problem hiding this comment.
Replace bitwise AND with logical AND operator
The code uses a bitwise AND operator (&) where a logical AND (&&) should be used. This is incorrect as bitwise operations work on the binary representation of numbers, while you need to check if both values exist.
Apply this fix:
- if(self::$volunteerId & $stateId){
+ if(self::$volunteerId && $stateId){
self::createInduction(self::$volunteerId, $stateId);
}📝 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(self::$volunteerId & $stateId){ | |
| self::createInduction(self::$volunteerId, $stateId); | |
| } | |
| if(self::$volunteerId && $stateId){ | |
| self::createInduction(self::$volunteerId, $stateId); | |
| } |
| if(empty($volunteerId)){ | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move empty check to the start of the method
The empty check for $volunteerId should be at the beginning of the method to fail fast and avoid unnecessary processing.
Apply this refactor:
private static function sendInductionEmail($volunteerId) {
+ if(empty($volunteerId)){
+ return;
+ }
if (self::isEmailAlreadySent($volunteerId)) {
return FALSE;
}
- if(empty($volunteerId)){
- 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(empty($volunteerId)){ | |
| return; | |
| } | |
| if(empty($volunteerId)){ | |
| return; | |
| } | |
| if (self::isEmailAlreadySent($volunteerId)) { | |
| return FALSE; | |
| } |
| if(empty($volunteerId)){ | ||
| return; | ||
| } |
There was a problem hiding this comment.
Improve error handling in fetchTypeOfInduction
The method should return a specific value when $volunteerId is empty instead of an implicit null return.
Apply this fix:
public static function fetchTypeOfInduction($volunteerId) {
$inductionType = 'Offline';
if(empty($volunteerId)){
- return;
+ return $inductionType; // Return default type if no volunteer 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.
| if(empty($volunteerId)){ | |
| return; | |
| } | |
| if(empty($volunteerId)){ | |
| return $inductionType; // Return default type if no volunteer ID | |
| } |
|
|
||
| if (!$coordinator) { | ||
| \CRM_Core_Error::debug_log_message('No coordinator available to assign.'); | ||
| \CRM_Core_Error::debug_log_message('No coordinator available to assign.2332412'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error logging message and avoid magic numbers
The debug message contains a magic number (2332412) which doesn't provide meaningful context. Consider using a more descriptive error message that includes relevant details about the coordinator assignment failure.
- \CRM_Core_Error::debug_log_message('No coordinator available to assign.2332412');
+ \CRM_Core_Error::debug_log_message(sprintf('No coordinator available to assign for Goonj Office ID: %d', $stateOfficeId));Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionGoonjActivitiesService.php (1)
120-130:⚠️ Potential issueFix the logical operator and variable naming inconsistency
The condition uses a bitwise AND operator (
&) instead of the logical AND operator (&&). This could lead to unexpected behavior. Also, there's an inconsistency in variable naming ($groupIDvs$groupId).- if($contactId & $groupID){ + if($contactId && $groupId){
🧹 Nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionGoonjActivitiesService.php (2)
128-130: Enhance error handling and logging patternsConsider standardizing the error handling and logging patterns across the service class. The current implementation could be improved by:
- Using structured logging with consistent format
- Including more context in error messages
- Implementing proper error recovery mechanisms
- \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); + \Civi::log()->error("Failed to add contact to group", [ + 'contact_id' => $contactId, + 'group_id' => $groupId, + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString() + ]); + // Consider implementing retry logic or fallback mechanism
Line range hint
1-1000: Consider splitting the service class for better maintainabilityThe
InstitutionGoonjActivitiesServiceclass handles multiple responsibilities including:
- Activity management
- Email notifications
- Logistics coordination
- Feedback management
Consider splitting this into smaller, focused service classes following the Single Responsibility Principle:
InstitutionActivityServiceInstitutionNotificationServiceInstitutionLogisticsServiceInstitutionFeedbackServiceThis would improve maintainability, testability, and make the code more modular.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(5 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionGoonjActivitiesService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php
- wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php
- wp-content/civi-extensions/goonjcustom/Civi/InductionService.php
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (2)
103-113: LGTM! Well-structured error handling.
The error handling implementation is robust with:
- Proper try-catch block
- Informative success and error logging
- Appropriate exception handling
102-102:
Fix incorrect bitwise operator in condition
The condition uses a bitwise AND operator (&) instead of the logical AND operator (&&). This could lead to unexpected behavior as it performs a bitwise operation instead of a logical comparison.
Apply this diff to fix the operator:
- if($contactId & $groupId){
+ if($contactId && $groupId){Likely invalid or redundant comment.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation