Conversation
WalkthroughThis pull request updates the method for retrieving organization address data across multiple components. The field previously referenced as Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Script
participant GroupAPI as Group API
participant OrgAPI as Organization API
CLI->>GroupAPI: getInstituteFromGroup()
GroupAPI-->>CLI: Return list of contacts
loop For each contact
CLI->>OrgAPI: Fetch organization details
OrgAPI-->>CLI: Return organization data
CLI->>OrgAPI: Update organization with mapped POC fields
OrgAPI-->>CLI: Confirmation/Error message
end
CLI->>CLI: Log completion message
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 3
🔭 Outside diff range comments (2)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionDroppingCenterLinks.php (1)
108-124:⚠️ Potential issueSanitize URL parameters to prevent XSS.
The URL generation includes user-provided data without proper sanitization, which could lead to XSS attacks.
Consider using
urlencode()for all user-provided parameters:$links = [ [ 'label' => 'Vehicle Dispatch', 'url' => self::createUrl( '/institution-dropping-center-vehicle-dispatch', "Camp_Vehicle_Dispatch.Institution_Dropping_Center={$this->_institutionDroppingCenterId}" . "&Eck_Collection_Camp1={$this->_institutionDroppingCenterId}" . "&Camp_Vehicle_Dispatch.To_which_PU_Center_material_is_being_sent={$this->_processingCenterId}" . "&Camp_Vehicle_Dispatch.Filled_by={$contactId}" . - "&Camp_Institution_Data.Name_of_the_institution={$nameOfInstitution}" . - "&Camp_Institution_Data.Address={$address}" . - "&Camp_Institution_Data.Email={$pocEmail}" . - "&Camp_Institution_Data.Contact_Number={$pocContactNumber}", + "&Camp_Institution_Data.Name_of_the_institution=" . urlencode($nameOfInstitution) . + "&Camp_Institution_Data.Address=" . urlencode($address) . + "&Camp_Institution_Data.Email=" . urlencode($pocEmail) . + "&Camp_Institution_Data.Contact_Number=" . urlencode($pocContactNumber), $contactId ), ],wp-content/civi-extensions/goonjcustom/Civi/InstitutionMaterialContributionService.php (1)
218-218:⚠️ Potential issueUse secure file loading mechanism.
Using
file_get_contentsdirectly on user-accessible paths could be dangerous if the paths are manipulated.Consider using WordPress's built-in file handling functions:
- $imageData = array_map(fn ($path) => base64_encode(file_get_contents($path)), $paths); + $imageData = array_map(function($path) { + if (!file_exists($path) || !is_readable($path)) { + return ''; + } + return base64_encode(file_get_contents($path)); + }, $paths);
🧹 Nitpick comments (5)
wp-content/civi-extensions/goonjcustom/cli/assign-contact-data.php (2)
17-17: Fix typo in group name constant.The constant
SOURCE_GROUP_NAMEcontains a typo in the word "institution".-define('SOURCE_GROUP_NAME', 'instituion-group'); +define('SOURCE_GROUP_NAME', 'institution-group');
71-81: Simplify data presence check.The current implementation checks each field individually. Consider using array_filter for a more concise check.
- $hasData = !empty($organization['Institute_Registration.City']) - || !empty($organization['Institute_Registration.Address']) - || !empty($organization['Institute_Registration.Postal_Code']) - || !empty($organization['Institute_Registration.Contact_number_of_Institution']) - || !empty($organization['Institute_Registration.Email_of_Institute']); + $hasData = !empty(array_filter($organization, fn($value) => !empty($value)));wp-content/civi-extensions/goonjcustom/Civi/InstitutionMaterialContributionService.php (1)
224-335: Move HTML template to a separate file.The HTML generation is embedded in the service class, violating the Single Responsibility Principle.
Consider moving the HTML template to a separate file (e.g.,
templates/contribution-receipt.php) and use WordPress's template loading functions to include it.wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (2)
573-578: Address field update looks good, but consider extracting common functionality.The address field update is correct, but there's significant code duplication between this service and
InstitutionCollectionCampService. Consider:
- Creating a shared trait for common organization operations
- Moving email templates to separate files
602-614: Move HTML email templates to separate files.The HTML email templates are currently hardcoded in the service class. This violates separation of concerns and makes the templates harder to maintain.
Consider moving these templates to separate
.html.tplfiles in a templates directory:- $html = " - <p>Dear MMT team,</p> - <p>This is to inform you that a vehicle has been sent from camp <strong>$campCode</strong> at <strong>$campAddress</strong>.</p> - ..."; + $templatePath = __DIR__ . '/../templates/emails/mmt-notification.html.tpl'; + $html = file_get_contents($templatePath); + $html = str_replace( + ['{{campCode}}', '{{campAddress}}'], + [$campCode, $campAddress], + $html + );Also applies to: 732-736, 854-866
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionCollectionCampLinks.php(2 hunks)wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionDroppingCenterLinks.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionMaterialContributionService.php(1 hunks)wp-content/civi-extensions/goonjcustom/cli/assign-contact-data.php(1 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionCollectionCampLinks.php (1)
109-149:⚠️ Potential issueSanitize URL parameters to prevent XSS.
Similar to the dropping center links, the URL generation includes unsanitized user data.
Apply the same fix using
urlencode()for all user-provided parameters in the URL generation.wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1)
789-813: Move HTML email templates to separate files.Similar to InstitutionDroppingCenterService, the email templates should be moved out of the service class.
Also applies to: 825-836, 841-866
| $organization = Organization::get(FALSE) | ||
| ->addSelect( | ||
| 'Institute_Registration.City', | ||
| 'Institute_Registration.Address', | ||
| 'Institute_Registration.Postal_Code', | ||
| 'Institute_Registration.Contact_number_of_Institution', | ||
| 'Institute_Registration.Email_of_Institute' | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update field names to use new schema.
The organization query still uses old field names from Institute_Registration. For consistency with other files in this PR, these should be updated to use the new schema.
$organization = Organization::get(FALSE)
->addSelect(
- 'Institute_Registration.City',
- 'Institute_Registration.Address',
- 'Institute_Registration.Postal_Code',
- 'Institute_Registration.Contact_number_of_Institution',
- 'Institute_Registration.Email_of_Institute'
+ 'address_primary.city',
+ 'address_primary.street_address',
+ 'address_primary.postal_code',
+ 'phone_primary.phone',
+ 'email_primary.email'
)📝 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.
| $organization = Organization::get(FALSE) | |
| ->addSelect( | |
| 'Institute_Registration.City', | |
| 'Institute_Registration.Address', | |
| 'Institute_Registration.Postal_Code', | |
| 'Institute_Registration.Contact_number_of_Institution', | |
| 'Institute_Registration.Email_of_Institute' | |
| ) | |
| $organization = Organization::get(FALSE) | |
| ->addSelect( | |
| 'address_primary.city', | |
| 'address_primary.street_address', | |
| 'address_primary.postal_code', | |
| 'phone_primary.phone', | |
| 'email_primary.email' | |
| ) |
| $this->_organization = Organization::get(FALSE) | ||
| ->addSelect('display_name', 'Institute_Registration.Email_of_Institute', 'Institute_Registration.Contact_number_of_Institution', 'Institute_Registration.Address') | ||
| ->addSelect('display_name', 'Institute_Registration.Email_of_Institute', 'Institute_Registration.Contact_number_of_Institution', 'address_primary.street_address') | ||
| ->addWhere('id', '=', $organizationId) | ||
| ->execute()->single(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize field selection to use new schema.
The organization query mixes old and new schema field names. For consistency, use the new schema throughout.
$this->_organization = Organization::get(FALSE)
- ->addSelect('display_name', 'Institute_Registration.Email_of_Institute', 'Institute_Registration.Contact_number_of_Institution', 'address_primary.street_address')
+ ->addSelect('display_name', 'email_primary.email', 'phone_primary.phone', 'address_primary.street_address')
->addWhere('id', '=', $organizationId)
->execute()->single();📝 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.
| $this->_organization = Organization::get(FALSE) | |
| ->addSelect('display_name', 'Institute_Registration.Email_of_Institute', 'Institute_Registration.Contact_number_of_Institution', 'Institute_Registration.Address') | |
| ->addSelect('display_name', 'Institute_Registration.Email_of_Institute', 'Institute_Registration.Contact_number_of_Institution', 'address_primary.street_address') | |
| ->addWhere('id', '=', $organizationId) | |
| ->execute()->single(); | |
| $this->_organization = Organization::get(FALSE) | |
| ->addSelect('display_name', 'email_primary.email', 'phone_primary.phone', 'address_primary.street_address') | |
| ->addWhere('id', '=', $organizationId) | |
| ->execute()->single(); |
| ->addSelect('address_primary.street_address', 'display_name') | ||
| ->addWhere('id', '=', $organizationId) | ||
| ->execute()->single(); | ||
|
|
||
| $nameOfInstitution = $organization['display_name']; | ||
| $addressOfInstitution = $organization['Institute_Registration.Address']; | ||
| $addressOfInstitution = $organization['address_primary.street_address']; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address field update looks good, but consider consolidating duplicate code.
The address field update is correct, but this service shares a lot of duplicate code with InstitutionDroppingCenterService, including:
- Organization address retrieval
- Email template generation
- Coordinator assignment logic
Consider creating an abstract base service or traits to share common functionality:
trait OrganizationAddressable {
protected function getOrganizationAddress($organizationId) {
return Organization::get(TRUE)
->addSelect('address_primary.street_address', 'display_name')
->addWhere('id', '=', $organizationId)
->execute()->single();
}
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wp-content/civi-extensions/goonjcustom/cli/assign-group-to-individual.php (3)
17-17: Consider a more descriptive constant name.The constant name
SOURCE_GROUP_NAMEcould be more specific to better reflect its purpose in assigning POCs for institutions. Consider renaming it toINSTITUTION_GROUP_NAMEorSOURCE_INSTITUTION_GROUP.-define('SOURCE_GROUP_NAME', 'Institution'); +define('INSTITUTION_GROUP_NAME', 'Institution');
12-14: Consider implementing structured logging.While the error handling is thorough, the logging could be improved by implementing a structured logging system. This would make it easier to parse and analyze logs, especially when running at scale.
Consider using a logging library or implementing a simple logging class:
class Logger { public static function info(string $message, array $context = []): void { echo json_encode([ 'level' => 'INFO', 'timestamp' => date('c'), 'message' => $message, 'context' => $context ]) . "\n"; } public static function error(string $message, array $context = []): void { echo json_encode([ 'level' => 'ERROR', 'timestamp' => date('c'), 'message' => $message, 'context' => $context ]) . "\n"; } } // Usage example: Logger::info("Skipping contact", ['contact_id' => $contactId, 'reason' => 'No state assigned']);Also applies to: 73-76, 88-91, 95-98, 107-110
16-17: Consider externalizing configuration.Hard-coded configuration values like group names could be moved to a configuration file. This would make the script more maintainable and easier to update across environments.
Consider creating a config file:
// config/groups.php return [ 'institution' => [ 'source_group' => 'Institution', 'use_case' => 'chapter-contacts', ] ]; // Usage in script $config = require_once __DIR__ . '/config/groups.php'; define('SOURCE_GROUP_NAME', $config['institution']['source_group']);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionCollectionCampLinks.php(2 hunks)wp-content/civi-extensions/goonjcustom/cli/assign-group-to-individual.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionCollectionCampLinks.php
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/cli/assign-group-to-individual.php (1)
24-32: Well-structured implementation with clear separation of concerns!The code demonstrates good practices:
- Functions follow single responsibility principle
- Clear error handling with fallback logic
- Proper type hints and return types
- Descriptive variable names and logging
Also applies to: 43-65, 70-120
1 participant
Summary by CodeRabbit