Refactor: Store entity ID in Eck_Collection_Camp1 in directive#307
Refactor: Store entity ID in Eck_Collection_Camp1 in directive#307
Eck_Collection_Camp1 in directive#307Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
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 (
|
belwalshubham
changed the title
Eck_Collection_Camp1 in directive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (5)
Line range hint
391-477: Improve error handling and consider refactoring generateCollectionCampCodeThe changes to generateCollectionCampCode enhance the code generation process, but there are opportunities for further improvement:
- Consider extracting the code generation logic into a separate private method for better organization and testability.
- Implement more robust error handling, especially for database operations.
- The nested if statements could be simplified to reduce complexity.
Example refactoring:
private function generateCode($year, $stateCode, $eventCode, $campId) { return $year . '/' . $stateCode . '/' . $eventCode . '/' . $campId; } public static function generateCollectionCampCode(string $op, string $objectName, $objectId, &$objectRef) { if ($objectName != 'Eck_Collection_Camp' || !$objectId) { return; } $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; if ($newStatus !== 'authorized') { return; } // Fetch necessary data (year, state, event code, etc.) // ... $newTitle = self::generateCode($year, $stateCode, $eventCode, $campId); try { EckEntity::update('Collection_Camp') ->addWhere('id', '=', $campId) ->addValue('title', $newTitle) ->execute(); } catch (\Exception $e) { \Civi::log()->error('Failed to update Collection Camp title: ' . $e->getMessage()); } }This refactoring improves code organization, error handling, and testability.
Line range hint
704-793: Enhance error handling and refactor generateQrCode methodThe generateQrCode method is well-implemented, but consider the following improvements:
- Implement more granular error handling for each step of the process.
- Consider extracting some of the logic into smaller, reusable methods.
- Use constants for magic strings and numbers.
Example refactoring:
private const QR_CODE_VERSION = 5; private const QR_CODE_ECC_LEVEL = QRCode::ECC_L; private const QR_CODE_SCALE = 10; private static function generateQrCodeOptions(): QROptions { return new QROptions([ 'version' => self::QR_CODE_VERSION, 'outputType' => QRCode::OUTPUT_IMAGE_PNG, 'eccLevel' => self::QR_CODE_ECC_LEVEL, 'scale' => self::QR_CODE_SCALE, ]); } private static function saveQrCodeAttachment($collectionCampId, $qrcode, $fileName, $qrFieldId) { $params = [ 'entity_id' => $collectionCampId, 'name' => $fileName, 'mime_type' => 'image/png', 'field_name' => $qrFieldId, 'options' => [ 'move-file' => $qrcode, ], ]; try { $result = civicrm_api3('Attachment', 'create', $params); if (empty($result['id'])) { throw new \Exception('Failed to create attachment'); } return $result['values'][$result['id']]['url']; } catch (\CiviCRM_API3_Exception $e) { \Civi::log()->error('Error saving QR code attachment: ' . $e->getMessage()); throw $e; } } public static function generateQrCode($collectionCampId) { try { $baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL; $url = "{$baseUrl}actions/collection-camp/{$collectionCampId}"; $options = self::generateQrCodeOptions(); $qrcode = (new QRCode($options))->render($url); $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); if (file_put_contents($tempFilePath, $qrcode) === false) { throw new \Exception('Failed to write QR code to temporary file'); } $qrField = CustomField::get(FALSE) ->addSelect('id') ->addWhere('custom_group_id:name', '=', 'Collection_Camp_QR_Code') ->addWhere('name', '=', 'QR_Code') ->setLimit(1) ->execute() ->first(); if (!$qrField) { throw new \Exception('No field to save QR Code'); } $qrFieldId = 'custom_' . $qrField['id']; $attachmentUrl = self::saveQrCodeAttachment($collectionCampId, $tempFilePath, $fileName, $qrFieldId); return true; } catch (\Exception $e) { \Civi::log()->error('Error generating QR code: ' . $e->getMessage()); return false; } }This refactoring improves error handling, code organization, and maintainability.
Line range hint
1075-1145: Enhance mailNotificationToMmt method for better error handling and securityThe mailNotificationToMmt method is functional, but consider the following improvements:
- Implement more robust error handling, especially for database operations and email sending.
- Consider extracting some of the logic into smaller, reusable methods for better organization.
- Use parameterized queries or proper escaping for any user input used in database queries to prevent SQL injection.
- Validate email addresses before sending to prevent potential abuse.
Example improvements:
private static function getMMTEmail($goonjFieldId) { try { $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 (empty($coordinators['contact_id_a'])) { throw new \Exception('No MMT coordinator found'); } $email = Email::get(FALSE) ->addSelect('email') ->addWhere('contact_id', '=', $coordinators['contact_id_a']) ->execute()->single(); if (empty($email['email'])) { throw new \Exception('No email found for MMT coordinator'); } return $email['email']; } catch (\Exception $e) { \Civi::log()->error('Error retrieving MMT email: ' . $e->getMessage()); return null; } } public static function mailNotificationToMmt($op, $groupID, $entityID, &$params) { if ($op !== 'create') { return; } try { $goonjField = self::findOfficeId($params); if (!$goonjField) { throw new \Exception('Office ID not found'); } // Retrieve necessary information (camp details, etc.) // ... $mmtEmail = self::getMMTEmail($goonjField['value']); if (!$mmtEmail || !filter_var($mmtEmail, FILTER_VALIDATE_EMAIL)) { throw new \Exception('Invalid MMT email'); } $fromEmail = OptionValue::get(FALSE) ->addSelect('label') ->addWhere('option_group_id:name', '=', 'from_email_address') ->addWhere('is_default', '=', TRUE) ->execute()->single(); $mailParams = [ 'subject' => 'Material Acknowledgement for Camp: ' . $campCode . ' at ' . $campAddress, 'from' => $fromEmail['label'], 'toEmail' => $mmtEmail, 'replyTo' => $fromEmail['label'], 'html' => self::goonjcustom_material_management_email_html($collectionCampId, $campCode, $campAddress, $vehicleDispatchId), ]; if (!\CRM_Utils_Mail::send($mailParams)) { throw new \Exception('Failed to send email'); } EckEntity::update('Collection_Source_Vehicle_Dispatch', FALSE) ->addValue('Acknowledgement_For_Logistics.Filled_by', $coordinators['contact_id_a']) ->addWhere('Camp_Vehicle_Dispatch.Collection_Camp_Intent_Id', '=', $collectionCampId) ->execute(); } catch (\Exception $e) { \Civi::log()->error('Error in mailNotificationToMmt: ' . $e->getMessage()); } }These changes improve error handling, code organization, and add some basic security checks.
Line range hint
1214-1265: Optimize createActivityForCollectionCamp for better performance and error handlingThe createActivityForCollectionCamp method is functional, but consider the following improvements:
- Implement more robust error handling, especially for database operations.
- Consider using batch operations for creating multiple activities to improve performance.
- Extract some of the logic into smaller, reusable methods for better organization.
Example improvements:
private static function getCollectionCampSubtype() { try { return OptionValue::get(TRUE) ->addSelect('value') ->addWhere('option_group_id:name', '=', 'eck_sub_types') ->addWhere('grouping', '=', 'Collection_Camp_Activity') ->addWhere('name', '=', 'Collection_Camp') ->execute()->single()['value']; } catch (\Exception $e) { \Civi::log()->error('Error retrieving Collection Camp subtype: ' . $e->getMessage()); return null; } } private static function createActivities($campId, $activities, $startDate, $endDate, $initiator) { $subtype = self::getCollectionCampSubtype(); if (!$subtype) { throw new \Exception('Failed to retrieve Collection Camp subtype'); } $batchActivities = []; foreach ($activities as $activityName) { if ($activityName == 'Others') { $activityName = $objectRef['Collection_Camp_Intent_Details.Other_activity'] ?? ''; if (!$activityName) { continue; } } $batchActivities[] = [ 'title' => $activityName, 'subtype' => $subtype, 'Collection_Camp_Activity.Collection_Camp_Id' => $campId, 'Collection_Camp_Activity.Start_Date' => $startDate, 'Collection_Camp_Activity.End_Date' => $endDate, 'Collection_Camp_Activity.Organizing_Person' => $initiator, ]; } if (!empty($batchActivities)) { EckEntity::save('Collection_Camp_Activity', FALSE) ->setRecords($batchActivities) ->execute(); } } public static function createActivityForCollectionCamp(string $op, string $objectName, $objectId, &$objectRef) { if ($objectName != 'Eck_Collection_Camp' || !$objectId) { return; } $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; if (!$newStatus || $newStatus !== 'authorized') { return; } try { $collectionCamp = EckEntity::get('Collection_Camp', FALSE) ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id', 'title') ->addWhere('id', '=', $objectId) ->execute()->single(); if ($collectionCamp['Collection_Camp_Core_Details.Status'] === 'authorized') { return; } $activities = $objectRef['Collection_Camp_Intent_Details.Here_are_some_activities_to_pick_from_but_feel_free_to_invent_yo']; $startDate = $objectRef['Collection_Camp_Intent_Details.Start_Date']; $endDate = $objectRef['Collection_Camp_Intent_Details.End_Date']; $initiator = $objectRef['Collection_Camp_Core_Details.Contact_Id']; self::createActivities($objectId, $activities, $startDate, $endDate, $initiator); } catch (\Exception $e) { \Civi::log()->error('Error in createActivityForCollectionCamp: ' . $e->getMessage()); } }These changes improve error handling, code organization, and potentially performance by using batch operations for creating activities.
Line range hint
1-1265: Overall improvements with opportunities for further refinementThe changes to the CollectionCampService class significantly enhance its functionality and maintainability. Key improvements include:
- Refactoring of the collectionCampTabset method for better organization and flexibility.
- Addition of QR code generation functionality.
- Enhanced logic for generating collection camp codes.
- Improved email notifications for material management.
- More robust activity creation for collection camps.
While these changes are positive, there are opportunities for further improvement:
- Implement more comprehensive error handling throughout the class.
- Consider extracting some complex logic into smaller, more focused methods.
- Optimize database operations, particularly when dealing with multiple entities or activities.
- Enhance security measures, especially when handling user input or generating URLs.
- Improve code documentation, particularly for complex methods.
These suggestions aim to further enhance the robustness, maintainability, and performance of the CollectionCampService class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2 hunks)
- wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCampService.tpl (1 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCampService.tpl (1)
1-5: Implementation aligns with PR objective, but consider potential improvements.The changes successfully implement the PR objective of storing the entity ID in Eck_Collection_Camp1 within the directive. The code introduces an AngularJS component that passes the entity ID to a custom directive, which aligns with the refactoring goal.
However, there are a few points to consider:
The use of
{ldelim}and{rdelim}suggests this is a Smarty template. It might be beneficial to use Smarty's built-in syntax for JSON-like structures to improve readability.The
optionsattribute is using a hardcoded keyEck_Collection_Camp1. Consider making this dynamic if there's a possibility of reusing this template for different entity types.The form with id "bootstrap-theme" is empty except for the directive. Ensure this is intentional and doesn't conflict with any existing Bootstrap themes.
Consider the following improvements:
- Use Smarty's JSON syntax:
<{$tabValue.directive} options='{literal}{"Eck_Collection_Camp1": {/literal}{$entity.id}}'></{$tabValue.directive}>
- If applicable, make the entity key dynamic:
<{$tabValue.directive} options='{literal}{{/literal}{$entityKey}: {$entity.id}{literal}}{/literal}'></{$tabValue.directive}>To ensure the changes are consistent across the codebase, run the following script:
This will help identify any inconsistencies in how the entity ID is being used across different templates.
✅ Verification successful
Verified: Implementation aligns with existing codebase patterns. No issues found.
- The usage of
Eck_Collection_Camp1andentity.idin the reviewed file matches other template files:
wp-content/civi-extensions/de.systopia.eck/templates/CRM/Eck/Page/Entity/Edit.tplwp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCamp.tplwp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCampService.tpl🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of Eck_Collection_Camp1 and entity.id in template files # Test: Search for other occurrences of Eck_Collection_Camp1 in template files echo "Occurrences of Eck_Collection_Camp1 in template files:" rg --type html "Eck_Collection_Camp1" wp-content/civi-extensions # Test: Search for other uses of entity.id in template files echo "Occurrences of entity.id in template files:" rg --type html "entity\.id" wp-content/civi-extensionsLength of output: 337
Script:
#!/bin/bash # Description: Verify the usage of Eck_Collection_Camp1 and entity.id in .tpl template files # Test: Search for occurrences of Eck_Collection_Camp1 in .tpl files echo "Occurrences of Eck_Collection_Camp1 in .tpl files:" rg "Eck_Collection_Camp1" --type tpl wp-content/civi-extensions # Test: Search for occurrences of entity.id in .tpl files echo "Occurrences of entity.id in .tpl files:" rg "entity\.id" --type tpl wp-content/civi-extensionsLength of output: 375
Script:
#!/bin/bash # Description: Verify the usage of Eck_Collection_Camp1 and entity.id in .tpl template files # Test: Search for occurrences of Eck_Collection_Camp1 in .tpl files echo "Occurrences of Eck_Collection_Camp1 in .tpl files:" rg "Eck_Collection_Camp1" --glob "*.tpl" wp-content/civi-extensions # Test: Search for occurrences of entity.id in .tpl files echo "Occurrences of entity.id in .tpl files:" rg "entity\.id" --glob "*.tpl" wp-content/civi-extensionsLength of output: 1134
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
20-21: LGTM: New QR code functionality addedThe addition of QRCode and QROptions imports indicates new QR code generation functionality. This is a good enhancement for the CollectionCampService class.
76-114: Excellent refactoring of collectionCampTabset methodThe changes to the collectionCampTabset method significantly improve code maintainability and readability:
- The new $tabConfigs array centralizes tab configurations, making it easier to add or modify tabs.
- The use of a loop to populate the $tabs array reduces code duplication.
- The structure is more flexible and easier to extend in the future.
These changes align well with the DRY (Don't Repeat Yourself) principle and improve overall code quality.
2 participants
CollectionCampService.phpEck_Collection_Camp1in directiveSummary by CodeRabbit
New Features
Bug Fixes