Material contibution #354
Material contibution #354
Conversation
WalkthroughThe pull request introduces significant enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (5)
wp-content/plugins/goonj-blocks/goonj-blocks.php (2)
91-93: Approved: New fields added to$entity_fieldsarrayThe addition of these new fields for the Dropping Centre entity enhances the data retrieval capabilities of the plugin. This change aligns well with the function's purpose and doesn't introduce any violations of software development best practices.
However, to improve code readability and maintainability, consider grouping related fields together. You could reorganize the
$entity_fieldsarray to keep all Dropping Centre fields adjacent to each other.Here's a suggestion for improved organization:
$entity_fields = array( 'id', 'title', 'Collection_Camp_Intent_Details.Start_Date', 'Collection_Camp_Intent_Details.End_Date', 'Collection_Camp_Intent_Details.Location_Area_of_camp', 'Collection_Camp_Intent_Details.City', 'Collection_Camp_Intent_Details.State', 'Dropping_Centre.Start_Time', 'Dropping_Centre.End_Time', 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_', );This organization makes it easier to identify and manage fields related to specific entities.
Line range hint
46-155: Suggestion: Refactorgb_goonj_blocks_check_action_target_existsfor improved organizationWhile the current implementation works, there are opportunities to improve code organization and reduce duplication:
The function
gb_goonj_blocks_check_action_target_existsis handling multiple responsibilities. Consider breaking it down into smaller, more focused functions.There's code duplication in the switch statement for 'collection-camp' and 'dropping-center' cases. These could be combined into a single case with a shared logic.
The error handling (404 response) is repeated. This could be extracted into a separate function for reusability.
Here's a high-level suggestion for refactoring:
function gb_goonj_blocks_check_action_target_exists() { if (!should_process_action()) { return; } $target = get_query_var('target'); $id = intval(get_query_var('id')); load_civicrm(); $action_target = get_action_target($target, $id); if ($action_target === false) { handle_404(); } else { global $wp_query; $wp_query->set('action_target', $action_target); } } function should_process_action() { return is_page('actions') && get_query_var('target') && get_query_var('id'); } function load_civicrm() { if (function_exists('civicrm_initialize')) { civicrm_initialize(); } } function get_action_target($target, $id) { switch ($target) { case 'collection-camp': case 'dropping-center': return get_collection_camp_or_dropping_center($id); case 'processing-center': return get_processing_center($id); default: return false; } } function handle_404() { global $wp_query; $wp_query->set_404(); status_header(404); nocache_headers(); include get_query_template('404'); exit; } // Implement get_collection_camp_or_dropping_center and get_processing_center functionsThis refactoring improves code organization, reduces duplication, and makes the code more maintainable and testable.
wp-content/civi-extensions/goonjcustom/Civi/QrCodeService.php (2)
58-61: Enhance Error Handling for File Write FailuresCurrently, if
file_put_contentsfails, it logs the error but continues execution. This could lead to further errors down the line.Consider throwing an exception or returning early to handle the failure appropriately.
Adjust the code as follows:
if (!$numBytes) { \CRM_Core_Error::debug_log_message('Failed to write QR code to temporary file for collection camp ID ' . $collectionCampId); + throw new \Exception('Failed to write QR code to temporary file.'); }This ensures that the failure is caught in the
catchblock, and the method exits gracefully.
101-104: Catch More Specific ExceptionsCatching the base
\Exceptionclass may mask more specific exceptions that could be handled differently.Consider catching specific exceptions or using
\Throwableto catch all errors:- catch (\Exception $e) { + catch (\Throwable $e) { \CRM_Core_Error::debug_log_message('Error generating QR code: ' . $e->getMessage()); return FALSE; }Additionally, include the stack trace for more detailed logging:
\CRM_Core_Error::debug_log_message('Error generating QR code: ' . $e->getMessage() . PHP_EOL . $e->getTraceAsString());wp-content/plugins/goonj-blocks/build/render.php (1)
99-99: Verify consistency in action button styling and labelingThe addition of the "Record your Material Contribution" button is appropriate. Ensure that its styling and labeling are consistent with other action buttons to maintain a cohesive user interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (4 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/QrCodeService.php (1 hunks)
- wp-content/plugins/goonj-blocks/build/render.php (2 hunks)
- wp-content/plugins/goonj-blocks/goonj-blocks.php (1 hunks)
- wp-content/plugins/goonj-blocks/src/render.php (2 hunks)
🧰 Additional context used
📓 Learnings (4)
📓 Common learnings
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-08T09:24:47.239Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-09T05:09:48.296Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-08T09:24:47.239Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-09T05:09:48.296Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.wp-content/civi-extensions/goonjcustom/Civi/QrCodeService.php (2)
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-08T09:24:47.239Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520 Timestamp: 2024-10-09T05:09:48.296Z Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.wp-content/plugins/goonj-blocks/src/render.php (1)
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/plugins/goonj-blocks/src/render.php:45-47 Timestamp: 2024-10-08T09:25:18.536Z Learning: In the `wp-content/plugins/goonj-blocks/src/render.php` file, the team considers error handling for `DateTime` object creation unnecessary when using `$action_target['Dropping_Centre.Start_Time']` and `$action_target['Dropping_Centre.End_Time']`.
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/QrCodeService.php (1)
48-50:⚠️ Potential issueIncorrect Handling of QR Code Data
When using
QRCode::OUTPUT_IMAGE_PNG, therendermethod returns binary image data, not a base64-encoded string with a data URI prefix. The current code performsstr_replaceandbase64_decode, which can corrupt the image data.Verify and adjust the handling of
$qrcode:Run the following script to confirm the output of
QRCode::render:Suggested Fix:
Remove the unnecessary data manipulation:
- // Remove the base64 header and decode the image data. - $qrcode = str_replace('data:image/png;base64,', '', $qrcode); - $qrcode = base64_decode($qrcode);The
$qrcodevariable should already contain the correct binary data suitable for writing directly to a file.wp-content/plugins/goonj-blocks/build/render.php (1)
48-61: Great use of configuration array for target typesConsolidating the configuration data into the
$target_configarray enhances readability and makes it easier to manage settings for different target types.
| /** | ||
| * | ||
| */ | ||
| class QrCodeService extends AutoSubscriber { |
There was a problem hiding this comment.
Unnecessary Extension of AutoSubscriber
The QrCodeService class extends AutoSubscriber, but the getSubscribedEvents() method returns an empty array. Since the class isn't subscribing to any events, consider removing the extension to adhere to the Single Responsibility Principle and eliminate unnecessary inheritance.
| public static function generateQrCode($collectionCampId, $collectionCampSubtype) { | ||
| try { | ||
| $baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL; | ||
|
|
||
| if ($collectionCampSubtype === 'Dropping_Center') { | ||
| $url = sprintf(self::DROPPING_CENTER_URL_PATTERN, $baseUrl, $collectionCampId); | ||
| } | ||
| else { | ||
| $url = sprintf(self::COLLECTION_CAMP_URL_PATTERN, $baseUrl, $collectionCampId); | ||
| } | ||
|
|
||
| $options = new QROptions([ | ||
| 'version' => 5, | ||
| 'outputType' => QRCode::OUTPUT_IMAGE_PNG, | ||
| 'eccLevel' => QRCode::ECC_L, | ||
| 'scale' => 10, | ||
| ]); | ||
|
|
||
| $qrcode = (new QRCode($options))->render($url); | ||
|
|
||
| // Remove the base64 header and decode the image data. | ||
| $qrcode = str_replace('data:image/png;base64,', '', $qrcode); | ||
| $qrcode = base64_decode($qrcode); | ||
|
|
||
| $baseFileName = "qr_code_{$collectionCampId}.png"; | ||
| $fileName = \CRM_Utils_File::makeFileName($baseFileName); | ||
| $tempFilePath = \CRM_Utils_File::tempnam($baseFileName); | ||
|
|
||
| $numBytes = file_put_contents($tempFilePath, $qrcode); | ||
|
|
||
| if (!$numBytes) { | ||
| \CRM_Core_Error::debug_log_message('Failed to write QR code to temporary file for collection camp ID ' . $collectionCampId); | ||
| return FALSE; | ||
| } | ||
|
|
||
| $customFields = CustomField::get(FALSE) | ||
| ->addSelect('id') | ||
| ->addWhere('custom_group_id:name', '=', 'Collection_Camp_QR_Code') | ||
| ->addWhere('name', '=', 'QR_Code') | ||
| ->setLimit(1) | ||
| ->execute(); | ||
|
|
||
| $qrField = $customFields->first(); | ||
|
|
||
| if (!$qrField) { | ||
| \CRM_Core_Error::debug_log_message('No field to save QR Code for collection camp ID ' . $collectionCampId); | ||
| return FALSE; | ||
| } | ||
|
|
||
| $qrFieldId = 'custom_' . $qrField['id']; | ||
|
|
||
| // Save the QR code as an attachment linked to the collection camp. | ||
| $params = [ | ||
| 'entity_id' => $collectionCampId, | ||
| 'name' => $fileName, | ||
| 'mime_type' => 'image/png', | ||
| 'field_name' => $qrFieldId, | ||
| 'options' => [ | ||
| 'move-file' => $tempFilePath, | ||
| ], | ||
| ]; | ||
|
|
||
| $result = civicrm_api3('Attachment', 'create', $params); | ||
|
|
||
| if (!empty($result['is_error'])) { | ||
| \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId); | ||
| return FALSE; | ||
| } | ||
|
|
||
| $attachment = $result['values'][$result['id']]; | ||
| $attachmentUrl = $attachment['url']; | ||
|
|
||
| } | ||
| catch (\Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error generating QR code: ' . $e->getMessage()); | ||
| return FALSE; | ||
| } | ||
|
|
||
| return TRUE; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Method generateQrCode Violates Single Responsibility Principle
The generateQrCode method is handling multiple responsibilities:
- Generating the QR code.
- Handling file operations (writing to a temp file).
- Interacting with the database to retrieve custom fields.
- Creating an attachment via the CiviCRM API.
Consider refactoring this method into smaller, dedicated methods for each responsibility. This will enhance readability, maintainability, and testability of the code.
Refactoring suggestions:
-
Extract QR Code Generation:
private static function createQrCode($url) { $options = new QROptions([ 'version' => 5, 'outputType' => QRCode::OUTPUT_IMAGE_PNG, 'eccLevel' => QRCode::ECC_L, 'scale' => 10, ]); return (new QRCode($options))->render($url); }
-
Extract File Handling:
private static function saveQrCodeToFile($qrcodeData, $collectionCampId) { $baseFileName = "qr_code_{$collectionCampId}.png"; $fileName = \CRM_Utils_File::makeFileName($baseFileName); $tempFilePath = \CRM_Utils_File::tempnam($baseFileName); $numBytes = file_put_contents($tempFilePath, $qrcodeData); if (!$numBytes) { \CRM_Core_Error::debug_log_message('Failed to write QR code to temporary file for collection camp ID ' . $collectionCampId); return FALSE; } return ['tempFilePath' => $tempFilePath, 'fileName' => $fileName]; }
-
Extract Custom Field Retrieval:
private static function getQrCodeCustomFieldId() { $customFields = CustomField::get(FALSE) ->addSelect('id') ->addWhere('custom_group_id:name', '=', 'Collection_Camp_QR_Code') ->addWhere('name', '=', 'QR_Code') ->setLimit(1) ->execute(); $qrField = $customFields->first(); if (!$qrField) { return FALSE; } return 'custom_' . $qrField['id']; }
-
Extract Attachment Creation:
private static function attachQrCodeToCollectionCamp($collectionCampId, $fileName, $tempFilePath, $qrFieldId) { $params = [ 'entity_id' => $collectionCampId, 'name' => $fileName, 'mime_type' => 'image/png', 'field_name' => $qrFieldId, 'options' => [ 'move-file' => $tempFilePath, ], ]; $result = civicrm_api3('Attachment', 'create', $params); if (!empty($result['is_error'])) { \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId . '. Error: ' . $result['error_message']); return FALSE; } return TRUE; }
By breaking down the method, each part becomes more modular and easier to maintain.
| if (!empty($result['is_error'])) { | ||
| \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId); | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
Improve Error Logging for API Failures
When the API call to create the attachment fails, the error message lacks specific details.
Modify the error logging to include the API error message:
if (!empty($result['is_error'])) {
- \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId);
+ \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId . '. Error: ' . $result['error_message']);
return FALSE;
}This provides clearer insight into the cause of the failure, aiding in faster debugging.
📝 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 (!empty($result['is_error'])) { | |
| \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId); | |
| return FALSE; | |
| } | |
| if (!empty($result['is_error'])) { | |
| \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId . '. Error: ' . $result['error_message']); | |
| return FALSE; | |
| } |
| $dropping_center_material_contribution_link = sprintf( | ||
| '/dropping-center-contribution?source=%s&target_id=%s', | ||
| $action_target['title'], | ||
| $action_target['id'], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor link generation to eliminate duplicate code
The link generation for 'dropping-center' and 'collection-camp' shares similar structures. To adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability, consider abstracting this logic into a reusable function.
You can create a function like this:
function generate_contribution_link($type, $action_target) {
if ($type === 'collection-camp') {
return sprintf(
'/collection-camp-contribution?source=%s&target_id=%s&state_province_id=%s&city=%s',
$action_target['title'],
$action_target['id'],
$action_target['Collection_Camp_Intent_Details.State'],
$action_target['Collection_Camp_Intent_Details.City'],
);
} elseif ($type === 'dropping-center') {
return sprintf(
'/dropping-center-contribution?source=%s&target_id=%s',
$action_target['title'],
$action_target['id'],
);
}
return '';
}Then, update the assignments:
-$material_contribution_link = sprintf(
- '/collection-camp-contribution?source=%s&target_id=%s&state_province_id=%s&city=%s',
- $action_target['title'],
- $action_target['id'],
- $action_target['Collection_Camp_Intent_Details.State'],
- $action_target['Collection_Camp_Intent_Details.City'],
-);
-
-$dropping_center_material_contribution_link = sprintf(
- '/dropping-center-contribution?source=%s&target_id=%s',
- $action_target['title'],
- $action_target['id'],
-);
+ $contribution_link = generate_contribution_link($target, $action_target);And adjust the $target_config accordingly.
| if (isset($target_config[$target])) : | ||
| try { | ||
| $config = $target_config[$target]; | ||
| $start_date = new DateTime($action_target[$config['start_time']]); | ||
| $end_date = new DateTime($action_target[$config['end_time']]); | ||
| $address = $action_target[$config['address']]; | ||
| $contribution_link = $config['contribution_link']; | ||
| } | ||
| catch (Exception $e) { | ||
| \Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]); | ||
| echo '<div class="error">An error occurred. Please try again later.</div>'; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Improve exception handling to protect sensitive information
While the try-catch block effectively handles exceptions, logging the error message may expose sensitive information. Consider logging only essential details and providing a user-friendly message.
Apply this change to safeguard sensitive data:
catch (Exception $e) {
- \Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]);
+ \Civi::log()->error('Invalid date format for start or end time', ['target' => $target]);
echo '<div class="error">An error occurred. Please try again later.</div>';
return;
}This modification ensures that detailed exception messages aren't exposed, aligning with best security practices.
📝 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($target_config[$target])) : | |
| try { | |
| $config = $target_config[$target]; | |
| $start_date = new DateTime($action_target[$config['start_time']]); | |
| $end_date = new DateTime($action_target[$config['end_time']]); | |
| $address = $action_target[$config['address']]; | |
| $contribution_link = $config['contribution_link']; | |
| } | |
| catch (Exception $e) { | |
| \Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]); | |
| echo '<div class="error">An error occurred. Please try again later.</div>'; | |
| return; | |
| } | |
| if (isset($target_config[$target])) : | |
| try { | |
| $config = $target_config[$target]; | |
| $start_date = new DateTime($action_target[$config['start_time']]); | |
| $end_date = new DateTime($action_target[$config['end_time']]); | |
| $address = $action_target[$config['address']]; | |
| $contribution_link = $config['contribution_link']; | |
| } | |
| catch (Exception $e) { | |
| \Civi::log()->error('Invalid date format for start or end time', ['target' => $target]); | |
| echo '<div class="error">An error occurred. Please try again later.</div>'; | |
| return; | |
| } |
| try { | ||
| $config = $target_config[$target]; | ||
| $start_date = new DateTime($action_target[$config['start_time']]); | ||
| $end_date = new DateTime($action_target[$config['end_time']]); | ||
| $address = $action_target[$config['address']]; | ||
| $contribution_link = $config['contribution_link']; | ||
| } | ||
| catch (Exception $e) { | ||
| \Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]); | ||
| echo '<div class="error">An error occurred. Please try again later.</div>'; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Remove unnecessary error handling around DateTime creation
Previous team discussions concluded that error handling for DateTime object creation is unnecessary when using $action_target data. The try-catch block may be redundant, as the dates are expected to be valid. Removing it simplifies the code and aligns with team conventions.
Apply this diff to remove the try-catch block:
-if (isset($target_config[$target])) :
- try {
$config = $target_config[$target];
$start_date = new DateTime($action_target[$config['start_time']]);
$end_date = new DateTime($action_target[$config['end_time']]);
$address = $action_target[$config['address']];
$contribution_link = $config['contribution_link'];
- }
- catch (Exception $e) {
- \Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]);
- echo '<div class="error">An error occurred. Please try again later.</div>';
- return;
- }
+if (isset($target_config[$target])) :
+ $config = $target_config[$target];
+ $start_date = new DateTime($action_target[$config['start_time']]);
+ $end_date = new DateTime($action_target[$config['end_time']]);
+ $address = $action_target[$config['address']];
+ $contribution_link = $config['contribution_link'];This change removes the unnecessary error handling, streamlining the code.
Committable suggestion was skipped due to low confidence.
| $target_config = [ | ||
| 'dropping-center' => [ | ||
| 'start_time' => 'Dropping_Centre.Start_Time', | ||
| 'end_time' => 'Dropping_Centre.End_Time', | ||
| 'address' => 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_', | ||
| 'contribution_link' => $dropping_center_material_contribution_link, | ||
| ], | ||
| 'collection-camp' => [ | ||
| 'start_time' => 'Collection_Camp_Intent_Details.Start_Date', | ||
| 'end_time' => 'Collection_Camp_Intent_Details.End_Date', | ||
| 'address' => 'Collection_Camp_Intent_Details.Location_Area_of_camp', | ||
| 'contribution_link' => $material_contribution_link, | ||
| ], | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider including 'processing-center' in $target_config to reduce code duplication
The current implementation handles 'dropping-center' and 'collection-camp' using the $target_config array, but 'processing-center' is handled separately. To adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability, consider adding 'processing-center' to $target_config. This unifies the logic and simplifies future extensions.
Here's a suggested change:
],
'collection-camp' => [
'start_time' => 'Collection_Camp_Intent_Details.Start_Date',
'end_time' => 'Collection_Camp_Intent_Details.End_Date',
'address' => 'Collection_Camp_Intent_Details.Location_Area_of_camp',
'contribution_link' => $material_contribution_link,
],
+ 'processing-center' => [
+ 'address' => $action_target['address'],
+ 'visit_link' => $pu_visit_check_link,
+ 'contribution_link' => $pu_material_contribution_check_link,
+ ],
];Adjust the subsequent code to utilize the $config array for 'processing-center' as well. This might include handling the absence of start and end dates for 'processing-center' or adding default values where necessary.
Committable suggestion was skipped due to low confidence.
| $collectionCampSubtype = $collectionCamp['subtype:name']; | ||
|
|
||
| if (empty($collectionCampSubtype)) { | ||
| \Civi::log()->error('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate code duplication by using a helper method
The code in lines 547-552 duplicates the logic for retrieving and checking $collectionCampSubtype. Refactoring this into a shared private method will enhance code readability and simplify maintenance.
Update the reGenerateCollectionCampQr method:
- $collectionCampSubtype = $collectionCamp['subtype:name'];
-
- if (empty($collectionCampSubtype)) {
- \Civi::log()->error('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
- return;
- }
+ $collectionCampSubtype = self::getCollectionCampSubtype($collectionCampId);
+ if ($collectionCampSubtype === null) {
+ return;
+ }Ensure that the new helper method handles logging appropriately based on the context (e.g., use different log levels if necessary).
Committable suggestion was skipped due to low confidence.
| $collectionCampSubtype = $currentCollectionCamp['subtype:name']; | ||
|
|
||
| if (empty($collectionCampSubtype)) { | ||
| \Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code for handling $collectionCampSubtype
The logic in lines 505-510 for retrieving and validating $collectionCampSubtype is duplicated in the reGenerateCollectionCampQr method (lines 547-552). To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider extracting this code into a reusable private method.
Apply this diff to refactor the duplicated code:
+ private static function getCollectionCampSubtype(int $collectionCampId) {
+ $collectionCamp = EckEntity::get('Collection_Camp', TRUE)
+ ->addSelect('subtype:name')
+ ->addWhere('id', '=', $collectionCampId)
+ ->execute()->single();
+
+ $collectionCampSubtype = $collectionCamp['subtype:name'];
+
+ if (empty($collectionCampSubtype)) {
+ \Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
+ return null;
+ }
+
+ return $collectionCampSubtype;
+ }Then update the generateCollectionCampQr method:
- $collectionCampSubtype = $currentCollectionCamp['subtype:name'];
-
- if (empty($collectionCampSubtype)) {
- \Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
- return;
- }
+ $collectionCampSubtype = self::getCollectionCampSubtype($collectionCampId);
+ if ($collectionCampSubtype === null) {
+ 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.
| $collectionCampSubtype = $currentCollectionCamp['subtype:name']; | |
| if (empty($collectionCampSubtype)) { | |
| \Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId); | |
| return; | |
| } | |
| private static function getCollectionCampSubtype(int $collectionCampId) { | |
| $collectionCamp = EckEntity::get('Collection_Camp', TRUE) | |
| ->addSelect('subtype:name') | |
| ->addWhere('id', '=', $collectionCampId) | |
| ->execute()->single(); | |
| $collectionCampSubtype = $collectionCamp['subtype:name']; | |
| if (empty($collectionCampSubtype)) { | |
| \Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId); | |
| return null; | |
| } | |
| return $collectionCampSubtype; | |
| } | |
| $collectionCampSubtype = self::getCollectionCampSubtype($collectionCampId); | |
| if ($collectionCampSubtype === null) { | |
| return; | |
| } |
Description
Summary by CodeRabbit
New Features
Bug Fixes
Refactor