Complete Flow Of Institution Collection Camp Material Contribution#491
Conversation
WalkthroughThis pull request introduces several modifications across multiple files in the Goonj Blocks and CRM themes. Key changes include the addition of new registration links for "Dropping Center" and "Institution Collection Camp," updates to user identification logic to accommodate these new types, and enhancements in the data structure for handling specific targets. The control flow and error handling mechanisms remain largely unchanged, ensuring that existing functionality is preserved while expanding the capabilities of the system. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
wp-content/themes/goonj-crm/templates/form-check-user.php (4)
12-12: Consider moving the purpose configurations to a separate configuration file.The growing list of purposes that don't require email suggests this might be better managed through a configuration file rather than hardcoding in the template.
Consider creating a configuration file like this:
// wp-content/themes/goonj-crm/config/form-requirements.php return [ 'purposes_not_requiring_email' => [ 'material-contribution', 'processing-center-office-visit', 'processing-center-material-contribution', 'dropping-center-contribution', 'institution-collection-camp' ] ];Then update the template to use this configuration:
$purposes_not_requiring_email = include(get_template_directory() . '/config/form-requirements.php')['purposes_not_requiring_email']; $is_purpose_requiring_email = !in_array($purpose, $purposes_not_requiring_email);
Line range hint
16-39: Add CSRF protection to the form.The form lacks CSRF protection, which is a security best practice for WordPress forms.
Add WordPress nonce field to protect against CSRF attacks:
<form class="logged-out wp-block-loginout ml-30 mr-30 ml-md-0 mr-md-0" action="<?php echo home_url(); ?>" method="POST"> + <?php wp_nonce_field('goonj_check_user_nonce', 'goonj_check_user_nonce_field'); ?> <!-- Hidden input field with conditional action value -->Then verify the nonce in your form handling function:
if (!isset($_POST['goonj_check_user_nonce_field']) || !wp_verify_nonce($_POST['goonj_check_user_nonce_field'], 'goonj_check_user_nonce')) { wp_die('Invalid nonce verification'); }
Line range hint
27-28: Consider adding phone number format validation.While the phone input uses the HTML5
teltype, it doesn't enforce any specific format.Consider adding a pattern attribute for basic format validation:
-<input type="tel" id="phone" name="phone" required value="<?php echo isset($_POST['phone']) ? esc_attr(sanitize_text_field($_POST['phone'])) : ''; ?>"> +<input type="tel" id="phone" name="phone" required pattern="[0-9]{10}" title="Please enter a valid 10-digit phone number" value="<?php echo isset($_POST['phone']) ? esc_attr(sanitize_text_field($_POST['phone'])) : ''; ?>">
Form submission security needs improvement
The form implementation lacks proper security measures:
- No nonce field implementation to prevent CSRF attacks
- Form submission handler in
functions.phpdoesn't verify request origin- Direct POST data usage without proper validation in the handler
Key locations requiring attention:
wp-content/themes/goonj-crm/templates/form-check-user.php: Addwp_nonce_field()before the submit buttonwp-content/themes/goonj-crm/functions.php: Add nonce verification ingoonj_handle_user_identification_form()- Consider adding proper sanitization for all POST data in the handler function
🔗 Analysis chain
Line range hint
16-16: Verify form handling implementation.Let's ensure the form handling is consistent and there are no duplicate implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for form handling consistency and duplicates # Check for form action handlers echo "Checking for form action handlers..." rg -l "goonj-check-user" # Check for similar form implementations echo "Checking for similar form templates..." rg -l "form-check-user" # Check for nonce implementations in form handlers echo "Checking for nonce implementations..." rg "wp_nonce_field.*goonj.*check.*user"Length of output: 445
Script:
#!/bin/bash # Let's examine the form handling implementation in both files echo "Checking form-check-user.php content:" cat "wp-content/themes/goonj-crm/templates/form-check-user.php" echo -e "\nChecking functions.php form handling:" rg -A 10 -B 10 "goonj-check-user" "wp-content/themes/goonj-crm/functions.php" # Check for other form implementations with similar patterns echo -e "\nChecking for similar form patterns:" rg -l 'form.*action.*home_url' # Check for nonce field implementations echo -e "\nChecking for wp_nonce_field usage:" rg "wp_nonce_field"Length of output: 19697
wp-content/plugins/goonj-blocks/goonj-blocks.php (1)
Line range hint
42-156: Consider refactoring for better separation of concerns.The
gb_goonj_blocks_check_action_target_existsfunction has grown to handle multiple responsibilities:
- URL routing validation
- Data retrieval from CiviCRM
- Error handling
- Template selection
This makes the code harder to maintain and test. Consider breaking it down into smaller, focused functions or classes.
Here's a suggested approach:
- Create a separate service class for each target type
- Move data retrieval logic to dedicated repository classes
- Implement a factory pattern to instantiate the appropriate service
Example structure:
interface ActionTargetService { public function getTarget(int $id): ?array; } class InstitutionCollectionCampService implements ActionTargetService { private $repository; public function getTarget(int $id): ?array { return $this->repository->findById($id); } } class ActionTargetFactory { public function createService(string $target): ActionTargetService { switch($target) { case 'institution-collection-camp': return new InstitutionCollectionCampService(); // ... other cases } } }This would make the code more maintainable and easier to test. Would you like me to create a detailed implementation plan?
wp-content/plugins/goonj-blocks/build/render.php (1)
Line range hint
126-144: Add documentation for target data structure and standardize field namesThe target_data array would benefit from documentation explaining the field mappings and standardizing the city field names across different target types (District_City vs City).
Add PHPDoc comment above the target_data array:
/** * Defines the field mappings and configuration for different target types. * * @var array{ * dropping-center: array{ * volunteer_name: string, * address: string, * address_label: string, * contribution_link: string, * donation_link: string, * register_link: string * }, * ... * } */Consider standardizing the city field names in the CiviCRM custom fields to use consistent naming.
wp-content/plugins/goonj-blocks/src/render.php (2)
Line range hint
126-144: Enhance data structure organization and validationWhile the target_data structure is well-organized, it could benefit from:
- Centralizing field mappings
- Adding field validation
Consider enhancing the data structure:
// Add at the top of the file function validate_target_fields($target_type, $action_target, $required_fields) { foreach ($required_fields as $field) { if (empty($action_target[$field])) { throw new InvalidArgumentException( sprintf('Missing required field %s for target type %s', $field, $target_type) ); } } } // Update target_data structure to include required fields $target_data = [ 'dropping-center' => [ 'required_fields' => [ 'Dropping_Centre.State', 'Dropping_Centre.District_City', 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_' ], // ... rest of the configuration ], // ... other target types ]; // Add validation before using fields $target_info = $target_data[$target]; validate_target_fields($target, $action_target, $target_info['required_fields']);
166-166: Use consistent variable namingThe code uses specific names for registration links in declarations but assigns to a generic
$register_linkvariable. This could be confusing.Consider maintaining consistent naming:
-$register_link = $target_info['register_link']; +$target_register_link = $target_info['register_link']; // Update in template -<a href="<?php echo esc_url($register_link); ?>" class="wp-block-gb-action-button"> +<a href="<?php echo esc_url($target_register_link); ?>" class="wp-block-gb-action-button">wp-content/themes/goonj-crm/functions.php (2)
233-245: Consider refactoring duplicate URL generation logicThe URL generation logic for institution-collection-camp follows the same pattern as other cases but contributes to code duplication. Consider extracting the common URL generation logic into a helper function.
+ function generate_registration_url($email, $phone, $source, $flow, $state_id, $city) { + return sprintf( + '/individual-signup-with-volunteering/#?email=%s&phone=%s&source=%s&Individual_fields.Creation_Flow=%s&state_province_id=%s&city=%s', + $email, + $phone, + $source, + $flow, + sanitize_text_field($state_id), + sanitize_text_field($city) + ); + } case 'institution-collection-camp': - $individual_registration_form_path = sprintf( - '/individual-signup-with-volunteering/#?email=%s&phone=%s&source=%s&Individual_fields.Creation_Flow=%s&state_province_id=%s&city=%s', - $email, - $phone, - $source, - 'institution-collection-camp', - sanitize_text_field($state_id), - sanitize_text_field($city) - ); + $individual_registration_form_path = generate_registration_url( + $email, $phone, $source, 'institution-collection-camp', $state_id, $city + ); $redirect_url = $individual_registration_form_path; break;
559-562: Improve error logging consistencyThe error logging message could be more specific to help with debugging.
if (!$source) { - \Civi::log()->warning('Source is missing for material contribution flow', ['individualId' => $_GET['individualId']]); + \Civi::log()->warning('Source is missing for institution collection camp flow', [ + 'individualId' => $_GET['individualId'], + 'flow' => 'institution-collection-camp' + ]); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
wp-content/plugins/goonj-blocks/build/render.php(5 hunks)wp-content/plugins/goonj-blocks/goonj-blocks.php(1 hunks)wp-content/plugins/goonj-blocks/src/render.php(5 hunks)wp-content/themes/goonj-crm/functions.php(4 hunks)wp-content/themes/goonj-crm/templates/form-check-user.php(1 hunks)
🔇 Additional comments (4)
wp-content/plugins/goonj-blocks/goonj-blocks.php (1)
89-90: LGTM! Verify CiviCRM field existence.
The new fields follow the existing naming conventions and are logically grouped with other Institution Collection Camp fields.
Let's verify if these fields exist in the CiviCRM schema:
✅ Verification successful
Fields are properly defined and used in CiviCRM custom fields
The search results confirm that these fields are part of the Institution_Collection_Camp_Intent custom field group in CiviCRM and are properly used throughout the codebase:
- Fields are defined in the CiviCRM extension (
goonjcustom) - They are consistently used in the rendering logic
- They follow the same pattern as other existing fields in the same group
- The fields are properly integrated with the state/city handling logic in
InstitutionCollectionCampService.php
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new fields exist in CiviCRM custom fields
# Note: This assumes CiviCRM API files are present in the codebase
# Search for field definitions in PHP files
rg -l "Institution_Collection_Camp_Intent\.(State|District_City)" --type php
Length of output: 223
Script:
#!/bin/bash
# Let's examine the usage of these fields in the found files to understand their context and implementation
# Check render.php files content
echo "=== Content of wp-content/plugins/goonj-blocks/src/render.php ==="
rg -A 5 "Institution_Collection_Camp_Intent\.(State|District_City)" "wp-content/plugins/goonj-blocks/src/render.php"
echo -e "\n=== Content of wp-content/plugins/goonj-blocks/build/render.php ==="
rg -A 5 "Institution_Collection_Camp_Intent\.(State|District_City)" "wp-content/plugins/goonj-blocks/build/render.php"
# Check if there are any related field definitions or schema files
echo -e "\n=== Searching for any field definitions or schema files ==="
fd -e php -e json -e xml schema custom_fields fields
Length of output: 1945
Script:
#!/bin/bash
# Let's check for any CiviCRM custom field definitions or related configuration
echo "=== Searching for CiviCRM custom field configurations ==="
rg -A 5 "Institution_Collection_Camp_Intent" --type php
# Also check for any XML configurations that might define these fields
echo -e "\n=== Searching for XML configurations ==="
fd -e xml . | xargs rg -l "Institution_Collection_Camp_Intent"
# Check if these fields are used in any database or schema related files
echo -e "\n=== Searching in database related files ==="
fd "(db|database|schema|install|upgrade)\.(php|sql|xml)$" | xargs rg -l "Institution_Collection_Camp_Intent"
Length of output: 9022
wp-content/plugins/goonj-blocks/build/render.php (2)
166-166: LGTM! Proper security measures in place
The register link is properly retrieved from the target_data array and correctly escaped using WordPress's esc_url() function.
27-47: Verify CiviCRM field existence
Let's ensure all the custom fields used in URL generation exist in CiviCRM to prevent potential null values or errors.
Also applies to: 126-144
✅ Verification successful
Custom fields are properly defined in CiviCRM
Based on the search results, all the custom fields used in URL generation are properly defined in CiviCRM:
Collection_Camp_Intent_Details.Stateis defined and used in CollectionCampServiceDropping_Centre.Stateis defined and used in DroppingCenterServiceInstitution_Collection_Camp_Intent.Stateis defined and used in InstitutionCollectionCampService
The fields are also properly registered in goonj-blocks.php and consistently used across templates and services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence of custom fields in CiviCRM
# Search for custom field definitions
rg -A 2 "custom_group_id:name|custom_field:name" --type php
# Search for field usage in forms or templates
rg -A 2 "Collection_Camp_Intent_Details\.State|Dropping_Centre\.State|Institution_Collection_Camp_Intent\.State" --type php
Length of output: 14932
wp-content/themes/goonj-crm/functions.php (1)
141-141: LGTM: Email requirement change is consistent
The addition of 'institution-collection-camp' to the non-email requiring purposes array follows the established pattern.
| $collection_camp_register_link = sprintf( | ||
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['Collection_Camp_Intent_Details.State'], | ||
| $action_target['Collection_Camp_Intent_Details.City'], | ||
| ); | ||
|
|
||
| $dropping_center_register_link = sprintf( | ||
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['Dropping_Centre.State'], | ||
| $action_target['Dropping_Centre.District_City'], | ||
| ); | ||
|
|
||
| $institution_collection_camp_register_link = sprintf( | ||
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['Institution_Collection_Camp_Intent.State'], | ||
| $action_target['Institution_Collection_Camp_Intent.District_City'], | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate registration link generation logic
The code contains three nearly identical blocks for generating registration links, violating the DRY principle. Consider refactoring this into a helper function.
Here's a suggested refactor:
-$collection_camp_register_link = sprintf(
- '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['Collection_Camp_Intent_Details.State'],
- $action_target['Collection_Camp_Intent_Details.City'],
-);
-
-$dropping_center_register_link = sprintf(
- '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['Dropping_Centre.State'],
- $action_target['Dropping_Centre.District_City'],
-);
-
-$institution_collection_camp_register_link = sprintf(
- '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['Institution_Collection_Camp_Intent.State'],
- $action_target['Institution_Collection_Camp_Intent.District_City'],
-);
+function generate_registration_link($action_target, $state_field, $city_field) {
+ return sprintf(
+ '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
+ $action_target['title'],
+ $action_target[$state_field],
+ $action_target[$city_field]
+ );
+}
+
+$collection_camp_register_link = generate_registration_link(
+ $action_target,
+ 'Collection_Camp_Intent_Details.State',
+ 'Collection_Camp_Intent_Details.City'
+);
+
+$dropping_center_register_link = generate_registration_link(
+ $action_target,
+ 'Dropping_Centre.State',
+ 'Dropping_Centre.District_City'
+);
+
+$institution_collection_camp_register_link = generate_registration_link(
+ $action_target,
+ 'Institution_Collection_Camp_Intent.State',
+ 'Institution_Collection_Camp_Intent.District_City'
+);📝 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.
| $collection_camp_register_link = sprintf( | |
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | |
| $action_target['title'], | |
| $action_target['Collection_Camp_Intent_Details.State'], | |
| $action_target['Collection_Camp_Intent_Details.City'], | |
| ); | |
| $dropping_center_register_link = sprintf( | |
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | |
| $action_target['title'], | |
| $action_target['Dropping_Centre.State'], | |
| $action_target['Dropping_Centre.District_City'], | |
| ); | |
| $institution_collection_camp_register_link = sprintf( | |
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | |
| $action_target['title'], | |
| $action_target['Institution_Collection_Camp_Intent.State'], | |
| $action_target['Institution_Collection_Camp_Intent.District_City'], | |
| ); | |
| function generate_registration_link($action_target, $state_field, $city_field) { | |
| return sprintf( | |
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | |
| $action_target['title'], | |
| $action_target[$state_field], | |
| $action_target[$city_field] | |
| ); | |
| } | |
| $collection_camp_register_link = generate_registration_link( | |
| $action_target, | |
| 'Collection_Camp_Intent_Details.State', | |
| 'Collection_Camp_Intent_Details.City' | |
| ); | |
| $dropping_center_register_link = generate_registration_link( | |
| $action_target, | |
| 'Dropping_Centre.State', | |
| 'Dropping_Centre.District_City' | |
| ); | |
| $institution_collection_camp_register_link = generate_registration_link( | |
| $action_target, | |
| 'Institution_Collection_Camp_Intent.State', | |
| 'Institution_Collection_Camp_Intent.District_City' | |
| ); |
| $collection_camp_register_link = sprintf( | ||
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['Collection_Camp_Intent_Details.State'], | ||
| $action_target['Collection_Camp_Intent_Details.City'], | ||
| ); | ||
|
|
||
| $dropping_center_register_link = sprintf( | ||
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['Dropping_Centre.State'], | ||
| $action_target['Dropping_Centre.District_City'], | ||
| ); | ||
|
|
||
| $institution_collection_camp_register_link = sprintf( | ||
| '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['Institution_Collection_Camp_Intent.State'], | ||
| $action_target['Institution_Collection_Camp_Intent.District_City'], | ||
| ); |
There was a problem hiding this comment.
Ensure proper URL parameter encoding
The registration link generation doesn't properly encode URL parameters, which could lead to URL injection vulnerabilities.
Add URL encoding for all parameters:
function generate_registration_link($title, $state_field, $city_field, $action_target) {
return sprintf(
'/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $title,
- $action_target[$state_field],
- $action_target[$city_field]
+ urlencode($title),
+ urlencode($action_target[$state_field]),
+ urlencode($action_target[$city_field])
);
}Committable suggestion skipped: line range outside the PR's diff.
🛠️ Refactor suggestion
Refactor duplicate registration link generation logic
The code contains three nearly identical sprintf calls that differ only in their field mappings. This violates the DRY principle and increases maintenance burden.
Consider consolidating the link generation logic:
-$collection_camp_register_link = sprintf(
- '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['Collection_Camp_Intent_Details.State'],
- $action_target['Collection_Camp_Intent_Details.City'],
-);
-
-$dropping_center_register_link = sprintf(
- '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['Dropping_Centre.State'],
- $action_target['Dropping_Centre.District_City'],
-);
-
-$institution_collection_camp_register_link = sprintf(
- '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['Institution_Collection_Camp_Intent.State'],
- $action_target['Institution_Collection_Camp_Intent.District_City'],
-);
+function generate_registration_link($title, $state_field, $city_field, $action_target) {
+ return sprintf(
+ '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
+ $title,
+ $action_target[$state_field],
+ $action_target[$city_field]
+ );
+}
+
+$field_mappings = [
+ 'collection_camp' => [
+ 'state' => 'Collection_Camp_Intent_Details.State',
+ 'city' => 'Collection_Camp_Intent_Details.City'
+ ],
+ 'dropping_center' => [
+ 'state' => 'Dropping_Centre.State',
+ 'city' => 'Dropping_Centre.District_City'
+ ],
+ 'institution_collection_camp' => [
+ 'state' => 'Institution_Collection_Camp_Intent.State',
+ 'city' => 'Institution_Collection_Camp_Intent.District_City'
+ ]
+];
+
+$collection_camp_register_link = generate_registration_link(
+ $action_target['title'],
+ $field_mappings['collection_camp']['state'],
+ $field_mappings['collection_camp']['city'],
+ $action_target
+);Committable suggestion skipped: line range outside the PR's diff.
| if ($purpose === 'institution-collection-camp') { | ||
| $institution_collection_camp_form_path = sprintf( | ||
| '/institution-collection-camp/collection-camp-material-contribution/#?email=%s&phone=%s&Material_Contribution.Institution_Collection_Camp=%s&source_contact_id=%s', | ||
| $email, | ||
| $phone, | ||
| $target_id, | ||
| $found_contacts['id'] | ||
| ); | ||
| wp_redirect( $institution_collection_camp_form_path ); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in redirection logic
While the redirection logic is correct, it lacks error handling for edge cases.
if ($purpose === 'institution-collection-camp') {
+ if (empty($target_id)) {
+ error_log('Error: Missing target_id for institution-collection-camp redirection');
+ wp_redirect(home_url('/error-page/'));
+ exit;
+ }
$institution_collection_camp_form_path = sprintf(
'/institution-collection-camp/collection-camp-material-contribution/#?email=%s&phone=%s&Material_Contribution.Institution_Collection_Camp=%s&source_contact_id=%s',
$email,
$phone,
$target_id,
$found_contacts['id']
);
wp_redirect( $institution_collection_camp_form_path );
exit;
}📝 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 ($purpose === 'institution-collection-camp') { | |
| $institution_collection_camp_form_path = sprintf( | |
| '/institution-collection-camp/collection-camp-material-contribution/#?email=%s&phone=%s&Material_Contribution.Institution_Collection_Camp=%s&source_contact_id=%s', | |
| $email, | |
| $phone, | |
| $target_id, | |
| $found_contacts['id'] | |
| ); | |
| wp_redirect( $institution_collection_camp_form_path ); | |
| exit; | |
| } | |
| if ($purpose === 'institution-collection-camp') { | |
| if (empty($target_id)) { | |
| error_log('Error: Missing target_id for institution-collection-camp redirection'); | |
| wp_redirect(home_url('/error-page/')); | |
| exit; | |
| } | |
| $institution_collection_camp_form_path = sprintf( | |
| '/institution-collection-camp/collection-camp-material-contribution/#?email=%s&phone=%s&Material_Contribution.Institution_Collection_Camp=%s&source_contact_id=%s', | |
| $email, | |
| $phone, | |
| $target_id, | |
| $found_contacts['id'] | |
| ); | |
| wp_redirect( $institution_collection_camp_form_path ); | |
| exit; | |
| } |
| case 'institution-collection-camp': | ||
| if ( ! $source ) { | ||
| \Civi::log()->warning('Source is missing for material contribution flow', ['individualId' => $_GET['individualId']]); | ||
| return; | ||
| } | ||
| // If the individual was created during a material contribution process, | ||
| // We need to determine from where they were attempting to contribute. | ||
|
|
||
| // First, we check if the source of Individual is Institution Collection Camp. | ||
| $institutionCollectionCamp = \Civi\Api4\EckEntity::get( 'Collection_Camp', false ) | ||
| ->addWhere( 'title', '=', $source ) | ||
| ->addWhere('subtype:name', '=', 'Institution_Collection_Camp') | ||
| ->execute()->first(); | ||
|
|
||
| if ( ! empty( $institutionCollectionCamp['id'] ) ) { | ||
| $redirectPath = sprintf( | ||
| '/institution-collection-camp/material-contribution/#?Material_Contribution.Institution_Collection_Camp=%s&source_contact_id=%s', | ||
| $institutionCollectionCamp['id'], | ||
| $individual['id'] | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate entity retrieval logic
The entity retrieval and URL generation logic is duplicated across multiple cases. Consider extracting this into reusable functions.
+ function get_collection_camp_entity($source, $subtype) {
+ return \Civi\Api4\EckEntity::get('Collection_Camp', false)
+ ->addWhere('title', '=', $source)
+ ->addWhere('subtype:name', '=', $subtype)
+ ->execute()->first();
+ }
+ function generate_material_contribution_url($entity_id, $individual_id, $camp_type) {
+ $urls = [
+ 'Institution_Collection_Camp' => '/institution-collection-camp/material-contribution/#?Material_Contribution.Institution_Collection_Camp=%s&source_contact_id=%s',
+ 'Dropping_Center' => '/dropping-center/material-contribution/#?Material_Contribution.Dropping_Center=%s&source_contact_id=%s',
+ 'Collection_Camp' => '/material-contribution/#?Material_Contribution.Collection_Camp=%s&source_contact_id=%s'
+ ];
+ return sprintf($urls[$camp_type], $entity_id, $individual_id);
+ }
case 'institution-collection-camp':
if (!$source) {
\Civi::log()->warning('Source is missing for material contribution flow', ['individualId' => $_GET['individualId']]);
return;
}
- $institutionCollectionCamp = \Civi\Api4\EckEntity::get('Collection_Camp', false)
- ->addWhere('title', '=', $source)
- ->addWhere('subtype:name', '=', 'Institution_Collection_Camp')
- ->execute()->first();
+ $institutionCollectionCamp = get_collection_camp_entity($source, 'Institution_Collection_Camp');
if (!empty($institutionCollectionCamp['id'])) {
- $redirectPath = sprintf(
- '/institution-collection-camp/material-contribution/#?Material_Contribution.Institution_Collection_Camp=%s&source_contact_id=%s',
- $institutionCollectionCamp['id'],
- $individual['id']
- );
+ $redirectPath = generate_material_contribution_url(
+ $institutionCollectionCamp['id'],
+ $individual['id'],
+ 'Institution_Collection_Camp'
+ );
break;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (3)
Line range hint
1-300: Consider splitting the class to improve separation of concernsThe
InstitutionCollectionCampServiceclass currently handles multiple responsibilities:
- QR code generation
- State/office management
- UI tabset configuration
- Event subscription management
Consider splitting these into separate service classes to improve maintainability and follow the Single Responsibility Principle.
239-244: Enhance error handling for tab configurationConsider adding error handling and logging for the new tab configuration:
'materialContribution' => [ + if (!file_exists($config['template'])) { + \CRM_Core_Error::debug_log_message( + "Template file {$config['template']} not found for tab {$key}" + ); + } 'title' => ts('Material Contribution'), 'module' => 'afsearchInstitutionCollectionCampMaterialContribution', 'directive' => 'afsearch-institution-collection-camp-material-contribution', 'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl', ],
Line range hint
1-300: Improve method documentationSeveral methods have empty PHPDoc blocks. Consider adding comprehensive documentation, especially for:
- Parameter types and descriptions
- Return types
- Method purpose and behavior
- Examples where appropriate
Example for the tabset method:
/** * Configures tabs for the Institution Collection Camp entity view. * * @param string $tabsetName The name of the tabset being rendered * @param array &$tabs Array of tabs to be displayed * @param array $context Additional context about the current entity * * @return void */ public static function institutionCollectionCampTabset($tabsetName, &$tabs, $context)
| 'materialContribution' => [ | ||
| 'title' => ts('Material Contribution'), | ||
| 'module' => 'afsearchInstitutionCollectionCampMaterialContribution', | ||
| 'directive' => 'afsearch-institution-collection-camp-material-contribution', | ||
| 'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl', | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
Security hardening needed for Material Contribution tab
Based on the code analysis, several security concerns need addressing:
- No CSRF protection found in the form implementation - add
CRM_Core_Form::addElement('hidden', '_CSRF_TOKEN')to protect against cross-site request forgeries - Template
CollectionCamp.tpldirectly outputs variables without escaping - use{$variable|escape}for all user-supplied data - Permission checks are inconsistent - implement proper access control checks similar to other services in the extension that use
CRM_Core_Permission::check() - No input validation/sanitization found for the material contribution form - add proper validation
🔗 Analysis chain
Consider security hardening measures
For the new material contribution tab:
- Ensure all user input is properly escaped
- Add CSRF protection for any forms
- Validate user permissions before displaying sensitive information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for permission checks in related files
rg "CRM_Core_Permission::check" --type php
# Look for existing CSRF protection patterns
rg "CRM_Core_Form::addElement.*(_CSRF_TOKEN)" --type php
Length of output: 91427
Script:
#!/bin/bash
# Look for existing CSRF protection patterns in the extension
rg "CRM_Core_Form::addElement.*(_CSRF_TOKEN|qfKey)" wp-content/civi-extensions/goonjcustom/
# Check for permission checks in the extension
rg "CRM_Core_Permission::check|checkPermission" wp-content/civi-extensions/goonjcustom/
# Look for input escaping patterns
rg "CRM_Utils_Type::escape|ts\(.*escape" wp-content/civi-extensions/goonjcustom/
# Check for template usage
fd -e "tpl" . wp-content/civi-extensions/goonjcustom/
Length of output: 1337
Script:
#!/bin/bash
# Check the template content for proper escaping
cat wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCamp.tpl
# Check the form implementation for CSRF and permission checks
rg -A 10 "class.*InstitutionCollectionCampService" wp-content/civi-extensions/goonjcustom/
# Look for form validation and input sanitization
rg "validate|sanitize|clean" wp-content/civi-extensions/goonjcustom/
Length of output: 6009
💡 Codebase verification
Missing Angular module registration for Material Contribution tab
The template file exists, but there's a critical issue with Angular module naming consistency:
- The new tab uses
afsearchInstitutionCollectionCampMaterialContribution - But similar functionality in CollectionCampService uses
afsearchCollectionCampMaterialContributions(plural) - No JavaScript module registration found for either naming variant
Required actions:
- Create Angular module registration in a JavaScript file under
wp-content/civi-extensions/goonjcustom/js/ - Ensure consistent naming between InstitutionCollectionCampService and CollectionCampService (singular vs plural)
🔗 Analysis chain
Add validation for template and module existence
While the new tab configuration follows the existing pattern, consider adding validation to ensure:
- The template file 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl' exists
- The Angular module 'afsearchInstitutionCollectionCampMaterialContribution' is properly registered
Let me analyze the Angular module registration patterns to ensure we're not missing any potential locations where the module might be defined.
Let me check for similar patterns and module registrations to understand if this follows the established conventions in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the template file exists
fd -e tpl "CollectionCamp.tpl$" -x echo "Template found: {}"
# Search for Angular module registration
rg "afsearchInstitutionCollectionCampMaterialContribution" --type js
Length of output: 238
Script:
#!/bin/bash
# Search for Angular module definition in JavaScript files
ast-grep --pattern 'angular.module("afsearchInstitutionCollectionCampMaterialContribution"'
# Search for module registration in any file
rg -l "afsearchInstitutionCollectionCampMaterialContribution" -g "!*.php"
# Check if there are similar module patterns in the codebase
rg "angular\.module\(.*Institution.*Collection.*Camp.*" --type js
Length of output: 9507
Script:
#!/bin/bash
# Let's check for similar Angular module patterns in the codebase to understand the convention
rg "afsearch.*Collection.*Camp" --type js -A 2
# Check for any JavaScript files in the extension directory that might contain the module
fd -e js . wp-content/civi-extensions/goonjcustom -x echo "Found JS file: {}"
# Look for similar tab configurations in PHP files
rg "'module' => 'afsearch.*Collection.*Camp.*'" --type php -A 3
Length of output: 2953
| $heading_text = $headings[$target]; | ||
|
|
||
| $register_link = sprintf( | ||
| $collection_camp_register_link = sprintf( |
There was a problem hiding this comment.
Why do we need to change the prev variable name? It doesn’t seem necessary since, as you mentioned in the PR where you’re adding the functionality for the three buttons, the current name still aligns with its purpose. Can you clarify why this change is required?
There was a problem hiding this comment.
The register link here is currently used for volunteer registration, and right now it only works for collection camps. However, we also need this functionality for institution collection camps and dropping center. To achieve this, we're passing different state and city IDs as parameters, which is why we need to update the code. If you check the code below, you'll see how it can be dynamically adjusted based on these parameters. Let me know if you have any questions; we can also get on a call, and I can give you a demo to explain everything.
| $action_target['Institution_Collection_Camp_Intent.District_City'], | ||
| ); | ||
|
|
||
|
|
| \Civi::log()->warning('Source is missing for material contribution flow', ['individualId' => $_GET['individualId']]); | ||
| return; |
There was a problem hiding this comment.
why we are using the warning instead of \Civi::log()->info . Are we wan't to show anything on UI
There was a problem hiding this comment.
I think info is the right approach here. If there is no source, we can simply log it without triggering an error or any other action. I've updated the code.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
wp-content/plugins/goonj-blocks/src/render.php (2)
Line range hint
125-143: Consider adding type safety for target data structureThe target data array would benefit from being defined as a class or interface to ensure type safety and make the structure more maintainable.
Example structure:
interface TargetInfo { public function getVolunteerName(): ?string; public function getAddress(): string; public function getAddressLabel(): string; public function getContributionLink(): string; public function getDonationLink(): string; public function getRegisterLink(): string; } class DroppingCenterTarget implements TargetInfo { private array $data; public function __construct(array $data) { $this->data = $data; } public function getVolunteerName(): string { return $this->data['Collection_Camp_Core_Details.Contact_Id.display_name']; } // ... implement other methods }
165-165: Use consistent variable namingThe variable
$register_linkuses a generic name while earlier in the code specific names are used (e.g.,$dropping_center_register_link). Consider using consistent naming throughout.- $register_link = $target_info['register_link']; + $target_register_link = $target_info['register_link'];wp-content/themes/goonj-crm/functions.php (1)
233-245: Refactor suggestion: Extract URL generation to a helper functionThe URL generation logic is duplicated across multiple cases. Consider extracting it to a reusable function to improve maintainability.
+function generate_individual_signup_url($email, $phone, $source, $flow, $state_id, $city) { + return sprintf( + '/individual-signup-with-volunteering/#?email=%s&phone=%s&source=%s&Individual_fields.Creation_Flow=%s&state_province_id=%s&city=%s', + $email, + $phone, + $source, + $flow, + sanitize_text_field($state_id), + sanitize_text_field($city) + ); +} case 'institution-collection-camp': - $individual_registration_form_path = sprintf( - '/individual-signup-with-volunteering/#?email=%s&phone=%s&source=%s&Individual_fields.Creation_Flow=%s&state_province_id=%s&city=%s', - $email, - $phone, - $source, - 'institution-collection-camp', - sanitize_text_field($state_id), - sanitize_text_field($city) - ); + $individual_registration_form_path = generate_individual_signup_url( + $email, + $phone, + $source, + 'institution-collection-camp', + $state_id, + $city + ); $redirect_url = $individual_registration_form_path; break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
wp-content/plugins/goonj-blocks/build/render.php(5 hunks)wp-content/plugins/goonj-blocks/src/render.php(5 hunks)wp-content/themes/goonj-crm/functions.php(4 hunks)
🔇 Additional comments (6)
wp-content/plugins/goonj-blocks/build/render.php (3)
Line range hint 125-143: LGTM! Well-structured target data configuration
The target data structure is consistently organized across different target types, with clear field mappings and logical grouping of related data.
165-165: LGTM! Consistent variable assignment
The assignment follows the established pattern and correctly retrieves the registration link from the target data structure.
27-47: 🛠️ Refactor suggestion
Refactor duplicate registration link generation logic
The code still contains three nearly identical blocks for generating registration links. While this was flagged in a previous review, I'd like to enhance the suggested solution to make it even more maintainable.
Building upon the previous review's suggestion, here's an enhanced solution that adds type safety and better organization:
+// Add at the top of the file
+const REGISTRATION_CONFIGS = [
+ 'collection_camp' => [
+ 'state_field' => 'Collection_Camp_Intent_Details.State',
+ 'city_field' => 'Collection_Camp_Intent_Details.City',
+ ],
+ 'dropping_center' => [
+ 'state_field' => 'Dropping_Centre.State',
+ 'city_field' => 'Dropping_Centre.District_City',
+ ],
+ 'institution_collection_camp' => [
+ 'state_field' => 'Institution_Collection_Camp_Intent.State',
+ 'city_field' => 'Institution_Collection_Camp_Intent.District_City',
+ ],
+];
+
+function generate_registration_link(array $action_target, string $type): string {
+ if (!isset(REGISTRATION_CONFIGS[$type])) {
+ throw new InvalidArgumentException("Unknown registration type: $type");
+ }
+
+ $config = REGISTRATION_CONFIGS[$type];
+ return sprintf(
+ '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
+ $action_target['title'],
+ $action_target[$config['state_field']],
+ $action_target[$config['city_field']]
+ );
+}
-$collection_camp_register_link = sprintf(
- '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['Collection_Camp_Intent_Details.State'],
- $action_target['Collection_Camp_Intent_Details.City'],
-);
-
-$dropping_center_register_link = sprintf(
- '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['Dropping_Centre.State'],
- $action_target['Dropping_Centre.District_City'],
-);
-
-$institution_collection_camp_register_link = sprintf(
- '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['Institution_Collection_Camp_Intent.State'],
- $action_target['Institution_Collection_Camp_Intent.District_City'],
-);
+$collection_camp_register_link = generate_registration_link($action_target, 'collection_camp');
+$dropping_center_register_link = generate_registration_link($action_target, 'dropping_center');
+$institution_collection_camp_register_link = generate_registration_link($action_target, 'institution_collection_camp');This enhanced solution:
- Centralizes configuration in a constant
- Adds type safety with PHP type hints
- Provides better error handling
- Makes adding new registration types easier
wp-content/plugins/goonj-blocks/src/render.php (1)
27-46: Previous refactoring suggestion for duplicate code remains valid
The registration link generation logic is duplicated across three blocks. The previous review comment suggesting consolidation using a helper function is still applicable.
wp-content/themes/goonj-crm/functions.php (2)
141-141: LGTM: Email requirement modification is consistent
The addition of 'institution-collection-camp' to the non-email requiring purposes array is consistent with the existing pattern.
331-341: Verify error handling in redirection logic
While the redirection logic is correct, it should include error handling for missing target_id as suggested in a previous review.
| case 'institution-collection-camp': | ||
| if ( ! $source ) { | ||
| \Civi::log()->info('Source is missing for material contribution flow', ['individualId' => $_GET['individualId']]); | ||
| return; | ||
| } | ||
| // If the individual was created during a material contribution process, | ||
| // We need to determine from where they were attempting to contribute. | ||
|
|
||
| // First, we check if the source of Individual is Institution Collection Camp. | ||
| $institutionCollectionCamp = \Civi\Api4\EckEntity::get( 'Collection_Camp', false ) | ||
| ->addWhere( 'title', '=', $source ) | ||
| ->addWhere('subtype:name', '=', 'Institution_Collection_Camp') | ||
| ->execute()->first(); | ||
|
|
||
| if ( ! empty( $institutionCollectionCamp['id'] ) ) { | ||
| $redirectPath = sprintf( | ||
| '/institution-collection-camp/material-contribution/#?Material_Contribution.Institution_Collection_Camp=%s&source_contact_id=%s', | ||
| $institutionCollectionCamp['id'], | ||
| $individual['id'] | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve code organization and reduce duplication
The entity retrieval and URL generation logic is similar to other cases. Consider:
- Using a shared helper function for entity retrieval
- Using a constant for the subtype name
- Using a URL generation helper
+const INSTITUTION_COLLECTION_CAMP_SUBTYPE = 'Institution_Collection_Camp';
+function get_collection_camp_entity($source, $subtype) {
+ return \Civi\Api4\EckEntity::get('Collection_Camp', false)
+ ->addWhere('title', '=', $source)
+ ->addWhere('subtype:name', '=', $subtype)
+ ->execute()->first();
+}
+function generate_material_contribution_url($entity_id, $individual_id, $type) {
+ return sprintf(
+ '/institution-collection-camp/material-contribution/#?Material_Contribution.%s=%s&source_contact_id=%s',
+ $type,
+ $entity_id,
+ $individual_id
+ );
+}
case 'institution-collection-camp':
if (!$source) {
\Civi::log()->info('Source is missing for material contribution flow', ['individualId' => $_GET['individualId']]);
return;
}
- $institutionCollectionCamp = \Civi\Api4\EckEntity::get('Collection_Camp', false)
- ->addWhere('title', '=', $source)
- ->addWhere('subtype:name', '=', 'Institution_Collection_Camp')
- ->execute()->first();
+ $institutionCollectionCamp = get_collection_camp_entity($source, INSTITUTION_COLLECTION_CAMP_SUBTYPE);
if (!empty($institutionCollectionCamp['id'])) {
- $redirectPath = sprintf(
- '/institution-collection-camp/material-contribution/#?Material_Contribution.Institution_Collection_Camp=%s&source_contact_id=%s',
- $institutionCollectionCamp['id'],
- $individual['id']
- );
+ $redirectPath = generate_material_contribution_url(
+ $institutionCollectionCamp['id'],
+ $individual['id'],
+ 'Institution_Collection_Camp'
+ );
break;
}Committable suggestion skipped: line range outside the PR's diff.
belwalshubham
deleted the
feat/complete-flow-of-material-contribution
branch
2 participants
added the functionality for these three buttons.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes