Add Dynamic Button for PU Activities using shortcode: Proceed to Office Visit or Material Contribution#357
Add Dynamic Button for PU Activities using shortcode: Proceed to Office Visit or Material Contribution#357nishant22029 wants to merge 65 commits intodevelopfrom
Conversation
WalkthroughThe changes in this pull request introduce significant enhancements to the Goonj CRM theme, particularly within the Changes
Possibly related PRs
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: 6
🧹 Outside diff range and nitpick comments (2)
wp-content/themes/goonj-crm/functions.php (2)
567-580: Simplify the activity processing loop for clarity.The loop used to process activities can be optimized for better readability. Additionally, consider using null coalescing operators for clarity when retrieving
$officeId.Apply this diff as a suggestion:
foreach ($fetchUserActivities as $activity) { - $officeId = $activity['Office_Visit.Goonj_Processing_Center'] ?: $activity['Material_Contribution.Goonj_Office']; + $officeId = $activity['Office_Visit.Goonj_Processing_Center'] ?? $activity['Material_Contribution.Goonj_Office']; $activityType = $activity['activity_type_id:label']; if ($officeId && in_array($activityType, ['Office visit', 'Material Contribution'])) { $officeActivities[$officeId][] = $activityType; if ($activityType === 'Office visit') { $hasOfficeVisit = true; } elseif ($activityType === 'Material Contribution') { $hasMaterialContribution = true; } } }
594-598: Use consistent URL generation with WordPress functions.Instead of hardcoding URLs, use WordPress functions like
home_url()orsite_url()to generate paths. This ensures the URLs are correctly formed, especially if the site URL changes.Apply this diff as a suggestion:
if ($hasOfficeVisit) { - $redirectPath = '/material-contribution/details/'; + $redirectPath = home_url('/material-contribution/details/'); $buttonText = __('Material Contribution', 'goonj-crm'); } elseif ($hasMaterialContribution) { - $redirectPath = '/processing-center/material-contribution/details/'; + $redirectPath = home_url('/processing-center/material-contribution/details/'); $buttonText = __('Office Visit', 'goonj-crm'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/themes/goonj-crm/functions.php (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
wp-content/themes/goonj-crm/functions.php (1)
617-618: Ensure the shortcode is registered after the function definition.The shortcode
goonj_pu_activity_buttonis added usingadd_shortcode()on line 617~ immediately after the function definition. Ensure that this placement is intentional and that the function exists before the shortcode is registered to avoid unexpected behavior.
| function goonj_generate_button_html($buttonUrl, $buttonText) { | ||
| return sprintf( | ||
| '<div class="volunteer-button-container"> | ||
| <a href="%s" class="wp-block-button__link has-white-color has-vivid-red-background-color has-text-color has-background has-link-color wp-element-button volunteer-button-link"> | ||
| Wish to Volunteer? | ||
| %s | ||
| </a> | ||
| </div>', | ||
| esc_url($buttonUrl) | ||
| esc_url($buttonUrl), | ||
| esc_html($buttonText) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider separating HTML from PHP logic for better maintainability.
Embedding HTML directly within PHP functions can make the code harder to read and maintain. Consider using template files or output buffers to separate the presentation from the logic. This aligns with the single responsibility principle and improves code clarity.
| function goonj_pu_activity_button() { | ||
| $activityId = isset($_GET['activityId']) ? intval($_GET['activityId']) : 0; | ||
|
|
||
| if (empty($activityId)) { | ||
| \Civi::log()->warning('Activity ID is missing'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Fetch activity details | ||
| $activity = \Civi\Api4\Activity::get(FALSE) | ||
| ->addSelect('source_contact_id', 'Office_Visit.Goonj_Processing_Center', 'Material_Contribution.Goonj_Office', 'activity_type_id:label') | ||
| ->addJoin('ActivityContact AS activity_contact', 'LEFT') | ||
| ->addWhere('id', '=', $activityId) | ||
| ->execute() | ||
| ->first(); | ||
|
|
||
| if (!$activity) { | ||
| \Civi::log()->info('No activities found for Activity ID:', ['activityId' => $activityId]); | ||
| return; | ||
| } | ||
|
|
||
| $individualId = $activity['source_contact_id']; | ||
| $activityTypeLabel = $activity['activity_type_id:label']; | ||
|
|
||
| // Mapping activity type labels to their respective Goonj Office fields | ||
| $officeMapping = [ | ||
| 'Material Contribution' => 'Material_Contribution.Goonj_Office', | ||
| 'Office visit' => 'Office_Visit.Goonj_Processing_Center', | ||
| ]; | ||
|
|
||
| // Determine the Goonj Office ID based on the activity type | ||
| $goonjOfficeId = null; // Initialize to null | ||
| if (array_key_exists($activityTypeLabel, $officeMapping)) { | ||
| $goonjOfficeId = $activity[$officeMapping[$activityTypeLabel]] ?? null; // Retrieve the office ID if it exists | ||
| } | ||
|
|
||
| // Handle case if Goonj Office ID is still null | ||
| if (is_null($goonjOfficeId)) { | ||
| \Civi::log()->warning('Goonj Office ID is null for Activity ID:', ['activityId' => $activityId]); | ||
| // Additional handling if needed (e.g., return or throw an error) | ||
| } | ||
|
|
||
|
|
||
| // Define today's date range | ||
| $todayDate = date('Y-m-d'); | ||
| $startOfDay = $todayDate . ' 00:00:00'; | ||
| $endOfDay = $todayDate . ' 23:59:59'; | ||
|
|
||
| // Fetch user's pu activities for the day | ||
| $fetchUserActivities = \Civi\Api4\Activity::get(FALSE) | ||
| ->addSelect('Office_Visit.Goonj_Processing_Center', 'activity_type_id:label', 'Material_Contribution.Goonj_Office') | ||
| ->addWhere('source_contact_id', '=', $individualId) | ||
| ->addWhere('activity_type_id:label', 'IN', ['Office visit', 'Material Contribution']) | ||
| ->addWhere('created_date', '>=', $startOfDay) | ||
| ->addWhere('created_date', '<=', $endOfDay) | ||
| ->execute(); | ||
|
|
||
| \Civi::log()->info('fetchUserActivities', ['fetchUserActivities' => $fetchUserActivities]); | ||
|
|
||
| // Initialize flags for activity types and office activities array | ||
| $hasOfficeVisit = false; | ||
| $hasMaterialContribution = false; | ||
| $officeActivities = []; | ||
|
|
||
| // Single loop to process activities | ||
| foreach ($fetchUserActivities as $activity) { | ||
| $officeId = $activity['Office_Visit.Goonj_Processing_Center'] ?: $activity['Material_Contribution.Goonj_Office']; | ||
| $activityType = $activity['activity_type_id:label']; | ||
|
|
||
| // Track if both activity types exist and group by office ID | ||
| if ($officeId && in_array($activityType, ['Office visit', 'Material Contribution'])) { | ||
| $officeActivities[$officeId][] = $activityType; | ||
|
|
||
| if ($activityType === 'Office visit') { | ||
| $hasOfficeVisit = true; | ||
| } elseif ($activityType === 'Material Contribution') { | ||
| $hasMaterialContribution = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check if any office ID has both activity types | ||
| foreach ($officeActivities as $activityTypes) { | ||
| if (count(array_unique($activityTypes)) === 2) { | ||
| return; // Both activity types exist, no button needed | ||
| } | ||
| } | ||
|
|
||
| // Determine redirect path and button text | ||
| $redirectPath = ''; | ||
| $buttonText = ''; | ||
|
|
||
| if ($hasOfficeVisit) { | ||
| $redirectPath = '/material-contribution/details/'; | ||
| $buttonText = 'Material Contribution'; | ||
| } elseif ($hasMaterialContribution) { | ||
| $redirectPath = '/processing-center/material-contribution/details/'; | ||
| $buttonText = 'Office Visit'; | ||
| } | ||
|
|
||
| $redirectParams = [ | ||
| 'source_contact_id' => $individualId, | ||
| 'Office_Visit.Goonj_Processing_Center' => $hasOfficeVisit ? $goonjOfficeId : null, | ||
| 'Material_Contribution.Goonj_Office' => $hasMaterialContribution ? $goonjOfficeId : null, | ||
| ]; | ||
|
|
||
| // Build redirect URL with parameters | ||
| $redirectPathWithParams = $redirectPath . '#?' . http_build_query(array_filter($redirectParams)); | ||
|
|
||
| return goonj_generate_button_html($redirectPathWithParams, $buttonText); | ||
| } catch (\Exception $e) { | ||
| \Civi::log()->error('Error in goonj_pu_activity_button: ' . $e->getMessage()); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor goonj_pu_activity_button() to adhere to the single responsibility principle.
The goonj_pu_activity_button() function is lengthy and handles multiple concerns such as data fetching, business logic, and HTML generation. This violates the single responsibility principle. Consider breaking it down into smaller, reusable functions or classes to enhance readability and maintainability.
There was a problem hiding this comment.
refactored this function
There was a problem hiding this comment.
@nishant22029, great to hear that you've refactored the goonj_pu_activity_button() function! This will enhance the code's readability and maintainability.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| if ($hasOfficeVisit) { | ||
| $redirectPath = '/material-contribution/details/'; | ||
| $buttonText = 'Material Contribution'; | ||
| } elseif ($hasMaterialContribution) { | ||
| $redirectPath = '/processing-center/material-contribution/details/'; | ||
| $buttonText = 'Office Visit'; | ||
| } |
There was a problem hiding this comment.
Ensure consistency between redirect paths and button texts.
There's a potential mismatch in the redirect paths and button texts:
- When
$hasOfficeVisitis true,$redirectPathis set to/material-contribution/details/, and$buttonTextis 'Material Contribution'. - When
$hasMaterialContributionis true,$redirectPathis/processing-center/material-contribution/details/, and$buttonTextis 'Office Visit'.
This seems inconsistent and might lead to user confusion or incorrect redirection.
Please verify and correct the redirect paths and button texts to match the intended actions.
| $buttonText = 'Wish to Volunteer?'; | ||
|
|
||
| return goonj_generate_volunteer_button_html($redirectPathWithParams); | ||
| return goonj_generate_button_html($redirectPathWithParams, $buttonText); |
There was a problem hiding this comment.
Use localization functions for translatable strings.
The button text 'Wish to Volunteer?' is hardcoded. To support internationalization and localization, wrap the string with WordPress localization functions like __() or _e().
Apply this diff to fix the issue:
-$buttonText = 'Wish to Volunteer?';
+$buttonText = __('Wish to Volunteer?', 'goonj-crm');📝 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.
| $buttonText = 'Wish to Volunteer?'; | |
| return goonj_generate_volunteer_button_html($redirectPathWithParams); | |
| return goonj_generate_button_html($redirectPathWithParams, $buttonText); | |
| $buttonText = __('Wish to Volunteer?', 'goonj-crm'); | |
| return goonj_generate_button_html($redirectPathWithParams, $buttonText); |
| 'source_contact_id' => $individualId, | ||
| 'Office_Visit.Goonj_Processing_Center' => $hasOfficeVisit ? $goonjOfficeId : null, | ||
| 'Material_Contribution.Goonj_Office' => $hasMaterialContribution ? $goonjOfficeId : null, | ||
| ]; |
There was a problem hiding this comment.
Avoid including null values in query parameters.
In the $redirectParams array, null values for parameters like 'Office_Visit.Goonj_Processing_Center' or 'Material_Contribution.Goonj_Office' may result in query strings with keys but no values, which can cause confusion in URL parsing.
Filter out null values before building the query string:
$redirectParams = [
'source_contact_id' => $individualId,
'Office_Visit.Goonj_Processing_Center' => $hasOfficeVisit ? $goonjOfficeId : null,
'Material_Contribution.Goonj_Office' => $hasMaterialContribution ? $goonjOfficeId : null,
];
+ $redirectParams = array_filter($redirectParams, function($value) {
+ return !is_null($value);
+ });📝 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.
| 'source_contact_id' => $individualId, | |
| 'Office_Visit.Goonj_Processing_Center' => $hasOfficeVisit ? $goonjOfficeId : null, | |
| 'Material_Contribution.Goonj_Office' => $hasMaterialContribution ? $goonjOfficeId : null, | |
| ]; | |
| 'source_contact_id' => $individualId, | |
| 'Office_Visit.Goonj_Processing_Center' => $hasOfficeVisit ? $goonjOfficeId : null, | |
| 'Material_Contribution.Goonj_Office' => $hasMaterialContribution ? $goonjOfficeId : null, | |
| ]; | |
| $redirectParams = array_filter($redirectParams, function($value) { | |
| return !is_null($value); | |
| }); |
| if (is_null($goonjOfficeId)) { | ||
| \Civi::log()->warning('Goonj Office ID is null for Activity ID:', ['activityId' => $activityId]); | ||
| // Additional handling if needed (e.g., return or throw an error) | ||
| } |
There was a problem hiding this comment.
Handle null $goonjOfficeId to prevent potential errors.
When $goonjOfficeId is null, a warning is logged, but the execution continues, which might lead to issues when $goonjOfficeId is used later in the code. It's important to handle this scenario explicitly to prevent unexpected behavior.
Apply this diff to fix the issue:
if (is_null($goonjOfficeId)) {
\Civi::log()->warning('Goonj Office ID is null for Activity ID:', ['activityId' => $activityId]);
+ return; // Exit early since office ID is essential for further processing
}📝 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 (is_null($goonjOfficeId)) { | |
| \Civi::log()->warning('Goonj Office ID is null for Activity ID:', ['activityId' => $activityId]); | |
| // Additional handling if needed (e.g., return or throw an error) | |
| } | |
| if (is_null($goonjOfficeId)) { | |
| \Civi::log()->warning('Goonj Office ID is null for Activity ID:', ['activityId' => $activityId]); | |
| return; // Exit early since office ID is essential for further processing | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
wp-content/themes/goonj-crm/templates/button-template.php (2)
3-4: Good use of escaping functions. Consider adding default values.The use of
esc_url()andesc_html()for escaping the URL and button text is excellent for security. However, to improve robustness, consider adding default values for$buttonUrland$buttonText.Here's a suggestion to add default values:
- <a href="<?php echo esc_url($buttonUrl); ?>" class="wp-block-button__link has-white-color has-vivid-red-background-color has-text-color has-background has-link-color wp-element-button volunteer-button-link"> - <?php echo esc_html($buttonText); ?> + <a href="<?php echo esc_url($buttonUrl ?? '#'); ?>" class="wp-block-button__link has-white-color has-vivid-red-background-color has-text-color has-background has-link-color wp-element-button volunteer-button-link"> + <?php echo esc_html($buttonText ?? 'Click here'); ?>
3-3: Good use of CSS classes. Consider using a constant for class list.The use of multiple CSS classes provides flexibility in styling. However, to improve maintainability, consider defining the class list as a constant or variable.
Here's a suggestion to improve maintainability:
<?php $button_classes = 'wp-block-button__link has-white-color has-vivid-red-background-color has-text-color has-background has-link-color wp-element-button volunteer-button-link'; ?> <a href="<?php echo esc_url($buttonUrl ?? '#'); ?>" class="<?php echo esc_attr($button_classes); ?>"> <?php echo esc_html($buttonText ?? 'Click here'); ?> </a>This approach allows for easier modification of the class list in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/themes/goonj-crm/functions.php (3 hunks)
- wp-content/themes/goonj-crm/templates/button-template.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
wp-content/themes/goonj-crm/templates/button-template.php (2)
1-1: LGTM! Clear file identification.The comment at the beginning of the file clearly identifies its purpose as a button template. This is a good practice for maintaining code readability and organization.
2-6: LGTM! Semantic HTML structure.The HTML structure is clean and semantic. Using a
<div>as a container and an<a>element as a button is appropriate, especially if the button leads to a new page or action.wp-content/themes/goonj-crm/functions.php (2)
486-486: Use Localization Functions for Translatable StringsThe button text
'Wish to Volunteer?'is hardcoded. To support internationalization and localization, wrap the string with WordPress localization functions like__()or_e().
497-541: Good Refactoring to Support Single Responsibility PrincipleThe
goonj_pu_activity_buttonfunction has been refactored to delegate tasks to smaller functions likefetch_activity_detailsandgenerate_activity_button. This improves code readability and aligns with the single responsibility principle.
| function generate_activity_button($officeActivities, $goonjOfficeId, $individualId) { | ||
| $hasOfficeVisit = false; | ||
| $hasMaterialContribution = false; | ||
|
|
||
| // Check for office visit and material contribution activity types | ||
| foreach ($officeActivities as $activityTypes) { | ||
| if (in_array('Office visit', $activityTypes)) { | ||
| $hasOfficeVisit = true; | ||
| } | ||
| if (in_array('Material Contribution', $activityTypes)) { | ||
| $hasMaterialContribution = true; | ||
| } | ||
| } | ||
|
|
||
| $redirectPath = ''; | ||
| $buttonText = ''; | ||
|
|
||
| // Set the correct redirect path and button text based on activities completed | ||
| if ($hasOfficeVisit) { | ||
| $redirectPath = '/processing-center/material-contribution/details/'; | ||
| $buttonText = 'Proceed to Material Contribution'; | ||
| } elseif ($hasMaterialContribution) { | ||
| $redirectPath = '/processing-center/office-visit/details/'; | ||
| $buttonText = 'Proceed to Office Visit'; | ||
| } | ||
|
|
||
| $redirectParams = [ | ||
| 'source_contact_id' => $individualId, | ||
| 'Material_Contribution.Goonj_Office' => $hasOfficeVisit ? $goonjOfficeId : null, | ||
| 'Office_Visit.Goonj_Processing_Center' => $hasMaterialContribution ? $goonjOfficeId : null, | ||
| ]; | ||
|
|
||
| // Create the full URL with query parameters | ||
| $redirectPathWithParams = $redirectPath . '#?' . http_build_query(array_filter($redirectParams)); | ||
|
|
||
| return goonj_generate_button_html($redirectPathWithParams, $buttonText); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to Eliminate Duplicate Code and Simplify Logic
The generate_activity_button function contains duplicate code and complex conditional logic. Refactoring can improve readability and adherence to the DRY (Don't Repeat Yourself) principle.
Consider this refactored version:
function generate_activity_button($officeActivities, $goonjOfficeId, $individualId) {
$activityTypes = ['Office visit', 'Material Contribution'];
$completedActivities = $officeActivities[$goonjOfficeId] ?? [];
$pendingActivities = array_diff($activityTypes, $completedActivities);
if (empty($pendingActivities)) {
return; // All activities completed
}
$activityMap = [
'Office visit' => [
'redirectPath' => '/processing-center/office-visit/details/',
'buttonText' => __('Proceed to Office Visit', 'goonj-crm'),
'queryParam' => 'Office_Visit.Goonj_Processing_Center',
],
'Material Contribution' => [
'redirectPath' => '/processing-center/material-contribution/details/',
'buttonText' => __('Proceed to Material Contribution', 'goonj-crm'),
'queryParam' => 'Material_Contribution.Goonj_Office',
],
];
$nextActivity = reset($pendingActivities);
$details = $activityMap[$nextActivity];
$redirectParams = [
'source_contact_id' => $individualId,
$details['queryParam'] => $goonjOfficeId,
];
$redirectPathWithParams = $details['redirectPath'] . '#?' . http_build_query($redirectParams);
return goonj_generate_button_html($redirectPathWithParams, $details['buttonText']);
}This refactoring reduces duplication and simplifies the logic by using arrays and built-in functions.
There was a problem hiding this comment.
@nishant22029, great job updating the code! The refactored function looks much cleaner and more maintainable now. Thank you for making the changes.
(^・ω・^ )
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| if ($hasOfficeVisit) { | ||
| $redirectPath = '/processing-center/material-contribution/details/'; | ||
| $buttonText = 'Proceed to Material Contribution'; | ||
| } elseif ($hasMaterialContribution) { | ||
| $redirectPath = '/processing-center/office-visit/details/'; | ||
| $buttonText = 'Proceed to Office Visit'; | ||
| } |
There was a problem hiding this comment.
Use Localization Functions for Button Texts
The button texts 'Proceed to Material Contribution' and 'Proceed to Office Visit' are hardcoded. For better localization support, wrap these strings with WordPress localization functions.
Apply this diff to fix the issue:
if ($hasOfficeVisit) {
$redirectPath = '/processing-center/material-contribution/details/';
- $buttonText = 'Proceed to Material Contribution';
+ $buttonText = __('Proceed to Material Contribution', 'goonj-crm');
} elseif ($hasMaterialContribution) {
$redirectPath = '/processing-center/office-visit/details/';
- $buttonText = 'Proceed to Office Visit';
+ $buttonText = __('Proceed to Office Visit', 'goonj-crm');
}This change ensures that the button texts can be translated into other languages.
📝 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 ($hasOfficeVisit) { | |
| $redirectPath = '/processing-center/material-contribution/details/'; | |
| $buttonText = 'Proceed to Material Contribution'; | |
| } elseif ($hasMaterialContribution) { | |
| $redirectPath = '/processing-center/office-visit/details/'; | |
| $buttonText = 'Proceed to Office Visit'; | |
| } | |
| if ($hasOfficeVisit) { | |
| $redirectPath = '/processing-center/material-contribution/details/'; | |
| $buttonText = __('Proceed to Material Contribution', 'goonj-crm'); | |
| } elseif ($hasMaterialContribution) { | |
| $redirectPath = '/processing-center/office-visit/details/'; | |
| $buttonText = __('Proceed to Office Visit', 'goonj-crm'); | |
| } |
There was a problem hiding this comment.
@nishant22029 Great! Thank you for making the change.
(_/)
( ^.^ )
(")__(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| function goonj_generate_button_html($buttonUrl, $buttonText) { | ||
| set_query_var('buttonUrl', $buttonUrl); | ||
| set_query_var('buttonText', $buttonText); | ||
|
|
||
| ob_start(); | ||
| get_template_part('templates/button-template'); | ||
| return ob_get_clean(); |
There was a problem hiding this comment.
Avoid Using Global State: Pass Variables Directly to Templates
The goonj_generate_button_html function relies on set_query_var to pass variables to the template, which modifies global state and can lead to unintended side effects. Instead, pass variables directly to the template to enhance code clarity and maintainability.
Apply this diff to refactor the function:
function goonj_generate_button_html($buttonUrl, $buttonText) {
- set_query_var('buttonUrl', $buttonUrl);
- set_query_var('buttonText', $buttonText);
-
ob_start();
- get_template_part('templates/button-template');
+ get_template_part('templates/button-template', null, [
+ 'buttonUrl' => $buttonUrl,
+ 'buttonText' => $buttonText,
+ ]);
return ob_get_clean();
}This approach avoids modifying global variables and makes the data flow more explicit.
📝 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.
| function goonj_generate_button_html($buttonUrl, $buttonText) { | |
| set_query_var('buttonUrl', $buttonUrl); | |
| set_query_var('buttonText', $buttonText); | |
| ob_start(); | |
| get_template_part('templates/button-template'); | |
| return ob_get_clean(); | |
| function goonj_generate_button_html($buttonUrl, $buttonText) { | |
| ob_start(); | |
| get_template_part('templates/button-template', null, [ | |
| 'buttonUrl' => $buttonUrl, | |
| 'buttonText' => $buttonText, | |
| ]); | |
| return ob_get_clean(); | |
| } |
There was a problem hiding this comment.
@nishant22029 Thank you for implementing the change!
(^o^/)
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| 'Individual1' => $individualId, | ||
| 'message' => 'individual-user' | ||
| ]); | ||
| $buttonText = 'Wish to Volunteer?'; |
There was a problem hiding this comment.
Use localization functions for translatable strings.
The button text 'Wish to Volunteer?' is hardcoded. For internationalization support, wrap the string with WordPress localization functions like __().
Apply this diff to make the string translatable:
$redirectPathWithParams = $redirectPath . '#?' . http_build_query([
'Individual1' => $individualId,
'message' => 'individual-user'
]);
-$buttonText = 'Wish to Volunteer?';
+$buttonText = __('Wish to Volunteer?', 'goonj-crm');This change enables the string to be translated into other languages.
📝 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.
| $buttonText = 'Wish to Volunteer?'; | |
| $buttonText = __('Wish to Volunteer?', 'goonj-crm'); |
There was a problem hiding this comment.
@nishant22029, thank you for making the changes! 🎉
(\_/)
(•_•)
/ >🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| $details['queryParam'] => $goonjOfficeId, | ||
| ]; | ||
|
|
||
| $redirectPathWithParams = $details['redirectPath'] . '#?' . http_build_query($redirectParams); |
There was a problem hiding this comment.
Correct URL generation by placing query parameters before the fragment identifier.
Appending query parameters after # can prevent them from being sent to the server, as they are treated as fragment identifiers. Adjust the URL construction to include query parameters before the # symbol.
Apply this diff to fix the URL construction:
- $redirectPathWithParams = $details['redirectPath'] . '#?' . http_build_query($redirectParams);
+ $redirectPathWithParams = $details['redirectPath'] . '?' . http_build_query($redirectParams);This ensures that query parameters are properly recognized and processed by the server.
📝 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.
| $redirectPathWithParams = $details['redirectPath'] . '#?' . http_build_query($redirectParams); | |
| $redirectPathWithParams = $details['redirectPath'] . '?' . http_build_query($redirectParams); |
There was a problem hiding this comment.
@nishant22029, great! Thank you for making the changes.
(^•^)/
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| add_shortcode('goonj_contribution_volunteer_signup_button', 'goonj_contribution_volunteer_signup_button'); | ||
|
|
||
| function goonj_pu_activity_button() { | ||
| $activityId = isset($_GET['activityId']) ? intval($_GET['activityId']) : 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance input validation using filter_input() for activityId.
Currently, activityId is retrieved directly from $_GET and cast using intval(). For better security and input validation, use filter_input() with FILTER_VALIDATE_INT to safely retrieve and validate the activityId parameter.
Apply this diff to improve input sanitization:
-function goonj_pu_activity_button() {
- $activityId = isset($_GET['activityId']) ? intval($_GET['activityId']) : 0;
+function goonj_pu_activity_button() {
+ $activityId = filter_input(INPUT_GET, 'activityId', FILTER_VALIDATE_INT);
+ if ($activityId === false || $activityId === null) {
+ $activityId = 0;
+ }📝 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.
| $activityId = isset($_GET['activityId']) ? intval($_GET['activityId']) : 0; | |
| function goonj_pu_activity_button() { | |
| $activityId = filter_input(INPUT_GET, 'activityId', FILTER_VALIDATE_INT); | |
| if ($activityId === false || $activityId === null) { | |
| $activityId = 0; | |
| } |
| function fetch_contact_activities_for_today($individualId) { | ||
| $todayDate = date('Y-m-d'); | ||
| $startOfDay = $todayDate . ' 00:00:00'; | ||
| $endOfDay = $todayDate . ' 23:59:59'; | ||
|
|
||
| return \Civi\Api4\Activity::get(FALSE) | ||
| ->addSelect('Office_Visit.Goonj_Processing_Center', 'activity_type_id:label', 'Material_Contribution.Goonj_Office') | ||
| ->addWhere('source_contact_id', '=', $individualId) | ||
| ->addWhere('activity_type_id:label', 'IN', ['Office visit', 'Material Contribution']) | ||
| ->addWhere('created_date', '>=', $startOfDay) | ||
| ->addWhere('created_date', '<=', $endOfDay) | ||
| ->execute(); | ||
| } | ||
|
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
wp-content/themes/goonj-crm/functions.php (1)
430-437: Add PHPDoc comments togoonj_generate_button_htmlfor better readability.Including PHPDoc comments will improve code maintainability by providing clear documentation of the function's purpose, parameters, and return values.
| function goonj_generate_activity_button($officeActivities, $goonjOfficeId, $individualId) { | ||
| $activityTypes = ['Office visit', 'Material Contribution']; | ||
| $completedActivities = $officeActivities[$goonjOfficeId] ?? []; | ||
| $pendingActivities = array_diff($activityTypes, $completedActivities); | ||
|
|
||
| if (empty($pendingActivities)) { | ||
| return; // All activities completed | ||
| } | ||
|
|
||
| $activityMap = [ | ||
| 'Office visit' => [ | ||
| 'redirectPath' => '/processing-center/office-visit/details/', | ||
| 'buttonText' => __('Proceed to Office Visit', 'goonj-crm'), | ||
| 'queryParam' => 'Office_Visit.Goonj_Processing_Center', | ||
| ], | ||
| 'Material Contribution' => [ | ||
| 'redirectPath' => '/processing-center/material-contribution/details/', | ||
| 'buttonText' => __('Proceed to Material Contribution', 'goonj-crm'), | ||
| 'queryParam' => 'Material_Contribution.Goonj_Office', | ||
| ], | ||
| ]; | ||
|
|
||
| $nextActivity = reset($pendingActivities); | ||
| $details = $activityMap[$nextActivity]; | ||
|
|
||
| $redirectParams = [ | ||
| 'source_contact_id' => $individualId, | ||
| $details['queryParam'] => $goonjOfficeId, | ||
| ]; | ||
|
|
||
| $redirectPathWithParams = $details['redirectPath'] . '#?' . http_build_query($redirectParams); | ||
|
|
||
| return goonj_generate_button_html($redirectPathWithParams, $details['buttonText']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor URL construction into a reusable function to eliminate duplication.
The logic for building $redirectPathWithParams is repeated in different parts of the codebase. Extracting this into a utility function promotes code reuse and adheres to the DRY (Don't Repeat Yourself) principle.
Example utility function:
function build_redirect_url($basePath, $params) {
return $basePath . '?' . http_build_query($params);
}Update the code to use the utility function:
-$redirectPathWithParams = $details['redirectPath'] . '?'
- . http_build_query($redirectParams);
+$redirectPathWithParams = build_redirect_url(
+ $details['redirectPath'],
+ $redirectParams
+);| function fetch_contact_pu_activities_for_today($individualId) { | ||
|
|
||
| $timezone = new \DateTimeZone('UTC'); | ||
| $today = new \DateTime('now', $timezone); | ||
| $startOfDay = $today->setTime(0, 0)->format('Y-m-d H:i:s'); | ||
| $endOfDay = $today->setTime(23, 59, 59)->format('Y-m-d H:i:s'); |
There was a problem hiding this comment.
Clone the DateTime object to prevent mutability issues.
In fetch_contact_pu_activities_for_today(), modifying the same DateTime object for $startOfDay and $endOfDay can lead to unexpected results due to object mutability. Cloning the object before setting the time ensures accurate date ranges.
Apply this diff to fix the issue:
$timezone = new \DateTimeZone('UTC');
$today = new \DateTime('now', $timezone);
-$startOfDay = $today->setTime(0, 0)->format('Y-m-d H:i:s');
-$endOfDay = $today->setTime(23, 59, 59)->format('Y-m-d H:i:s');
+$startOfDay = (clone $today)->setTime(0, 0)->format('Y-m-d H:i:s');
+$endOfDay = (clone $today)->setTime(23, 59, 59)->format('Y-m-d H:i:s');📝 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.
| function fetch_contact_pu_activities_for_today($individualId) { | |
| $timezone = new \DateTimeZone('UTC'); | |
| $today = new \DateTime('now', $timezone); | |
| $startOfDay = $today->setTime(0, 0)->format('Y-m-d H:i:s'); | |
| $endOfDay = $today->setTime(23, 59, 59)->format('Y-m-d H:i:s'); | |
| function fetch_contact_pu_activities_for_today($individualId) { | |
| $timezone = new \DateTimeZone('UTC'); | |
| $today = new \DateTime('now', $timezone); | |
| $startOfDay = (clone $today)->setTime(0, 0)->format('Y-m-d H:i:s'); | |
| $endOfDay = (clone $today)->setTime(23, 59, 59)->format('Y-m-d H:i:s'); |
nishant22029
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
wp-content/themes/goonj-crm/functions.php (1)
617-618: Use defined constants for activity types instead of magic stringsIn the
$activityTypesarray, activity type labels are hard-coded as'Office visit'and'Material Contribution'. Defining these strings as constants can improve code readability and maintainability, reducing the risk of typos or inconsistencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/themes/goonj-crm/functions.php (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
wp-content/themes/goonj-crm/functions.php (1)
430-437: Functiongoonj_generate_button_html()is well-structuredThe
goonj_generate_button_html()function appropriately abstracts the button generation logic, enhancing code reusability and maintainability.
| // Function to determine which activity button (if any) should be displayed for the user | ||
| function goonj_pu_activity_button() { | ||
| // Retrieve the activity ID from the URL parameters (GET request) | ||
| $activityId = isset($_GET['activityId']) ? intval($_GET['activityId']) : 0; | ||
|
|
||
| // If no valid activity ID is provided, log an error and exit the function | ||
| if (empty($activityId)) { | ||
| \Civi::log()->info('Activity ID is missing', ['activityId' => $activityId]); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Fetch the details of the current activity (either Office Visit or Material Contribution) | ||
| $activity = goonj_fetch_pu_activity_details($activityId); | ||
|
|
||
| // If no activity details are found, log an error and exit the function | ||
| if (!$activity) { | ||
| \Civi::log()->info('No activities found for Activity ID:', ['activityId' => $activityId]); | ||
| return; | ||
| } | ||
|
|
||
| // Extract the individual's ID from the activity data | ||
| $individualId = $activity['source_contact_id']; | ||
|
|
||
| // Fetch the Goonj office ID from the activity details (can be from Office Visit or Material Contribution) | ||
| $goonjOfficeId = goonj_get_goonj_office_id($activity); | ||
|
|
||
| // If the office ID is null, log an error and exit the function | ||
| if (is_null($goonjOfficeId)) { | ||
| \Civi::log()->info('Goonj Office ID is null for Activity ID:', ['activityId' => $activityId]); | ||
| return; | ||
| } | ||
|
|
||
| // Fetch all activities for this individual that occurred today (Office Visits and Material Contributions) | ||
| $contactActivities = fetch_contact_pu_activities_for_today($individualId); | ||
|
|
||
| // Process these activities to determine what has already been completed at this office | ||
| $officeActivities = goonj_process_pu_activities($contactActivities); | ||
|
|
||
| // If both Office Visit and Material Contribution have been completed at the same office, exit the function | ||
| if (goonj_check_if_both_pu_activity_types_exist($officeActivities)) { | ||
| return; // Both activities are done, no button needed | ||
| } | ||
|
|
||
| // Generate the appropriate button for the next pending activity and return the HTML | ||
| return goonj_generate_activity_button($officeActivities, $goonjOfficeId, $individualId); | ||
|
|
||
| } catch (\Exception $e) { | ||
| // Log any exceptions that occur during the process | ||
| \Civi::log()->error('Error in goonj_pu_activity_button: ' . $e->getMessage()); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor goonj_pu_activity_button() to adhere to the single responsibility principle
The goonj_pu_activity_button() function handles multiple responsibilities, including input validation, data retrieval, business logic, and output generation. This violates the single responsibility principle and can make the code harder to read and maintain. Consider breaking it down into smaller, focused functions that each handle a specific task.
| // Define the mapping for each activity type to its respective redirect path, button text, and query parameter | ||
| $activityMap = [ | ||
| 'Office visit' => [ | ||
| 'redirectPath' => '/processing-center/office-visit/details/', | ||
| 'buttonText' => __('Proceed to Office Visit', 'goonj-crm'), | ||
| 'queryParam' => 'Office_Visit.Goonj_Processing_Center', | ||
| ], | ||
| 'Material Contribution' => [ | ||
| 'redirectPath' => '/processing-center/material-contribution/details/', | ||
| 'buttonText' => __('Proceed to Material Contribution', 'goonj-crm'), | ||
| 'queryParam' => 'Material_Contribution.Goonj_Office', | ||
| ], | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract activity mapping to configuration or class constants
The $activityMap array in goonj_generate_activity_button() is hard-coded within the function. To enhance maintainability and facilitate future updates, consider extracting this mapping to a configuration file or defining it as a class constant. This approach centralizes the activity mapping data, making it easier to manage and reducing duplication.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
wp-content/themes/goonj-crm/templates/primary-button.php (1)
1-5: Good structure, but let's enhance accessibility!The markup looks clean and follows WordPress coding standards. Great job using
esc_url()andesc_html()for security! However, we can make this button even better:
- Add
role="button"to the anchor tag to explicitly define its purpose.- Consider adding an
aria-labelwith a more descriptive text about the button's action.Here's a suggested improvement:
<div class="volunteer-button-container"> - <a href="<?php echo esc_url( $args['button_url'] ); ?>" class="wp-block-button__link has-white-color has-vivid-red-background-color has-text-color has-background has-link-color wp-element-button volunteer-button-link"> + <a href="<?php echo esc_url( $args['button_url'] ); ?>" class="wp-block-button__link has-white-color has-vivid-red-background-color has-text-color has-background has-link-color wp-element-button volunteer-button-link" role="button" aria-label="<?php echo esc_attr( $args['button_aria_label'] ?? $args['button_text'] ); ?>"> <?php echo esc_html( $args['button_text'] ); ?> </a> </div>This change improves accessibility while maintaining the existing functionality. The
aria-labelfallback tobutton_textensures compatibility if the new attribute isn't provided.wp-content/themes/goonj-crm/functions.php (2)
Line range hint
691-755: Simplify logic and improve error handling ingoonj_redirect_after_individual_creation.The function's complexity could be reduced, and its error handling improved:
- Consider using a switch statement or a mapping object for different creation flows to simplify the logic.
- Implement more robust error handling and logging, especially for edge cases.
- Extract the redirection URL generation into separate functions for each flow type.
Here's a suggestion for simplifying the logic:
function goonj_redirect_after_individual_creation() { if (!isset($_GET['goonjAction']) || $_GET['goonjAction'] !== 'individualCreated' || !isset($_GET['individualId'])) { return; } $individual = goonj_fetch_individual_details($_GET['individualId']); if (!$individual) { \Civi::log()->error('Failed to fetch individual details', ['individualId' => $_GET['individualId']]); return; } $redirectPath = goonj_get_redirect_path($individual); if ($redirectPath) { wp_safe_redirect($redirectPath); exit; } } function goonj_get_redirect_path($individual) { $flowHandlers = [ 'material-contribution' => 'goonj_handle_material_contribution_flow', 'office-visit' => 'goonj_handle_office_visit_flow', 'office-visit-contribution' => 'goonj_handle_office_visit_contribution_flow', ]; $creationFlow = $individual['Individual_fields.Creation_Flow']; if (isset($flowHandlers[$creationFlow])) { return $flowHandlers[$creationFlow]($individual); } \Civi::log()->warning('Unknown creation flow', ['flow' => $creationFlow, 'individualId' => $individual['id']]); return ''; } // Implement individual handler functions for each flow typeThis approach would make the function more maintainable and easier to extend in the future.
Line range hint
1-755: Overall code structure and maintainability improvements needed.While the code is functional, there are several areas where it could be improved to enhance maintainability, readability, and adherence to best practices:
Separation of Concerns: Many functions are handling multiple responsibilities. Consider breaking these down into smaller, more focused functions or classes.
Error Handling: Implement a more consistent and robust error handling strategy throughout the file. This could include custom exceptions and more detailed logging.
Configuration Management: Move hardcoded values and mappings to configuration files or database tables for easier management and updates.
Code Organization: Group related functions together and consider moving some functionality into separate files or classes for better organization.
Dependency Injection: Implement dependency injection to improve testability and reduce tight coupling between components.
Documentation: Add more inline documentation, especially for complex logic and CiviCRM API interactions.
Input Validation: Enhance input validation and sanitization, particularly for user inputs and GET/POST parameters.
Consider implementing a service layer to encapsulate CiviCRM API calls and business logic. This would help separate the WordPress-specific code from the CiviCRM integration logic, making the codebase more modular and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/themes/goonj-crm/functions.php (5 hunks)
- wp-content/themes/goonj-crm/templates/primary-button.php (1 hunks)
🧰 Additional context used
| function goonj_pu_activity_button() { | ||
| // Retrieve the activity ID from the URL parameters (GET request) | ||
| $activityId = isset( $_GET['activityId'] ) ? intval( $_GET['activityId'] ) : 0; | ||
|
|
||
| // If no valid activity ID is provided, log an error and exit the function | ||
| if ( empty( $activityId ) ) { | ||
| \Civi::log()->info( 'Activity ID is missing', array( 'activityId' => $activityId ) ); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Fetch the details of the current activity (either Office Visit or Material Contribution) | ||
| $activity = goonj_fetch_pu_activity_details( $activityId ); | ||
|
|
||
| // If no activity details are found, log an error and exit the function | ||
| if ( ! $activity ) { | ||
| \Civi::log()->info( 'No activities found for Activity ID:', array( 'activityId' => $activityId ) ); | ||
| return; | ||
| } | ||
|
|
||
| // Extract the individual's ID from the activity data | ||
| $individualId = $activity['source_contact_id']; | ||
|
|
||
| // Fetch the Goonj office ID from the activity details (can be from Office Visit or Material Contribution) | ||
| $goonjOfficeId = goonj_get_goonj_office_id( $activity ); | ||
|
|
||
| // If the office ID is null, log an error and exit the function | ||
| if ( is_null( $goonjOfficeId ) ) { | ||
| \Civi::log()->info( 'Goonj Office ID is null for Activity ID:', array( 'activityId' => $activityId ) ); | ||
| return; | ||
| } | ||
|
|
||
| $contactSubTypes = $contact['contact_sub_type'] ?? []; | ||
| // Fetch all activities for this individual that occurred today (Office Visits and Material Contributions) | ||
| $contactActivities = fetch_contact_pu_activities_for_today( $individualId ); | ||
|
|
||
| // Process these activities to determine what has already been completed at this office | ||
| $officeActivities = goonj_process_pu_activities( $contactActivities ); | ||
|
|
||
| // If both Office Visit and Material Contribution have been completed at the same office, exit the function | ||
| if ( goonj_check_if_both_pu_activity_types_exist( $officeActivities ) ) { | ||
| return; // Both activities are done, no button needed | ||
| } | ||
|
|
||
| // Generate the appropriate button for the next pending activity and return the HTML | ||
| return goonj_generate_activity_button( $officeActivities, $goonjOfficeId, $individualId ); | ||
|
|
||
| } catch ( \Exception $e ) { | ||
| // Log any exceptions that occur during the process | ||
| \Civi::log()->error( 'Error in goonj_pu_activity_button: ' . $e->getMessage() ); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance modularity and error handling in goonj_pu_activity_button.
While the function is well-structured, it's handling multiple responsibilities and could benefit from further modularization. Consider the following improvements:
- Extract the activity fetching and processing logic into separate service methods.
- Implement a more consistent error handling strategy, possibly using custom exceptions.
- Consider using a factory pattern for button generation to improve extensibility.
Here's a high-level suggestion for refactoring:
function goonj_pu_activity_button() {
try {
$activityService = new ActivityService();
$buttonFactory = new ButtonFactory();
$activityId = $activityService->getActivityIdFromRequest();
$activity = $activityService->fetchActivityDetails($activityId);
$officeActivities = $activityService->getOfficeActivities($activity);
return $buttonFactory->createButton($officeActivities);
} catch (ActivityException $e) {
\Civi::log()->error('Error in goonj_pu_activity_button: ' . $e->getMessage());
return null;
}
}This approach would improve separation of concerns and make the code more maintainable.
| function goonj_generate_activity_button( $officeActivities, $goonjOfficeId, $individualId ) { | ||
| // Define the two activity types: Office visit and Material Contribution | ||
| $activityTypes = array( 'Office visit', 'Material Contribution' ); | ||
|
|
||
| // Get the list of completed activities for the specified office | ||
| // If no activities are found for this office, an empty array is returned | ||
| $completedActivities = $officeActivities[ $goonjOfficeId ] ?? array(); | ||
|
|
||
| // Find the activities that are still pending by comparing with the list of activity types | ||
| $pendingActivities = array_diff( $activityTypes, $completedActivities ); | ||
|
|
||
| // If all activities have been completed, no button is needed, so exit the function | ||
| if ( empty( $pendingActivities ) ) { | ||
| return; | ||
| } | ||
|
|
||
| // Define the mapping for each activity type to its respective redirect path, button text, and query parameter | ||
| $activityMap = array( | ||
| 'Office visit' => array( | ||
| 'redirectPath' => '/processing-center/office-visit/details/', | ||
| 'buttonText' => __( 'Proceed to Office Visit', 'goonj-crm' ), | ||
| 'queryParam' => 'Office_Visit.Goonj_Processing_Center', | ||
| ), | ||
| 'Material Contribution' => array( | ||
| 'redirectPath' => '/processing-center/material-contribution/details/', | ||
| 'buttonText' => __( 'Proceed to Material Contribution', 'goonj-crm' ), | ||
| 'queryParam' => 'Material_Contribution.Goonj_Office', | ||
| ), | ||
| ); | ||
|
|
||
| // Get the first pending activity from the list of pending activities | ||
| $nextActivity = reset( $pendingActivities ); | ||
| // Fetch the relevant details for the next activity, including the redirect path and button text | ||
| $details = $activityMap[ $nextActivity ]; | ||
|
|
||
| // Prepare the query parameters needed for the redirection, including the individual and office IDs | ||
| $redirectParams = array( | ||
| 'source_contact_id' => $individualId, | ||
| $details['queryParam'] => $goonjOfficeId, | ||
| ); | ||
|
|
||
| // Generate the full redirect URL with the necessary query parameters | ||
| $redirectPathWithParams = $details['redirectPath'] . '#?' . http_build_query( $redirectParams ); | ||
|
|
||
| // Generate the HTML for the button using the redirect URL and the button text | ||
| return goonj_generate_button_html( $redirectPathWithParams, $details['buttonText'] ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance configurability of goonj_generate_activity_button.
The function is well-structured, but it could benefit from increased flexibility:
- Move the
$activityMapto a configuration file or database to allow easier updates without code changes. - Consider using dependency injection for the button generation to improve testability.
Here's a suggestion for externalizing the activity map:
function goonj_get_activity_map() {
// This could be loaded from a config file or database
return [
'Office visit' => [
'redirectPath' => '/processing-center/office-visit/details/',
'buttonText' => __('Proceed to Office Visit', 'goonj-crm'),
'queryParam' => 'Office_Visit.Goonj_Processing_Center',
],
'Material Contribution' => [
'redirectPath' => '/processing-center/material-contribution/details/',
'buttonText' => __('Proceed to Material Contribution', 'goonj-crm'),
'queryParam' => 'Material_Contribution.Goonj_Office',
],
];
}
function goonj_generate_activity_button($officeActivities, $goonjOfficeId, $individualId) {
$activityMap = goonj_get_activity_map();
// Rest of the function remains the same
}This change would make it easier to manage activity types and their associated details.
|
|
||
| function goonj_contribution_volunteer_signup_button() { | ||
| $activityId = isset($_GET['activityId']) ? intval($_GET['activityId']) : 0; | ||
|
|
||
| if (empty($activityId)) { | ||
| \Civi::log()->warning('Activity ID is missing'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| $activities = \Civi\Api4\Activity::get(FALSE) | ||
| ->addSelect('source_contact_id') | ||
| ->addJoin('ActivityContact AS activity_contact', 'LEFT') | ||
| ->addWhere('id', '=', $activityId) | ||
| ->addWhere('activity_type_id:label', '=', 'Material Contribution') | ||
| ->execute(); | ||
|
|
||
| if ($activities->count() === 0) { | ||
| \Civi::log()->info('No activities found for Activity ID:', ['activityId' => $activityId]); | ||
| return; | ||
| } | ||
|
|
||
| $activity = $activities->first(); | ||
| $individualId = $activity['source_contact_id']; | ||
|
|
||
| $contact = \Civi\Api4\Contact::get(FALSE) | ||
| ->addSelect('contact_sub_type') | ||
| ->addWhere('id', '=', $individualId) | ||
| ->execute() | ||
| ->first(); | ||
|
|
||
| if (empty($contact)) { | ||
| \Civi::log()->info('Contact not found', ['contact' => $contact['id']]); | ||
| $activityId = isset( $_GET['activityId'] ) ? intval( $_GET['activityId'] ) : 0; | ||
|
|
||
| if ( empty( $activityId ) ) { | ||
| \Civi::log()->warning( 'Activity ID is missing' ); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| $activities = \Civi\Api4\Activity::get( false ) | ||
| ->addSelect( 'source_contact_id' ) | ||
| ->addJoin( 'ActivityContact AS activity_contact', 'LEFT' ) | ||
| ->addWhere( 'id', '=', $activityId ) | ||
| ->addWhere( 'activity_type_id:label', '=', 'Material Contribution' ) | ||
| ->execute(); | ||
|
|
||
| if ( $activities->count() === 0 ) { | ||
| \Civi::log()->info( 'No activities found for Activity ID:', array( 'activityId' => $activityId ) ); | ||
| return; | ||
| } | ||
|
|
||
| $activity = $activities->first(); | ||
| $individualId = $activity['source_contact_id']; | ||
|
|
||
| $contact = \Civi\Api4\Contact::get( false ) | ||
| ->addSelect( 'contact_sub_type' ) | ||
| ->addWhere( 'id', '=', $individualId ) | ||
| ->execute() | ||
| ->first(); | ||
|
|
||
| if ( empty( $contact ) ) { | ||
| \Civi::log()->info( 'Contact not found', array( 'contact' => $contact['id'] ) ); | ||
| return; | ||
| } | ||
|
|
||
| $contactSubTypes = $contact['contact_sub_type'] ?? array(); | ||
|
|
||
| // If the individual is already a volunteer, don't show the button | ||
| if ( in_array( 'Volunteer', $contactSubTypes ) ) { | ||
| return; | ||
| } | ||
|
|
||
| $redirectPath = '/volunteer-registration/form-with-details/'; | ||
| $redirectPathWithParams = $redirectPath . '#?' . http_build_query( | ||
| array( | ||
| 'Individual1' => $individualId, | ||
| 'message' => 'individual-user', | ||
| ) | ||
| ); | ||
| $buttonText = __( 'Wish to Volunteer?', 'goonj-crm' ); | ||
|
|
||
| return goonj_generate_button_html( $redirectPathWithParams, $buttonText ); | ||
| } catch ( \Exception $e ) { | ||
| \Civi::log()->error( 'Error in goonj_contribution_volunteer_signup_button: ' . $e->getMessage() ); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to improve separation of concerns and enhance security.
The goonj_contribution_volunteer_signup_button function is handling multiple responsibilities, which violates the Single Responsibility Principle. Consider breaking it down into smaller, focused functions. Additionally, there's a potential security risk with how the activity ID is handled.
- Separate the data retrieval, business logic, and HTML generation into distinct functions.
- Use
filter_input()for safer input handling of the activity ID. - Consider moving the CiviCRM API calls to a separate service class for better organization.
Here's a suggested refactor for the activity ID retrieval:
-$activityId = isset($_GET['activityId']) ? intval($_GET['activityId']) : 0;
+$activityId = filter_input(INPUT_GET, 'activityId', FILTER_VALIDATE_INT) ?: 0;📝 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.
| function goonj_contribution_volunteer_signup_button() { | |
| $activityId = isset($_GET['activityId']) ? intval($_GET['activityId']) : 0; | |
| if (empty($activityId)) { | |
| \Civi::log()->warning('Activity ID is missing'); | |
| return; | |
| } | |
| try { | |
| $activities = \Civi\Api4\Activity::get(FALSE) | |
| ->addSelect('source_contact_id') | |
| ->addJoin('ActivityContact AS activity_contact', 'LEFT') | |
| ->addWhere('id', '=', $activityId) | |
| ->addWhere('activity_type_id:label', '=', 'Material Contribution') | |
| ->execute(); | |
| if ($activities->count() === 0) { | |
| \Civi::log()->info('No activities found for Activity ID:', ['activityId' => $activityId]); | |
| return; | |
| } | |
| $activity = $activities->first(); | |
| $individualId = $activity['source_contact_id']; | |
| $contact = \Civi\Api4\Contact::get(FALSE) | |
| ->addSelect('contact_sub_type') | |
| ->addWhere('id', '=', $individualId) | |
| ->execute() | |
| ->first(); | |
| if (empty($contact)) { | |
| \Civi::log()->info('Contact not found', ['contact' => $contact['id']]); | |
| $activityId = isset( $_GET['activityId'] ) ? intval( $_GET['activityId'] ) : 0; | |
| if ( empty( $activityId ) ) { | |
| \Civi::log()->warning( 'Activity ID is missing' ); | |
| return; | |
| } | |
| try { | |
| $activities = \Civi\Api4\Activity::get( false ) | |
| ->addSelect( 'source_contact_id' ) | |
| ->addJoin( 'ActivityContact AS activity_contact', 'LEFT' ) | |
| ->addWhere( 'id', '=', $activityId ) | |
| ->addWhere( 'activity_type_id:label', '=', 'Material Contribution' ) | |
| ->execute(); | |
| if ( $activities->count() === 0 ) { | |
| \Civi::log()->info( 'No activities found for Activity ID:', array( 'activityId' => $activityId ) ); | |
| return; | |
| } | |
| $activity = $activities->first(); | |
| $individualId = $activity['source_contact_id']; | |
| $contact = \Civi\Api4\Contact::get( false ) | |
| ->addSelect( 'contact_sub_type' ) | |
| ->addWhere( 'id', '=', $individualId ) | |
| ->execute() | |
| ->first(); | |
| if ( empty( $contact ) ) { | |
| \Civi::log()->info( 'Contact not found', array( 'contact' => $contact['id'] ) ); | |
| return; | |
| } | |
| $contactSubTypes = $contact['contact_sub_type'] ?? array(); | |
| // If the individual is already a volunteer, don't show the button | |
| if ( in_array( 'Volunteer', $contactSubTypes ) ) { | |
| return; | |
| } | |
| $redirectPath = '/volunteer-registration/form-with-details/'; | |
| $redirectPathWithParams = $redirectPath . '#?' . http_build_query( | |
| array( | |
| 'Individual1' => $individualId, | |
| 'message' => 'individual-user', | |
| ) | |
| ); | |
| $buttonText = __( 'Wish to Volunteer?', 'goonj-crm' ); | |
| return goonj_generate_button_html( $redirectPathWithParams, $buttonText ); | |
| } catch ( \Exception $e ) { | |
| \Civi::log()->error( 'Error in goonj_contribution_volunteer_signup_button: ' . $e->getMessage() ); | |
| return; | |
| } | |
| } | |
| function goonj_contribution_volunteer_signup_button() { | |
| $activityId = filter_input(INPUT_GET, 'activityId', FILTER_VALIDATE_INT) ?: 0; | |
| if ( empty( $activityId ) ) { | |
| \Civi::log()->warning( 'Activity ID is missing' ); | |
| return; | |
| } | |
| try { | |
| $activities = \Civi\Api4\Activity::get( false ) | |
| ->addSelect( 'source_contact_id' ) | |
| ->addJoin( 'ActivityContact AS activity_contact', 'LEFT' ) | |
| ->addWhere( 'id', '=', $activityId ) | |
| ->addWhere( 'activity_type_id:label', '=', 'Material Contribution' ) | |
| ->execute(); | |
| if ( $activities->count() === 0 ) { | |
| \Civi::log()->info( 'No activities found for Activity ID:', array( 'activityId' => $activityId ) ); | |
| return; | |
| } | |
| $activity = $activities->first(); | |
| $individualId = $activity['source_contact_id']; | |
| $contact = \Civi\Api4\Contact::get( false ) | |
| ->addSelect( 'contact_sub_type' ) | |
| ->addWhere( 'id', '=', $individualId ) | |
| ->execute() | |
| ->first(); | |
| if ( empty( $contact ) ) { | |
| \Civi::log()->info( 'Contact not found', array( 'contact' => $contact['id'] ) ); | |
| return; | |
| } | |
| $contactSubTypes = $contact['contact_sub_type'] ?? array(); | |
| // If the individual is already a volunteer, don't show the button | |
| if ( in_array( 'Volunteer', $contactSubTypes ) ) { | |
| return; | |
| } | |
| $redirectPath = '/volunteer-registration/form-with-details/'; | |
| $redirectPathWithParams = $redirectPath . '#?' . http_build_query( | |
| array( | |
| 'Individual1' => $individualId, | |
| 'message' => 'individual-user', | |
| ) | |
| ); | |
| $buttonText = __( 'Wish to Volunteer?', 'goonj-crm' ); | |
| return goonj_generate_button_html( $redirectPathWithParams, $buttonText ); | |
| } catch ( \Exception $e ) { | |
| \Civi::log()->error( 'Error in goonj_contribution_volunteer_signup_button: ' . $e->getMessage() ); | |
| return; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
wp-content/themes/goonj-crm/functions.php (4)
Line range hint
72-101: Consider enhancing security in login validation.The login validation functions are well-implemented, but there's room for a security improvement:
In the
goonj_check_empty_login_fieldsfunction, consider using WordPress'swp_safe_redirect()instead ofwp_redirect()for added security.Apply this diff to enhance security:
- wp_redirect( $redirect_url ); + wp_safe_redirect( $redirect_url );This change ensures that the redirect URL is safe and prevents potential open redirect vulnerabilities.
Line range hint
103-126: Enhance security in password reset handling.The password reset functions are well-implemented, but there's room for a security improvement:
In the
goonj_custom_password_reset_redirectionfunction, consider usingwp_safe_redirect()instead ofwp_redirect()for added security.Apply this diff to enhance security:
- wp_redirect( add_query_arg( 'password-reset', 'success', home_url() ) ); + wp_safe_redirect( add_query_arg( 'password-reset', 'success', home_url() ) );This change ensures that the redirect URL is safe and prevents potential open redirect vulnerabilities.
Line range hint
129-399: Refactor user identification form handling for improved maintainability.The
goonj_handle_user_identification_formfunction is quite complex and handles multiple responsibilities. Consider refactoring this function to improve readability and maintainability:
- Extract the logic for different purposes into separate functions.
- Use early returns to reduce nesting.
- Consider using a strategy pattern or separate handler classes for different purposes.
Here's a high-level example of how you might start refactoring:
function goonj_handle_user_identification_form() { if (!should_process_form()) { return; } $user_data = get_user_data_from_post(); $purpose_handler = get_purpose_handler($user_data['purpose']); $purpose_handler->handle($user_data); } function should_process_form() { return isset($_POST['action']) && $_POST['action'] === 'goonj-check-user'; } function get_user_data_from_post() { // Extract and return user data from $_POST } function get_purpose_handler($purpose) { $handlers = [ 'material-contribution' => new MaterialContributionHandler(), 'dropping-center' => new DroppingCenterHandler(), // ... other handlers ]; return $handlers[$purpose] ?? new DefaultHandler(); } // Then create separate classes for each purpose handlerThis refactoring would make the code more modular, easier to test, and simpler to maintain.
Line range hint
437-496: Refactor individual creation redirection for improved maintainability.The
goonj_redirect_after_individual_creationfunction is complex and handles multiple responsibilities. Consider refactoring this function to improve readability and maintainability:
- Extract the redirection logic for different creation flows into separate functions.
- Use early returns to reduce nesting.
- Consider using a strategy pattern or separate handler classes for different creation flows.
Here's a high-level example of how you might start refactoring:
function goonj_redirect_after_individual_creation() { if (!should_handle_redirection()) { return; } $individual = get_individual_data(); $redirect_handler = get_redirect_handler($individual['Individual_fields.Creation_Flow']); $redirect_path = $redirect_handler->get_redirect_path($individual); if (empty($redirect_path)) { return; } wp_safe_redirect($redirect_path); exit; } function should_handle_redirection() { return isset($_GET['goonjAction']) && $_GET['goonj_action'] === 'individualCreated' && isset($_GET['individualId']); } function get_individual_data() { // Fetch and return individual data using CiviCRM API } function get_redirect_handler($creation_flow) { $handlers = [ 'material-contribution' => new MaterialContributionRedirectHandler(), 'office-visit' => new OfficeVisitRedirectHandler(), 'office-visit-contribution' => new OfficeVisitContributionRedirectHandler(), ]; return $handlers[$creation_flow] ?? new DefaultRedirectHandler(); } // Then create separate classes for each redirect handlerThis refactoring would make the code more modular, easier to test, and simpler to maintain.
wp-content/themes/goonj-crm/engine/helpers.php (1)
19-19: Enhance readability of the return statementThe return statement in
goonj_get_goonj_office_idis complex and may hinder readability. Simplifying it can make the code more maintainable.Consider refactoring as follows:
if ( array_key_exists( $activityTypeLabel, $officeMapping ) ) { $key = $officeMapping[ $activityTypeLabel ]; return $activity[ $key ] ?? null; } return null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/themes/goonj-crm/engine/helpers.php (1 hunks)
- wp-content/themes/goonj-crm/engine/shortcodes.php (1 hunks)
- wp-content/themes/goonj-crm/functions.php (5 hunks)
🧰 Additional context used
🔇 Additional comments (6)
wp-content/themes/goonj-crm/functions.php (6)
Line range hint
3-41: LGTM: File inclusions and script enqueuing look good.The file inclusions and script enqueuing functions follow WordPress best practices. The code is well-organized and modular.
Line range hint
44-47: LGTM: Theme setup function is appropriate.The
goonj_theme_setupfunction correctly adds an editor style, following WordPress conventions.
Line range hint
51-70: LGTM: User redirection functions are well-implemented.The
goonj_redirect_logged_in_user_to_civi_dashboardandgoonj_custom_login_failed_redirectfunctions handle user redirections appropriately, following WordPress conventions.
Line range hint
401-423: LGTM: Volunteer induction checking function is well-implemented.The
goonj_is_volunteer_inductedfunction effectively uses the CiviCRM API to check for completed induction activities. The logic is clear and concise.
Line range hint
425-427: LGTM: Custom message placeholder function is appropriate.The
goonj_custom_message_placeholderfunction provides a simple and clear way to insert a custom message container in the HTML.
Line range hint
429-435: LGTM: Query variables function is well-implemented.The
goonj_query_varsfunction correctly adds custom query variables following WordPress conventions.
| $officeMapping = array( | ||
| 'Material Contribution' => 'Material_Contribution.Goonj_Office', | ||
| 'Office visit' => 'Office_Visit.Goonj_Processing_Center', | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate activity type mappings to adhere to DRY principles
The arrays $officeMapping and $activityMap both map activity types to related data, leading to duplicate code. Merging these mappings enhances maintainability and reduces redundancy.
Consider unifying the mappings:
$activityMap = array(
'Office visit' => array(
'field' => 'Office_Visit.Goonj_Processing_Center',
'redirectPath' => '/processing-center/office-visit/details/',
'buttonText' => __( 'Proceed to Office Visit', 'goonj-crm' ),
),
'Material Contribution' => array(
'field' => 'Material_Contribution.Goonj_Office',
'redirectPath' => '/processing-center/material-contribution/details/',
'buttonText' => __( 'Proceed to Material Contribution', 'goonj-crm' ),
),
);Update goonj_get_goonj_office_id:
function goonj_get_goonj_office_id( $activity ) {
global $activityMap;
$activityTypeLabel = $activity['activity_type_id:name'];
$field = $activityMap[ $activityTypeLabel ]['field'] ?? null;
return $field ? ( $activity[ $field ] ?? null ) : null;
}And adjust goonj_generate_activity_button accordingly.
Also applies to: 79-90
| $timezone = new \DateTimeZone( 'UTC' ); | ||
| $today = new \DateTime( 'now', $timezone ); | ||
| $startOfDay = $today->setTime( 0, 0 )->format( 'Y-m-d H:i:s' ); | ||
| $endOfDay = $today->setTime( 23, 59, 59 )->format( 'Y-m-d H:i:s' ); |
There was a problem hiding this comment.
Fix DateTime mutation causing incorrect date ranges
In goonj_fetch_contact_pu_activities_for_today, the use of $today->setTime() modifies the original $today object. This results in both $startOfDay and $endOfDay having the end time of the day, leading to incorrect activity retrieval.
Apply this diff to correct the date range calculation:
- $startOfDay = $today->setTime( 0, 0 )->format( 'Y-m-d H:i:s' );
- $endOfDay = $today->setTime( 23, 59, 59 )->format( 'Y-m-d H:i:s' );
+ $startOfDay = (clone $today)->setTime( 0, 0 )->format( 'Y-m-d H:i:s' );
+ $endOfDay = (clone $today)->setTime( 23, 59, 59 )->format( 'Y-m-d H:i:s' );📝 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.
| $timezone = new \DateTimeZone( 'UTC' ); | |
| $today = new \DateTime( 'now', $timezone ); | |
| $startOfDay = $today->setTime( 0, 0 )->format( 'Y-m-d H:i:s' ); | |
| $endOfDay = $today->setTime( 23, 59, 59 )->format( 'Y-m-d H:i:s' ); | |
| $timezone = new \DateTimeZone( 'UTC' ); | |
| $today = new \DateTime( 'now', $timezone ); | |
| $startOfDay = (clone $today)->setTime( 0, 0 )->format( 'Y-m-d H:i:s' ); | |
| $endOfDay = (clone $today)->setTime( 23, 59, 59 )->format( 'Y-m-d H:i:s' ); |
| function goonj_collection_camp_landing_page() { | ||
| ob_start(); | ||
| get_template_part( 'templates/collection-landing-page' ); | ||
| return ob_get_clean(); | ||
| } | ||
|
|
||
| function goonj_collection_camp_past_data() { | ||
| ob_start(); | ||
| get_template_part( 'templates/collection-camp-data' ); | ||
| return ob_get_clean(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code to enhance maintainability
The functions goonj_collection_camp_landing_page and goonj_collection_camp_past_data have similar implementations. Refactor them to reduce code duplication and adhere to the Single Responsibility Principle.
Consider creating a generic function:
function goonj_render_template( $template_name ) {
ob_start();
get_template_part( $template_name );
return ob_get_clean();
}
function goonj_collection_camp_landing_page() {
return goonj_render_template( 'templates/collection-landing-page' );
}
function goonj_collection_camp_past_data() {
return goonj_render_template( 'templates/collection-camp-data' );
}| $activities = \Civi\Api4\Activity::get( false ) | ||
| ->addSelect( 'source_contact_id' ) | ||
| ->addJoin( 'ActivityContact AS activity_contact', 'LEFT' ) | ||
| ->addWhere( 'id', '=', $activity_id ) | ||
| ->addWhere( 'activity_type_id:label', '=', 'Material Contribution' ) | ||
| ->execute(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate API calls to improve code readability and reusability
The API calls within goonj_contribution_volunteer_signup_button and goonj_pu_activity_button are directly implemented within the functions. Consider encapsulating these API calls into separate functions or classes to enhance readability and promote code reuse.
This refactoring aligns with best practices by separating concerns and making the codebase easier to maintain.
Also applies to: 95-101
| function goonj_check_user_action( $atts ) { | ||
| get_template_part( 'templates/form', 'check-user', array( 'purpose' => $atts['purpose'] ) ); | ||
| return ob_get_clean(); | ||
| } |
There was a problem hiding this comment.
Add missing ob_start() to initiate output buffering
In the goonj_check_user_action function, ob_start() is not called before get_template_part(), but ob_get_clean() is used afterward. This could lead to unexpected behavior or empty output because the output buffer is not initiated.
Apply this diff to fix the issue:
function goonj_check_user_action( $atts ) {
+ ob_start();
get_template_part( 'templates/form', 'check-user', array( 'purpose' => $atts['purpose'] ) );
return ob_get_clean();
}📝 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.
| function goonj_check_user_action( $atts ) { | |
| get_template_part( 'templates/form', 'check-user', array( 'purpose' => $atts['purpose'] ) ); | |
| return ob_get_clean(); | |
| } | |
| function goonj_check_user_action( $atts ) { | |
| ob_start(); | |
| get_template_part( 'templates/form', 'check-user', array( 'purpose' => $atts['purpose'] ) ); | |
| return ob_get_clean(); | |
| } |
| if ( empty( $contact ) ) { | ||
| \Civi::log()->info( 'Contact not found', array( 'contact' => $contact['id'] ) ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Prevent accessing undefined index when $contact is empty
When $contact is empty, the code attempts to access $contact['id'] in the log message, which can cause a PHP notice due to an undefined index.
Apply this diff to fix the issue:
if ( empty( $contact ) ) {
- \Civi::log()->info( 'Contact not found', array( 'contact' => $contact['id'] ) );
+ \Civi::log()->info( 'Contact not found', array( 'contact_id' => $individual_id ) );
return;
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range and nitpick comments (14)
wp-content/civi-extensions/goonjcustom/Civi/HelperService.php (2)
5-8: Improve class documentation and consider a more specific class name.The current class documentation "Collection Camp Outcome Service" doesn't accurately reflect the purpose of the
HelperServiceclass. Additionally, the class nameHelperServiceis quite generic.Consider the following improvements:
- Update the class documentation to accurately describe the purpose of this service class.
- Consider renaming the class to something more specific, like
EmailHelperServiceorCommunicationHelperService, to better reflect its functionality.
10-16: Enhance method documentation and LGTM for implementation.The
getDefaultFromEmail()method implementation looks good. However, the documentation could be more descriptive.Consider updating the method documentation to:
/** * Get the default "From" email address. * * @return string Formatted email address in the form "Name" <email@example.com> */This provides more context about what the method does and what it returns.
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (3)
7-11: Add a PHPDoc comment for the CollectionSource trait.While the trait name is descriptive, it would be beneficial to add a PHPDoc comment explaining the purpose and functionality of the
CollectionSourcetrait. This will improve code documentation and make it easier for other developers to understand and use the trait.Here's a suggested PHPDoc comment:
/** * Trait CollectionSource * * This trait provides functionality for managing and retrieving subtype IDs * within the CiviCRM framework. It includes methods for caching and checking * subtype IDs to optimize performance in subtype-related operations. */ trait CollectionSource { // ... existing code ... }
13-27: Enhance documentation and consider constant definitions for getSubtypeId() method.The
getSubtypeId()method implements good practices like caching and clean API query construction. However, there are a few areas for improvement:
- Add a PHPDoc comment explaining the method's purpose, return value, and any exceptions it might throw.
- The constants
ENTITY_NAMEandENTITY_SUBTYPE_NAMEare used but not defined in this file. Consider adding them to the trait or documenting where they should be defined.Here's a suggested improvement:
/** * Get the subtype ID for the current entity. * * This method caches the subtype ID for improved performance in subsequent calls. * * @return int The subtype ID. * @throws \Civi\API\Exception\UnauthorizedException If the API call fails due to permissions. */ public static function getSubtypeId() { // ... existing code ... }Also, consider adding a comment explaining where
ENTITY_NAMEandENTITY_SUBTYPE_NAMEshould be defined:// Note: ENTITY_NAME and ENTITY_SUBTYPE_NAME should be defined in the class using this trait.
29-39: Improve documentation for isCurrentSubtype() method.The
isCurrentSubtype()method implements good practices like early return and type casting. However, it would benefit from better documentation:
- Add a PHPDoc comment explaining the method's purpose, parameters, and return value.
- Consider using more explicit type checking for
$objectRef['subtype'].Here's a suggested improvement:
/** * Check if the provided object reference matches the current subtype. * * @param array $objectRef The object reference to check. * @return bool TRUE if the object reference matches the current subtype, FALSE otherwise. */ private static function isCurrentSubtype($objectRef) { if (!isset($objectRef['subtype']) || $objectRef['subtype'] === '') { return FALSE; } $subtypeId = self::getSubtypeId(); return (int) $objectRef['subtype'] === $subtypeId; }This improvement adds clear documentation and uses more explicit type checking for the
subtypekey in the$objectRefarray.wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (3)
62-66: Include stack trace in error logs for better debuggingCurrently, only the exception message is logged. Including the stack trace can provide more context and assist in diagnosing issues more effectively.
Modify the logging statement to include the stack trace:
\Civi::log()->error('Error processing camp reminder', [ 'id' => $camp['id'], 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), ]);
37-37: Remove unused variable$returnValuesThe variable
$returnValuesis initialized but not utilized within the function. Removing it can simplify the code and eliminate unnecessary variables.You can remove the variable and adjust the return statement:
-$returnValues = [];-return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'collection_camp_outcome_reminder_cron'); +return civicrm_api3_create_success([], $params, 'Goonjcustom', 'collection_camp_outcome_reminder_cron');Also applies to: 69-69
57-67: Handle exceptions more granularlyCurrently, if an exception occurs while processing a camp reminder, the exception is caught, and the loop continues. Consider implementing additional logic to retry the operation or escalate the issue if certain types of exceptions occur.
Evaluate the types of exceptions that may be thrown and decide if some require immediate attention or different handling strategies. This can improve the robustness of the cron job.
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackReminderCron.php (1)
40-70: Add Unit Tests for the Volunteer Feedback Reminder CronUnit tests would help ensure that the
civicrm_api3_goonjcustom_volunteer_feedback_reminder_cronfunction behaves as expected under various scenarios, such as when there are camps needing reminders, when exceptions occur, and when there are no camps to process.Would you like me to assist in creating unit tests for this function or open a new GitHub issue to track this task?
wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php (2)
9-11: Add descriptive PHPDoc comments for the trait and its methodsThe trait
QrCodeableand its methodsgenerateQrCodeandsaveQrCodelack detailed PHPDoc comments. Adding comprehensive documentation, including descriptions of parameters, return values, and any exceptions thrown, will enhance code readability and maintainability.Also applies to: 14-16, 47-49
70-73: Handle missing custom field more explicitlyWhen the custom field is not found, the code logs an error and returns
FALSE. To improve robustness:
- Consider throwing an exception to clearly indicate that a critical configuration is missing.
- Provide a more detailed error message, including the
customGroupNameandcustomFieldName, to assist in troubleshooting.This will make it easier to identify configuration issues during development and maintenance.
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (1)
61-71: Ensure proper URL concatenation for$dataWhen constructing the
$dataURL ingenerateDroppingCenterQrCode, ensure that$baseUrlends with a trailing slash to avoid malformed URLs. This attention to detail prevents potential issues with URL formation.Consider updating the code as follows:
$baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL; + $baseUrl = rtrim($baseUrl, '/') . '/'; $data = "{$baseUrl}actions/dropping-center/{$id}";Alternatively, use built-in functions to generate URLs if available.
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
Line range hint
26-550: RefactorCollectionCampServiceto adhere to the Single Responsibility PrincipleThe
CollectionCampServiceclass (lines 26-550) currently handles multiple responsibilities, including event subscriptions, QR code generation, email notifications, address handling, and more. This violates the Single Responsibility Principle, making the codebase harder to maintain, test, and extend.Consider refactoring the class into smaller, focused classes or services, each responsible for a specific domain of functionality. For example:
- QR Code Generation: Extract QR code-related methods into a separate
QrCodeServiceclass.- Email Notification: Create an
EmailNotificationServiceto handle all email-related functionality.- Event Handling: Use dedicated event subscriber classes for different types of events.
This separation will improve code readability, facilitate unit testing, and enhance maintainability.
20-21: Organize trait imports and usage consistentlyThe
usestatements for traitsCollectionSourceandQrCodeableare added in lines 20-21 and used in lines 27-28. For better code organization and readability, maintain a consistent order and grouping for imports and trait uses.Consider placing the
usestatements alphabetically or grouping related traits together to improve clarity.Also applies to: 27-28
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (5 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampVolunteerFeedbackService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (3 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/HelperService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackReminderCron.php (1 hunks)
- wp-content/themes/goonj-crm/engine/helpers.php (1 hunks)
- wp-content/themes/goonj-crm/engine/shortcodes.php (1 hunks)
🧰 Additional context used
📓 Learnings (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-08T09:24:47.239Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-09T05:09:48.296Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (2)
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-08T09:24:47.239Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-09T05:09:48.296Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
🔇 Additional comments (9)
wp-content/civi-extensions/goonjcustom/Civi/HelperService.php (2)
1-3: LGTM! Namespace declaration looks good.The PHP opening tag and namespace declaration follow best practices and PSR-4 autoloading standards.
1-18: LGTM! Consider future expansions for this helper service.The overall structure of the file is clean and follows the Single Responsibility Principle. As this is a helper service, consider the following suggestions for future improvements:
- Add more related helper methods to this class as needed, keeping them focused on email or communication-related functionality.
- If the class grows significantly, consider breaking it down into more specific helper classes.
- Add unit tests for the
getDefaultFromEmail()method and any future methods to ensure reliability.These suggestions are for future consideration and don't need to be addressed in this PR.
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (1)
1-5: LGTM: Namespace and use statement are properly defined.The namespace
Civi\Traitsand the use statement forCivi\Api4\OptionValueare correctly defined, following PSR-4 autoloading standards. This setup indicates proper integration with CiviCRM's API v4.wp-content/themes/goonj-crm/engine/helpers.php (1)
6-9: Consider consolidating activity type mappingsAs mentioned in a previous review, the arrays
$officeMappingand$activityMapboth map activity types to related data, leading to duplicate code. Merging these mappings would enhance maintainability and reduce redundancy.Consider unifying the mappings as suggested in the previous review:
$activityMap = array( 'Office visit' => array( 'field' => 'Office_Visit.Goonj_Processing_Center', 'redirectPath' => '/processing-center/office-visit/details/', 'buttonText' => __( 'Proceed to Office Visit', 'goonj-crm' ), 'queryParam' => 'Office_Visit.Goonj_Processing_Center', ), 'Material Contribution' => array( 'field' => 'Material_Contribution.Goonj_Office', 'redirectPath' => '/processing-center/material-contribution/details/', 'buttonText' => __( 'Proceed to Material Contribution', 'goonj-crm' ), 'queryParam' => 'Material_Contribution.Goonj_Office', ), );Then update both
goonj_get_goonj_office_idandgoonj_generate_activity_buttonto use this unified mapping.Also applies to: 28-39
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (1)
54-54: Ensure consistent timezone when comparing dates in queriesWhen comparing
End_Datefrom the database with$endOfDay, it's crucial to ensure that both are in the same timezone to avoid inaccuracies in the query results due to timezone discrepancies.Please verify that
Collection_Camp_Intent_Details.End_Dateis stored in UTC or the same timezone as$endOfDay. If not, you may need to adjust the date comparison or convert timezones accordingly.wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php (1)
19-31:⚠️ Potential issueVerify the handling of QR code output format
The code removes a base64 header and decodes the image data after generating the QR code. However, when using
QRCode::OUTPUT_IMAGE_PNG, therendermethod should return raw binary PNG data, not a base64-encoded string with a header. This suggests there may be a misconfiguration or misunderstanding of the QRCode library's output options.Run the following script to inspect the usage of QR code output options in the codebase:
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (1)
93-99: Sanitize variables in email content to prevent XSS vulnerabilities.Variables like
$attendeeNameare inserted into the email HTML without sanitization, which can lead to XSS attacks if the data contains malicious input.[security]
Use
htmlspecialchars()to sanitize user input:-$html = " - <p>Dear $attendeeName,</p> - <p>This is a kind reminder to complete the Camp Outcome Form at the earliest. Your feedback is crucial in helping us assess the overall response and impact of the camp/drive.</p> - <p>You can access the form using the link below:</p> - <p><a href=\"$campOutcomeFormUrl\">Camp Outcome Form</a></p> - <p>We appreciate your cooperation.</p> - <p>Warm Regards,<br>Urban Relations Team</p>"; +$safeAttendeeName = htmlspecialchars($attendeeName, ENT_QUOTES, 'UTF-8'); +$safeCampOutcomeFormUrl = htmlspecialchars($campOutcomeFormUrl, ENT_QUOTES, 'UTF-8'); +$html = " + <p>Dear $safeAttendeeName,</p> + <p>This is a kind reminder to complete the Camp Outcome Form at the earliest. Your feedback is crucial in helping us assess the overall response and impact of the camp/drive.</p> + <p>You can access the form using the link below:</p> + <p><a href=\"$safeCampOutcomeFormUrl\">Camp Outcome Form</a></p> + <p>We appreciate your cooperation.</p> + <p>Warm Regards,<br>Urban Relations Team</p>";wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
470-472: EnsureisCurrentSubtypemethod handles exceptions and null valuesIn the
generateCollectionCampQrmethod (lines 470-472), the condition uses!self::isCurrentSubtype($objectRef). IfisCurrentSubtypedoes not handle null values or exceptions properly, it could cause unexpected behavior or errors.Verify that
isCurrentSubtypeis robust against null or malformed$objectRefand includes necessary exception handling:private static function isCurrentSubtype($objectRef) { if (!$objectRef || !isset($objectRef['subtype'])) { return FALSE; } // Additional logic... }
546-546: 🛠️ Refactor suggestionAvoid code duplication when regenerating QR codes
In the
reGenerateCollectionCampQrmethod (line 546), the call toself::generateCollectionCampQrCode($collectionCampId);duplicates the logic used elsewhere for QR code generation.To streamline the code, consider centralizing QR code generation logic in a single method or service. This reduces redundancy and simplifies maintenance.
⛔ Skipped due to learnings
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-09T05:09:48.296Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-08T09:24:47.239Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
| function goonj_get_goonj_office_id( $activity ) { | ||
| $activityTypeName = $activity['activity_type_id:name']; | ||
| $officeMapping = array( | ||
| 'Material Contribution' => 'Material_Contribution.Goonj_Office', | ||
| 'Office visit' => 'Office_Visit.Goonj_Processing_Center', | ||
| ); | ||
| $officeCustomFieldName = $officeMapping[$activityTypeName]; | ||
|
|
||
| return $activity[$officeCustomFieldName]; | ||
| // return array_key_exists( $activityTypeName, $officeMapping ) ? $activity[ $officeMapping[ $activityTypeName ] ] ?? null : null; | ||
| } |
There was a problem hiding this comment.
Improve error handling and use the safer implementation
The current implementation of goonj_get_goonj_office_id function lacks proper error handling for unknown activity types. This could lead to undefined index errors if an unexpected activity type is provided.
Consider implementing the following improvements:
- Use the commented-out line as it provides better error handling:
return array_key_exists( $activityTypeName, $officeMapping ) ? $activity[ $officeMapping[ $activityTypeName ] ] ?? null : null;- Add a logging mechanism to track occurrences of unknown activity types:
if (!array_key_exists($activityTypeName, $officeMapping)) {
error_log("Unknown activity type encountered: " . $activityTypeName);
return null;
}These changes will make the function more robust and easier to debug in case of issues.
| $details['queryParam'] => $office_id, | ||
| ); | ||
|
|
||
| $redirectPathWithParams = $details['redirectPath'] . '#?' . http_build_query( $redirectParams ); |
There was a problem hiding this comment.
Fix URL construction for proper query parameter handling
The current URL construction in the goonj_generate_activity_button function uses '#?' which is incorrect for query parameters. This may lead to improper redirection.
Please update the URL construction as follows:
$redirectPathWithParams = $details['redirectPath'] . '?' . http_build_query( $redirectParams );This change ensures that the query parameters are properly appended to the URL.
| function goonj_generate_activity_button( $activity, $office_id, $individual_id ) { | ||
| // Define all activity types to check pending activities | ||
| $activityTypes = array( 'Office visit', 'Material Contribution' ); | ||
|
|
||
| // Activity type that contact has already completed | ||
| $completedActivityType = $activity['activity_type_id:name']; | ||
|
|
||
| // Calculate the pending activities by comparing with $activityTypes | ||
| $pendingActivities = array_diff( $activityTypes, array( $completedActivityType ) ); | ||
|
|
||
| // Define the mapping for each activity to the corresponding button and redirect info | ||
| $activityMap = array( | ||
| 'Office visit' => array( | ||
| 'redirectPath' => '/processing-center/office-visit/details/', | ||
| 'buttonText' => __( 'Proceed to Office Visit', 'goonj-crm' ), | ||
| 'queryParam' => 'Office_Visit.Goonj_Processing_Center', | ||
| ), | ||
| 'Material Contribution' => array( | ||
| 'redirectPath' => '/processing-center/material-contribution/details/', | ||
| 'buttonText' => __( 'Proceed to Material Contribution', 'goonj-crm' ), | ||
| 'queryParam' => 'Material_Contribution.Goonj_Office', | ||
| ), | ||
| ); | ||
|
|
||
| // Get the next pending activity | ||
| $nextActivity = reset( $pendingActivities ); | ||
| $details = $activityMap[ $nextActivity ]; | ||
|
|
||
| // Prepare the URL with query params | ||
| $redirectParams = array( | ||
| 'source_contact_id' => $individual_id, | ||
| $details['queryParam'] => $office_id, | ||
| ); | ||
|
|
||
| $redirectPathWithParams = $details['redirectPath'] . '#?' . http_build_query( $redirectParams ); | ||
|
|
||
| // Generate and return the button HTML | ||
| return goonj_generate_button_html( $redirectPathWithParams, $details['buttonText'] ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve function robustness and readability
The goonj_generate_activity_button function could benefit from some improvements:
- Handle the case where all activities are completed:
if (empty($pendingActivities)) {
return ''; // or some appropriate message
}-
Consider refactoring the activity mapping into a separate constant or configuration file to improve maintainability.
-
Use early returns to reduce nesting and improve readability:
if (empty($pendingActivities)) {
return '';
}
$nextActivity = reset($pendingActivities);
if (!isset($activityMap[$nextActivity])) {
error_log("Unknown activity type encountered: " . $nextActivity);
return '';
}
$details = $activityMap[$nextActivity];
// ... rest of the functionThese changes will make the function more robust and easier to maintain.
| $collectionCamps = EckEntity::get('Collection_Camp', TRUE) | ||
| ->addSelect( | ||
| 'Logistics_Coordination.Camp_to_be_attended_by', | ||
| 'Collection_Camp_Intent_Details.End_Date', | ||
| 'Camp_Outcome.Last_Reminder_Sent', | ||
| 'title', | ||
| 'Collection_Camp_Intent_Details.Location_Area_of_camp', | ||
| ) | ||
| ->addWhere('Camp_Outcome.Rate_the_camp', 'IS NULL') | ||
| ->addWhere('Logistics_Coordination.Email_Sent', '=', 1) | ||
| ->addWhere('Collection_Camp_Core_Details.Status', '=', 'authorized') | ||
| ->addWhere('Collection_Camp_Intent_Details.End_Date', '<=', $endOfDay) | ||
| ->execute(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Abstract query conditions for maintainability
Hardcoding strings like 'authorized' and field names directly in the query can make future modifications error-prone. Consider defining constants or using a configuration file for such values to enhance maintainability and reduce the likelihood of typos.
Define a constant for the status:
const STATUS_AUTHORIZED = 'authorized';Update the query condition:
->addWhere('Collection_Camp_Core_Details.Status', '=', 'authorized')
->addWhere('Collection_Camp_Core_Details.Status', '=', self::STATUS_AUTHORIZED)| catch (\Exception $e) { | ||
| \Civi::log()->error('Error processing camp reminder', [ | ||
| 'id' => $camp['id'], | ||
| 'error' => $e->getMessage(), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider catching \Throwable instead of \Exception
Catching \Throwable allows you to handle all types of errors and exceptions, including those that do not extend from \Exception, such as \Error. This ensures that all possible issues are caught and logged.
Update the catch block as follows:
- catch (\Exception $e) {
+ catch (\Throwable $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.
| catch (\Exception $e) { | |
| \Civi::log()->error('Error processing camp reminder', [ | |
| 'id' => $camp['id'], | |
| 'error' => $e->getMessage(), | |
| ]); | |
| } | |
| catch (\Throwable $e) { | |
| \Civi::log()->error('Error processing camp reminder', [ | |
| 'id' => $camp['id'], | |
| 'error' => $e->getMessage(), | |
| ]); | |
| } |
| $entity = EckEntity::get(self::ENTITY_NAME, TRUE) | ||
| ->addWhere('id', '=', $entityId) | ||
| ->execute(); | ||
|
|
||
| $entity = $entityResults->first(); | ||
| ->execute()->single(); | ||
|
|
||
| $entitySubtypeValue = $entity['subtype']; | ||
| $subtypeId = self::getSubtypeId(); | ||
|
|
||
| $subtypeResults = OptionValue::get(TRUE) | ||
| ->addSelect('name') | ||
| ->addWhere('grouping', '=', self::ENTITY_NAME) | ||
| ->addWhere('value', '=', $entitySubtypeValue) | ||
| ->execute(); | ||
|
|
||
| $subtype = $subtypeResults->first(); | ||
|
|
||
| if (!$subtype) { | ||
| return FALSE; | ||
| } | ||
|
|
||
| $subtypeName = $subtype['name']; | ||
|
|
||
| if ($subtypeName !== self::ENTITY_SUBTYPE_NAME) { | ||
| return FALSE; | ||
| } | ||
|
|
||
| return TRUE; | ||
| return (int) $entitySubtypeValue === $subtypeId; |
There was a problem hiding this comment.
Ensure entity existence before accessing its subtype
In the isViewingDroppingCenter method, if $entity is null, attempting to access $entity['subtype'] will cause an error. Checking whether $entity is not null before accessing its properties enhances code reliability.
Apply this diff to add a null check:
$entity = EckEntity::get(self::ENTITY_NAME, TRUE)
->addWhere('id', '=', $entityId)
->execute()->single();
+ if (!$entity) {
+ return FALSE;
+ }Committable suggestion was skipped due to low confidence.
| public static function generateDroppingCenterQr(string $op, string $objectName, $objectId, &$objectRef) { | ||
| if ($objectName !== 'Eck_Collection_Camp' || !$objectId || !self::isCurrentSubtype($objectRef)) { | ||
| return; | ||
| } | ||
|
|
||
| $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; | ||
| if (!$newStatus) { | ||
| return; | ||
| } | ||
|
|
||
| $collectionCamps = EckEntity::get('Collection_Camp', TRUE) | ||
| ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id') | ||
| ->addWhere('id', '=', $objectId) | ||
| ->execute(); | ||
|
|
||
| $currentCollectionCamp = $collectionCamps->first(); | ||
| $currentStatus = $currentCollectionCamp['Collection_Camp_Core_Details.Status']; | ||
| $collectionCampId = $currentCollectionCamp['id']; | ||
|
|
||
| // Check for status change. | ||
| if ($currentStatus !== $newStatus && $newStatus === 'authorized') { | ||
| self::generateDroppingCenterQrCode($collectionCampId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential null value for $currentCollectionCamp
In the generateDroppingCenterQr method, after fetching $currentCollectionCamp, there is no check to ensure it's not null before accessing its properties. This could lead to a null pointer exception if the entity is not found. It's best practice to verify that $currentCollectionCamp is not null before proceeding.
Apply this diff to add a null check:
$currentCollectionCamp = $collectionCamps->first();
+ if (!$currentCollectionCamp) {
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function generateDroppingCenterQr(string $op, string $objectName, $objectId, &$objectRef) { | |
| if ($objectName !== 'Eck_Collection_Camp' || !$objectId || !self::isCurrentSubtype($objectRef)) { | |
| return; | |
| } | |
| $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; | |
| if (!$newStatus) { | |
| return; | |
| } | |
| $collectionCamps = EckEntity::get('Collection_Camp', TRUE) | |
| ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id') | |
| ->addWhere('id', '=', $objectId) | |
| ->execute(); | |
| $currentCollectionCamp = $collectionCamps->first(); | |
| $currentStatus = $currentCollectionCamp['Collection_Camp_Core_Details.Status']; | |
| $collectionCampId = $currentCollectionCamp['id']; | |
| // Check for status change. | |
| if ($currentStatus !== $newStatus && $newStatus === 'authorized') { | |
| self::generateDroppingCenterQrCode($collectionCampId); | |
| } | |
| } | |
| public static function generateDroppingCenterQr(string $op, string $objectName, $objectId, &$objectRef) { | |
| if ($objectName !== 'Eck_Collection_Camp' || !$objectId || !self::isCurrentSubtype($objectRef)) { | |
| return; | |
| } | |
| $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; | |
| if (!$newStatus) { | |
| return; | |
| } | |
| $collectionCamps = EckEntity::get('Collection_Camp', TRUE) | |
| ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id') | |
| ->addWhere('id', '=', $objectId) | |
| ->execute(); | |
| $currentCollectionCamp = $collectionCamps->first(); | |
| if (!$currentCollectionCamp) { | |
| return; | |
| } | |
| $currentStatus = $currentCollectionCamp['Collection_Camp_Core_Details.Status']; | |
| $collectionCampId = $currentCollectionCamp['id']; | |
| // Check for status change. | |
| if ($currentStatus !== $newStatus && $newStatus === 'authorized') { | |
| self::generateDroppingCenterQrCode($collectionCampId); | |
| } | |
| } |
| $entity = EckEntity::get(self::ENTITY_NAME, TRUE) | ||
| ->addWhere('id', '=', $entityId) | ||
| ->execute(); | ||
|
|
||
| $entity = $entityResults->first(); | ||
| ->execute()->single(); | ||
|
|
||
| $entitySubtypeValue = $entity['subtype']; | ||
|
|
||
| $subtypeResults = OptionValue::get(TRUE) | ||
| ->addSelect('name') | ||
| ->addWhere('grouping', '=', self::ENTITY_NAME) | ||
| ->addWhere('value', '=', $entitySubtypeValue) | ||
| ->execute(); | ||
|
|
||
| $subtype = $subtypeResults->first(); | ||
|
|
||
| if (!$subtype) { | ||
| return FALSE; | ||
| } | ||
|
|
||
| $subtypeName = $subtype['name']; | ||
|
|
||
| if ($subtypeName !== self::ENTITY_SUBTYPE_NAME) { | ||
| return FALSE; | ||
| } | ||
| $subtypeId = self::getSubtypeId(); | ||
|
|
||
| return TRUE; | ||
| return (int) $entitySubtypeValue === $subtypeId; |
There was a problem hiding this comment.
Add null checks to prevent potential null reference errors
In the isViewingCollectionCamp method (lines 131-139), the $entity retrieved may be null if the query returns no results. Accessing $entity['subtype'] without checking can lead to a null reference error.
Ensure that $entity is not null before accessing its properties:
$entity = EckEntity::get(self::ENTITY_NAME, TRUE)
->addWhere('id', '=', $entityId)
->execute()
->single();
if (!$entity) {
return FALSE;
}
$entitySubtypeValue = $entity['subtype'];This check prevents potential runtime errors when the entity is not found.
| private static function generateCollectionCampQrCode($id) { | ||
| $baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL; | ||
| $data = "{$baseUrl}actions/collection-camp/{$id}"; | ||
|
|
||
| $baseFileName = "qr_code_{$collectionCampId}.png"; | ||
|
|
||
| $fileName = \CRM_Utils_File::makeFileName($baseFileName); | ||
|
|
||
| $tempFilePath = \CRM_Utils_File::tempnam($baseFileName); | ||
|
|
||
| $numBytes = file_put_contents($tempFilePath, $qrcode); | ||
|
|
||
| if (!$numBytes) { | ||
| \CRM_Core_Error::debug_log_message('Failed to write QR code to temporary file for collection camp ID ' . $collectionCampId); | ||
| return FALSE; | ||
| } | ||
|
|
||
| $customFields = CustomField::get(FALSE) | ||
| ->addSelect('id') | ||
| ->addWhere('custom_group_id:name', '=', 'Collection_Camp_QR_Code') | ||
| ->addWhere('name', '=', 'QR_Code') | ||
| ->setLimit(1) | ||
| ->execute(); | ||
|
|
||
| $qrField = $customFields->first(); | ||
|
|
||
| if (!$qrField) { | ||
| \CRM_Core_Error::debug_log_message('No field to save QR Code for collection camp ID ' . $collectionCampId); | ||
| return FALSE; | ||
| } | ||
|
|
||
| $qrFieldId = 'custom_' . $qrField['id']; | ||
|
|
||
| // Save the QR code as an attachment linked to the collection camp. | ||
| $params = [ | ||
| 'entity_id' => $collectionCampId, | ||
| 'name' => $fileName, | ||
| 'mime_type' => 'image/png', | ||
| 'field_name' => $qrFieldId, | ||
| 'options' => [ | ||
| 'move-file' => $tempFilePath, | ||
| ], | ||
| ]; | ||
|
|
||
| $result = civicrm_api3('Attachment', 'create', $params); | ||
|
|
||
| if (empty($result['id'])) { | ||
| \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId); | ||
| return FALSE; | ||
| } | ||
|
|
||
| $attachment = $result['values'][$result['id']]; | ||
| $saveOptions = [ | ||
| 'customGroupName' => 'Collection_Camp_QR_Code', | ||
| 'customFieldName' => 'QR_Code', | ||
| ]; | ||
|
|
||
| $attachmentUrl = $attachment['url']; | ||
| } | ||
| catch (\CiviCRM_API3_Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error generating QR code: ' . $e->getMessage()); | ||
| return FALSE; | ||
| } | ||
| self::generateQrCode($data, $id, $saveOptions); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unnecessary wrapper methods to reduce code redundancy
The generateCollectionCampQrCode method (lines 501-510) acts as a thin wrapper around generateQrCode, adding minimal additional logic. This may introduce unnecessary complexity and code duplication.
Consider refactoring by directly calling generateQrCode with the appropriate parameters where needed, unless generateCollectionCampQrCode is anticipated to include more logic in the future:
// Instead of calling generateCollectionCampQrCode($id), call generateQrCode($data, $id, $saveOptions) directly.This adheres to the DRY (Don't Repeat Yourself) principle and simplifies the codebase.
| $baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL; | ||
| $data = "{$baseUrl}actions/collection-camp/{$id}"; |
There was a problem hiding this comment.
Ensure $baseUrl ends with a slash to form correct URLs
In the generateCollectionCampQrCode method (lines 502-503), the $data URL may be incorrect if $baseUrl does not end with a slash. This can lead to malformed URLs and unreachable QR code links.
To prevent this issue, ensure that $baseUrl ends with a slash:
$baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL;
+$baseUrl = rtrim($baseUrl, '/') . '/';
$data = "{$baseUrl}actions/collection-camp/{$id}";This modification guarantees the correct concatenation of the base URL and the endpoint.
📝 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.
| $baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL; | |
| $data = "{$baseUrl}actions/collection-camp/{$id}"; | |
| $baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL; | |
| $baseUrl = rtrim($baseUrl, '/') . '/'; | |
| $data = "{$baseUrl}actions/collection-camp/{$id}"; |
|
Closing this PR due to extra changes moved changes in this pr - #368 |
3 participants
Target Issue: #78
This PR introduces a shortcode, goonj_pu_activity_button, which dynamically displays the appropriate button, either
Proceed to Office VisitorProceed to Material Contributionbased on individual activity. If both activities (Office Visit and Material Contribution) have already been completed for the day, no button will be displayed.Shortcode Implementation (goonj_pu_activity_button):
Fetches activity details such as source_contact_id, Office_Visit.Goonj_Processing_Center, Material_Contribution.Goonj_Office, and activity_type_id:label using the
goonj_fetch_pu_activity_detailsfunction.These details are used to determine which button to display, or whether no button should be shown.
Goonj Office ID Fetching :
goonj_get_goonj_office_idusing this function correct Goonj office ID based on the type of activity completed:For Office Visit activities, the office ID is fetched from the Office_Visit.Goonj_Processing_Center field.
For Material Contribution activities, it is retrieved from the Material_Contribution.Goonj_Office field.
Activity Fetching : Using this
fetch_contact_pu_activities_for_todayfunction all activities are fetched that completed by the contact for the current day.Processing Activities : This function
goonj_process_pu_activitiesProcesses and analyzes which activities the user has completed, determining which button (if any) should be displayed.Activity Type Check : This function
check_if_both_activity_types_existchecks whether both Office Visit and Material Contribution activities have been completed. If both are done, no button will be displayed.Button Generation : This function
goonj_generate_activity_buttonGenerates the appropriate button, including the correct label and URL parameters, based on the contact’s activity status.Template Refactor : Refactored the existing
goonj_generate_volunteer_html_buttonfunction to reuse HTML content in the newly added goonj_generate_button_html function. The button HTML is now added in thebutton-template.phpfile, improving reusability and consistency across both functions.Summary by CodeRabbit
Summary by CodeRabbit
New Features
goonj_pu_activity_button,goonj_contribution_volunteer_signup_button,goonj_collection_landing_page, andgoonj_collection_camp_past.Bug Fixes
Refactor
Chores