Conversation
…at/dropping-center-dispatch-email
…at/dropping-center-outcome-details
WalkthroughThe pull request introduces several changes across multiple files related to the Goonj custom extensions in CiviCRM. A new cron job function, Changes
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: 4
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/DroppingCenterCron.php (1)
39-39: Remove debugging statements to clean up production codeThe
error_logstatement on line 39 appears to be for debugging purposes. Remove or comment out debugging statements before deploying to production to prevent unnecessary logging and potential performance issues.Apply this diff to remove the debugging statement:
- error_log("productSaleAmountByTrackingId : $productSaleAmountByTrackingId, Result: " . print_r($productSaleAmountByTrackingId, TRUE));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/DroppingCenterCron.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/DroppingCenterCron.php (1)
15-15:⚠️ Potential issueReview inconsistent 'checkPermissions' settings across API calls
The
checkPermissionsparameter varies betweenTRUEandFALSEin different API calls (lines 15, 48, 59, 78, 92, 106, and 120). Ensure that this is intentional and aligns with the desired permission checks for each operation to maintain security best practices.Consider standardizing the
checkPermissionssetting or documenting the reason for the differences.Also applies to: 48-48, 59-59, 78-78, 92-92, 106-106, 120-120
| foreach ($cashContributionByTrackingId as $trackingId => $cashContributionSum) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Cash_Contribution' => max($cashContributionSum, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate similar update operations to adhere to DRY principle
The code blocks at lines 70-80, 84-94, 98-108, and 112-122 perform similar update operations with minor differences in the fields being updated. Refactoring these into a reusable function can eliminate code duplication and simplify future maintenance.
Consider creating a helper function:
function updateCollectionCamp($trackingId, $fieldName, $value, $checkPermissions = FALSE) {
return civicrm_api4('Eck_Collection_Camp', 'update', [
'values' => [
$fieldName => max($value, 0),
],
'where' => [
['id', '=', $trackingId],
],
'checkPermissions' => $checkPermissions,
]);
}Then, replace the update blocks with calls to this function:
- foreach ($cashContributionByTrackingId as $trackingId => $cashContributionSum) {
- $results = civicrm_api4('Eck_Collection_Camp', 'update', [
- 'values' => [
- 'Dropping_Center_Outcome.Cash_Contribution' => max($cashContributionSum, 0),
- ],
- 'where' => [
- ['id', '=', $trackingId],
- ],
- 'checkPermissions' => FALSE,
- ]);
- }
+ foreach ($cashContributionByTrackingId as $trackingId => $cashContributionSum) {
+ updateCollectionCamp($trackingId, 'Dropping_Center_Outcome.Cash_Contribution', $cashContributionSum);
+ }Apply similar replacements for the other update operations.
Also applies to: 84-94, 98-108, 112-122
| function civicrm_api3_goonjcustom_dropping_center_cron($params) { | ||
| $returnValues = []; | ||
|
|
||
| $collectionCamps = civicrm_api4('Eck_Collection_Camp', 'get', [ | ||
| 'select' => [ | ||
| 'Donation_Box_Register_Tracking.Cash_Contribution', | ||
| 'Dropping_Centre.Donation_Tracking_Id', | ||
| 'Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_', | ||
| ], | ||
| 'where' => [ | ||
| ['Dropping_Centre.Donation_Tracking_Id', 'IS NOT NULL'], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
||
| $cashContributionByTrackingId = []; | ||
| $productSaleAmountByTrackingId = []; | ||
| $footfallByTrackingId = []; | ||
| $vehicleByTrackingId = []; | ||
|
|
||
| foreach ($collectionCamps as $camp) { | ||
| $trackingId = $camp['Dropping_Centre.Donation_Tracking_Id']; | ||
|
|
||
| if (!isset($cashContributionByTrackingId[$trackingId])) { | ||
| $cashContributionByTrackingId[$trackingId] = 0; | ||
| } | ||
| if (!isset($productSaleAmountByTrackingId[$trackingId])) { | ||
| $productSaleAmountByTrackingId[$trackingId] = 0; | ||
| } | ||
|
|
||
| if (isset($camp['Donation_Box_Register_Tracking.Cash_Contribution']) && $camp['Donation_Box_Register_Tracking.Cash_Contribution'] !== null) { | ||
| $cashContributionByTrackingId[$trackingId] += (int) $camp['Donation_Box_Register_Tracking.Cash_Contribution']; | ||
| } | ||
|
|
||
| if (isset($camp['Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_']) && $camp['Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_'] !== null) { | ||
| $productSaleAmountByTrackingId[$trackingId] += (float) $camp['Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_']; | ||
| error_log("productSaleAmountByTrackingId : $productSaleAmountByTrackingId, Result: " . print_r($productSaleAmountByTrackingId, TRUE)); | ||
| } | ||
|
|
||
| $activities = civicrm_api4('Activity', 'get', [ | ||
| 'select' => ['subject'], | ||
| 'where' => [ | ||
| ['activity_type_id:name', '=', 'Material Contribution'], | ||
| ['Material_Contribution.Collection_Camp', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| $footfallByTrackingId[$trackingId] = count($activities); | ||
|
|
||
| $collectionSourceVehicleDispatches = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [ | ||
| 'select' => [ | ||
| 'Camp_Vehicle_Dispatch.Collection_Camp_Intent_Id', | ||
| ], | ||
| 'where' => [ | ||
| ['Camp_Vehicle_Dispatch.Collection_Camp_Intent_Id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
||
| $vehicleByTrackingId[$trackingId] = count($collectionSourceVehicleDispatches); | ||
| } | ||
|
|
||
| $returnValues['cashContributionByTrackingId'] = $cashContributionByTrackingId; | ||
| $returnValues['productSaleAmountByTrackingId'] = $productSaleAmountByTrackingId; | ||
| $returnValues['footfallByTrackingId'] = $footfallByTrackingId; | ||
|
|
||
| // Update Cash Contributions | ||
| foreach ($cashContributionByTrackingId as $trackingId => $cashContributionSum) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Cash_Contribution' => max($cashContributionSum, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
||
| } | ||
|
|
||
| // Update Product Sale Amount | ||
| foreach ($productSaleAmountByTrackingId as $trackingId => $productSaleSum) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Product_Sale_Amount_GBG_' => max($productSaleSum, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
||
| } | ||
|
|
||
| // Update Footfall Count based on Material Contribution activities | ||
| foreach ($footfallByTrackingId as $trackingId => $footfallCount) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Footfall_at_the_center' => max($footfallCount, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
||
| } | ||
|
|
||
| // Update Vehicle Count based on Material dispatch | ||
| foreach ($vehicleByTrackingId as $trackingId => $vehicleCount) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Total_no_of_vehicle_material_collected' => max($vehicleCount, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
||
| } | ||
|
|
||
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'dropping_center_cron'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor function to follow Single Responsibility Principle
The civicrm_api3_goonjcustom_dropping_center_cron function handles data retrieval, processing, and updating within a single function. Consider breaking this function into smaller, dedicated functions for each task. This will make the code more modular, easier to read, and maintain.
For example, you could create separate functions for:
- Retrieving data (
getCollectionCamps,getActivities,getVehicleDispatches) - Processing data (
calculateContributions,calculateFootfall,calculateVehicleCounts) - Updating the database (
updateCashContributions,updateProductSales,updateFootfallCounts,updateVehicleCounts)
| if (!isset($cashContributionByTrackingId[$trackingId])) { | ||
| $cashContributionByTrackingId[$trackingId] = 0; | ||
| } | ||
| if (!isset($productSaleAmountByTrackingId[$trackingId])) { | ||
| $productSaleAmountByTrackingId[$trackingId] = 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive array initialization to improve code readability
Lines 26-31 contain repetitive code for initializing array entries to zero. Consider combining these initializations to reduce duplication and enhance readability.
Apply this diff to consolidate the array initializations:
- if (!isset($cashContributionByTrackingId[$trackingId])) {
- $cashContributionByTrackingId[$trackingId] = 0;
- }
- if (!isset($productSaleAmountByTrackingId[$trackingId])) {
- $productSaleAmountByTrackingId[$trackingId] = 0;
- }
+ foreach (['cashContributionByTrackingId', 'productSaleAmountByTrackingId'] as $arrayName) {
+ if (!isset(${$arrayName}[$trackingId])) {
+ ${$arrayName}[$trackingId] = 0;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!isset($cashContributionByTrackingId[$trackingId])) { | |
| $cashContributionByTrackingId[$trackingId] = 0; | |
| } | |
| if (!isset($productSaleAmountByTrackingId[$trackingId])) { | |
| $productSaleAmountByTrackingId[$trackingId] = 0; | |
| } | |
| foreach (['cashContributionByTrackingId', 'productSaleAmountByTrackingId'] as $arrayName) { | |
| if (!isset(${$arrayName}[$trackingId])) { | |
| ${$arrayName}[$trackingId] = 0; | |
| } | |
| } |
| ['id', '=', $trackingId], | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
Potential mismatch between 'trackingId' and 'Eck_Collection_Camp' id
Upon closer inspection, it appears that the 'trackingId' used in the update operations may not directly correspond to the 'id' of the 'Eck_Collection_Camp' entity as initially suspected. Here's what we found:
- The 'Donation_Tracking_Id' is associated with the 'Dropping_Centre' entity, not 'Eck_Collection_Camp'.
- The $trackingId variable is derived from 'Dropping_Centre.Donation_Tracking_Id'.
- This $trackingId is then used in where clauses with 'id', which could lead to unintended updates if these ids don't match.
To address this potential issue:
- Verify if the 'id' used in the where clauses (lines 76, 90, 104, 118) is indeed meant to reference the 'Eck_Collection_Camp' entity.
- If not, consider renaming the variable or adding comments to clarify its purpose and relationship to different entities.
- If it is meant to reference 'Eck_Collection_Camp', ensure that 'Dropping_Centre.Donation_Tracking_Id' correctly maps to the 'Eck_Collection_Camp' id.
This ambiguity could lead to bugs or unexpected behavior. It's crucial to clarify the relationships between these entities and ensure that the correct ids are being used in update operations.
🔗 Analysis chain
Confirm correct mapping of 'trackingId' in update queries
In the update operations at lines 76-77, 90-91, 104-105, and 118-119, the 'where' clause uses ['id', '=', $trackingId]. Ensure that $trackingId corresponds to the id of the Eck_Collection_Camp entity. If Donation_Tracking_Id is a different field, adjust the 'where' clause to reference the correct field to prevent unintended updates.
To verify, you might check the field mappings in the Eck_Collection_Camp entity to confirm whether Donation_Tracking_Id matches the id field.
Also applies to: 90-91, 104-105, 118-119
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'Donation_Tracking_Id' matches 'id' in 'Eck_Collection_Camp'
# Test: Search for the field definitions. Expect: 'Donation_Tracking_Id' is a separate field.
ast-grep --lang php --pattern '$collectionCamps = civicrm_api4("Eck_Collection_Camp", "get", $$$);'
Length of output: 9147
Script:
#!/bin/bash
# Description: Find all PHP files with valid UTF-8 encoding and search for the specific pattern
# Step 1: Find PHP files with valid UTF-8 encoding
valid_files=$(find . -type f -name "*.php" -exec bash -c 'file -i "$1" | grep -q "charset=utf-8"' _ {} \; -print)
# Step 2: Run ast-grep on the valid PHP files
for file in $valid_files; do
ast-grep --lang php --pattern '$collectionCamps = civicrm_api4("Eck_Collection_Camp", "get", $$$);' "$file"
done
Length of output: 391684
Script:
#!/bin/bash
# Search for Donation_Tracking_Id and id in the specific file
echo "Searching in DroppingCenterCron.php:"
grep -n -E "(Donation_Tracking_Id|id)" wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/DroppingCenterCron.php
# If the above doesn't yield results, search in all PHP files in the civi-extensions directory
echo "\nSearching in all PHP files in civi-extensions:"
find wp-content/civi-extensions -name "*.php" -type f | xargs grep -n -E "(Donation_Tracking_Id|id).*(Eck_Collection_Camp)"
Length of output: 1248
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterEmailDispatchService.php (2)
7-9: Remove Empty DocBlocks or Provide Meaningful DocumentationThe class and several methods have empty DocBlocks. Empty comments can clutter the code and reduce readability. Please either remove them or provide meaningful descriptions to enhance code understanding.
Also applies to: 12-14, 21-23, 84-86
93-105: Use HEREDOC Syntax for Multi-line Strings to Enhance ReadabilityEmbedding HTML within double-quoted strings can reduce readability. Using HEREDOC syntax makes the multi-line string clearer and easier to maintain.
Apply this fix:
$emailHtml = <<<EOD <html> <body> <p>Dear {$initiatorName},</p> <p>Thank you so much for your invaluable efforts in running the Goonj Dropping Center. Your dedication plays a crucial role in our work, and we deeply appreciate your continued support.</p> <p>Please fill out this Dispatch Form – <a href="{$campVehicleDispatchFormUrl}">[link]</a> once the vehicle is loaded and ready to head to Goonj’s processing center. This will help us to verify and acknowledge the materials as soon as they arrive.</p> <p>We truly appreciate your cooperation and continued commitment to our cause.</p> <p>Warm Regards,<br>Team Goonj.</p> </body> </html> EOD;wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (2)
125-150: Remove debugging statements before deploymentThe
error_logstatements used for debugging should be removed or replaced with proper logging mechanisms to avoid cluttering the logs in a production environment.Consider removing these
error_logstatements or using a controlled logging system with appropriate log levels.
79-178: Add documentation for new methodsIncluding PHPDoc blocks for the newly added methods
findOfficeIdandmailNotificationToMmtwill improve code maintainability and clarity.Consider adding method descriptions, parameter explanations, and return type annotations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterEmailDispatchService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (5 hunks)
🧰 Additional context used
🔇 Additional comments (6)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
Line range hint
1-1022: Consider refactoring for improved maintainability and completenessThe
CollectionCampServiceclass is quite large and seems to handle multiple responsibilities. This could potentially violate the Single Responsibility Principle. Consider breaking it down into smaller, more focused classes to improve maintainability and testability.Additionally, there are several long and complex methods in this class. For example,
setOfficeDetails,linkCollectionCampToContact, andsendLogisticsEmailare quite lengthy. Consider refactoring these methods into smaller, more manageable functions.Lastly, there are multiple TODO comments throughout the code. It's important to address these comments to ensure the completeness and reliability of the codebase. Here's a list of TODO comments found in the file:
#!/bin/bash # Search for TODO comments in the file rg "TODO" wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.phpPlease review and address these TODO items, or create separate issues to track them if they can't be resolved immediately.
To improve the overall architecture:
- Consider using the Factory pattern to create different types of camps or activities.
- Implement the Strategy pattern for different notification methods (email, SMS, etc.).
- Use the Observer pattern for event handling instead of the current event subscription method.
60-60: Clarification needed: Disabled mail notification to Material Management TeamThe 'mailNotificationToMmt' event subscription has been commented out. This change will prevent the Material Management Team from receiving notifications related to custom hook events.
Could you please provide more context on why this notification is being disabled? If this is a temporary change, consider adding a TODO comment explaining the reason and when it should be re-enabled.
To ensure this change doesn't have unintended consequences, let's check for any references to this method:
If this is indeed a permanent change, consider removing the commented line entirely to keep the code clean.
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (4)
9-12: Imports added appropriatelyThe added use statements correctly import the necessary classes for email handling and API interactions.
23-23: ConstantMATERIAL_RELATIONSHIP_TYPE_NAMEdefined correctlyThe constant is appropriately defined for reuse within the class, enhancing maintainability.
32-32: Subscribed to new event hook&hook_civicrm_customsuccessfullyThe event hook is correctly subscribed to the
mailNotificationToMmtmethod for custom operations.
203-208: New tab 'Send Dispatch Email' added correctlyThe new tab configuration for 'Send Dispatch Email' is appropriately incorporated into the
droppingCenterTabsetmethod.
| $afformName = $objectRef->afform_name; | ||
|
|
There was a problem hiding this comment.
Ensure Properties Exist Before Access to Prevent Undefined Property Notices
Accessing $objectRef->afform_name without verifying its existence may lead to undefined property notices if afform_name is not set. Please add a check to ensure the property exists before accessing it.
Apply this fix:
if (isset($objectRef->afform_name)) {
$afformName = $objectRef->afform_name;
} else {
// Handle the case where afform_name is not set
return;
}| $droppingCenterId = $dataArray['Eck_Collection_Camp1'][0]['fields']['id']; | ||
|
|
There was a problem hiding this comment.
Verify Array Indices Before Access to Prevent Undefined Index Errors
Accessing $dataArray['Eck_Collection_Camp1'][0]['fields']['id'] without verifying each index may result in undefined index errors if any index is missing. Please add checks to ensure all indices exist before accessing them.
Apply this fix:
if (
isset($dataArray['Eck_Collection_Camp1']) &&
isset($dataArray['Eck_Collection_Camp1'][0]['fields']['id'])
) {
$droppingCenterId = $dataArray['Eck_Collection_Camp1'][0]['fields']['id'];
} else {
// Handle the case where the ID is not available
return;
}| $contactId = $droppingCenter['Collection_Camp_Core_Details.Contact_Id'] ?? NULL; | ||
| $droppingCenterGoonjOffice = $droppingCenter['Collection_Camp_Intent_Details.Goonj_Office'] ?? NULL; |
There was a problem hiding this comment.
Avoid Overwriting Variables to Maintain Clarity
The variable $droppingCenterGoonjOffice is assigned at lines 47 and 79 with potentially different data. Overwriting variables can lead to confusion and bugs. Consider using distinct variable names to represent different data.
Apply this fix:
-$droppingCenterGoonjOffice = $droppingCenter['Collection_Camp_Intent_Details.Goonj_Office'] ?? NULL;
+$droppingCenterGoonjOfficeFromIntent = $droppingCenter['Collection_Camp_Intent_Details.Goonj_Office'] ?? NULL;
...
-$droppingCenterGoonjOffice = $goonjOfficeRecord['Dropping_Centre.Goonj_Office'] ?? 'N/A';
+$droppingCenterGoonjOfficeFromOfficeRecord = $goonjOfficeRecord['Dropping_Centre.Goonj_Office'] ?? 'N/A';Then decide which variable to use based on context.
Also applies to: 79-79
| $collectionCamp = civicrm_api4('Eck_Collection_Camp', 'get', [ | ||
| 'select' => [ | ||
| 'Dropping_Centre.Goonj_Office', | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', 390], | ||
| ], | ||
| 'limit' => 25, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Hardcoding IDs; Use Configuration or Constants Instead
Hardcoding the ID 390 can make maintenance difficult and error-prone. It's better to use a constant or configuration parameter to improve code flexibility and readability.
Apply this fix:
define('GOONJ_OFFICE_COLLECTION_CAMP_ID', 390);
...
$collectionCamp = civicrm_api4('Eck_Collection_Camp', 'get', [
// ...
'where' => [
['id', '=', GOONJ_OFFICE_COLLECTION_CAMP_ID],
],
// ...
]);| $campVehicleDispatchFormUrl = $homeUrl . '/vehicle-dispatch/#?Camp_Vehicle_Dispatch.Collection_Camp=' . $droppingCenterId . '&Camp_Vehicle_Dispatch.Filled_by=' . $contactId . '&Camp_Vehicle_Dispatch.To_which_PU_Center_material_is_being_sent=' . $droppingCenterGoonjOffice . '&Eck_Collection_Camp1=' . $droppingCenterId; | ||
| $campOutcomeFormUrl = $homeUrl . '/camp-outcome-form/#?Eck_Collection_Camp1=' . $droppingCenterId . '&Camp_Outcome.Filled_By=' . $contactId; |
There was a problem hiding this comment.
Properly Encode URL Parameters to Prevent Injection Attacks
When constructing URLs with dynamic parameters, it's important to URL-encode them to prevent security vulnerabilities like injection attacks. Please ensure all parameters are properly encoded.
Apply this fix:
$campVehicleDispatchFormUrl = $homeUrl . '/vehicle-dispatch/#?' .
'Camp_Vehicle_Dispatch.Collection_Camp=' . urlencode($droppingCenterId) . '&' .
'Camp_Vehicle_Dispatch.Filled_by=' . urlencode($contactId) . '&' .
'Camp_Vehicle_Dispatch.To_which_PU_Center_material_is_being_sent=' . urlencode($droppingCenterGoonjOffice) . '&' .
'Eck_Collection_Camp1=' . urlencode($droppingCenterId);
$campOutcomeFormUrl = $homeUrl . '/camp-outcome-form/#?' .
'Eck_Collection_Camp1=' . urlencode($droppingCenterId) . '&' .
'Camp_Outcome.Filled_By=' . urlencode($contactId);| $contactData = civicrm_api4('Contact', 'get', [ | ||
| 'select' => [ | ||
| 'email_primary.email', | ||
| 'phone_primary.phone', | ||
| 'display_name', | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $contactId], | ||
| ], | ||
| 'limit' => 1, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Duplicate API Calls into Reusable Methods
The code contains duplicate API calls to fetch data from Eck_Collection_Camp and Contact. Refactoring these into reusable methods will adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability.
Apply this fix:
private static function getDroppingCenterData($droppingCenterId) {
// API call to fetch dropping center data
}
private static function getContactData($contactId) {
// API call to fetch contact data
}Then replace the inline API calls with these methods.
Also applies to: 68-77
| public static function goonjcustom_material_management_email_html($collectionCampId, $droppingCenterCode, $droppingCenterAddress, $vehicleDispatchId) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
| $materialdispatchUrl = $homeUrl . '/acknowledgement-form-for-dispatch/#?Eck_Collection_Source_Vehicle_Dispatch1=' . $vehicleDispatchId . '&Camp_Vehicle_Dispatch.Collection_Camp=' . $collectionCampId . '&id=' . $vehicleDispatchId . '&Eck_Collection_Camp1=' . $collectionCampId; | ||
| $html = " | ||
| <p>Dear MMT team,</p> | ||
| <p>This is to inform you that a vehicle has been sent from the dropping center <strong>$droppingCenterCode</strong> at <strong>$droppingCenterAddress</strong>.</p> | ||
| <p>Kindly acknowledge the details by clicking on this form <a href=\"$materialdispatchUrl\"> Link </a> when it is received at the center.</p> | ||
| <p>Warm regards,<br>Urban Relations Team</p>"; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
Sanitize variables in email HTML to prevent XSS vulnerabilities
User-generated content inserted into the HTML should be sanitized to protect against cross-site scripting attacks.
Escape HTML entities for all variables used in the HTML content:
$droppingCenterCode = htmlspecialchars($droppingCenterCode, ENT_QUOTES, 'UTF-8');
$droppingCenterAddress = htmlspecialchars($droppingCenterAddress, ENT_QUOTES, 'UTF-8');
$materialdispatchUrl = htmlspecialchars($materialdispatchUrl, ENT_QUOTES, 'UTF-8');| $fromEmail = OptionValue::get(FALSE) | ||
| ->addSelect('label') | ||
| ->addWhere('option_group_id:name', '=', 'from_email_address') | ||
| ->addWhere('is_default', '=', TRUE) | ||
| ->execute()->single(); | ||
|
|
There was a problem hiding this comment.
Validate $fromEmail before sending the email
Ensure that the default 'from' email address is correctly retrieved to prevent errors during email sending.
Include a check for $fromEmail:
$fromEmail = OptionValue::get(FALSE)
->addSelect('label')
->addWhere('option_group_id:name', '=', 'from_email_address')
->addWhere('is_default', '=', TRUE)
->execute()->single();
+ if (!$fromEmail) {
+ // Handle missing default from email address
+ 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.
| $fromEmail = OptionValue::get(FALSE) | |
| ->addSelect('label') | |
| ->addWhere('option_group_id:name', '=', 'from_email_address') | |
| ->addWhere('is_default', '=', TRUE) | |
| ->execute()->single(); | |
| $fromEmail = OptionValue::get(FALSE) | |
| ->addSelect('label') | |
| ->addWhere('option_group_id:name', '=', 'from_email_address') | |
| ->addWhere('is_default', '=', TRUE) | |
| ->execute()->single(); | |
| if (!$fromEmail) { | |
| // Handle missing default from email address | |
| return; | |
| } |
| $email = Email::get(FALSE) | ||
| ->addSelect('email') | ||
| ->addWhere('contact_id', '=', $mmtId) | ||
| ->execute()->single(); | ||
|
|
||
| $mmtEmail = $email['email']; | ||
|
|
There was a problem hiding this comment.
Ensure $email is valid before using
If the Email::get query returns no results, accessing $email['email'] will cause an error.
Add a check to verify $email is not null:
$email = Email::get(FALSE)
->addSelect('email')
->addWhere('contact_id', '=', $mmtId)
->execute()->single();
+ if (!$email) {
+ // Handle the absence of an email address appropriately
+ return;
+ }
$mmtEmail = $email['email'];Committable suggestion was skipped due to low confidence.
| $mmtId = $coordinators['contact_id_a']; | ||
|
|
There was a problem hiding this comment.
Add null check for $coordinators before accessing
Accessing $coordinators['contact_id_a'] without ensuring $coordinators is not null may lead to errors if the query returns no results.
Include a null check for $coordinators:
$coordinators = Relationship::get(FALSE)
->addWhere('contact_id_b', '=', $goonjFieldId)
->addWhere('relationship_type_id:name', '=', self::MATERIAL_RELATIONSHIP_TYPE_NAME)
->addWhere('is_current', '=', TRUE)
->execute()->first();
+ if (!$coordinators) {
+ return;
+ }
$mmtId = $coordinators['contact_id_a'];Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/Traits/SubtypeSource.php (2)
5-7: Enhance trait documentationWhile the current documentation provides a basic description, it could be more informative.
Consider expanding the PHPDoc to include:
- The purpose of the trait
- How and when it should be used
- Any dependencies or requirements
Example:
/** * Trait to handle subtype name retrieval by entity ID. * * This trait provides a method to fetch the subtype name for a given entity ID * using the CiviCRM API4. It's designed to be used in classes that need to * retrieve subtype information for Eck_Collection_Camp entities. * * @see \Civi\API\Api4 */
10-18: Improve method documentationThe PHPDoc for
getSubtypeNameByEntityIdis good, but could be enhanced.Consider adding:
- A brief explanation of what a subtype is in this context
- Mention of the API used (CiviCRM API4)
- Possible exceptions that might be thrown
Example:
/** * Get the subtype name by entity ID. * * Retrieves the subtype name for an Eck_Collection_Camp entity using CiviCRM API4. * Subtypes in this context refer to specific categories or classifications of collection camps. * * @param int $entityID * The ID of the Eck_Collection_Camp entity. * * @return string|null * The subtype name or NULL if not found. * * @throws \Civi\API\Exception\UnauthorizedException * If the user doesn't have permission to access the API. * @throws \API_Exception * If there's an error in the API call. */wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (2)
25-28: Consider grouping related constants together for improved readability.The newly added constants are spread out among existing ones. Consider grouping related constants together, such as:
const ENTITY_NAME = 'Collection_Camp'; const ENTITY_SUBTYPE_NAME = 'Dropping_Center'; const RELATIONSHIP_TYPE_NAME = 'Collection Camp Coordinator of'; const MATERIAL_RELATIONSHIP_TYPE_NAME = 'Material Management Team of'; const FALLBACK_OFFICE_NAME = 'Delhi';This organization improves code readability and makes it easier to understand the relationships between constants.
Line range hint
94-177: Refactor common array filtering logic into a separate method.The
findStateFieldandfindOfficeIdmethods contain similar array filtering logic. To improve code maintainability and reduce duplication, consider extracting this common logic into a separate method. For example:private static function findCustomFieldInArray(array $array, string $entityTable, string $customFieldName, string $customGroupName) { $filteredItems = array_filter($array, fn($item) => $item['entity_table'] === $entityTable); if (empty($filteredItems)) { return FALSE; } $customField = CustomField::get(FALSE) ->addSelect('id') ->addWhere('name', '=', $customFieldName) ->addWhere('custom_group_id:name', '=', $customGroupName) ->execute() ->first(); if (!$customField) { return FALSE; } $customFieldId = $customField['id']; $itemIndex = array_search(TRUE, array_map(fn($item) => $item['entity_table'] === $entityTable && $item['custom_field_id'] == $customFieldId, $filteredItems )); return $itemIndex !== FALSE ? $filteredItems[$itemIndex] : FALSE; }Then, you can simplify
findStateFieldandfindOfficeIdby calling this new method with appropriate parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (4 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterEmailDispatchService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (5 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/Traits/SubtypeSource.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterEmailDispatchService.php
🧰 Additional context used
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/Civi/Traits/SubtypeSource.php (3)
1-3: LGTM: Namespace declaration looks good!The namespace
Civi\Traitsis appropriately declared, following PSR-4 autoloading standards.
8-8: LGTM: Trait declaration is correctThe trait
SubtypeSourceis properly declared.
19-32: 🛠️ Refactor suggestionRefactor method for improved error handling and readability
The method implementation could be improved for better error handling and readability.
Consider the following changes:
- Use try-catch for explicit error handling
- Use null coalescing operator for more concise code
- Add type hinting for the return value
Here's a suggested refactor:
public static function getSubtypeNameByEntityId(int $entityID): ?string { try { $result = civicrm_api4('Eck_Collection_Camp', 'get', [ 'select' => ['subtype:name'], 'where' => [['id', '=', $entityID]], 'limit' => 1, ]); return $result[0]['subtype:name'] ?? null; } catch (\Throwable $e) { // Log the error or handle it as appropriate for your application \Civi::log()->error('Error fetching subtype name: ' . $e->getMessage()); return null; } }This refactored version:
- Adds type hinting for the parameter and return value
- Uses a try-catch block for error handling
- Simplifies the result extraction using the null coalescing operator
- Limits the API result to 1 record for efficiency
To ensure this refactor doesn't break existing functionality, please run the following verification script:
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (1)
254-264:⚠️ Potential issueImprove email HTML generation and security in
goonjcustom_material_management_email_html.The current implementation of
goonjcustom_material_management_email_htmlhas room for improvement:
Template separation:
Consider moving the HTML structure to a separate template file. This would improve maintainability and allow for easier updates to the email content.Input sanitization:
Implement proper sanitization for all variables used in the HTML content to prevent potential XSS vulnerabilities. For example:$droppingCenterCode = htmlspecialchars($droppingCenterCode, ENT_QUOTES, 'UTF-8'); $droppingCenterAddress = htmlspecialchars($droppingCenterAddress, ENT_QUOTES, 'UTF-8'); $materialdispatchUrl = htmlspecialchars($materialdispatchUrl, ENT_QUOTES, 'UTF-8');Use of a templating engine:
Consider using a templating engine like Smarty or Twig, which are commonly used in CiviCRM, to generate the email content. This would provide better separation of concerns and built-in security features.Implementing these changes will improve the method's maintainability and security.
| private static function getFallbackCoordinator() { | ||
| $fallbackOffice = self::getFallbackOffice(); | ||
| $fallbackCoordinators = Relationship::get(FALSE) | ||
| ->addWhere('contact_id_b', '=', $fallbackOffice['id']) | ||
| ->addWhere('relationship_type_id:name', '=', self::RELATIONSHIP_TYPE_NAME) | ||
| ->addWhere('is_current', '=', FALSE) | ||
| ->execute(); | ||
|
|
||
| $coordinatorCount = $fallbackCoordinators->count(); | ||
|
|
||
| $randomIndex = rand(0, $coordinatorCount - 1); | ||
| $coordinator = $fallbackCoordinators->itemAt($randomIndex); | ||
|
|
||
| return $coordinator; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more deterministic approach for fallback coordinator selection.
The current implementation uses a random selection for the fallback coordinator. While this provides a fair distribution, it might lead to inconsistent assignments across different executions. Consider implementing a more deterministic approach, such as:
- Selecting the coordinator with the least number of current assignments.
- Using a round-robin selection based on a stored index.
- Choosing based on a hash of the current timestamp or other unique identifier.
This would provide more consistent and potentially more balanced coordinator assignments.
| public static function mailNotificationToMmt($op, $groupID, $entityID, &$params) { | ||
| if ($op !== 'create') { | ||
| return; | ||
| } | ||
|
|
||
| if (!($goonjField = self::findOfficeId($params))) { | ||
| return; | ||
| } | ||
|
|
||
| $goonjFieldId = $goonjField['value']; | ||
| $vehicleDispatchId = $goonjField['entity_id']; | ||
|
|
||
| $collectionSourceVehicleDispatch = EckEntity::get('Collection_Source_Vehicle_Dispatch', FALSE) | ||
| ->addSelect('Camp_Vehicle_Dispatch.Collection_Camp') | ||
| ->addWhere('id', '=', $vehicleDispatchId) | ||
| ->execute()->first(); | ||
|
|
||
| error_log("collectionSourceVehicleDispatch: " . print_r($collectionSourceVehicleDispatch, TRUE)); | ||
|
|
||
| $collectionCampId = $collectionSourceVehicleDispatch['Camp_Vehicle_Dispatch.Collection_Camp']; | ||
|
|
||
| error_log("collectionCampId: " . print_r($collectionCampId, TRUE)); | ||
|
|
||
| $collectionCamp = EckEntity::get('Collection_Camp', FALSE) | ||
| ->addSelect('Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_', 'title') | ||
| ->addWhere('id', '=', $collectionCampId) | ||
| ->execute()->single(); | ||
|
|
||
| error_log("collectionCamp: " . print_r($collectionCamp, TRUE)); | ||
|
|
||
| $droppingCenterCode = $collectionCamp['title']; | ||
| $droppingCenterAddress = $collectionCamp['Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_']; | ||
| error_log("goonjFieldId: " . print_r($goonjFieldId, TRUE)); | ||
| $coordinators = Relationship::get(FALSE) | ||
| ->addWhere('contact_id_b', '=', $goonjFieldId) | ||
| ->addWhere('relationship_type_id:name', '=', self::MATERIAL_RELATIONSHIP_TYPE_NAME) | ||
| ->addWhere('is_current', '=', TRUE) | ||
| ->execute()->first(); | ||
|
|
||
| $mmtId = $coordinators['contact_id_a']; | ||
|
|
||
| error_log("mmtId: " . print_r($mmtId, TRUE)); | ||
|
|
||
| if (empty($mmtId)) { | ||
| return; | ||
| } | ||
|
|
||
| $email = Email::get(FALSE) | ||
| ->addSelect('email') | ||
| ->addWhere('contact_id', '=', $mmtId) | ||
| ->execute()->single(); | ||
|
|
||
| $mmtEmail = $email['email']; | ||
|
|
||
| $fromEmail = OptionValue::get(FALSE) | ||
| ->addSelect('label') | ||
| ->addWhere('option_group_id:name', '=', 'from_email_address') | ||
| ->addWhere('is_default', '=', TRUE) | ||
| ->execute()->single(); | ||
|
|
||
| // Email to material management team member. | ||
| $mailParams = [ | ||
| 'subject' => 'Dropping center Address ' . $droppingCenterAddress . ' - Material Acknowledgement', | ||
| 'from' => $fromEmail['label'], | ||
| 'toEmail' => $mmtEmail, | ||
| 'replyTo' => $fromEmail['label'], | ||
| 'html' => self::goonjcustom_material_management_email_html($collectionCampId, $droppingCenterCode, $droppingCenterAddress, $vehicleDispatchId), | ||
| ]; | ||
| \CRM_Utils_Mail::send($mailParams); | ||
|
|
||
| } |
There was a problem hiding this comment.
Optimize database queries and improve error handling in mailNotificationToMmt.
The mailNotificationToMmt method has several areas for improvement:
-
Database query optimization:
Consider combining multiple queries into a single query where possible. For example, you could join theEckEntityandRelationshipqueries to reduce the number of database calls. -
Error logging:
Remove or modify error logging statements that might expose sensitive information in production. Instead, consider using a logging framework that allows for different log levels in different environments. -
Error handling:
Add error handling for the email sending process. For example:try { \CRM_Utils_Mail::send($mailParams); } catch (\Exception $e) { // Log the error and handle it appropriately \Civi::log()->error('Failed to send email notification: ' . $e->getMessage()); }
-
Input validation:
Add input validation for critical variables like$mmtEmailand$fromEmailbefore using them in the email sending process.
Implementing these changes will improve the method's robustness and security.
belwalshubham
changed the base branch from
develop
to
feat/assign-goonj-office-by-state
belwalshubham
changed the base branch from
feat/assign-goonj-office-by-state
to
develop
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (3)
23-25: Constants updated appropriatelyThe
RELATIONSHIP_TYPE_NAMEconstant has been updated, and a newMATERIAL_RELATIONSHIP_TYPE_NAMEconstant has been added. These changes align with the new functionality in the class.Consider adding a brief comment explaining the purpose of these constants, especially the new
MATERIAL_RELATIONSHIP_TYPE_NAME, to improve code readability.
35-38: Event subscription updated correctlyThe
getSubscribedEventsmethod has been updated to include the newmailNotificationToMmtcallback for thehook_civicrm_customevent. This change aligns with the new email notification functionality.Consider adding a brief comment explaining the purpose of each callback in the
hook_civicrm_customarray to improve code readability and maintainability.
Line range hint
1-455: Overall assessment: Good additions with room for improvementThe changes to the
DroppingCenterServiceclass introduce new email notification functionality and improve dropping center management. The new methods and updates to existing ones are generally well-integrated with the existing code structure.However, there are several areas where the code quality and security can be improved:
- The
mailNotificationToMmtmethod is quite long and could benefit from being broken down into smaller, more focused methods.- Error handling and logging should be improved throughout the class, replacing
error_logstatements with CiviCRM's logging mechanism.- Input sanitization is needed in the
goonjcustom_material_management_email_htmlmethod to prevent potential XSS vulnerabilities.- Some methods, like
findOfficeId, could be optimized for better readability and efficiency.Consider implementing the following architectural improvements:
- Introduce a separate EmailService class to handle email-related functionality, improving separation of concerns.
- Use dependency injection for services like logging and email sending, making the class more testable and flexible.
- Implement a configuration file for constants and other configurable values, making the class more maintainable.
These changes will enhance the overall quality, security, and maintainability of the
DroppingCenterServiceclass.wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampCron.php (1)
84-85: Remove unnecessary debug loggingThe
error_logstatement at line 84 is logging$productSaleAmountByTrackingId, which is an array. Logging large arrays or sensitive data can clutter logs and potentially expose sensitive information.If this logging was used for debugging purposes and is no longer needed, consider removing it. If logging is necessary, ensure that you log specific and non-sensitive information.
Apply this diff to clean up the code:
- error_log("productSaleAmountByTrackingId : $productSaleAmountByTrackingId, Result: " . print_r($productSaleAmountByTrackingId, TRUE));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (5 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampCron.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/DroppingCenterOutcomeCron.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (3)
12-13: New imports look good!The added use statements for
OptionValueare appropriate for the new functionality in the class.
350-355: New tab configuration added correctlyThe
droppingCenterTabsetmethod has been updated to include a new tab configuration for 'SendDispatchEmail'. This addition aligns well with the new email notification functionality introduced in the class.The structure of the new tab configuration is consistent with the existing ones, maintaining code consistency.
170-240: 🛠️ Refactor suggestion
⚠️ Potential issueRefactor
mailNotificationToMmtfor improved readability and error handlingThe
mailNotificationToMmtmethod is quite long and could benefit from several improvements:
- Break down the method into smaller, more focused methods to improve readability and maintainability.
- Replace
error_logstatements with proper logging using CiviCRM's logging mechanism.- Improve error handling, especially for database queries and email sending.
- Consider using early returns to reduce nesting.
Here's a suggested structure for refactoring:
public static function mailNotificationToMmt($op, $groupID, $entityID, &$params) { if ($op !== 'create') { return; } $goonjField = self::findOfficeId($params); if (!$goonjField) { \Civi::log()->warning('No Goonj field found for mailNotificationToMmt'); return; } $vehicleDispatchData = self::getVehicleDispatchData($goonjField); if (!$vehicleDispatchData) { return; } $collectionCampData = self::getCollectionCampData($vehicleDispatchData['collectionCampId']); if (!$collectionCampData) { return; } $mmtEmail = self::getMmtEmail($goonjField['value']); if (!$mmtEmail) { return; } $fromEmail = self::getDefaultFromEmail(); if (!$fromEmail) { return; } self::sendNotificationEmail($mmtEmail, $fromEmail, $collectionCampData, $vehicleDispatchData); } // Implement the helper methods (getVehicleDispatchData, getCollectionCampData, getMmtEmail, getDefaultFromEmail, sendNotificationEmail)This structure improves readability, error handling, and maintainability by breaking down the large method into smaller, focused methods.
Replace
error_logstatements with CiviCRM's logging mechanism:\Civi::log()->info('collectionSourceVehicleDispatch: ' . print_r($collectionSourceVehicleDispatch, TRUE));Ensure proper error handling is implemented for database queries and email sending. Add try-catch blocks where necessary:
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampCron.php (1)
186-187:⚠️ Potential issueFix: Extra closing brace causing syntax error
There are two closing braces at lines 186 and 187. Given the structure of the code, this may result in a syntax error due to an unmatched brace.
Ensure that each opening brace
{has a corresponding closing brace}. Remove the extra closing brace if it's not required.Apply this diff to fix the issue:
-}Likely invalid or redundant comment.
| private static function findOfficeId(array $array) { | ||
| $filteredItems = array_filter($array, fn($item) => $item['entity_table'] === 'civicrm_eck_collection_source_vehicle_dispatch'); | ||
|
|
||
| if (empty($filteredItems)) { | ||
| return FALSE; | ||
| } | ||
|
|
||
| $goonjOfficeId = CustomField::get(FALSE) | ||
| ->addSelect('id') | ||
| ->addWhere('custom_group_id:name', '=', 'Camp_Vehicle_Dispatch') | ||
| ->addWhere('name', '=', 'To_which_PU_Center_material_is_being_sent') | ||
| ->execute() | ||
| ->first(); | ||
|
|
||
| if (!$goonjOfficeId) { | ||
| return FALSE; | ||
| } | ||
|
|
||
| $goonjOfficeFieldId = $goonjOfficeId['id']; | ||
|
|
||
| $goonjOfficeIndex = array_search(TRUE, array_map(fn($item) => | ||
| $item['entity_table'] === 'civicrm_eck_collection_source_vehicle_dispatch' && | ||
| $item['custom_field_id'] == $goonjOfficeFieldId, | ||
| $filteredItems | ||
| )); | ||
|
|
||
| return $goonjOfficeIndex !== FALSE ? $filteredItems[$goonjOfficeIndex] : FALSE; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize findOfficeId method for better readability and efficiency
The findOfficeId method serves its purpose but could be improved:
- Consider using early returns to reduce nesting and improve readability.
- The combination of
array_filterandarray_searchwitharray_mapcould be replaced with a single loop for better efficiency.
Here's a suggested refactoring:
private static function findOfficeId(array $array) {
$filteredItems = array_filter($array, fn($item) => $item['entity_table'] === 'civicrm_eck_collection_source_vehicle_dispatch');
if (empty($filteredItems)) {
return FALSE;
}
$goonjOfficeId = CustomField::get(FALSE)
->addSelect('id')
->addWhere('custom_group_id:name', '=', 'Camp_Vehicle_Dispatch')
->addWhere('name', '=', 'To_which_PU_Center_material_is_being_sent')
->execute()
->first();
if (!$goonjOfficeId) {
return FALSE;
}
$goonjOfficeFieldId = $goonjOfficeId['id'];
foreach ($filteredItems as $item) {
if ($item['custom_field_id'] == $goonjOfficeFieldId) {
return $item;
}
}
return FALSE;
}This refactoring improves readability and efficiency by using early returns and replacing the combination of array_search and array_map with a simple loop.
| /** | ||
| * | ||
| */ | ||
| public static function goonjcustom_material_management_email_html($collectionCampId, $droppingCenterCode, $droppingCenterAddress, $vehicleDispatchId) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
| $materialdispatchUrl = $homeUrl . '/acknowledgement-form-for-dispatch/#?Eck_Collection_Source_Vehicle_Dispatch1=' . $vehicleDispatchId . '&Camp_Vehicle_Dispatch.Collection_Camp=' . $collectionCampId . '&id=' . $vehicleDispatchId . '&Eck_Collection_Camp1=' . $collectionCampId; | ||
| $html = " | ||
| <p>Dear MMT team,</p> | ||
| <p>This is to inform you that a vehicle has been sent from the dropping center <strong>$droppingCenterCode</strong> at <strong>$droppingCenterAddress</strong>.</p> | ||
| <p>Kindly acknowledge the details by clicking on this form <a href=\"$materialdispatchUrl\"> Link </a> when it is received at the center.</p> | ||
| <p>Warm regards,<br>Urban Relations Team</p>"; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve security and maintainability of goonjcustom_material_management_email_html
The goonjcustom_material_management_email_html method generates HTML content for email notifications. While it's functional, there are some areas for improvement:
- Sanitize input parameters to prevent potential XSS vulnerabilities.
- Consider using a template engine or heredoc syntax for better readability of the HTML structure.
- Move the HTML template to a separate file for better separation of concerns.
Sanitize input parameters to prevent XSS:
$droppingCenterCode = htmlspecialchars($droppingCenterCode, ENT_QUOTES, 'UTF-8');
$droppingCenterAddress = htmlspecialchars($droppingCenterAddress, ENT_QUOTES, 'UTF-8');
$materialdispatchUrl = htmlspecialchars($materialdispatchUrl, ENT_QUOTES, 'UTF-8');Consider using heredoc syntax for better readability:
$html = <<<HTML
<p>Dear MMT team,</p>
<p>This is to inform you that a vehicle has been sent from the dropping center <strong>{$droppingCenterCode}</strong> at <strong>{$droppingCenterAddress}</strong>.</p>
<p>Kindly acknowledge the details by clicking on this form <a href="{$materialdispatchUrl}">Link</a> when it is received at the center.</p>
<p>Warm regards,<br>Urban Relations Team</p>
HTML;Consider moving the HTML template to a separate file and using a template engine for better separation of concerns and maintainability.
| $activities = civicrm_api4('Activity', 'get', [ | ||
| 'select' => ['subject'], | ||
| 'where' => [ | ||
| ['activity_type_id:name', '=', 'Material Contribution'], | ||
| ['Material_Contribution.Dropping_Center', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| $footfallByTrackingId[$trackingId] = count($activities); | ||
|
|
||
| // Calculate vehicle dispatch count. | ||
| $collectionSourceVehicleDispatches = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [ | ||
| 'select' => ['Camp_Vehicle_Dispatch.Collection_Camp'], | ||
| 'where' => [['Camp_Vehicle_Dispatch.Collection_Camp', '=', $trackingId]], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| $vehicleDispatchesByTrackingId[$trackingId] = count($collectionSourceVehicleDispatches); | ||
|
|
||
| // Calculate number of bags received. | ||
| $bagData = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [ | ||
| 'select' => [ | ||
| 'Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office', | ||
| 'Camp_Vehicle_Dispatch.Collection_Camp', | ||
| ], | ||
| 'where' => [ | ||
| ['Camp_Vehicle_Dispatch.Collection_Camp', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize performance by reducing API calls within the loop
Multiple API calls are being made inside the loop for each dropping center. This can lead to performance issues with large datasets.
Consider fetching all required data in bulk outside the loop and processing it in memory to minimize the number of API calls.
| foreach ($cashContributionByTrackingId as $trackingId => $cashContributionSum) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Cash_Contribution' => max($cashContributionSum, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
| } | ||
|
|
||
| // Update Product Sale Amount. | ||
| foreach ($productSaleAmountByTrackingId as $trackingId => $productSaleSum) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Product_Sale_Amount_GBG_' => max($productSaleSum, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
| } | ||
|
|
||
| // Update Footfall Count based on Material Contribution activities. | ||
| foreach ($footfallByTrackingId as $trackingId => $footfallCount) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Footfall_at_the_center' => max($footfallCount, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| } | ||
|
|
||
| // Update Vehicle Count based on dispatches. | ||
| foreach ($vehicleDispatchesByTrackingId as $trackingId => $vehicleCount) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Total_no_of_vehicle_material_collected' => max($vehicleCount, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| } | ||
|
|
||
| // Update Bags Received. | ||
| foreach ($bagsReceivedByTrackingId as $trackingId => $bagsReceived) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Total_no_of_bags_received_from_center' => max($bagsReceived, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate duplicate update code
The update operations for different metrics are similar and repeated multiple times.
Consider creating a generic function to handle these updates. This will reduce code duplication and improve maintainability.
| function civicrm_api3_goonjcustom_dropping_center_outcome_cron($params) { | ||
| $returnValues = []; | ||
|
|
||
| $droppingCenters = civicrm_api4('Eck_Collection_Camp', 'get', [ | ||
| 'select' => [ | ||
| 'Donation_Box_Register_Tracking.Cash_Contribution', | ||
| 'Dropping_Centre.Donation_Tracking_Id', | ||
| 'Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_', | ||
| ], | ||
| 'where' => [ | ||
| ['Dropping_Centre.Donation_Tracking_Id', 'IS NOT NULL'], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
||
| $cashContributionByTrackingId = []; | ||
| $productSaleAmountByTrackingId = []; | ||
| $footfallByTrackingId = []; | ||
| $vehicleDispatchesByTrackingId = []; | ||
| $bagsReceivedByTrackingId = []; | ||
|
|
||
| foreach ($droppingCenters as $center) { | ||
| $trackingId = $center['Dropping_Centre.Donation_Tracking_Id']; | ||
|
|
||
| if (!isset($cashContributionByTrackingId[$trackingId])) { | ||
| $cashContributionByTrackingId[$trackingId] = 0; | ||
| } | ||
| if (!isset($productSaleAmountByTrackingId[$trackingId])) { | ||
| $productSaleAmountByTrackingId[$trackingId] = 0; | ||
| } | ||
|
|
||
| // Calculate cash contibution. | ||
| $cashContributionByTrackingId[$trackingId] += (int) ($center['Donation_Box_Register_Tracking.Cash_Contribution'] ?? 0); | ||
|
|
||
| // Calculate product sale amounts. | ||
| $productSaleAmountByTrackingId[$trackingId] += (float) ($center['Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_'] ?? 0); | ||
|
|
||
| // Calculate footfall. | ||
| $activities = civicrm_api4('Activity', 'get', [ | ||
| 'select' => ['subject'], | ||
| 'where' => [ | ||
| ['activity_type_id:name', '=', 'Material Contribution'], | ||
| ['Material_Contribution.Dropping_Center', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| $footfallByTrackingId[$trackingId] = count($activities); | ||
|
|
||
| // Calculate vehicle dispatch count. | ||
| $collectionSourceVehicleDispatches = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [ | ||
| 'select' => ['Camp_Vehicle_Dispatch.Collection_Camp'], | ||
| 'where' => [['Camp_Vehicle_Dispatch.Collection_Camp', '=', $trackingId]], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| $vehicleDispatchesByTrackingId[$trackingId] = count($collectionSourceVehicleDispatches); | ||
|
|
||
| // Calculate number of bags received. | ||
| $bagData = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [ | ||
| 'select' => [ | ||
| 'Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office', | ||
| 'Camp_Vehicle_Dispatch.Collection_Camp', | ||
| ], | ||
| 'where' => [ | ||
| ['Camp_Vehicle_Dispatch.Collection_Camp', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
||
| $totalBags = 0; | ||
|
|
||
| foreach ($bagData as $record) { | ||
| if (!empty($record['Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office'])) { | ||
| $totalBags += (int) $record['Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office']; | ||
| } | ||
| } | ||
|
|
||
| // Store the totalBags count into $bagsReceivedByTrackingId for further use. | ||
| $bagsReceivedByTrackingId[$trackingId] = $totalBags; | ||
| } | ||
|
|
||
| // Update Cash Contributions. | ||
| foreach ($cashContributionByTrackingId as $trackingId => $cashContributionSum) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Cash_Contribution' => max($cashContributionSum, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
| } | ||
|
|
||
| // Update Product Sale Amount. | ||
| foreach ($productSaleAmountByTrackingId as $trackingId => $productSaleSum) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Product_Sale_Amount_GBG_' => max($productSaleSum, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
| } | ||
|
|
||
| // Update Footfall Count based on Material Contribution activities. | ||
| foreach ($footfallByTrackingId as $trackingId => $footfallCount) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Footfall_at_the_center' => max($footfallCount, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| } | ||
|
|
||
| // Update Vehicle Count based on dispatches. | ||
| foreach ($vehicleDispatchesByTrackingId as $trackingId => $vehicleCount) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Total_no_of_vehicle_material_collected' => max($vehicleCount, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| } | ||
|
|
||
| // Update Bags Received. | ||
| foreach ($bagsReceivedByTrackingId as $trackingId => $bagsReceived) { | ||
| civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => ['Dropping_Center_Outcome.Total_no_of_bags_received_from_center' => max($bagsReceived, 0)], | ||
| 'where' => [['id', '=', $trackingId]], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| } | ||
|
|
||
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'dropping_center_outcome_cron'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function violates Single Responsibility Principle
The function civicrm_api3_goonjcustom_dropping_center_outcome_cron handles multiple responsibilities, including data retrieval, processing, and updating. This makes the code harder to maintain and test.
Consider splitting the function into smaller, focused functions:
- Data Retrieval: A function to fetch all necessary data.
- Data Processing: A function to calculate metrics.
- Data Updating: A function to perform the updates.
This refactoring will enhance code readability and maintainability.
| $productSaleAmountByTrackingId[$trackingId] = 0; | ||
| } | ||
|
|
||
| // Calculate cash contibution. |
There was a problem hiding this comment.
Typo in comment: Correct 'contibution' to 'contribution'
There's a spelling error in the comment on line 41.
Apply this diff to fix the typo:
-// Calculate cash contibution.
+// Calculate cash contribution.📝 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.
| // Calculate cash contibution. | |
| // Calculate cash contribution. |
| 'where' => [ | ||
| ['Dropping_Centre.Donation_Tracking_Id', 'IS NOT NULL'], | ||
| ], | ||
| 'checkPermissions' => FALSE, |
There was a problem hiding this comment.
💡 Codebase verification
Review 'checkPermissions' usage for consistency and security
The 'checkPermissions' parameter is used inconsistently throughout the codebase, which could lead to security vulnerabilities and maintenance difficulties. Here are the key issues:
- Inconsistent defaults: Some functions default to TRUE, others to FALSE.
- Security risks: Many instances set 'checkPermissions' to FALSE, potentially bypassing important security checks.
- Code duplication: Similar API calls with 'checkPermissions' are repeated across files.
- Lack of clear pattern: There's no obvious reasoning behind TRUE vs FALSE choices.
- Potential for bugs: Inconsistent usage may cause unexpected behavior.
Recommendations:
- Review all instances where 'checkPermissions' is FALSE for security implications.
- Establish a consistent policy for 'checkPermissions' default values.
- Refactor to reduce duplication and improve consistency.
- Add documentation explaining when and why 'checkPermissions' should be FALSE.
- Consider creating a centralized method to handle permission checks, reducing the need for inline 'checkPermissions' parameters.
These changes will improve code maintainability, reduce the risk of security vulnerabilities, and make the codebase more robust overall.
🔗 Analysis chain
Verify consistency of 'checkPermissions' parameter in API calls
The checkPermissions parameter is inconsistently set across API calls, which could lead to permission issues.
Run the following script to identify all instances of checkPermissions in the codebase:
This will help verify whether checkPermissions is consistently set as intended.
Also applies to: 54-54, 62-62, 75-75, 93-95, 102-105, 111-114, 120-123, 129-132
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all occurrences of 'checkPermissions' in PHP files.
# Search for 'checkPermissions' and display surrounding lines for context.
rg --type php 'checkPermissions' -A 2 -B 2
Length of output: 372225
|
|
||
| $collectionCamps = civicrm_api4('Eck_Collection_Camp', 'get', [ | ||
| 'select' => [ | ||
| 'Donation_Box_Register_Tracking.Cash_Contribution', | ||
| 'Dropping_Centre.Donation_Tracking_Id', | ||
| 'Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_', | ||
| ], | ||
| 'where' => [ | ||
| ['Dropping_Centre.Donation_Tracking_Id', 'IS NOT NULL'], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
||
| $cashContributionByTrackingId = []; | ||
| $productSaleAmountByTrackingId = []; | ||
| $footfallByTrackingId = []; | ||
| $vehicleByTrackingId = []; | ||
| $testing = []; | ||
|
|
||
| $test = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [ | ||
| 'select' => [ | ||
| 'Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office', | ||
| ], | ||
| 'where' => [ | ||
| ['Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
||
|
|
||
| foreach ($collectionCamps as $camp) { | ||
| $trackingId = $camp['Dropping_Centre.Donation_Tracking_Id']; | ||
|
|
||
| if (!isset($cashContributionByTrackingId[$trackingId])) { | ||
| $cashContributionByTrackingId[$trackingId] = 0; | ||
| } | ||
| if (!isset($productSaleAmountByTrackingId[$trackingId])) { | ||
| $productSaleAmountByTrackingId[$trackingId] = 0; | ||
| } | ||
|
|
||
| if (isset($camp['Donation_Box_Register_Tracking.Cash_Contribution']) && $camp['Donation_Box_Register_Tracking.Cash_Contribution'] !== null) { | ||
| $cashContributionByTrackingId[$trackingId] += (int) $camp['Donation_Box_Register_Tracking.Cash_Contribution']; | ||
| } | ||
|
|
||
| if (isset($camp['Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_']) && $camp['Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_'] !== null) { | ||
| $productSaleAmountByTrackingId[$trackingId] += (float) $camp['Donation_Box_Register_Tracking.Product_Sale_Amount_GBG_']; | ||
| error_log("productSaleAmountByTrackingId : $productSaleAmountByTrackingId, Result: " . print_r($productSaleAmountByTrackingId, TRUE)); | ||
| } | ||
|
|
||
| $activities = civicrm_api4('Activity', 'get', [ | ||
| 'select' => ['subject'], | ||
| 'where' => [ | ||
| ['activity_type_id:name', '=', 'Material Contribution'], | ||
| ['Material_Contribution.Dropping_Center', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| $footfallByTrackingId[$trackingId] = count($activities); | ||
|
|
||
| $collectionSourceVehicleDispatches = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [ | ||
| 'select' => [ | ||
| 'Camp_Vehicle_Dispatch.Collection_Camp', | ||
| ], | ||
| 'where' => [ | ||
| ['Camp_Vehicle_Dispatch.Collection_Camp', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
||
| $vehicleByTrackingId[$trackingId] = count($collectionSourceVehicleDispatches); | ||
|
|
||
|
|
||
| } | ||
|
|
||
| $returnValues['cashContributionByTrackingId'] = $cashContributionByTrackingId; | ||
| $returnValues['productSaleAmountByTrackingId'] = $productSaleAmountByTrackingId; | ||
| $returnValues['footfallByTrackingId'] = $footfallByTrackingId; | ||
|
|
||
| try { | ||
| CollectionCampService::sendLogisticsEmail($camp); | ||
| CollectionCampService::updateContributorCount($camp); | ||
| // Update Cash Contributions | ||
| foreach ($cashContributionByTrackingId as $trackingId => $cashContributionSum) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Cash_Contribution' => max($cashContributionSum, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
||
| } | ||
|
|
||
| // Update Product Sale Amount | ||
| foreach ($productSaleAmountByTrackingId as $trackingId => $productSaleSum) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Product_Sale_Amount_GBG_' => max($productSaleSum, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
||
| } | ||
|
|
||
| } | ||
| catch (\Exception $e) { | ||
| \Civi::log()->info('Error processing camp', [ | ||
| 'id' => $camp['id'], | ||
| 'error' => $e->getMessage(), | ||
| // Update Footfall Count based on Material Contribution activities | ||
| foreach ($footfallByTrackingId as $trackingId => $footfallCount) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Footfall_at_the_center' => max($footfallCount, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| // Update Vehicle Count based on Material dispatch | ||
| foreach ($vehicleByTrackingId as $trackingId => $vehicleCount) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Total_no_of_vehicle_material_collected' => max($vehicleCount, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
||
| // Update Vehicle Count based on Material dispatch | ||
| foreach ($testing as $trackingId => $count) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Total_no_of_bags_received_from_center' => max($count, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor: Improve single responsibility and readability
The function civicrm_api3_goonjcustom_collection_camp_cron is handling multiple responsibilities, including data retrieval, processing, and updating. This can make the code harder to read and maintain.
Consider refactoring the code by:
- Splitting the logic into smaller functions: Break down the large function into smaller, single-purpose functions. For example, create separate functions for data retrieval, processing calculations, and updating records.
- Using object-oriented approaches: If possible, encapsulate related functionalities within classes or services to promote reusability and testability.
- Adding comments and documentation: Provide clear comments and documentation for each function to enhance readability.
Refactoring will enhance code maintainability and align with best practices.
| // Update Vehicle Count based on Material dispatch | ||
| foreach ($testing as $trackingId => $count) { | ||
| $results = civicrm_api4('Eck_Collection_Camp', 'update', [ | ||
| 'values' => [ | ||
| 'Dropping_Center_Outcome.Total_no_of_bags_received_from_center' => max($count, 0), | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Fix: Uninitialized variable $testing used in loop
The variable $testing is iterated over in the foreach loop starting at line 171 but is not populated anywhere in the code. This could lead to an empty loop or unintended behavior.
To resolve this:
- Initialize and populate
$testing: Ensure that$testingis assigned meaningful data before the loop if it's needed. - Remove the loop if unnecessary: If
$testingis not required, remove the loop to simplify the code.
Apply this diff to fix the issue:
-// Remove the unused loop
-// Update Vehicle Count based on Material dispatch
-foreach ($testing as $trackingId => $count) {
- $results = civicrm_api4('Eck_Collection_Camp', 'update', [
- 'values' => [
- 'Dropping_Center_Outcome.Total_no_of_bags_received_from_center' => max($count, 0),
- ],
- 'where' => [
- ['id', '=', $trackingId],
- ],
- 'checkPermissions' => TRUE,
- ]);
-}Committable suggestion was skipped due to low confidence.
| $test = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [ | ||
| 'select' => [ | ||
| 'Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office', | ||
| ], | ||
| 'where' => [ | ||
| ['Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office', '=', $trackingId], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Fix: Undefined variable $trackingId and unused variable $test
The variable $trackingId is used in the 'where' clause at line 62 but is not defined prior to this usage, leading to an undefined variable error. Additionally, the variable $test is declared but not used elsewhere in the code.
To resolve this:
- Define
$trackingIdbefore using it: Ensure that$trackingIdis assigned a value before line 62. Since$trackingIdis obtained from$camp['Dropping_Centre.Donation_Tracking_Id']inside theforeachloop starting at line 69, consider moving this API call inside that loop. - Remove or utilize
$test: If$testis not necessary, remove it to clean up the code. If it serves a purpose, ensure that it is used appropriately in the subsequent code.
Apply this diff to fix the issue:
-// Before the loop
-$test = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [
- 'select' => [
- 'Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office',
- ],
- 'where' => [
- ['Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office', '=', $trackingId],
- ],
- 'checkPermissions' => TRUE,
-]);
+// Inside the foreach loop after $trackingId is defined
+foreach ($collectionCamps as $camp) {
+ $trackingId = $camp['Dropping_Centre.Donation_Tracking_Id'];
+ // Your existing code...
+
+ // If needed, use $test here
+ $test = civicrm_api4('Eck_Collection_Source_Vehicle_Dispatch', 'get', [
+ 'select' => [
+ 'Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office',
+ ],
+ 'where' => [
+ ['Acknowledgement_For_Logistics.No_of_bags_received_at_PU_Office', '=', $trackingId],
+ ],
+ 'checkPermissions' => TRUE,
+ ]);
+
+ // Use $test as required...
+}Committable suggestion was skipped due to low confidence.
|
Closing! because of large number of merge conflicts |
1 participant
Description
A daily cron job executes to update the outcome Fields.
Updating Record :
Summary by CodeRabbit
New Features
Bug Fixes