added cron jobs and attendee feedback form dynamic form link#540
added cron jobs and attendee feedback form dynamic form link#540nishant22029 wants to merge 16 commits intodevelopfrom
Conversation
WalkthroughThe changes in this pull request enhance 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 (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjActivitiesCron.php (1)
83-86: Use Appropriate Logging Level for Exception HandlingExceptions are currently logged using
\Civi::log()->info. To ensure errors are properly flagged and monitored, consider using\Civi::log()->errorwhen logging exceptions.wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackGoonjActivityCron.php (1)
113-129: Consider Utilizing a Templating System for Email ContentThe function
goonjcustom_volunteer_feedback_collection_activity_email_htmlconstructs HTML email content within the code. For better readability and maintainability, consider using a templating system or moving the HTML content to a separate template file.wp-content/plugins/goonj-blocks/goonj-blocks.php (1)
95-95: Ensure Consistent Naming ConventionsThe field name
'Goonj_Activities.Select_Attendee_feedback_form'uses inconsistent casing (feedback_formvs.Feedback_Form). For consistency and clarity, consider renaming it to'Goonj_Activities.Select_Attendee_Feedback_Form'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
wp-content/civi-extensions/goonjcustom/Civi/GoonjActivitiesService.php(3 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjActivitiesCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackGoonjActivityCron.php(1 hunks)wp-content/plugins/goonj-blocks/build/render.php(1 hunks)wp-content/plugins/goonj-blocks/goonj-blocks.php(1 hunks)wp-content/plugins/goonj-blocks/src/render.php(1 hunks)
🔇 Additional comments (5)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackGoonjActivityCron.php (2)
38-43: Refactor Duplicated Code for Retrieving Option Values
The code from lines 38-43 duplicates the option value retrieval logic found in GoonjActivitiesCron.php. Refactoring this into a shared method will promote code reuse and comply with the DRY principle.
102-104: Use Appropriate Logging Level for Exception Handling
Similar to the previous file, exceptions are logged using \Civi::log()->info. Utilize \Civi::log()->error to log exceptions appropriately and improve error monitoring.
wp-content/plugins/goonj-blocks/src/render.php (1)
109-111: Add Null Checks to Prevent Undefined Index Errors
As mentioned in build/render.php, ensure that $action_target['Goonj_Activities.Select_Attendee_feedback_form'] is set before using it to avoid undefined index errors.
wp-content/civi-extensions/goonjcustom/Civi/GoonjActivitiesService.php (2)
375-376: LGTM: Tab configuration update.
The module and directive names have been updated correctly for the 'campOutcome' tab.
647-653: LGTM: Email sender configuration.
The implementation correctly retrieves and caches the default sender's email address.
|
|
||
| $optionValues = OptionValue::get(FALSE) | ||
| ->addWhere('option_group_id:name', '=', 'eck_sub_types') | ||
| ->addWhere('name', '=', 'Goonj_activities') | ||
| ->addWhere('grouping', '=', 'Collection_Camp') | ||
| ->setLimit(1) | ||
| ->execute()->single(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Duplicated Code for Retrieving Option Values
The code segment from lines 38-44 duplicates logic that's also present in VolunteerFeedbackGoonjActivityCron.php. Extract this common functionality into a shared method to adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability.
| '%s#?Eck_Collection_Camp1=%s', | ||
| $action_target['Goonj_Activities.Select_Attendee_feedback_form'], | ||
| $action_target['id'] |
There was a problem hiding this comment.
Add Null Checks to Prevent Undefined Index Errors
The code assumes that $action_target['Goonj_Activities.Select_Attendee_feedback_form'] exists. To prevent potential undefined index errors, add a check to ensure this key is set before using it.
Apply the following diff to address the issue:
+if (isset($action_target['Goonj_Activities.Select_Attendee_feedback_form'])) {
$attendee_activity_feedback_link = sprintf(
'%s#?Eck_Collection_Camp1=%s',
$action_target['Goonj_Activities.Select_Attendee_feedback_form'],
$action_target['id']
);
+} else {
+ $attendee_activity_feedback_link = '#';
+}Committable suggestion skipped: line range outside the PR's diff.
| const GOONJ_ACTIVITIES_INTENT_FB_NAME = 'afformGoonjActivitiesIndividualIntentForm'; | ||
| const RELATIONSHIP_TYPE_NAME = 'Goonj Activities Coordinator of'; | ||
| private static $goonjActivitiesAddress = NULL; | ||
| private static $fromAddress = NULL; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving email-related functionality to a separate service class.
The class is handling multiple responsibilities (QR codes, activities, tabs, and now emails), which violates the Single Responsibility Principle. Consider extracting email-related functionality into a dedicated GoonjEmailService class.
| private static function getLogisticsEmailHtml($contactName, $collectionCampId, $campAttendedById, $collectionCampGoonjOffice, $campCode, $campAddress, $outcomeFormLink) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
| // Construct the full URLs for the forms. | ||
| $campOutcomeFormUrl = $homeUrl . $outcomeFormLink . '#?Eck_Collection_Camp1=' . $collectionCampId . '&Camp_Outcome.Filled_By=' . $campAttendedById; | ||
| \Civi::log()->info('campOutcomeFormUrl', ['campOutcomeFormUrl'=>$campOutcomeFormUrl]); | ||
| $html = " | ||
| <p>Dear $contactName,</p> | ||
| <p>Thank you for attending the goonj activity <strong>$campCode</strong> at <strong>$campAddress</strong>. Their is one forms that require your attention during and after the goonj activity:</p> | ||
| <ol> | ||
| Please complete this form from the goonj activity location once the goonj activity ends.</li> | ||
| <li><a href=\"$campOutcomeFormUrl\">Goonj Activity Outcome Form</a><br> | ||
| This feedback form should be filled out after the goonj activity/session ends, once you have an overview of the event's outcomes.</li> | ||
| </ol> | ||
| <p>We appreciate your cooperation.</p> | ||
| <p>Warm Regards,<br>Urban Relations Team</p>"; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix HTML content and improve email template.
Issues in the email template:
- Typo: "Their" should be "There"
- Malformed HTML list with a closing
</li>without an opening tag - Hardcoded strings should be moved to language files for i18n
Apply this diff to fix the issues:
private static function getLogisticsEmailHtml($contactName, $collectionCampId, $campAttendedById, $collectionCampGoonjOffice, $campCode, $campAddress, $outcomeFormLink) {
$homeUrl = \CRM_Utils_System::baseCMSURL();
// Construct the full URLs for the forms.
$campOutcomeFormUrl = $homeUrl . $outcomeFormLink . '#?Eck_Collection_Camp1=' . $collectionCampId . '&Camp_Outcome.Filled_By=' . $campAttendedById;
- \Civi::log()->info('campOutcomeFormUrl', ['campOutcomeFormUrl'=>$campOutcomeFormUrl]);
$html = "
<p>Dear $contactName,</p>
- <p>Thank you for attending the goonj activity <strong>$campCode</strong> at <strong>$campAddress</strong>. Their is one forms that require your attention during and after the goonj activity:</p>
+ <p>Thank you for attending the goonj activity <strong>$campCode</strong> at <strong>$campAddress</strong>. There is one form that requires your attention during and after the goonj activity:</p>
<ol>
- Please complete this form from the goonj activity location once the goonj activity ends.</li>
<li><a href=\"$campOutcomeFormUrl\">Goonj Activity Outcome Form</a><br>
This feedback form should be filled out after the goonj activity/session ends, once you have an overview of the event's outcomes.</li>
</ol>
<p>We appreciate your cooperation.</p>
<p>Warm Regards,<br>Urban Relations Team</p>";
return $html;
}Committable suggestion skipped: line range outside the PR's diff.
| public static function sendActivityLogisticsEmail($collectionCamp) { | ||
| try { | ||
| $campId = $collectionCamp['id']; | ||
| $activityCode = $collectionCamp['title']; | ||
| $activityOffice = $collectionCamp['Goonj_Activities.Goonj_Office']; | ||
| $activityAddress = $collectionCamp['Goonj_Activities.Where_do_you_wish_to_organise_the_activity_']; | ||
| $activityAttendedById = $collectionCamp['Logistics_Coordination.Camp_to_be_attended_by']; | ||
| $logisticEmailSent = $collectionCamp['Logistics_Coordination.Email_Sent']; | ||
| $outcomeFormLink = $collectionCamp['Goonj_Activities.Select_Goonj_POC_Attendee_Outcome_Form']; | ||
|
|
||
| $startDate = new \DateTime($collectionCamp['Goonj_Activities.Start_Date']); | ||
|
|
||
|
|
||
| $today = new \DateTimeImmutable(); | ||
| $endOfToday = $today->setTime(23, 59, 59); | ||
|
|
||
|
|
||
| if (true) { | ||
| $campAttendedBy = Contact::get(FALSE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $activityAttendedById) | ||
| ->execute()->single(); | ||
|
|
||
| $attendeeEmail = $campAttendedBy['email.email']; | ||
| $attendeeName = $campAttendedBy['display_name']; | ||
|
|
||
| if (!$attendeeEmail) { | ||
| throw new \Exception('Attendee email missing'); | ||
| } | ||
|
|
||
| $mailParams = [ | ||
| 'subject' => 'Goonj Activity Notification: ' . $activityCode . ' at ' . $activityAddress, | ||
| 'from' => self::getFromAddress(), | ||
| 'toEmail' => $attendeeEmail, | ||
| 'replyTo' => self::getFromAddress(), | ||
| 'html' => self::getLogisticsEmailHtml($attendeeName, $campId, $activityAttendedById, $activityOffice, $activityCode, $activityAddress, $outcomeFormLink), | ||
| ]; | ||
|
|
||
| $emailSendResult = \CRM_Utils_Mail::send($mailParams); | ||
|
|
||
| if ($emailSendResult) { | ||
| \Civi::log()->info("Logistics email sent for collection camp: $campId"); | ||
| EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Logistics_Coordination.Email_Sent', 1) | ||
| ->addWhere('id', '=', $campId) | ||
| ->execute(); | ||
| } | ||
| } | ||
| } | ||
| catch (\Exception $e) { | ||
| \Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage()); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Critical: Improve error handling and add input validation.
Several issues need attention:
- The condition
if (true)at line 605 seems incorrect - Missing validation for required fields before processing
- Exception handling could be more specific
Apply this diff to improve the implementation:
public static function sendActivityLogisticsEmail($collectionCamp) {
+ if (empty($collectionCamp['id'])) {
+ throw new \InvalidArgumentException('Collection camp ID is required');
+ }
+
try {
$campId = $collectionCamp['id'];
+ // Validate required fields
+ $requiredFields = [
+ 'title' => 'Activity code',
+ 'Goonj_Activities.Goonj_Office' => 'Activity office',
+ 'Goonj_Activities.Where_do_you_wish_to_organise_the_activity_' => 'Activity address',
+ 'Logistics_Coordination.Camp_to_be_attended_by' => 'Camp attendee',
+ ];
+
+ foreach ($requiredFields as $field => $label) {
+ if (empty($collectionCamp[$field])) {
+ throw new \InvalidArgumentException("$label is required");
+ }
+ }
+
$activityCode = $collectionCamp['title'];
$activityOffice = $collectionCamp['Goonj_Activities.Goonj_Office'];
$activityAddress = $collectionCamp['Goonj_Activities.Where_do_you_wish_to_organise_the_activity_'];
$activityAttendedById = $collectionCamp['Logistics_Coordination.Camp_to_be_attended_by'];
$logisticEmailSent = $collectionCamp['Logistics_Coordination.Email_Sent'];
$outcomeFormLink = $collectionCamp['Goonj_Activities.Select_Goonj_POC_Attendee_Outcome_Form'];
$startDate = new \DateTime($collectionCamp['Goonj_Activities.Start_Date']);
$today = new \DateTimeImmutable();
$endOfToday = $today->setTime(23, 59, 59);
- if (true) {
+ if (!$logisticEmailSent) {
$campAttendedBy = Contact::get(FALSE)
->addSelect('email.email', 'display_name')
->addJoin('Email AS email', 'LEFT')
->addWhere('id', '=', $activityAttendedById)
->execute()->single();
$attendeeEmail = $campAttendedBy['email.email'];
$attendeeName = $campAttendedBy['display_name'];
if (!$attendeeEmail) {
throw new \Exception('Attendee email missing');
}
$mailParams = [
'subject' => 'Goonj Activity Notification: ' . $activityCode . ' at ' . $activityAddress,
'from' => self::getFromAddress(),
'toEmail' => $attendeeEmail,
'replyTo' => self::getFromAddress(),
'html' => self::getLogisticsEmailHtml($attendeeName, $campId, $activityAttendedById, $activityOffice, $activityCode, $activityAddress, $outcomeFormLink),
];
$emailSendResult = \CRM_Utils_Mail::send($mailParams);
if ($emailSendResult) {
\Civi::log()->info("Logistics email sent for collection camp: $campId");
EckEntity::update('Collection_Camp', FALSE)
->addValue('Logistics_Coordination.Email_Sent', 1)
->addWhere('id', '=', $campId)
->execute();
}
}
}
- catch (\Exception $e) {
+ catch (\InvalidArgumentException $e) {
+ \Civi::log()->error("Invalid input in sendLogisticsEmail for $campId: " . $e->getMessage());
+ throw $e;
+ }
+ catch (\Exception $e) {
\Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage());
+ throw $e;
}
}📝 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 sendActivityLogisticsEmail($collectionCamp) { | |
| try { | |
| $campId = $collectionCamp['id']; | |
| $activityCode = $collectionCamp['title']; | |
| $activityOffice = $collectionCamp['Goonj_Activities.Goonj_Office']; | |
| $activityAddress = $collectionCamp['Goonj_Activities.Where_do_you_wish_to_organise_the_activity_']; | |
| $activityAttendedById = $collectionCamp['Logistics_Coordination.Camp_to_be_attended_by']; | |
| $logisticEmailSent = $collectionCamp['Logistics_Coordination.Email_Sent']; | |
| $outcomeFormLink = $collectionCamp['Goonj_Activities.Select_Goonj_POC_Attendee_Outcome_Form']; | |
| $startDate = new \DateTime($collectionCamp['Goonj_Activities.Start_Date']); | |
| $today = new \DateTimeImmutable(); | |
| $endOfToday = $today->setTime(23, 59, 59); | |
| if (true) { | |
| $campAttendedBy = Contact::get(FALSE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $activityAttendedById) | |
| ->execute()->single(); | |
| $attendeeEmail = $campAttendedBy['email.email']; | |
| $attendeeName = $campAttendedBy['display_name']; | |
| if (!$attendeeEmail) { | |
| throw new \Exception('Attendee email missing'); | |
| } | |
| $mailParams = [ | |
| 'subject' => 'Goonj Activity Notification: ' . $activityCode . ' at ' . $activityAddress, | |
| 'from' => self::getFromAddress(), | |
| 'toEmail' => $attendeeEmail, | |
| 'replyTo' => self::getFromAddress(), | |
| 'html' => self::getLogisticsEmailHtml($attendeeName, $campId, $activityAttendedById, $activityOffice, $activityCode, $activityAddress, $outcomeFormLink), | |
| ]; | |
| $emailSendResult = \CRM_Utils_Mail::send($mailParams); | |
| if ($emailSendResult) { | |
| \Civi::log()->info("Logistics email sent for collection camp: $campId"); | |
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addValue('Logistics_Coordination.Email_Sent', 1) | |
| ->addWhere('id', '=', $campId) | |
| ->execute(); | |
| } | |
| } | |
| } | |
| catch (\Exception $e) { | |
| \Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage()); | |
| } | |
| } | |
| public static function sendActivityLogisticsEmail($collectionCamp) { | |
| if (empty($collectionCamp['id'])) { | |
| throw new \InvalidArgumentException('Collection camp ID is required'); | |
| } | |
| try { | |
| $campId = $collectionCamp['id']; | |
| // Validate required fields | |
| $requiredFields = [ | |
| 'title' => 'Activity code', | |
| 'Goonj_Activities.Goonj_Office' => 'Activity office', | |
| 'Goonj_Activities.Where_do_you_wish_to_organise_the_activity_' => 'Activity address', | |
| 'Logistics_Coordination.Camp_to_be_attended_by' => 'Camp attendee', | |
| ]; | |
| foreach ($requiredFields as $field => $label) { | |
| if (empty($collectionCamp[$field])) { | |
| throw new \InvalidArgumentException("$label is required"); | |
| } | |
| } | |
| $activityCode = $collectionCamp['title']; | |
| $activityOffice = $collectionCamp['Goonj_Activities.Goonj_Office']; | |
| $activityAddress = $collectionCamp['Goonj_Activities.Where_do_you_wish_to_organise_the_activity_']; | |
| $activityAttendedById = $collectionCamp['Logistics_Coordination.Camp_to_be_attended_by']; | |
| $logisticEmailSent = $collectionCamp['Logistics_Coordination.Email_Sent']; | |
| $outcomeFormLink = $collectionCamp['Goonj_Activities.Select_Goonj_POC_Attendee_Outcome_Form']; | |
| $startDate = new \DateTime($collectionCamp['Goonj_Activities.Start_Date']); | |
| $today = new \DateTimeImmutable(); | |
| $endOfToday = $today->setTime(23, 59, 59); | |
| if (!$logisticEmailSent) { | |
| $campAttendedBy = Contact::get(FALSE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $activityAttendedById) | |
| ->execute()->single(); | |
| $attendeeEmail = $campAttendedBy['email.email']; | |
| $attendeeName = $campAttendedBy['display_name']; | |
| if (!$attendeeEmail) { | |
| throw new \Exception('Attendee email missing'); | |
| } | |
| $mailParams = [ | |
| 'subject' => 'Goonj Activity Notification: ' . $activityCode . ' at ' . $activityAddress, | |
| 'from' => self::getFromAddress(), | |
| 'toEmail' => $attendeeEmail, | |
| 'replyTo' => self::getFromAddress(), | |
| 'html' => self::getLogisticsEmailHtml($attendeeName, $campId, $activityAttendedById, $activityOffice, $activityCode, $activityAddress, $outcomeFormLink), | |
| ]; | |
| $emailSendResult = \CRM_Utils_Mail::send($mailParams); | |
| if ($emailSendResult) { | |
| \Civi::log()->info("Logistics email sent for collection camp: $campId"); | |
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addValue('Logistics_Coordination.Email_Sent', 1) | |
| ->addWhere('id', '=', $campId) | |
| ->execute(); | |
| } | |
| } | |
| } | |
| catch (\InvalidArgumentException $e) { | |
| \Civi::log()->error("Invalid input in sendLogisticsEmail for $campId: " . $e->getMessage()); | |
| throw $e; | |
| } | |
| catch (\Exception $e) { | |
| \Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage()); | |
| throw $e; | |
| } | |
| } |
resolve conflict
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (21)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (3)
Line range hint
103-159: Consider refactoring repeated permissions array.The permissions array
['goonj_chapter_admin', 'urbanops']is duplicated across multiple tab configurations. This violates the DRY principle and makes maintenance harder.Consider extracting the common permissions into a class constant:
class CollectionCampService extends AutoSubscriber { + const DEFAULT_TAB_PERMISSIONS = ['goonj_chapter_admin', 'urbanops']; // ... other code ... public static function collectionCampTabset($tabsetName, &$tabs, $context) { // ... other code ... $tabConfigs = [ 'logistics' => [ 'title' => ts('Logistics'), 'module' => 'afsearchCollectionCampLogistics', 'directive' => 'afsearch-collection-camp-logistics', 'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl', - 'permissions' => ['goonj_chapter_admin', 'urbanops'], + 'permissions' => self::DEFAULT_TAB_PERMISSIONS, ], // Apply similar changes to other tabs ];
Line range hint
1277-1309: Refactor email template handling to reduce duplication.The email content is duplicated between online and offline receipt workflows with minimal differences. Additionally, HTML templates are hardcoded in the method.
Consider these improvements:
- Extract email templates to separate template files
- Create a unified template generation method:
class CollectionCampService extends AutoSubscriber { + private static function getEmailTemplate($donorName, $isOffline = false) { + $type = $isOffline ? 'offline' : 'online'; + return [ + 'text' => "Dear {$donorName},\n\nThank you for your {$type} contribution...", + 'html' => self::renderEmailTemplate('contribution_receipt', [ + 'donorName' => $donorName, + 'isOffline' => $isOffline, + ]), + ]; + } public static function alterReceiptMail(&$params, $context) { if (empty($params['workflow'])) { return; } $isOffline = $params['workflow'] === 'contribution_offline_receipt'; $donorName = $isOffline ? (!empty($params['toName']) ? $params['toName'] : 'Valued Supporter') : (!empty($params['tplParams']['displayName']) ? $params['tplParams']['displayName'] : 'Valued Supporter'); $template = self::getEmailTemplate($donorName, $isOffline); $params['text'] = $template['text']; $params['html'] = $template['html']; }
Line range hint
1277-1293: Add parameter validation for the email workflow.The method assumes the presence of required parameters without validation.
Add validation for required parameters:
public static function alterReceiptMail(&$params, $context) { + if (empty($params['workflow']) || !in_array($params['workflow'], [ + 'contribution_online_receipt', + 'contribution_offline_receipt' + ])) { + return; + }wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (3)
261-270: Extract duplicate relationship query logicThe relationship query logic is duplicated. Consider extracting it into a private method.
Create a helper method:
+ private static function findRelationship(int $organizationId, string $relationshipType): ?array { + return Relationship::get(FALSE) + ->addWhere('contact_id_a', '=', $organizationId) + ->addWhere('relationship_type_id:name', '=', $relationshipType) + ->execute(); + }Then simplify the code:
- $relationships = Relationship::get(FALSE) - ->addWhere('contact_id_a', '=', $organizationId) - ->addWhere('relationship_type_id:name', '=', $relationshipType) - ->execute(); - if (empty($relationships)) { - $relationships = Relationship::get(FALSE) - ->addWhere('contact_id_a', '=', $organizationId) - ->addWhere('relationship_type_id:name', '=', $alternateType) - ->execute(); - } + $relationships = self::findRelationship($organizationId, $relationshipType) + ?: self::findRelationship($organizationId, $alternateType);
247-259: Simplify control flow using early returnsThe nested if-else structure can be simplified using early returns and guard clauses.
Consider this refactor:
- if ($subtypeName === 'Institution_Collection_Camp') { - $organizationId = $collectionCamp['Institution_Collection_Camp_Intent.Organization_Name.id']; - $relationshipType = 'Institution POC of'; - $alternateType = 'Primary Institution POC of'; - } - elseif ($subtypeName === 'Institution_Dropping_Center') { - $organizationId = $collectionCamp['Institution_Dropping_Center_Intent.Organization_Name.id']; - $relationshipType = 'Institution POC of'; - $alternateType = 'Secondary Institution POC of'; - } - else { - return $collectionCamp['Collection_Camp_Core_Details.Contact_Id']; - } + if ($subtypeName !== 'Institution_Collection_Camp' && + $subtypeName !== 'Institution_Dropping_Center') { + return $collectionCamp['Collection_Camp_Core_Details.Contact_Id']; + } + + $isCollectionCamp = $subtypeName === 'Institution_Collection_Camp'; + $organizationId = $collectionCamp[ + $isCollectionCamp + ? 'Institution_Collection_Camp_Intent.Organization_Name.id' + : 'Institution_Dropping_Center_Intent.Organization_Name.id' + ]; + $alternateType = $isCollectionCamp + ? self::RELATIONSHIP_TYPE_PRIMARY_POC + : self::RELATIONSHIP_TYPE_SECONDARY_POC;
248-248: Define field names as class constantsField names are hardcoded throughout the method. These should be defined as class constants to improve maintainability and prevent typos.
Add these constants at the class level:
+ private const FIELD_ORGANIZATION_NAME_COLLECTION = 'Institution_Collection_Camp_Intent.Organization_Name.id'; + private const FIELD_ORGANIZATION_NAME_DROPPING = 'Institution_Dropping_Center_Intent.Organization_Name.id'; + private const FIELD_CONTACT_ID = 'Collection_Camp_Core_Details.Contact_Id';Then use them in the code:
- $organizationId = $collectionCamp['Institution_Collection_Camp_Intent.Organization_Name.id']; + $organizationId = $collectionCamp[self::FIELD_ORGANIZATION_NAME_COLLECTION];Also applies to: 253-253, 258-258
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (4)
Line range hint
435-512: Consider extracting common permissions to a constant.The permission array
['goonj_chapter_admin', 'urbanops']is duplicated across multiple tab configurations. This violates the DRY principle and makes permission updates error-prone.Consider creating a class constant for the common permissions:
class DroppingCenterService extends AutoSubscriber { + const DEFAULT_TAB_PERMISSIONS = ['goonj_chapter_admin', 'urbanops']; // ... public static function droppingCenterTabset($tabsetName, &$tabs, $context) { $tabConfigs = [ 'logistics' => [ // ... - 'permissions' => ['goonj_chapter_admin', 'urbanops'], + 'permissions' => self::DEFAULT_TAB_PERMISSIONS, ], // Apply similar changes to other tabs ]; } }
Line range hint
271-284: Improve email handling with a dedicated email service.The email handling logic is scattered across multiple methods with similar patterns. This violates the Single Responsibility Principle and leads to code duplication.
Consider creating a dedicated EmailService class to handle all email-related functionality:
class EmailService { public function sendTemplatedEmail($template, $params, $recipient) { $mailParams = [ 'subject' => $params['subject'], 'from' => HelperService::getDefaultFromEmail(), 'toEmail' => $recipient, 'html' => $this->renderTemplate($template, $params), ]; try { return \CRM_Utils_Mail::send($mailParams); } catch (\Exception $e) { \CRM_Core_Error::debug_log_message('Failed to send email: ' . $e->getMessage()); return false; } } }Also applies to: 386-401
Line range hint
289-379: Refactor setOfficeDetails for better separation of concerns.The setOfficeDetails method is handling too many responsibilities: state field validation, office lookup, coordinator assignment, and entity updates. This makes the code harder to maintain and test.
Consider breaking down the method into smaller, focused methods:
private static function validateStateField($params) { $stateField = self::findStateField($params); if (!$stateField || !$stateField['value']) { throw new \CRM_Core_Exception('Invalid state field'); } return $stateField; } private static function findOfficeForState($stateId) { $office = Contact::get(FALSE) ->addSelect('id') ->addWhere('contact_type', '=', 'Organization') ->addWhere('contact_sub_type', 'CONTAINS', 'Goonj_Office') ->addWhere('Goonj_Office_Details.Collection_Camp_Catchment', 'CONTAINS', $stateId) ->execute() ->first(); return $office ?: self::getFallbackOffice(); }
Line range hint
1-24: Improve class documentation and error handling strategy.The class lacks comprehensive documentation about its purpose and responsibilities. Additionally, error handling is inconsistent across methods, with some using debug logs and others silently failing.
Consider adding proper class documentation and standardizing error handling:
/** + * DroppingCenterService handles the management of dropping centers in the Goonj system. + * + * This service is responsible for: + * - Managing dropping center tabs and permissions + * - Handling email notifications for material dispatch + * - Managing office assignments and coordinator relationships + * + * @package Civi\Goonjcustom + */ class DroppingCenterService extends AutoSubscriber { + /** + * Custom exception for dropping center related errors. + */ + class DroppingCenterException extends \CRM_Core_Exception {} use QrCodeable; use CollectionSource;wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackGoonjActivityCron.php (2)
70-70: Remove unnecessary logging or adjust logging levelThe logging statement for
$volunteerFeedbackFormmay not be necessary in production code and could clutter the logs. If it's used for debugging, consider removing it or adjusting the logging level accordingly.Apply this diff to remove the logging statement:
- \Civi::log()->info('volunteerFeedbackForm', ['volunteerFeedbackForm'=>$volunteerFeedbackForm]);
103-103: Adjust exception logging level for better error trackingCurrently, exceptions are logged with an
infolevel, which may not adequately highlight errors. Use theerrorlevel to ensure that issues are appropriately flagged and can be monitored effectively.Apply this diff to adjust the logging level:
- \Civi::log()->info("Error processing camp ID $collectionCampId: " . $e->getMessage()); + \Civi::log()->error("Error processing camp ID $collectionCampId: " . $e->getMessage());wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Token/InstitutionDroppingCenter.php (1)
43-47: Improve handling when 'collectionSourceId' is absentIf 'collectionSourceId' is missing in the context (line 43~), the method logs a debug message and returns an empty token. Consider providing a more informative error message or throwing an exception to aid in debugging and ensure robustness.
wp-content/civi-extensions/goonjcustom/goonjcustom.php (1)
148-165: Simplify the logic for setting 'inductionDate' and 'inductionTime'The code sets 'inductionDate' and 'inductionTime' to 'Not Scheduled' in two places (lines 148~ and 162~). This redundancy can be avoided for cleaner code.
Consider initializing $inductionDate and $inductionTime to 'Not Scheduled' before the if-condition, and updating them only if valid data is available.
Apply this diff to refactor the code:
$inductionDate = 'Not Scheduled'; $inductionTime = 'Not Scheduled'; if ($inductionDateTimeString !== 'Not Scheduled') { $dateTime = new DateTime($inductionDateTimeString); $inductionDate = $dateTime->format('Y-m-d'); $inductionTime = $dateTime->format('g:i A'); }wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (4)
524-533: Refactor to eliminate duplicate code when checking contact existenceThe pattern of fetching contact details and checking if the contact exists (lines 529~ to 533~) is repeated multiple times. Consider extracting this logic into a helper method to improve code reuse and readability.
659-668: Refactor duplicate contact existence checksSimilar to earlier code, the check for empty contacts (lines 666~ to 668~) duplicates code from other methods. Refactoring this into a reusable method can enhance maintainability and reduce code duplication.
713-724: Handle missing contacts consistently and log appropriatelyIn
handleRescheduleEmailActivity(lines 721~ to 724~), returning FALSE when a contact is not found may not be sufficient. Ensure consistent error handling across methods and consider logging this event to aid in debugging.
775-784: Improve error handling when contacts are not foundIn the
sendRemainderEmailsmethod (lines 782~ to 784~), the function returns FALSE if contacts are empty. Consistent error handling and logging of such events will improve debuggability and maintain consistency across methods.wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (3)
Line range hint
775-824: Refactor duplicated permissions arrayThe permissions array
['goonj_chapter_admin', 'urbanops']is duplicated across multiple tab configurations. Consider extracting it into a constant or variable to improve maintainability.+ private const DEFAULT_PERMISSIONS = ['goonj_chapter_admin', 'urbanops']; $tabConfigs = [ 'logistics' => [ 'title' => ts('Logistics'), 'module' => 'afsearchInstitutionCollectionCampLogistics', 'directive' => 'afsearch-institution-collection-camp-logistics', 'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl', - 'permissions' => ['goonj_chapter_admin', 'urbanops'], + 'permissions' => self::DEFAULT_PERMISSIONS, ], // Apply similar changes to other tab configurations
834-836: Remove or improve the unclear commentThe comment "does not permission; just continue" is grammatically incorrect and doesn't add value. Consider removing it as the code is self-explanatory.
if (!\CRM_Core_Permission::checkAnyPerm($config['permissions'])) { - // does not permission; just continue continue; }
Line range hint
1-850: Consider splitting the class to improve maintainabilityThe
InstitutionCollectionCampServiceclass has grown to handle multiple responsibilities:
- Email notifications
- Permission management
- Tab configurations
- Coordinator assignments
- Camp outcome tracking
Consider splitting these responsibilities into separate service classes to improve maintainability and adhere to the Single Responsibility Principle:
InstitutionCollectionCampEmailServiceInstitutionCollectionCampPermissionServiceInstitutionCollectionCampTabServiceInstitutionCollectionCampCoordinatorServiceInstitutionCollectionCampOutcomeService
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Token/InstitutionDroppingCenter.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(5 hunks)wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php(3 hunks)wp-content/civi-extensions/goonjcustom/Civi/GoonjActivitiesService.php(5 hunks)wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(4 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(6 hunks)wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php(2 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjActivitiesCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackGoonjActivityCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/goonjcustom.php(4 hunks)wp-content/plugins/goonj-blocks/build/render.php(1 hunks)wp-content/plugins/goonj-blocks/src/render.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- wp-content/plugins/goonj-blocks/src/render.php
- wp-content/plugins/goonj-blocks/build/render.php
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjActivitiesCron.php
🔇 Additional comments (6)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (1)
8-8: LGTM: Import statement is correctly placed
The Relationship API import is properly grouped with other Civi\Api4 imports and is necessary for the new functionality.
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (1)
522-523: Verify permission check impact.
The change to use checkAnyPerm instead of individual permission checks could potentially grant access to users who previously didn't have it.
✅ Verification successful
Change to checkAnyPerm is appropriate and secure.
The switch from individual permission checks to checkAnyPerm for the array ['goonj_chapter_admin', 'urbanops'] is a valid and secure change because:
- The original code was checking for the same set of permissions across multiple tabs
- The permissions are consistently defined as an array requiring either 'goonj_chapter_admin' OR 'urbanops' access
checkAnyPermis the correct method to use when any one of multiple permissions should grant access- The change maintains the same access control logic while making it more efficient
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other permission checks in the codebase that might be affected
rg -A 2 "CRM_Core_Permission::check.*(?:goonj_chapter_admin|urbanops)"
# Check for existing roles and their permissions
ast-grep --pattern 'function install() {
$$$
CRM_Core_Permission::add($$$);
$$$
}'
Length of output: 9435
Script:
#!/bin/bash
# Let's try a different approach to find permission checks
rg -A 2 "CRM_Core_Permission::check"
# Also search for permission definitions
rg -A 2 "CRM_Core_Permission::add"
# Look for the specific file to understand the context better
fd -t f "DroppingCenterService.php" -x cat {}
Length of output: 71082
Script:
#!/bin/bash
# Let's look for the specific file to understand the permission configuration
fd -t f "DroppingCenterService.php" -x grep -A 5 -B 5 "permissions"
# Also search for any related permission configuration files
fd -t f "goonjcustom.php" -x cat {}
Length of output: 12373
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackGoonjActivityCron.php (1)
55-55: Review the need for date filtering in the query
The date filtering condition in the query is commented out. If filtering collection camps based on end date is required, consider uncommenting or updating the condition to ensure the cron job processes the correct set of camps.
Apply this diff to re-enable date filtering:
- // ->addWhere('Collection_Camp_Intent_Details.End_Date', '<=', $endOfDay)
+ ->addWhere('Goonj_Activities.End_Date', '<=', $endOfDay)Note: Ensure the field name used in the condition matches the correct data field.
wp-content/civi-extensions/goonjcustom/Civi/GoonjActivitiesService.php (3)
29-29: Consider adhering to the Single Responsibility Principle (SRP)
The GoonjActivitiesService class is handling multiple responsibilities (QR codes, activities, tabs, and emails), which violates the Single Responsibility Principle. Refactoring the email-related functionality into a dedicated GoonjEmailService class would improve maintainability and separation of concerns.
588-642:
Improve error handling and add input validation
Several issues in the sendActivityLogisticsEmail method need attention:
- The condition
if (true)at line 605~ seems incorrect and may be a placeholder. - Missing validation for required fields before processing.
- Exception handling could be more specific.
Apply this diff to enhance the implementation:
public static function sendActivityLogisticsEmail($collectionCamp) {
+ if (empty($collectionCamp['id'])) {
+ throw new \InvalidArgumentException('Collection camp ID is required');
+ }
+
try {
$campId = $collectionCamp['id'];
+ // Validate required fields
+ $requiredFields = [
+ 'title' => 'Activity code',
+ 'Goonj_Activities.Goonj_Office' => 'Activity office',
+ 'Goonj_Activities.Where_do_you_wish_to_organise_the_activity_' => 'Activity address',
+ 'Logistics_Coordination.Camp_to_be_attended_by' => 'Camp attendee',
+ ];
+
+ foreach ($requiredFields as $field => $label) {
+ if (empty($collectionCamp[$field])) {
+ throw new \InvalidArgumentException("$label is required");
+ }
+ }
+
$activityCode = $collectionCamp['title'];
$activityOffice = $collectionCamp['Goonj_Activities.Goonj_Office'];
$activityAddress = $collectionCamp['Goonj_Activities.Where_do_you_wish_to_organise_the_activity_'];
$activityAttendedById = $collectionCamp['Logistics_Coordination.Camp_to_be_attended_by'];
$logisticEmailSent = $collectionCamp['Logistics_Coordination.Email_Sent'];
$outcomeFormLink = $collectionCamp['Goonj_Activities.Select_Goonj_POC_Attendee_Outcome_Form'];
$startDate = new \DateTime($collectionCamp['Goonj_Activities.Start_Date']);
$today = new \DateTimeImmutable();
$endOfToday = $today->setTime(23, 59, 59);
- if (true) {
+ if (!$logisticEmailSent) {
// Existing code...
}
}
- catch (\Exception $e) {
+ catch (\InvalidArgumentException $e) {
+ \Civi::log()->error("Invalid input in sendLogisticsEmail for $campId: " . $e->getMessage());
+ throw $e;
+ }
+ catch (\Exception $e) {
\Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage());
+ throw $e;
}
}658-674: 🛠️ Refactor suggestion
Fix HTML content and improve the email template
Issues in the email template:
- Typo: "Their" should be "There" in line 666~.
- Malformed HTML list with a closing
</li>without an opening<li>at line 668~. - Hardcoded strings should be moved to language files for internationalization (i18n).
Apply this diff to correct the issues:
private static function getLogisticsEmailHtml($contactName, $collectionCampId, $campAttendedById, $collectionCampGoonjOffice, $campCode, $campAddress, $outcomeFormLink) {
// ... existing code ...
$html = "
<p>Dear $contactName,</p>
- <p>Thank you for attending the goonj activity <strong>$campCode</strong> at <strong>$campAddress</strong>. Their is one forms that require your attention during and after the goonj activity:</p>
+ <p>Thank you for attending the Goonj activity <strong>$campCode</strong> at <strong>$campAddress</strong>. There is one form that requires your attention during and after the Goonj activity:</p>
<ol>
- Please complete this form from the goonj activity location once the goonj activity ends.</li>
+ <li>Please complete this form from the Goonj activity location once the activity ends.</li>
<li><a href=\"$campOutcomeFormUrl\">Goonj Activity Outcome Form</a><br>
This feedback form should be filled out after the Goonj activity/session ends, once you have an overview of the event's outcomes.</li>
</ol>
<p>We appreciate your cooperation.</p>
<p>Warm Regards,<br>Urban Relations Team</p>";
return $html;
}| $relationshipType = 'Institution POC of'; | ||
| $alternateType = 'Primary Institution POC of'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define relationship types as class constants
Relationship type strings are hardcoded in multiple places. These should be defined as class constants for better maintainability and to avoid typos.
Add these constants at the class level:
+ private const RELATIONSHIP_TYPE_INSTITUTION_POC = 'Institution POC of';
+ private const RELATIONSHIP_TYPE_PRIMARY_POC = 'Primary Institution POC of';
+ private const RELATIONSHIP_TYPE_SECONDARY_POC = 'Secondary Institution POC of';Then use them in the code:
- $relationshipType = 'Institution POC of';
- $alternateType = 'Primary Institution POC of';
+ $relationshipType = self::RELATIONSHIP_TYPE_INSTITUTION_POC;
+ $alternateType = self::RELATIONSHIP_TYPE_PRIMARY_POC;Also applies to: 254-256
| private static function getInitiatorId(array $collectionCamp) { | ||
| $subtypeName = $collectionCamp['subtype:name']; |
There was a problem hiding this comment.
Add input validation to prevent undefined index errors
The method accepts an array parameter but doesn't validate the required keys before accessing them.
Add validation at the start of the method:
private static function getInitiatorId(array $collectionCamp) {
+ if (!isset($collectionCamp['subtype:name'])) {
+ throw new \InvalidArgumentException('Collection camp must have a subtype');
+ }
$subtypeName = $collectionCamp['subtype:name'];📝 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.
| private static function getInitiatorId(array $collectionCamp) { | |
| $subtypeName = $collectionCamp['subtype:name']; | |
| private static function getInitiatorId(array $collectionCamp) { | |
| if (!isset($collectionCamp['subtype:name'])) { | |
| throw new \InvalidArgumentException('Collection camp must have a subtype'); | |
| } | |
| $subtypeName = $collectionCamp['subtype:name']; |
| $endDate = new DateTime($camp['Goonj_Activities.End_Date']); | ||
| $collectionCampId = $camp['id']; | ||
| $endDateFormatted = $endDate->format('Y-m-d'); | ||
| $feedbackEmailSent = $camp['Logistics_Coordination.Feedback_Email_Sent']; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Utilize $feedbackEmailSent variable to prevent duplicate emails
The variable $feedbackEmailSent is retrieved but not used. Incorporate it into your condition to avoid sending feedback emails multiple times to the same recipient.
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| // Send email if the end date is today or earlier. | ||
| if (true) { |
There was a problem hiding this comment.
Replace placeholder condition with actual logic to control email sending
The if (true) condition ensures that the email sending block always executes, which may lead to sending duplicate emails or emails sent at inappropriate times. Replace it with a meaningful condition, such as checking if the feedback email has not been sent and if the activity end date has passed.
Apply this diff to fix the condition:
- if (true) {
+ if ($endDateFormatted <= $todayFormatted && !$feedbackEmailSent) {Committable suggestion skipped: line range outside the PR's diff.
| function goonjcustom_volunteer_feedback_collection_activity_email_html($organizingContactName, $collectionCampId, $campAddress, $volunteerFeedbackForm ) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
|
|
||
| // URL for the volunteer feedback form. | ||
| // $campVolunteerFeedback = $homeUrl . $volunteerFeedbackForm . '#?Eck_Collection_Camp1=' . $collectionCampId; | ||
| $campVolunteerFeedback = $homeUrl . 'volunteer-activity-feedback/#?Eck_Collection_Camp1=' . $collectionCampId; | ||
|
|
||
| $html = " | ||
| <p>Dear $organizingContactName,</p> | ||
| <p>Thank you for stepping up and organising the recent goonj activity at <strong>$campAddress</strong>! Your time, effort, and enthusiasm made all the difference, and we hope that it was a meaningful effort for you as well.</p> | ||
| <p>To help us improve, we’d love to hear your thoughts and experiences. Kindly take a few minutes to fill out our feedback form. Your input will be valuable to us:</p> | ||
| <p><a href=\"$campVolunteerFeedback\">Feedback Form Link</a></p> | ||
| <p>Feel free to share any highlights, suggestions, or challenges you faced. We're eager to learn how we can make it better together!</p> | ||
| <p>We look forward to continuing this journey together!</p> | ||
| <p>Warm Regards,<br>Team Goonj</p>"; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused parameter or restore dynamic URL generation
The parameter $volunteerFeedbackForm is passed to the function but is not used because the code that utilizes it is commented out. This results in an unnecessary parameter and potential confusion. Consider either removing the parameter or uncommenting and using it for dynamic URL generation to avoid hardcoding the feedback form URL.
If you choose to remove the unused parameter, apply this diff:
-function goonjcustom_volunteer_feedback_collection_activity_email_html($organizingContactName, $collectionCampId, $campAddress, $volunteerFeedbackForm ) {
+function goonjcustom_volunteer_feedback_collection_activity_email_html($organizingContactName, $collectionCampId, $campAddress) {
...
- 'html' => goonjcustom_volunteer_feedback_collection_activity_email_html($organizingContactName, $collectionCampId, $campAddress, $volunteerFeedbackForm),
+ 'html' => goonjcustom_volunteer_feedback_collection_activity_email_html($organizingContactName, $collectionCampId, $campAddress),Alternatively, if the dynamic URL is required, uncomment and use the variable:
- // $campVolunteerFeedback = $homeUrl . $volunteerFeedbackForm . '#?Eck_Collection_Camp1=' . $collectionCampId;
- $campVolunteerFeedback = $homeUrl . 'volunteer-activity-feedback/#?Eck_Collection_Camp1=' . $collectionCampId;
+ $campVolunteerFeedback = $homeUrl . $volunteerFeedbackForm . '#?Eck_Collection_Camp1=' . $collectionCampId;Committable suggestion skipped: line range outside the PR's diff.
| switch ($field) { | ||
| case 'venue': | ||
| $value = $this->formatVenue($collectionSource); | ||
| break; | ||
|
|
||
| case 'date': | ||
|
|
||
| case 'time': | ||
| $value = $collectionSource['Institution_Dropping_Center_Intent.Timing']; | ||
| break; | ||
|
|
||
| case 'contact': | ||
| $value = $this->formatVolunteers($collectionSource); | ||
| break; | ||
|
|
||
| case 'remarks': | ||
| $value = $collectionSource['Collection_Camp_Core_Details.Remarks']; | ||
| break; | ||
|
|
||
| case 'coordinator': | ||
| $value = $this->formatCoordinator($collectionSource); | ||
| break; | ||
|
|
||
| case 'address_city': | ||
| $value = $collectionSource['Institution_Dropping_Center_Intent.District_City']; | ||
| break; | ||
|
|
||
| default: | ||
| $value = ''; | ||
| break; | ||
|
|
||
| } |
There was a problem hiding this comment.
Ensure proper formatting of 'date' and 'time' tokens
In the evaluateToken method, the cases for 'date' (line 63~) and 'time' (line 65~) retrieve raw timing data without formatting, which may lead to unformatted outputs.
Consider using the formatDate and formatTime methods to format these tokens, ensuring that date and time are presented correctly.
Apply this diff to adjust the cases:
case 'date':
-
- case 'time':
- $value = $collectionSource['Institution_Dropping_Center_Intent.Timing'];
+ $timing = $collectionSource['Institution_Dropping_Center_Intent.Timing'];
+ $start = new \DateTime($timing['start']);
+ $end = new \DateTime($timing['end']);
+ $value = $this->formatDate($start, $end);
+ break;
+
+ case 'time':
+ $timing = $collectionSource['Institution_Dropping_Center_Intent.Timing'];
+ $start = new \DateTime($timing['start']);
+ $end = new \DateTime($timing['end']);
+ $value = $this->formatTime($start, $end);
+ break;Committable suggestion skipped: line range outside the PR's diff.
| ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id', 'Institution_Collection_Camp_Intent.Organization_Name.id', 'Institution_Dropping_Center_Intent.Organization_Name.id', 'subtype:name') | ||
| ->addWhere('id', '=', $objectId) | ||
| ->execute()->single(); | ||
|
|
There was a problem hiding this comment.
Critical: 'getInitiatorId' method removed but still referenced
The method getInitiatorId has been removed, but it is still being called on line 286~. This will lead to a fatal error when the method is not found.
Reintroduce the getInitiatorId method or adjust the code to obtain the initiator ID correctly.
| ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id', 'Institution_Collection_Camp_Intent.Organization_Name.id', 'Institution_Dropping_Center_Intent.Organization_Name.id', 'subtype', 'subtype:name') | ||
| ->addWhere('id', '=', $objectRef->id) | ||
| ->execute()->single(); | ||
|
|
There was a problem hiding this comment.
Critical: 'getInitiatorId' method called but not defined
In the handleAuthorizationEmailsPost method, the call to self::getInitiatorId($collectionCamp); (line 314~) will fail because the getInitiatorId method has been removed.
Implement the getInitiatorId method or modify the code to retrieve the initiator ID using an alternative approach.
| $nameOfInstitution = $collectionSourceVehicleDispatch['Camp_Institution_Data.Name_of_the_institution']; | ||
| $addressOfInstitution = $collectionSourceVehicleDispatch['Camp_Institution_Data.Address']; | ||
| $pocEmail = $collectionSourceVehicleDispatch['Camp_Institution_Data.Email']; | ||
| $pocContactNumber = $collectionSourceVehicleDispatch['Camp_Institution_Data.Contact_Numbe']; |
There was a problem hiding this comment.
Fix typo in field name
There's a typo in the field name 'Contact_Numbe' which should be 'Contact_Number'.
- $pocContactNumber = $collectionSourceVehicleDispatch['Camp_Institution_Data.Contact_Numbe'];
+ $pocContactNumber = $collectionSourceVehicleDispatch['Camp_Institution_Data.Contact_Number'];📝 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.
| $pocContactNumber = $collectionSourceVehicleDispatch['Camp_Institution_Data.Contact_Numbe']; | |
| $pocContactNumber = $collectionSourceVehicleDispatch['Camp_Institution_Data.Contact_Number']; |
| $materialdispatchUrl = $homeUrl . 'institution-camp-acknowledgement-dispatch/#?Eck_Collection_Source_Vehicle_Dispatch1=' . $vehicleDispatchId | ||
| . '&Camp_Vehicle_Dispatch.Collection_Camp=' . $collectionCampId | ||
| . '&id=' . $vehicleDispatchId | ||
| . '&Eck_Collection_Source_Vehicle_Dispatch_Eck_Collection_Camp_Collection_Camp_01.id=' . $collectionCampId | ||
| . '&Camp_Institution_Data.Name_of_the_institution=' . $nameOfInstitution | ||
| . '&Camp_Institution_Data.Address=' . $addressOfInstitution | ||
| . '&Camp_Institution_Data.Email=' . $pocEmail | ||
| . '&Camp_Institution_Data.Contact_Number=' . $pocContactNumber; |
There was a problem hiding this comment.
URL-encode parameters to handle special characters
The URL parameters should be URL-encoded to handle special characters in institution names, addresses, and other fields.
- $materialdispatchUrl = $homeUrl . 'institution-camp-acknowledgement-dispatch/#?Eck_Collection_Source_Vehicle_Dispatch1=' . $vehicleDispatchId
- . '&Camp_Vehicle_Dispatch.Collection_Camp=' . $collectionCampId
- . '&id=' . $vehicleDispatchId
- . '&Eck_Collection_Source_Vehicle_Dispatch_Eck_Collection_Camp_Collection_Camp_01.id=' . $collectionCampId
- . '&Camp_Institution_Data.Name_of_the_institution=' . $nameOfInstitution
- . '&Camp_Institution_Data.Address=' . $addressOfInstitution
- . '&Camp_Institution_Data.Email=' . $pocEmail
- . '&Camp_Institution_Data.Contact_Number=' . $pocContactNumber;
+ $materialdispatchUrl = $homeUrl . 'institution-camp-acknowledgement-dispatch/#?Eck_Collection_Source_Vehicle_Dispatch1=' . urlencode($vehicleDispatchId)
+ . '&Camp_Vehicle_Dispatch.Collection_Camp=' . urlencode($collectionCampId)
+ . '&id=' . urlencode($vehicleDispatchId)
+ . '&Eck_Collection_Source_Vehicle_Dispatch_Eck_Collection_Camp_Collection_Camp_01.id=' . urlencode($collectionCampId)
+ . '&Camp_Institution_Data.Name_of_the_institution=' . urlencode($nameOfInstitution)
+ . '&Camp_Institution_Data.Address=' . urlencode($addressOfInstitution)
+ . '&Camp_Institution_Data.Email=' . urlencode($pocEmail)
+ . '&Camp_Institution_Data.Contact_Number=' . urlencode($pocContactNumber);📝 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.
| $materialdispatchUrl = $homeUrl . 'institution-camp-acknowledgement-dispatch/#?Eck_Collection_Source_Vehicle_Dispatch1=' . $vehicleDispatchId | |
| . '&Camp_Vehicle_Dispatch.Collection_Camp=' . $collectionCampId | |
| . '&id=' . $vehicleDispatchId | |
| . '&Eck_Collection_Source_Vehicle_Dispatch_Eck_Collection_Camp_Collection_Camp_01.id=' . $collectionCampId | |
| . '&Camp_Institution_Data.Name_of_the_institution=' . $nameOfInstitution | |
| . '&Camp_Institution_Data.Address=' . $addressOfInstitution | |
| . '&Camp_Institution_Data.Email=' . $pocEmail | |
| . '&Camp_Institution_Data.Contact_Number=' . $pocContactNumber; | |
| $materialdispatchUrl = $homeUrl . 'institution-camp-acknowledgement-dispatch/#?Eck_Collection_Source_Vehicle_Dispatch1=' . urlencode($vehicleDispatchId) | |
| . '&Camp_Vehicle_Dispatch.Collection_Camp=' . urlencode($collectionCampId) | |
| . '&id=' . urlencode($vehicleDispatchId) | |
| . '&Eck_Collection_Source_Vehicle_Dispatch_Eck_Collection_Camp_Collection_Camp_01.id=' . urlencode($collectionCampId) | |
| . '&Camp_Institution_Data.Name_of_the_institution=' . urlencode($nameOfInstitution) | |
| . '&Camp_Institution_Data.Address=' . urlencode($addressOfInstitution) | |
| . '&Camp_Institution_Data.Email=' . urlencode($pocEmail) | |
| . '&Camp_Institution_Data.Contact_Number=' . urlencode($pocContactNumber); |
3 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes