Conversation
WalkthroughThe changes in this pull request enhance the Goonj CRM theme by introducing new functions and shortcodes, modifying user interaction features, and updating the HTML structure for buttons. New functions in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nishant22029
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (4)
wp-content/themes/goonj-crm/engine/shortcodes.php (2)
67-69: Provide feedback when user is already a volunteerCurrently, if the individual is already a volunteer, the function returns without any output, possibly leaving the user confused. To enhance user experience, display a message indicating that the user is already registered as a volunteer.
Add a user-friendly message:
if ( in_array( 'Volunteer', $contactSubTypes ) ) { - return; + return __( 'You are already registered as a volunteer. Thank you for your continued support!', 'goonj-crm' ); }
88-113: Enhance user feedback when activity data is missingIn
goonj_pu_activity_button, when essential data likeactivityIdorGoonj Office IDis missing, the function logs the issue and returns silently. To improve the user experience, consider displaying a notice or message to inform the user of the missing data, rather than leaving them without any feedback.Implement a user-facing message:
if ( ! $office_id ) { \Civi::log()->info( 'Goonj Office ID is null for Activity ID:', array( 'activityId' => $activity_id ) ); - return; + return __( 'Unable to proceed due to missing office information. Please contact support.', 'goonj-crm' ); }wp-content/themes/goonj-crm/functions.php (2)
Line range hint
196-280: Refactorgoonj_handle_user_identification_form()for single responsibilityThe
goonj_handle_user_identification_form()function is lengthy and handles multiple responsibilities, including input processing, database queries, and redirects. This violates the Single Responsibility Principle and can make maintenance challenging.Consider breaking this function into smaller, focused functions:
- Input Validation: Validate and sanitize all user inputs.
- Contact Retrieval: Handle database queries to retrieve contact information.
- Redirect Logic: Separate the redirect logic based on purpose into individual functions.
This refactor will improve readability, maintainability, and testability.
Line range hint
457-498: Simplifygoonj_redirect_after_individual_creation()functionThe
goonj_redirect_after_individual_creation()function contains complex logic with nested conditions and multiple cases. Simplifying it will enhance clarity.Suggested steps:
- Extract the switch cases into separate functions handling each
creationFlowtype.- Ensure consistent error handling and logging throughout the function.
- Simplify nested conditions where possible.
This will make the function easier to understand and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- 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)
- wp-content/themes/goonj-crm/templates/primary-button.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
wp-content/themes/goonj-crm/engine/shortcodes.php (2)
108-113: Validate the office ID before generating the activity buttonWhen calling
goonj_generate_activity_button, ensure that the$office_idis valid to prevent potential issues in the button generation process. An invalidoffice_idmight lead to incorrect URLs or broken buttons.Confirm that
$office_idis valid and not zero or an unexpected value.if ( ! $office_id ) { \Civi::log()->info( 'Goonj Office ID is null for Activity ID:', array( 'activityId' => $activity_id ) ); return; } +if ( $office_id <= 0 ) { + \Civi::log()->warning( 'Invalid Goonj Office ID:', array( 'officeId' => $office_id ) ); + return; +}
87-121:⚠️ Potential issueEnsure consistent sanitization of 'activityId' across functions
The
activityIdparameter is sanitized differently ingoonj_contribution_volunteer_signup_buttonandgoonj_pu_activity_button. For consistency and to follow best practices, useabsintin both functions to sanitizeactivityId, ensuring that only positive integers are processed.Update
goonj_contribution_volunteer_signup_buttonas follows:-$activity_id = isset( $_GET['activityId'] ) ? intval( $_GET['activityId'] ) : 0; +$activity_id = isset( $_GET['activityId'] ) ? absint( $_GET['activityId'] ) : 0;Likely invalid or redundant comment.
wp-content/themes/goonj-crm/functions.php (2)
3-4: Good practice: Modularizing code by including helper and shortcode filesIncluding
helpers.phpandshortcodes.phpenhances code organization and promotes separation of concerns, making the codebase more maintainable.
212-213: Secure user input by sanitizing$state_idand$cityAdding
sanitize_text_field()around$state_idand$cityensures that user-provided data is properly sanitized before being included in URLs, enhancing security.
| <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"> | ||
| <?php echo esc_html( $args['button_text'] ); ?> | ||
| </a> | ||
| </div> No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good job on creating a reusable button component, but let's make it even better!
The component is well-structured and follows security best practices by using esc_url and esc_html. However, there are a few areas where we can improve:
-
Class naming: The classes on the anchor tag are a mix of WordPress default classes and custom classes. Consider using more semantic class names and following a consistent naming convention (e.g., BEM).
-
Simplify styling: The long list of classes on the anchor tag could be simplified by creating a custom CSS class that encapsulates all these styles.
-
Indentation: Improve readability by using consistent indentation throughout the file.
Here's a suggested refactor:
-<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">
- <?php echo esc_html( $args['button_text'] ); ?>
- </a>
-</div>
+<div class="goonj-button-container">
+ <a href="<?php echo esc_url( $args['button_url'] ); ?>" class="goonj-button goonj-button--primary">
+ <?php echo esc_html( $args['button_text'] ); ?>
+ </a>
+</div>Then, create a CSS file (if not already present) to define the styles for .goonj-button and .goonj-button--primary.
Would you like me to create a separate CSS file for these button styles?
📝 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.
| <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"> | |
| <?php echo esc_html( $args['button_text'] ); ?> | |
| </a> | |
| </div> | |
| <div class="goonj-button-container"> | |
| <a href="<?php echo esc_url( $args['button_url'] ); ?>" class="goonj-button goonj-button--primary"> | |
| <?php echo esc_html( $args['button_text'] ); ?> | |
| </a> | |
| </div> |
| 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 goonj_generate_activity_button for improved maintainability and flexibility
The current implementation of this function has several areas for improvement:
- It's quite long and performs multiple tasks, violating the Single Responsibility Principle.
- It uses hardcoded arrays for activity types and activity mapping, which could lead to maintenance issues.
- It doesn't handle cases where all activities are completed.
Consider the following improvements:
- Break down the function into smaller, more focused functions.
- Move the activity types and mapping to a configuration file or database.
- Add handling for cases where all activities are completed.
Here's a suggested refactor:
function goonj_get_activity_types() {
// Fetch from config or DB
return array('Office visit', 'Material Contribution');
}
function goonj_get_activity_map() {
// Fetch from config or DB
return 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',
),
);
}
function goonj_get_next_pending_activity($completedActivityType) {
$activityTypes = goonj_get_activity_types();
$pendingActivities = array_diff($activityTypes, array($completedActivityType));
return reset($pendingActivities);
}
function goonj_generate_redirect_url($activity, $office_id, $individual_id) {
$activityMap = goonj_get_activity_map();
$details = $activityMap[$activity];
$redirectParams = array(
'source_contact_id' => $individual_id,
$details['queryParam'] => $office_id,
);
return $details['redirectPath'] . '#?' . http_build_query($redirectParams);
}
function goonj_generate_activity_button($activity, $office_id, $individual_id) {
$completedActivityType = $activity['activity_type_id:name'];
$nextActivity = goonj_get_next_pending_activity($completedActivityType);
if (!$nextActivity) {
return ''; // No pending activities
}
$activityMap = goonj_get_activity_map();
$redirectUrl = goonj_generate_redirect_url($nextActivity, $office_id, $individual_id);
$buttonText = $activityMap[$nextActivity]['buttonText'];
return goonj_generate_button_html($redirectUrl, $buttonText);
}This refactor improves maintainability, adheres to the Single Responsibility Principle, and handles cases where all activities are completed.
| function goonj_generate_button_html( $button_url, $button_text ) { | ||
| ob_start(); | ||
| get_template_part( | ||
| 'templates/primary-button', | ||
| null, | ||
| array( | ||
| 'button_url' => $button_url, | ||
| 'button_text' => $button_text, | ||
| ) | ||
| ); | ||
|
|
||
| return ob_get_clean(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate code for rendering templates
The functions goonj_generate_button_html (lines 15-27), goonj_collection_camp_landing_page (lines 123-127), and goonj_collection_camp_past_data (lines 129-133) all use similar patterns involving ob_start(), get_template_part(), and ob_get_clean(). To adhere to the DRY (Don't Repeat Yourself) principle, consider creating a generic function to handle rendering template parts with optional arguments. This will reduce code duplication and improve maintainability.
Apply this refactor to introduce a reusable function:
function goonj_render_template_part( $template_name, $args = array() ) {
ob_start();
get_template_part( $template_name, null, $args );
return ob_get_clean();
}Then, update the existing functions:
-function goonj_generate_button_html( $button_url, $button_text ) {
- ob_start();
- get_template_part(
- 'templates/primary-button',
- null,
- array(
- 'button_url' => $button_url,
- 'button_text' => $button_text,
- )
- );
- return ob_get_clean();
-}
+function goonj_generate_button_html( $button_url, $button_text ) {
+ return goonj_render_template_part(
+ 'templates/primary-button',
+ array(
+ 'button_url' => $button_url,
+ 'button_text' => $button_text,
+ )
+ );
+}Similarly, update other functions:
-function goonj_collection_camp_landing_page() {
- ob_start();
- get_template_part( 'templates/collection-landing-page' );
- return ob_get_clean();
-}
+function goonj_collection_camp_landing_page() {
+ return goonj_render_template_part( 'templates/collection-landing-page' );
+}-function goonj_collection_camp_past_data() {
- ob_start();
- get_template_part( 'templates/collection-camp-data' );
- return ob_get_clean();
-}
+function goonj_collection_camp_past_data() {
+ return goonj_render_template_part( 'templates/collection-camp-data' );
+}| function goonj_contribution_volunteer_signup_button() { | ||
| $activity_id = isset( $_GET['activityId'] ) ? intval( $_GET['activityId'] ) : 0; | ||
|
|
||
| if ( empty( $activity_id ) ) { | ||
| \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', '=', $activity_id ) | ||
| ->addWhere( 'activity_type_id:label', '=', 'Material Contribution' ) | ||
| ->execute(); | ||
|
|
||
| if ( $activities->count() === 0 ) { | ||
| \Civi::log()->info( 'No activities found for Activity ID:', array( 'activityId' => $activity_id ) ); | ||
| return; | ||
| } | ||
|
|
||
| $activity = $activities->first(); | ||
| $individual_id = $activity['source_contact_id']; | ||
|
|
||
| $contact = \Civi\Api4\Contact::get( false ) | ||
| ->addSelect( 'contact_sub_type' ) | ||
| ->addWhere( 'id', '=', $individual_id ) | ||
| ->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' => $individual_id, | ||
| '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 common activity retrieval logic into a helper function
Both goonj_contribution_volunteer_signup_button and goonj_pu_activity_button retrieve activity data using similar code. To adhere to the Single Responsibility Principle and reduce code duplication, consider extracting the shared logic into a dedicated helper function. This will improve code readability and make future maintenance easier.
Create a helper function for activity retrieval:
function goonj_get_activity_by_id( $activity_id, $select_fields = array(), $activity_type = null ) {
$query = \Civi\Api4\Activity::get( false )
->addSelect( $select_fields )
->addWhere( 'id', '=', $activity_id );
if ( $activity_type ) {
$query->addWhere( 'activity_type_id:label', '=', $activity_type );
}
return $query->execute()->first();
}Then update the functions:
// In goonj_contribution_volunteer_signup_button
- $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();
+ $activity = goonj_get_activity_by_id(
+ $activity_id,
+ array( 'source_contact_id' ),
+ 'Material Contribution'
+ );
// In goonj_pu_activity_button
- $activity = \Civi\Api4\Activity::get(false)
- ->addSelect('source_contact_id', 'Office_Visit.Goonj_Processing_Center', 'Material_Contribution.Goonj_Office', 'activity_type_id:name')
- ->addWhere('id', '=', $activity_id)
- ->execute()
- ->first();
+ $activity = goonj_get_activity_by_id(
+ $activity_id,
+ array(
+ 'source_contact_id',
+ 'Office_Visit.Goonj_Processing_Center',
+ 'Material_Contribution.Goonj_Office',
+ 'activity_type_id:name'
+ )
+ );Also applies to: 87-121
Enhance input validation and sanitization for 'activityId'
In goonj_contribution_volunteer_signup_button (lines 29-85), the activityId parameter from $_GET is sanitized using intval, which can result in zero for invalid values. To ensure robust validation and prevent potential issues, use absint to guarantee a positive integer, and check if $activity_id is greater than zero before proceeding.
Apply this diff to improve input handling:
-$activity_id = isset( $_GET['activityId'] ) ? intval( $_GET['activityId'] ) : 0;
+$activity_id = isset( $_GET['activityId'] ) ? absint( $_GET['activityId'] ) : 0;
-if ( empty( $activity_id ) ) {
+if ( $activity_id === 0 ) {
\Civi::log()->warning( 'Activity ID is missing' );
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.
| function goonj_contribution_volunteer_signup_button() { | |
| $activity_id = isset( $_GET['activityId'] ) ? intval( $_GET['activityId'] ) : 0; | |
| if ( empty( $activity_id ) ) { | |
| \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', '=', $activity_id ) | |
| ->addWhere( 'activity_type_id:label', '=', 'Material Contribution' ) | |
| ->execute(); | |
| if ( $activities->count() === 0 ) { | |
| \Civi::log()->info( 'No activities found for Activity ID:', array( 'activityId' => $activity_id ) ); | |
| return; | |
| } | |
| $activity = $activities->first(); | |
| $individual_id = $activity['source_contact_id']; | |
| $contact = \Civi\Api4\Contact::get( false ) | |
| ->addSelect( 'contact_sub_type' ) | |
| ->addWhere( 'id', '=', $individual_id ) | |
| ->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' => $individual_id, | |
| '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() { | |
| $activity_id = isset( $_GET['activityId'] ) ? absint( $_GET['activityId'] ) : 0; | |
| if ( $activity_id === 0 ) { | |
| \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', '=', $activity_id ) | |
| ->addWhere( 'activity_type_id:label', '=', 'Material Contribution' ) | |
| ->execute(); | |
| if ( $activities->count() === 0 ) { | |
| \Civi::log()->info( 'No activities found for Activity ID:', array( 'activityId' => $activity_id ) ); | |
| return; | |
| } | |
| $activity = $activities->first(); | |
| $individual_id = $activity['source_contact_id']; | |
| $contact = \Civi\Api4\Contact::get( false ) | |
| ->addSelect( 'contact_sub_type' ) | |
| ->addWhere( 'id', '=', $individual_id ) | |
| ->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' => $individual_id, | |
| '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; | |
| } | |
| } |
| ->addSelect('source_contact_id', 'Office_Visit.Goonj_Processing_Center', 'Material_Contribution.Goonj_Office', 'activity_type_id:name') | ||
| ->addWhere('id', '=', $activity_id) | ||
| ->execute() | ||
| ->first(); | ||
|
|
There was a problem hiding this comment.
Check for empty activity data before proceeding
After retrieving the activity, there is a possibility that $activity might be empty, which could cause errors in subsequent code. Ensure that you check if $activity exists before accessing its elements to prevent undefined variable issues.
Confirm the activity data:
$activity = \Civi\Api4\Activity::get(false)
// ... existing code ...
->execute()
->first();
-if ( ! $activity ) {
+if ( empty( $activity ) ) {
\Civi::log()->info( 'No activity found', array( 'activityId' => $activity_id ) );
return;
}Committable suggestion was skipped due to low confidence.
| $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.
Add exception handling during activity retrieval
In goonj_contribution_volunteer_signup_button, the retrieval of activities does not have its own exception handling, which could lead to unhandled exceptions if the API call fails. To improve robustness, wrap the activity retrieval logic in a try-catch block to handle potential exceptions gracefully.
Apply this change:
try {
+ $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();
} catch ( \Exception $e ) {
\Civi::log()->error( 'Error retrieving activities: ' . $e->getMessage() );
return;
}Committable suggestion was skipped due to low confidence.
| \Civi::log()->info( 'Contact not found', array( 'contact' => $contact['id'] ) ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Correct logging when contact is not found
At line 60, the log attempts to access $contact['id'] even though $contact is empty, which could cause a PHP notice or error. Update the log message to prevent accessing undefined array keys.
Modify the log statement:
-\Civi::log()->info( 'Contact not found', array( 'contact' => $contact['id'] ) );
+\Civi::log()->info( 'Contact not found', array( 'contactId' => $individual_id ) );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| \Civi::log()->info( 'Contact not found', array( 'contact' => $contact['id'] ) ); | |
| return; | |
| } | |
| \Civi::log()->info( 'Contact not found', array( 'contactId' => $individual_id ) ); | |
| return; | |
| } |
| if ( in_array( $purpose, array( 'processing-center-material-contribution', 'processing-center-office-visit' ) ) ) { | ||
| $organizationName = \Civi\Api4\Organization::get( false ) | ||
| ->addSelect( 'display_name' ) | ||
| ->addWhere( 'id', '=', $target_id ) |
There was a problem hiding this comment.
Handle potential null value for $organizationName
If the organization is not found, $organizationName may be null, leading to an Undefined index: display_name notice when accessing $organizationName['display_name'].
Consider adding a check to ensure $organizationName is not null before accessing 'display_name'. Here's a suggested fix:
if ( ! empty( $organizationName['display_name'] ) ) {
// Use $organizationName['display_name'] as intended
} else {
// Handle the case where the organization was not found
}| ->addWhere( 'option_group_id:name', '=', 'eck_sub_types' ) | ||
| ->addWhere( 'name', '=', 'Collection_Camp' ) | ||
| ->addWhere( 'grouping', '=', 'Collection_Camp' ) |
There was a problem hiding this comment.
Check for null value when fetching $optionValues
If no matching option value is found, $optionValues may be null, which could cause an Undefined index: value notice when accessing $optionValues['value'].
Add a check to ensure $optionValues is not null before using it:
if ( ! empty( $optionValues['value'] ) ) {
$collectionCampSubtype = $optionValues['value'];
} else {
// Handle the case where the option value was not found
}| case 'material-contribution': | ||
| if ( ! $source ) { | ||
| \Civi::log()->warning('Source is missing for material contribution flow', ['individualId' => $_GET['individualId']]); | ||
| \Civi::log()->warning( 'Source is missing for material contribution flow', array( 'individualId' => $_GET['individualId'] ) ); |
There was a problem hiding this comment.
Sanitize $_GET['individualId'] before logging
Directly using $_GET['individualId'] without sanitization can be risky. Sanitize the input before logging to prevent potential issues.
Apply this fix:
-\Civi::log()->warning( 'Source is missing for material contribution flow', array( 'individualId' => $_GET['individualId'] ) );
+\Civi::log()->warning( 'Source is missing for material contribution flow', array( 'individualId' => absint( $_GET['individualId'] ) ) );📝 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.
| \Civi::log()->warning( 'Source is missing for material contribution flow', array( 'individualId' => $_GET['individualId'] ) ); | |
| \Civi::log()->warning( 'Source is missing for material contribution flow', array( 'individualId' => absint( $_GET['individualId'] ) ) ); |
2 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.Summary by CodeRabbit
New Features
Bug Fixes
Refactor