Conversation
WalkthroughThe changes 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: 5
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (1)
255-262: Add logging for successful email sendingIn
processQueuedInductionEmail(), there is error handling for failed email sending but no confirmation log for successful operations. Adding a success log can aid in tracking and debugging.try { $result = civicrm_api3('Email', 'send', $params); + \Civi::log()->info('Queued induction email sent successfully', ['contactId' => $params['contact_id']]); } catch (\Exception $ex) {
| if (!$template) { | ||
| return FALSE; | ||
| \Civi::log()->warning('No email template found. Queuing the induction email.', ['contactId' => $contactId]); | ||
| self::queueInductionEmail($contactId); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| \Civi::log()->info('Induction email queued for contact', ['contactId' => $contactId]); | ||
|
|
||
| } | ||
| catch (\Exception $ex) { | ||
| \Civi::log()->error('Failed to queue induction email', [ | ||
| 'contactId' => $contactId, | ||
| 'error' => $ex->getMessage(), | ||
| ]); |
There was a problem hiding this comment.
Undefined variable $contactId in queueInductionEmail()
Within the queueInductionEmail() method, the variable $contactId is used in log messages, but it is not defined in this scope:
\Civi::log()->info('Induction email queued for contact', ['contactId' => $contactId]);and
\Civi::log()->error('Failed to queue induction email', [
'contactId' => $contactId,
'error' => $ex->getMessage(),
]);To resolve this, extract $contactId from the $params array at the beginning of the method:
$contactId = $params['contact_id'];Apply this diff to fix the issue:
+ $contactId = $params['contact_id'];
try {
$queue = \Civi::queue(\CRM_Goonjcustom_Engine::QUEUE_NAME, [This ensures that $contactId is defined and available for logging.
| // Check if the email was already sent. | ||
| if (self::emailAlreadySent($contactId)) { | ||
| \Civi::log()->info('Induction email already sent for contact', ['id' => $contactId]); | ||
| return FALSE; | ||
| } | ||
|
|
||
| // Retrieve the email template. | ||
| $template = MessageTemplate::get(FALSE) | ||
| ->addWhere('msg_title', 'LIKE', 'New_Volunteer_Registration%') | ||
| ->setLimit(1) | ||
| ->execute()->single(); | ||
|
|
||
| // If no template is found, queue the email task to be processed later. | ||
| if (!$template) { | ||
| return FALSE; | ||
| \Civi::log()->warning('No email template found. Queuing the induction email.', ['contactId' => $contactId]); | ||
| self::queueInductionEmail($contactId); | ||
| return TRUE; | ||
| } | ||
|
|
||
| // Prepare email parameters. | ||
| $emailParams = [ | ||
| 'contact_id' => $contactId, | ||
| 'template_id' => $template['id'], | ||
| 'cc' => self::$volunteerInductionAssigneeEmail, | ||
| ]; | ||
|
|
||
| // Fetch contact's email. | ||
| $contacts = Contact::get(FALSE) | ||
| ->addSelect('email.email') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute()->single(); | ||
|
|
||
| $email = $contacts['email.email']; | ||
|
|
||
| if (empty($email)) { | ||
| self::queueInductionEmail($emailParams); | ||
| return TRUE; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor sendInductionEmail() to adhere to the Single Responsibility Principle
The sendInductionEmail() method handles multiple tasks:
- Checking if the email was already sent.
- Retrieving the email template.
- Queuing the email if necessary.
- Fetching the contact's email.
- Sending the email.
This violates the Single Responsibility Principle, making the code harder to read and maintain.
Consider refactoring the method into smaller, dedicated methods:
isEmailAlreadySent($contactId)getEmailTemplate()fetchContactEmail($contactId)queueInductionEmail($emailParams)sendEmail($emailParams)
This separation of concerns will improve readability and maintainability.
| [self::class, 'processQueuedInductionEmail'], | ||
| [$params] | ||
| ), [ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid potential code duplication in queue item creation
The creation of a queue item in queueInductionEmail() uses an inline array for parameters:
$queue->createItem(new \CRM_Queue_Task(
[self::class, 'processQueuedInductionEmail'],
[$params]
), [
'weight' => 1,
]);If this pattern is used elsewhere, consider extracting it into a separate method to avoid code duplication and enhance code reuse.
| private static function queueInductionEmail($params) { | ||
| try { | ||
| $queue = \Civi::queue(\CRM_Goonjcustom_Engine::QUEUE_NAME, [ | ||
| 'type' => 'Sql', | ||
| 'error' => 'abort', | ||
| 'runner' => 'task', | ||
| ]); | ||
|
|
||
| $queue->createItem(new \CRM_Queue_Task( | ||
| [self::class, 'processQueuedInductionEmail'], | ||
| [$params] | ||
| ), [ | ||
| 'weight' => 1, | ||
| ]); | ||
|
|
||
| \Civi::log()->info('Induction email queued for contact', ['contactId' => $contactId]); | ||
|
|
||
| } | ||
| catch (\Exception $ex) { | ||
| \Civi::log()->error('Failed to queue induction email', [ | ||
| 'contactId' => $contactId, | ||
| 'error' => $ex->getMessage(), | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle exceptions more precisely in queueInductionEmail()
The catch block in queueInductionEmail() catches a general \Exception. For better error handling, consider catching more specific exceptions or rethrowing exceptions after logging, to avoid suppressing critical errors.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (3)
Line range hint
176-212: RefactorsendInductionEmailmethod to improve maintainabilityWhile the additions to this method enhance its functionality, it's now handling multiple responsibilities:
- Checking if the email was already sent
- Retrieving the email template
- Preparing email parameters
- Fetching the contact's email
- Sending or queuing the email
This violates the Single Responsibility Principle and makes the code less maintainable. Consider breaking this method down into smaller, more focused methods to improve readability and maintainability.
Here's a suggested refactoring approach:
private static function sendInductionEmail($contactId) { if (self::emailAlreadySent($contactId)) { \Civi::log()->info('Induction email already sent for contact', ['id' => $contactId]); return FALSE; } $template = self::getInductionEmailTemplate(); if (!$template) { return FALSE; } $emailParams = self::prepareEmailParameters($contactId, $template['id']); $email = self::getContactEmail($contactId); if (empty($email)) { return self::queueInductionEmail($emailParams); } return self::sendEmail($emailParams); } private static function getInductionEmailTemplate() { // Template retrieval logic here } private static function prepareEmailParameters($contactId, $templateId) { // Email parameter preparation logic here } private static function getContactEmail($contactId) { // Contact email fetching logic here } private static function sendEmail($emailParams) { // Email sending logic here }This refactoring will make the code more modular and easier to maintain.
Line range hint
411-422: Approve changes toemailAlreadySentwith a minor suggestionThe updates to the
emailAlreadySentmethod improve its accuracy by checking for a specific email activity type and subject. This change adheres well to the Single Responsibility Principle and enhances the overall functionality of the induction process.For improved clarity and maintainability, consider extracting the email subject string into a class constant. This would make it easier to update in the future if needed and improve code readability. Here's a suggested improvement:
class InductionService extends AutoSubscriber { // ... other constants ... const INDUCTION_EMAIL_SUBJECT = 'Volunteering with Goonj'; // ... other methods ... private static function emailAlreadySent($contactId) { $volunteerEmailActivity = Activity::get(FALSE) ->addWhere('activity_type_id:name', '=', 'Email') ->addWhere('subject', 'LIKE', '%' . self::INDUCTION_EMAIL_SUBJECT . '%') ->addWhere('target_contact_id', '=', $contactId) ->setLimit(1) ->execute(); return $volunteerEmailActivity->count() > 0; } }This minor change would make the code more maintainable and consistent with other constant usage in the class.
Line range hint
424-447: Approve changes tohasIndividualChangedToVolunteerwith a minor suggestionThe updates to the
hasIndividualChangedToVolunteermethod enhance its accuracy in detecting when an individual has changed to a volunteer status. The method now correctly checks if 'Volunteer' is present in the contact subtypes array, which is a good improvement.To further improve code clarity and maintainability, consider extracting the 'Volunteer' string into a class constant. This would make it easier to update in the future if needed and improve code consistency. Here's a suggested improvement:
class InductionService extends AutoSubscriber { // ... other constants ... const VOLUNTEER_SUBTYPE = 'Volunteer'; // ... other methods ... public static function hasIndividualChangedToVolunteer($op, $objectName, $id, &$params) { if ($op !== 'edit' || $objectName !== 'Individual') { return FALSE; } $newSubtypes = $params['contact_sub_type'] ?? []; if (!in_array(self::VOLUNTEER_SUBTYPE, $newSubtypes)) { \Civi::log()->info('Volunteer not found in subtypes, returning.'); return; } $contacts = Contact::get(FALSE) ->addSelect('contact_sub_type') ->addWhere('id', '=', $id) ->execute()->single(); if ($contacts['contact_sub_type'] === self::VOLUNTEER_SUBTYPE) { return; } self::$transitionedVolunteerId = $contacts['id']; } }This minor change would make the code more maintainable and consistent with other constant usage in the class, while also making it easier to update the volunteer subtype string if needed in the future.
| private static function queueInductionEmail($params) { | ||
| try { | ||
| $queue = \Civi::queue(\CRM_Goonjcustom_Engine::QUEUE_NAME, [ | ||
| 'type' => 'Sql', | ||
| 'error' => 'abort', | ||
| 'runner' => 'task', | ||
| ]); | ||
|
|
||
| $queue->createItem(new \CRM_Queue_Task( | ||
| [self::class, 'processQueuedInductionEmail'], | ||
| [$params] | ||
| ), [ | ||
| 'weight' => 1, | ||
| ]); | ||
|
|
||
| \Civi::log()->info('Induction email queued for contact', ['contactId' => $params['contactId']]); | ||
|
|
||
| } | ||
| catch (\Exception $ex) { | ||
| \Civi::log()->error('Failed to queue induction email', [ | ||
| 'contactId' => $params['contactId'], | ||
| 'error' => $ex->getMessage(), | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling in queueInductionEmail
The new queueInductionEmail method is a good addition for handling cases where emails can't be sent immediately. However, the current exception handling is too broad and might mask specific issues.
Consider catching more specific exceptions to provide better error handling and debugging capabilities. Here's a suggested improvement:
private static function queueInductionEmail($params) {
try {
$queue = \Civi::queue(\CRM_Goonjcustom_Engine::QUEUE_NAME, [
'type' => 'Sql',
'error' => 'abort',
'runner' => 'task',
]);
$queue->createItem(new \CRM_Queue_Task(
[self::class, 'processQueuedInductionEmail'],
[$params]
), [
'weight' => 1,
]);
\Civi::log()->info('Induction email queued for contact', ['contactId' => $params['contactId']]);
}
catch (\CRM_Core_Exception $ex) {
\Civi::log()->error('Failed to queue induction email due to CiviCRM error', [
'contactId' => $params['contactId'],
'error' => $ex->getMessage(),
]);
}
catch (\Exception $ex) {
\Civi::log()->error('Unexpected error while queueing induction email', [
'contactId' => $params['contactId'],
'error' => $ex->getMessage(),
]);
}
}This approach allows for more specific error handling and logging, which can be crucial for debugging and maintaining the system.
| public static function processQueuedInductionEmail($queue, $params) { | ||
| try { | ||
|
|
||
| $result = civicrm_api3('Email', 'send', $params); | ||
| } | ||
| catch (\Exception $ex) { | ||
| \Civi::log()->error('Failed to send queued induction email', [ | ||
| 'contactId' => $params['contactId'], | ||
| 'error' => $ex->getMessage(), | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and return value in processQueuedInductionEmail
The new processQueuedInductionEmail method is a good addition for processing queued emails. However, it could be improved in terms of error handling and providing feedback on the success or failure of the email sending process.
Consider the following improvements:
- Return a boolean value indicating success or failure.
- Use the result of the API call to determine success.
- Rethrow the exception after logging to allow the queue system to handle the error.
Here's a suggested implementation:
public static function processQueuedInductionEmail($queue, $params) {
try {
$result = civicrm_api3('Email', 'send', $params);
if ($result['is_error']) {
throw new \CRM_Core_Exception($result['error_message']);
}
\Civi::log()->info('Successfully sent queued induction email', [
'contactId' => $params['contactId'],
]);
return TRUE;
}
catch (\Exception $ex) {
\Civi::log()->error('Failed to send queued induction email', [
'contactId' => $params['contactId'],
'error' => $ex->getMessage(),
]);
throw $ex; // Rethrow the exception for the queue system to handle
}
}This implementation provides better error handling, logging, and allows the queue system to manage retries or other error-handling strategies as needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (3)
Line range hint
176-212: RefactorsendInductionEmailto adhere to Single Responsibility PrincipleOh boy, this
sendInductionEmailmethod is trying to be a jack of all trades! It's checking if the email was sent, fetching templates, preparing parameters, and even queuing emails. That's a lot for one method to handle, don't you think? Let's break it down and make it more focused:
- Extract email template retrieval into a separate method.
- Move email parameter preparation to its own method.
- Create a dedicated method for fetching contact's email.
- Consider creating a separate method for email queueing logic.
This refactoring will make the code more readable and maintainable. Remember, a method should do one thing and do it well!
Here's a sketch of how you could refactor this:
private static function sendInductionEmail($contactId) { if (self::isEmailAlreadySent($contactId)) { \Civi::log()->info('Induction email already sent for contact', ['id' => $contactId]); return FALSE; } $template = self::getInductionEmailTemplate(); if (!$template) { return FALSE; } $emailParams = self::prepareEmailParameters($contactId, $template['id']); $email = self::getContactEmail($contactId); if (empty($email)) { return self::queueInductionEmail($emailParams); } return self::sendEmail($emailParams); } private static function getInductionEmailTemplate() { // Template retrieval logic here } private static function prepareEmailParameters($contactId, $templateId) { // Email parameter preparation logic here } private static function getContactEmail($contactId) { // Contact email retrieval logic here } private static function sendEmail($emailParams) { // Email sending logic here }This refactoring will make each method more focused and easier to test and maintain.
Line range hint
407-418: Improve robustness of email subject check inisEmailAlreadySentHey there! I see you've given this method a fancy new name.
isEmailAlreadySentis much clearer thanemailAlreadySent. Good job on following naming conventions!But hold on a second, what's this I see? A hard-coded string in the email subject check? That's like trying to find a needle in a haystack with a blindfold on! Let's make this more robust:
- Consider using a constant for the email subject string.
- Think about using a more specific activity type for volunteer induction emails.
Here's how we could improve this:
private const VOLUNTEER_EMAIL_SUBJECT = 'Volunteering with Goonj'; private static function isEmailAlreadySent($contactId) { $volunteerEmailActivity = Activity::get(FALSE) ->addWhere('activity_type_id:name', '=', 'Volunteer_Induction_Email') ->addWhere('subject', 'LIKE', '%' . self::VOLUNTEER_EMAIL_SUBJECT . '%') ->addWhere('target_contact_id', '=', $contactId) ->setLimit(1) ->execute(); return $volunteerEmailActivity->count() > 0; }This way, if you need to change the email subject in the future, you only need to update it in one place. It's like giving your code a single source of truth - much easier to manage!
Line range hint
1-473: Consider refactoringInductionServiceto improve separation of concernsWhoa there, partner! This
InductionServiceclass is starting to look like a Swiss Army knife - it's doing everything from creating inductions to sending emails and now even queueing them! While it's great that we're handling all these scenarios, we might be violating the Single Responsibility Principle here.Let's think about breaking this down into more focused classes:
InductionCreationService: Handles creating inductions.InductionEmailService: Manages sending and queueing induction emails.InductionQueueService: Deals with queue-related operations.This separation would make each class more focused, easier to test, and simpler to maintain. It's like organizing your toolbox - when you need a specific tool, you know exactly where to find it!
Here's a high-level sketch of how this refactoring could look:
class InductionCreationService { public function createInduction($contactId, $stateId) { // Logic for creating inductions } } class InductionEmailService { public function sendInductionEmail($contactId) { // Logic for sending induction emails } public function queueInductionEmail($params) { // Logic for queueing induction emails } } class InductionQueueService { public function processQueuedInductionEmail($queue, $params) { // Logic for processing queued induction emails } } class InductionService { private $creationService; private $emailService; private $queueService; public function __construct( InductionCreationService $creationService, InductionEmailService $emailService, InductionQueueService $queueService ) { $this->creationService = $creationService; $this->emailService = $emailService; $this->queueService = $queueService; } // Other methods that orchestrate the use of these services }This refactoring would make the codebase more modular and easier to maintain. It's like turning your Swiss Army knife into a well-organized toolbox - each tool has its place and purpose!
| private static function queueInductionEmail($params) { | ||
| try { | ||
| $queue = \Civi::queue(\CRM_Goonjcustom_Engine::QUEUE_NAME, [ | ||
| 'type' => 'Sql', | ||
| 'error' => 'abort', | ||
| 'runner' => 'task', | ||
| ]); | ||
|
|
||
| $queue->createItem(new \CRM_Queue_Task( | ||
| [self::class, 'processQueuedInductionEmail'], | ||
| [$params] | ||
| ), [ | ||
| 'weight' => 1, | ||
| ]); | ||
|
|
||
| \Civi::log()->info('Induction email queued for contact', ['contactId' => $params['contactId']]); | ||
| } | ||
| catch (\CRM_Core_Exception $ex) { | ||
| \Civi::log()->error('Failed to queue induction email due to CiviCRM error', [ | ||
| 'contactId' => $params['contactId'], | ||
| 'error' => $ex->getMessage(), | ||
| ]); | ||
| } | ||
| catch (\Exception $ex) { | ||
| \Civi::log()->error('Unexpected error while queueing induction email', [ | ||
| 'contactId' => $params['contactId'], | ||
| 'error' => $ex->getMessage(), | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add return value in queueInductionEmail
Well, well, well, looks like we've got a new method on the block! The queueInductionEmail method is doing a good job with its try-catch block, but it's like a black box - we don't know if it succeeded or not! Let's make it more communicative:
- Return a boolean value indicating success or failure.
- Use more specific exception types in the catch blocks.
- Consider rethrowing exceptions after logging for better error propagation.
Here's how you could improve this method:
private static function queueInductionEmail($params) {
try {
$queue = \Civi::queue(\CRM_Goonjcustom_Engine::QUEUE_NAME, [
'type' => 'Sql',
'error' => 'abort',
'runner' => 'task',
]);
$queue->createItem(new \CRM_Queue_Task(
[self::class, 'processQueuedInductionEmail'],
[$params]
), [
'weight' => 1,
]);
\Civi::log()->info('Induction email queued for contact', ['contactId' => $params['contact_id']]);
return TRUE;
}
catch (\CRM_Core_Exception $ex) {
\Civi::log()->error('Failed to queue induction email due to CiviCRM error', [
'contactId' => $params['contact_id'],
'error' => $ex->getMessage(),
]);
throw $ex; // Rethrow for better error handling upstream
}
catch (\Exception $ex) {
\Civi::log()->error('Unexpected error while queueing induction email', [
'contactId' => $params['contact_id'],
'error' => $ex->getMessage(),
]);
throw $ex; // Rethrow for better error handling upstream
}
}This way, callers can know if the queueing was successful, and we're propagating exceptions for better error handling upstream. It's like giving your method a megaphone to communicate its success or failure!
| public static function processQueuedInductionEmail($queue, $params) { | ||
| try { | ||
| $result = civicrm_api3('Email', 'send', $params); | ||
| if ($result['is_error']) { | ||
| throw new \CRM_Core_Exception($result['error_message']); | ||
| } | ||
| \Civi::log()->info('Successfully sent queued induction email', [ | ||
| 'contactId' => $params['contactId'], | ||
| ]); | ||
| return TRUE; | ||
| } | ||
| catch (\Exception $ex) { | ||
| \Civi::log()->error('Failed to send queued induction email', [ | ||
| 'contactId' => $params['contactId'], | ||
| 'error' => $ex->getMessage(), | ||
| ]); | ||
| // Rethrow the exception for the queue system to handle. | ||
| throw $ex; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in processQueuedInductionEmail
Alright, let's talk about this processQueuedInductionEmail method. It's doing a good job with its try-catch block and rethrowing exceptions, but it's got a blind spot! What if the email sending fails without throwing an exception? We're living dangerously here!
Let's make this method bulletproof:
- Check the result of the API call to determine success.
- Handle the case where the API call might return an error without throwing an exception.
Here's how we can make this method more robust:
public static function processQueuedInductionEmail($queue, $params) {
try {
$result = civicrm_api3('Email', 'send', $params);
if (!empty($result['is_error'])) {
throw new \CRM_Core_Exception($result['error_message'] ?? 'Unknown error occurred while sending email');
}
\Civi::log()->info('Successfully sent queued induction email', [
'contactId' => $params['contact_id'],
]);
return TRUE;
}
catch (\Exception $ex) {
\Civi::log()->error('Failed to send queued induction email', [
'contactId' => $params['contact_id'],
'error' => $ex->getMessage(),
]);
throw $ex; // Rethrow for the queue system to handle
}
}This way, we're catching all possible failure scenarios. It's like giving your method a safety net and a helmet - ready for any curveball the email sending process might throw!
| throw new \CRM_Core_Exception($result['error_message']); | ||
| } | ||
| \Civi::log()->info('Successfully sent queued induction email', [ | ||
| 'contactId' => $params['contact_id'], |
| } | ||
| catch (\Exception $ex) { | ||
| \Civi::log()->error('Unexpected error while queueing induction email', [ | ||
| 'contactId' => $params['contact_id'], |
There was a problem hiding this comment.
let's log all the params and not just contactId
| 'contactId' => $params['contact_id'], | |
| 'paramas' => $params, |
| @@ -173,11 +173,13 @@ private static function findCoordinatorForOffice($officeId) { | |||
| * Common logic to send an email. | |||
| */ | |||
| private static function sendInductionEmail($contactId) { | |||
There was a problem hiding this comment.
can we say volunteerId instead of contactId?
| private static function sendInductionEmail($volunteerId) { | ||
| // Check if the email was already sent. | ||
| if (self::isEmailAlreadySent($contactId)) { | ||
| \Civi::log()->info('Induction email already sent for contact', ['id' => $volunteerId]); |
| if (self::emailAlreadySent($contactId)) { | ||
| \Civi::log()->info('Induction email already sent for contact', ['id' => $contactId]); | ||
| private static function sendInductionEmail($volunteerId) { | ||
| // Check if the email was already sent. |
| ]; | ||
|
|
||
| // Fetch contact's email. | ||
| $contacts = Contact::get(FALSE) |
There was a problem hiding this comment.
Since you are using single it will only return one result. Otherwise it will fail with an exception
| $contacts = Contact::get(FALSE) | |
| $volunteer = Contact::get(FALSE) |
There was a problem hiding this comment.
removed this now
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (2)
Line range hint
175-198: Apply Single Responsibility Principle insendInductionEmail()The
sendInductionEmail()method is handling multiple responsibilities:
- Checking if the email was already sent.
- Retrieving the email template.
- Preparing email parameters.
- Queuing the email.
This violates the Single Responsibility Principle, making the code less modular and harder to test.
Consider refactoring into dedicated methods:
isEmailAlreadySent($volunteerId)getEmailTemplate()prepareEmailParams($volunteerId, $template)queueInductionEmail($emailParams)This will enhance code readability and maintainability.
Line range hint
386-396: Ensure consistent parameter naming for clarityIn
isEmailAlreadySent()(line 386), the parameter is named$contactId, while elsewhere$volunteerIdis used for the same purpose. Inconsistent naming can lead to confusion and potential errors.Apply this diff to rename the parameter for consistency:
- private static function isEmailAlreadySent($contactId) { + private static function isEmailAlreadySent($volunteerId) { $volunteerEmailActivity = Activity::get(FALSE) - ->addWhere('target_contact_id', '=', $contactId) + ->addWhere('target_contact_id', '=', $volunteerId) ->setLimit(1) ->execute();This enhances code readability and aligns variable names throughout the class.
| try { | ||
| $queue = \Civi::queue(\CRM_Goonjcustom_Engine::QUEUE_NAME, [ | ||
| 'type' => 'Sql', | ||
| 'error' => 'abort', | ||
| 'runner' => 'task', | ||
| ]); | ||
|
|
||
| $queue->createItem(new \CRM_Queue_Task( | ||
| [self::class, 'processQueuedInductionEmail'], | ||
| [$params] | ||
| ), [ | ||
| 'weight' => 1, | ||
| ]); | ||
|
|
||
| \Civi::log()->info('Induction email queued for contact', ['contactId' => $params['contact_id']]); | ||
| } | ||
| catch (\CRM_Core_Exception $ex) { | ||
| \Civi::log()->error('Failed to queue induction email due to CiviCRM error', [ | ||
| 'contactId' => $params['contact_id'], | ||
| 'error' => $ex->getMessage(), | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate exception handling to adhere to DRY principle
The exception handling blocks in queueInductionEmail() (lines 206-228) and processQueuedInductionEmail() (lines 234-252) are nearly identical. This duplication violates the DRY (Don't Repeat Yourself) principle and can make maintenance harder.
Consider extracting the common exception handling logic into a separate private method, such as handleEmailQueueException($ex, $params), to improve code maintainability and readability.
Also applies to: 234-252
| if (self::isEmailAlreadySent($contactId)) { | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
Fix undefined variable $contactId in sendInductionEmail()
In lines 177-179, the variable $contactId is used but not defined within the sendInductionEmail() method. You likely intended to use $volunteerId instead, consistent with the parameter name.
Apply this diff to fix the issue:
private static function sendInductionEmail($volunteerId) {
- if (self::isEmailAlreadySent($contactId)) {
+ if (self::isEmailAlreadySent($volunteerId)) {
return FALSE;
}📝 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::isEmailAlreadySent($contactId)) { | |
| return FALSE; | |
| } | |
| if (self::isEmailAlreadySent($volunteerId)) { | |
| return FALSE; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (4)
205-228: Queue up the applause for this new function!Adding
queueInductionEmailis like giving your code a personal assistant to handle all those pesky email tasks. It's a great step towards better separation of concerns. However, let's make it even better!
Your error handling is playing it safe, but maybe a bit too safe. It's catching all
CRM_Core_Exceptions like a overzealous goalkeeper. How about we catch more specific exceptions? This way, we'll know exactly what went wrong, instead of just saying "Oops, something happened!"This function is like a black box - we don't know if it succeeded or not. Let's add a return value so we can celebrate its successes (or mourn its failures) appropriately.
Here's a suggestion to improve both points:
- private static function queueInductionEmail($params) { + private static function queueInductionEmail($params): bool { try { // ... (existing code) ... \Civi::log()->info('Induction email queued for contact', ['contactId' => $params['contact_id']]); + return true; } - catch (\CRM_Core_Exception $ex) { + catch (\CRM_Queue_Exception $ex) { \Civi::log()->error('Failed to queue induction email due to queue error', [ 'contactId' => $params['contact_id'], 'error' => $ex->getMessage(), ]); + return false; + } + catch (\CRM_Core_Exception $ex) { + \Civi::log()->error('Failed to queue induction email due to CiviCRM error', [ + 'contactId' => $params['contact_id'], + 'error' => $ex->getMessage(), + ]); + return false; } }This way, we're not just throwing exceptions into the void, we're catching them with style and purpose!
233-252: This function is like a mail carrier on steroids!
processQueuedInductionEmailis doing a great job of actually sending out those queued emails. It's like the final step in our email assembly line. However, let's make it even more informative:
Your error logging is good, but it could be great! Right now, it's like saying "The mail truck broke down" without mentioning where or why. Let's add more context to our error logs.
You're returning
TRUEon success, which is great! But let's make sure we're consistent with our error handling.Here's a suggestion to beef up our logging:
public static function processQueuedInductionEmail($queue, $params) { try { $result = civicrm_api3('Email', 'send', $params); if ($result['is_error']) { throw new \CRM_Core_Exception($result['error_message']); } \Civi::log()->info('Successfully sent queued induction email', [ - 'params' => $params, + 'volunteerId' => $params['contact_id'], + 'templateId' => $params['template_id'], ]); return TRUE; } catch (\Exception $ex) { \Civi::log()->error('Failed to send queued induction email', [ - 'params' => $params, + 'volunteerId' => $params['contact_id'], + 'templateId' => $params['template_id'], 'error' => $ex->getMessage(), + 'trace' => $ex->getTraceAsString(), ]); - // Rethrow the exception for the queue system to handle. - throw $ex; + return FALSE; } }This way, we're logging more specific information about which email failed to send, and we're handling errors consistently by returning
FALSEinstead of rethrowing the exception. It's like upgrading from a carrier pigeon to a GPS-tracked delivery drone!
Line range hint
386-397: This function got a fancy new name tag!Renaming
emailAlreadySenttoisEmailAlreadySentis like giving your function a clear job title. It's now obvious at a glance what this function does. Good job on improving clarity!However, I can't help but notice that we're being a bit vague about what constitutes a "sent" email. We're looking for an email with a subject "LIKE '%Volunteering with Goonj%'", which is about as precise as saying "I'm looking for a needle in a haystack that looks kind of needle-ish".
Let's add a comment to explain this logic:
private static function isEmailAlreadySent($contactId) { + // We consider an email as "already sent" if there's an email activity + // with a subject containing "Volunteering with Goonj" for this contact. + // This might need to be adjusted if the email subject changes in the future. $volunteerEmailActivity = Activity::get(FALSE) ->addWhere('activity_type_id:name', '=', 'Email') ->addWhere('subject', 'LIKE', '%Volunteering with Goonj%') ->addWhere('target_contact_id', '=', $contactId) ->setLimit(1) ->execute(); return $volunteerEmailActivity->count() > 0; }This way, future developers (including future you) will understand why we're matching the subject this way and know to update it if the email subject ever changes. It's like leaving a trail of breadcrumbs for your future self!
Line range hint
309-337: This function just got a memory upgrade!Adding
$volunteerInductionAssigneeEmailas a static variable is like giving your class a new pocket to keep important information in. It's a smart move that allows you to use this email address in other parts of the class without having to fetch it again. Efficiency for the win!However, I can't help but notice that we're being a bit optimistic here. We're assuming that every assignee has an email address, which is like assuming every superhero has a cape. What if they don't?
Let's add some error handling to cover our bases:
$assignee = Contact::get(FALSE) ->addSelect('email.email') ->addJoin('Email AS email', 'LEFT') ->addWhere('id', '=', $inductionFields['Assign']['value']) ->addWhere('email.is_primary', '=', TRUE) ->setLimit(1) ->execute()->single(); - self::$volunteerInductionAssigneeEmail = $assignee['email.email']; + if (isset($assignee['email.email'])) { + self::$volunteerInductionAssigneeEmail = $assignee['email.email']; + } else { + \Civi::log()->warning('Induction assignee has no primary email', [ + 'assigneeId' => $inductionFields['Assign']['value'], + ]); + self::$volunteerInductionAssigneeEmail = null; + }This way, if we encounter a caped crusader without an email address, we'll log a warning and set the email to null. It's like having a backup plan for when your superhero communication device fails!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (1)
Line range hint
175-200: Oh boy, this function is trying to be a jack of all trades!Our dear
sendInductionEmailfunction seems to have caught a bad case of responsibility overload. It's checking for existing emails, fetching templates, preparing parameters, and queuing emails all in one go! This violates the Single Responsibility Principle faster than you can say "spaghetti code". Let's break this function down into smaller, more focused functions to keep things tidy and maintainable.Also, I see you've sneakily changed
$contactIdto$volunteerId. While this improves clarity, it might cause a ripple effect in other parts of the codebase. Make sure to update all callers of this function!On the bright side, queuing the email instead of sending it directly is a smart move! It's like putting your emails on a conveyor belt instead of hand-delivering each one. This should help with system performance and reliability.
Here's a script to check for any lingering
$contactIdusage:
2 participants
Summary by CodeRabbit
New Features
Bug Fixes