Collection Camp Authorisation Status#362
Conversation
tarunnjoshi
changed the title
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…mp-post-authorization-status
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampCron.php (1)
Line range hint
31-92: Consider refactoring for better adherence to the Single Responsibility PrincipleThe
civicrm_api3_goonjcustom_collection_camp_cronfunction is quite long and handles multiple responsibilities, including:
- Retrieving option values
- Querying collection camps
- Processing each camp (sending emails and updating contributor count)
- Error handling
Consider breaking this function down into smaller, more focused functions to improve readability and maintainability. For example:
- Create a separate function for retrieving option values.
- Create a function for querying collection camps.
- Move the camp processing logic (inside the foreach loop) to a separate function.
This refactoring will make the code easier to understand, test, and maintain.
Would you like assistance in refactoring this function?
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackCollectionCampCron.php (2)
71-75: Indentation needs adjustmentThe new conditional block for handling aborted camps is logically correct and aligns with the PR objectives. The log message is helpful for debugging and monitoring. However, the indentation of this block is inconsistent with the rest of the code.
Please adjust the indentation to match the surrounding code. Here's the suggested change:
- // Skip if camp status is "aborted". - if ($campStatus === 'aborted') { - \Civi::log()->info("Skipping camp ID $collectionCampId as it is marked as 'aborted'"); - continue; - } + // Skip if camp status is "aborted". + if ($campStatus === 'aborted') { + \Civi::log()->info("Skipping camp ID $collectionCampId as it is marked as 'aborted'"); + continue; + }
Line range hint
52-75: Overall assessment: Changes implement required functionality with minor formatting issueThe modifications successfully implement the handling of aborted camps as per the PR objectives. The code is well-structured, follows existing conventions, and doesn't introduce unnecessary complexity. The only issue is a minor indentation inconsistency, which has been addressed in a previous comment.
To further improve the code, consider the following suggestions:
- Add a comment explaining the significance of the 'aborted' status and why these camps are skipped.
- Consider extracting the camp status check into a separate function for better readability and potential reuse.
These changes enhance the codebase's maintainability and align with software development best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (5 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampCron.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackCollectionCampCron.php (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampCron.php (1)
58-58: LGTM: New field added to track camp statusThe addition of 'Collection_Camp_Intent_Details.Camp_status_field' to the
addSelectmethod aligns with the PR objectives to implement changes to the camp status field. This change will allow for better tracking and management of camp statuses.wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackCollectionCampCron.php (2)
52-52: LGTM: New field added to select statementThe addition of
Collection_Camp_Intent_Details.Camp_status_fieldto the select statement is consistent with the PR objectives and follows the existing naming conventions. This change will allow the code to access the camp status information for further processing.
69-70: LGTM: New variable for camp statusThe assignment of
$campStatuswith the value fromCollection_Camp_Intent_Details.Camp_status_fieldis clear and follows good naming conventions. This variable will be used to check the camp status in the subsequent code.
| public static function updateCampStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) { | ||
| if ($objectName != 'Eck_Collection_Camp') { | ||
| return; | ||
| } | ||
|
|
||
| $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; | ||
|
|
||
| if (!$newStatus || !$objectId) { | ||
| return; | ||
| } | ||
|
|
||
| $collectionCamp = EckEntity::get('Collection_Camp', FALSE) | ||
| ->addSelect('Collection_Camp_Core_Details.Status') | ||
| ->addWhere('id', '=', $objectId) | ||
| ->execute()->single(); | ||
|
|
||
| $currentStatus = $collectionCamp['Collection_Camp_Core_Details.Status']; | ||
|
|
||
| // Check for status change. | ||
| if ($currentStatus !== $newStatus) { | ||
| if ($newStatus === 'authorized') { | ||
| // Access the subtype. | ||
| $subtypeId = $objectRef['subtype'] ?? NULL; | ||
| if ($subtypeId === NULL) { | ||
| return; | ||
| } | ||
|
|
||
| // Access the id within the decoded data. | ||
| $campId = $objectRef['id'] ?? NULL; | ||
| if ($campId === NULL) { | ||
| return; | ||
| } | ||
|
|
||
| // Update camp status to 'planned' when the camp is authorized | ||
| $results = EckEntity::update('Collection_Camp', TRUE) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_status_field', 'planned') | ||
| ->addWhere('id', '=', $campId) | ||
| ->execute(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce Duplicate Code in 'updateCampStatusAfterAuth'
The method updateCampStatusAfterAuth duplicates logic that exists in generateCollectionCampCode, particularly the checks for status change and accessing subtype and camp ID. This violates the DRY (Don't Repeat Yourself) principle.
Suggestion: Extract Common Logic into a Shared Method
Refactor the shared logic into a private method that both updateCampStatusAfterAuth and generateCollectionCampCode can call. This will reduce code duplication and improve maintainability.
There was a problem hiding this comment.
@tarunnjoshi, thank you for considering my suggestion! If you have any questions or need assistance with the refactoring, feel free to ask.
（＾・ω・＾）
…mp-post-authorization-status
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (1)
Line range hint
33-77: Suggestion: Consider refactoring for improved readability and maintainabilityWhile the function follows the single responsibility principle and implements good error handling, it's quite long and could benefit from some refactoring. Here are some suggestions to enhance readability and maintainability:
- Extract the camp fetching logic into a separate method.
- Consider using dependency injection for services like
CollectionCampOutcomeServiceandHelperService.- Use constants for magic strings like 'authorized' and field names.
Here's a potential refactoring example:
private function fetchEligibleCamps(DateTimeImmutable $now): array { $endOfDay = $now->setTime(23, 59, 59)->format('Y-m-d H:i:s'); return EckEntity::get('Collection_Camp', TRUE) ->addSelect( 'Logistics_Coordination.Camp_to_be_attended_by', 'Collection_Camp_Intent_Details.End_Date', 'Camp_Outcome.Last_Reminder_Sent', 'title', 'Collection_Camp_Intent_Details.Location_Area_of_camp', 'Collection_Camp_Intent_Details.Camp_status_field' ) ->addWhere('Camp_Outcome.Rate_the_camp', 'IS NULL') ->addWhere('Logistics_Coordination.Email_Sent', '=', 1) ->addWhere('Collection_Camp_Core_Details.Status', '=', self::STATUS_AUTHORIZED) ->addWhere('Collection_Camp_Intent_Details.End_Date', '<=', $endOfDay) ->execute(); } function civicrm_api3_goonjcustom_collection_camp_outcome_reminder_cron($params) { $now = new DateTimeImmutable(); $from = HelperService::getDefaultFromEmail(); $collectionCamps = $this->fetchEligibleCamps($now); foreach ($collectionCamps as $camp) { try { CollectionCampOutcomeService::processCampReminder($camp, $now, $from); } catch (\Exception $e) { \Civi::log()->error('Error processing camp reminder', [ 'id' => $camp['id'], 'error' => $e->getMessage(), ]); } } return civicrm_api3_create_success([], $params, 'Goonjcustom', 'collection_camp_outcome_reminder_cron'); }This refactoring improves readability and makes the code more modular. What do you think about these suggestions?
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (1)
28-34: Approve changes with a minor suggestion for improvement.The new code successfully implements the requirement to skip processing for aborted camps. This aligns well with the PR objectives. Good job on adding a log message for debugging purposes!
A minor suggestion to improve code readability:
Consider using a constant for the "aborted" status to improve maintainability:
+const CAMP_STATUS_ABORTED = 'aborted'; + $campStatus = $camp['Collection_Camp_Intent_Details.Camp_status_field']; // Skip if camp status is "aborted". -if ($campStatus === 'aborted') { +if ($campStatus === self::CAMP_STATUS_ABORTED) { \Civi::log()->info("Skipping camp ID $collectionCampId as it is marked as 'aborted'"); return FALSE; }wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
1099-1129: Nicely done! The email content is now more dynamic.The changes to
getLogisticsEmailHtmlmethod are well-implemented. The conditional inclusion of the camp outcome form link based on the camp status is a good practice. It improves the relevance of the email content for different scenarios.A small suggestion to further improve readability:
Consider extracting the URL generation logic into separate methods. This would make the main method body cleaner and easier to read. For example:
private static function getCampVehicleDispatchFormUrl($homeUrl, $collectionCampId, $campAttendedById, $collectionCampGoonjOffice) { return $homeUrl . 'camp-vehicle-dispatch-form/#?Camp_Vehicle_Dispatch.Collection_Camp=' . $collectionCampId . '&Camp_Vehicle_Dispatch.Filled_by=' . $campAttendedById . '&Camp_Vehicle_Dispatch.To_which_PU_Center_material_is_being_sent=' . $collectionCampGoonjOffice . '&Eck_Collection_Camp1=' . $collectionCampId; } private static function getCampOutcomeFormUrl($homeUrl, $collectionCampId, $campAttendedById) { return $homeUrl . '/camp-outcome-form/#?Eck_Collection_Camp1=' . $collectionCampId . '&Camp_Outcome.Filled_By=' . $campAttendedById; }Then, you can use these methods in
getLogisticsEmailHtml:$campVehicleDispatchFormUrl = self::getCampVehicleDispatchFormUrl($homeUrl, $collectionCampId, $campAttendedById, $collectionCampGoonjOffice); $campOutcomeFormUrl = ($campStatus !== 'aborted') ? self::getCampOutcomeFormUrl($homeUrl, $collectionCampId, $campAttendedById) : '';This would make the main method more concise and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (5 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (2)
Line range hint
1-77: Summary: Changes align with PR objectives, minor improvements suggestedThe changes made to this file align well with the PR objectives, particularly the addition of the
Camp_status_fieldto the selection criteria. The overall structure of the function is sound, with good error handling practices in place.To further enhance the code quality:
- Consider the refactoring suggestions to improve readability and maintainability.
- Clarify the usage of the newly added
Camp_status_fieldin the reminder processing logic.These improvements will make the code more robust and easier to maintain in the future.
50-50: Approved: New field added to selection criteriaThe addition of
Collection_Camp_Intent_Details.Camp_status_fieldto the selection criteria is consistent with the PR objectives. However, it's not immediately clear how this field is used in the rest of the function.Could you please clarify how the
Camp_status_fieldis utilized in the reminder processing logic? This will help ensure that the change is fully integrated and doesn't introduce any unintended side effects.To verify the usage of this new field, let's run the following script:
✅ Verification successful
Verified: Usage of
Camp_status_fieldacross the codebaseThe
Camp_status_fieldis consistently utilized in multiple functions and files, ensuring that the new field is properly integrated and does not introduce unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of Camp_status_field in the codebase # Search for Camp_status_field usage echo "Searching for Camp_status_field usage:" rg --type php "Camp_status_field" -C 5Length of output: 11792
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (2)
Line range hint
62-62: LGTM: Correct usage of the new$fromparameter.The
$fromparameter is correctly passed to thesendOutcomeReminderEmailmethod, which aligns with the updated method signature. This change ensures that the sender's email address is properly set for the reminder email.
Line range hint
17-17: Verify the usage of the new$fromparameter across the codebase.The method signature has been updated to include a new
$fromparameter. This change might affect how the method is called in other parts of the codebase.Let's check for any potential issues:
✅ Verification successful
Verify the usage of the new
$fromparameter across the codebase.The method signature for
processCampReminderhas been updated to include the$fromparameter. All calls to this method have been reviewed and correctly pass the new parameter. No issues detected with the usage of the$fromparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for calls to processCampReminder method # Expected: All calls should now include three arguments rg --type php 'processCampReminder\s*\(' -A 3Length of output: 1213
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
1174-1214: Good addition, but let's not forget about DRY!The
updateCampStatusAfterAuthmethod is a welcome addition that clearly defines when and how the camp status should be updated to 'planned'. The logic flow is sound, with appropriate checks in place.However, as pointed out in a previous review, there's still some code duplication between this method and
generateCollectionCampCode. Let's not lose sight of the DRY (Don't Repeat Yourself) principle!Remember the suggestion to extract the common logic into a shared method? That would be applicable here as well. Consider creating a private method that both
updateCampStatusAfterAuthandgenerateCollectionCampCodecan call to perform these common checks and data retrievals.For example:
private static function getCollectionCampDetails($objectRef) { $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; $subtypeId = $objectRef['subtype'] ?? NULL; $campId = $objectRef['id'] ?? NULL; if (!$newStatus || !$subtypeId || !$campId) { return NULL; } return [ 'newStatus' => $newStatus, 'subtypeId' => $subtypeId, 'campId' => $campId ]; }Then, you can use this method in both
updateCampStatusAfterAuthandgenerateCollectionCampCodeto reduce duplication and improve maintainability.
| public static function updateCampStatusOnOutcomeFilled($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(); | ||
|
|
||
| $collectionCampId = $collectionSourceVehicleDispatch['Camp_Vehicle_Dispatch.Collection_Camp']; | ||
|
|
||
| // Update camp status to 'completed' when the camp outcome form is submitted | ||
| $results = EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_status_field', 'completed') | ||
| ->addWhere('id', '=', $collectionCampId) | ||
| ->execute(); | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Good addition, but let's double-check that data retrieval!
The updateCampStatusOnOutcomeFilled method is a valuable addition that ensures the camp status is updated to 'completed' when the outcome form is submitted. This helps maintain accurate status tracking throughout the camp lifecycle.
However, there's a potential issue that needs addressing:
The method is using self::findOfficeId($params) to retrieve the $goonjField, which seems to be intended for finding office IDs in a different context (civicrm_eck_collection_source_vehicle_dispatch). This may lead to incorrect data retrieval and logic errors.
To fix this, you should implement a method that correctly retrieves the collectionCampId from the parameters of the camp outcome form submission. This ensures that the camp status is updated accurately when the outcome is filled.
Consider creating a new method like:
private static function findCollectionCampId(array $params) {
// Logic to find the collection camp ID from the outcome form parameters
// This will depend on how the camp ID is stored in the outcome form submission
}Then use this new method in updateCampStatusOnOutcomeFilled:
$collectionCampId = self::findCollectionCampId($params);
if (!$collectionCampId) {
return;
}
$results = EckEntity::update('Collection_Camp', FALSE)
->addValue('Collection_Camp_Intent_Details.Camp_status_field', 'completed')
->addWhere('id', '=', $collectionCampId)
->execute();This will ensure that the correct collection camp is being updated when the outcome form is submitted.
…mp-post-authorization-status
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
1092-1122: LGTM: Conditional inclusion of camp outcome form linkThe
getLogisticsEmailHtmlmethod has been updated to conditionally include the camp outcome form link based on the camp status. This change aligns with the PR objective of not including the camp outcome link for aborted camps.A minor suggestion for improvement:
Consider extracting the email content into a separate template file for better separation of concerns and easier maintenance. This would make the email content more manageable, especially if it grows or needs to be localized in the future.
1254-1282: LGTM: New helper method for checking camp status and IDsThe
checkCampStatusAndIdsmethod is a well-implemented helper function that provides a useful abstraction for checking camp status and IDs. It follows good practices by validating input and returning null for invalid cases.A minor suggestion for improvement:
Consider adding a type hint for the return value to improve code readability and maintainability. You can use PHP 7.1+ nullable return type declaration:
public static function checkCampStatusAndIds(string $objectName, $objectId, &$objectRef): ?array { // ... existing implementation ... }This change would make it clear to other developers that the method either returns an array or null.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (7 hunks)
🧰 Additional context used
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
56-62: LGTM: New event subscriptions addedThe new event subscriptions for 'updateCampStatusAfterAuth' and 'updateCampStatusOnOutcomeFilled' have been correctly added to the
getSubscribedEventsmethod. These additions align with the PR objectives of updating camp statuses based on specific triggers.
Line range hint
1043-1069: LGTM: Updated sendLogisticsEmail to include camp statusThe
sendLogisticsEmailmethod has been correctly updated to include the camp status when callinggetLogisticsEmailHtml. This change aligns with the PR objective of including camp status in the email content.The implementation looks good and follows best practices. Nice job on maintaining consistency with the existing code structure while adding this new functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackReminderCron.php (2)
Line range hint
52-56: Update query conditions to align with PR objectivesThe current query conditions don't fully align with the PR objectives. The
Collection_Camp_Core_Details.Statusis still set to 'authorized', which might not capture all the relevant camp statuses.Consider updating the query conditions to handle different camp statuses as per the PR objectives. For example:
->addWhere('Collection_Camp_Intent_Details.Camp_status_field', 'IN', ['Planned', 'Completed']) ->addWhere('Collection_Camp_Intent_Details.Camp_status_field', '!=', 'Aborted')This change ensures that reminders are sent for camps that are either planned or completed, but not for aborted camps.
Line range hint
58-67: Implement logic to update camp status to 'Completed'The current implementation doesn't include logic to update the camp status to 'Completed' after the outcome form is filled, as mentioned in the PR objectives.
Consider adding logic to update the camp status to 'Completed' after processing the volunteer feedback. This could be done within the
CollectionCampVolunteerFeedbackService::processVolunteerFeedbackRemindermethod or as a separate step in this function.Example implementation:
foreach ($campsNeedReminder as $camp) { try { $feedbackProcessed = CollectionCampVolunteerFeedbackService::processVolunteerFeedbackReminder($camp, $now, $from); if ($feedbackProcessed) { EckEntity::update('Collection_Camp', ['id' => $camp['id']]) ->addValue('Collection_Camp_Intent_Details.Camp_status_field', 'Completed') ->execute(); } } catch (\Exception $e) { \Civi::log()->error('Error processing volunteer feedback reminder', [ 'id' => $camp['id'], 'error' => $e->getMessage(), ]); } }wp-content/civi-extensions/goonjcustom/Civi/CollectionCampVolunteerFeedbackService.php (1)
Line range hint
16-22: Update the method's docblock to include the new parameter.The method signature has been updated to include a new
$fromparameter, but the docblock hasn't been updated to reflect this change. This violates the best practice of keeping documentation in sync with code changes.Please update the docblock to include the new parameter:
* @param \DateTimeImmutable $now * The current date. + * @param string $from + * The sender's email address. * * @throws \CRM_Core_Exception */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampVolunteerFeedbackService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/VolunteerFeedbackReminderCron.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampVolunteerFeedbackService.php (3)
37-43: Excellent addition of camp status check!The new check for aborted camps aligns perfectly with the PR objectives and follows good practices. It efficiently skips processing for aborted camps and includes appropriate logging. Well done!
Line range hint
65-65: Correct usage of the new$fromparameter.The
$fromparameter is correctly passed to thesendVolunteerFeedbackReminderEmailmethod, consistent with the earlier method signature change. This improves email handling as intended in the PR objectives.
Line range hint
1-138: Overall, the changes look good and align with the PR objectives.The implementation handles aborted camps as required and improves email functionality. The code is readable and follows the single responsibility principle. The only improvement needed is updating the docblock for the
processVolunteerFeedbackRemindermethod.Great job on maintaining code quality while implementing these new features!
| $collectionCampId = $camp['id']; | ||
| $campCode = $camp['title']; | ||
| $campAddress = $camp['Collection_Camp_Intent_Details.Location_Area_of_camp']; | ||
| $campStatus = $camp['Collection_Camp_Intent_Details.Camp_status_field']; |
There was a problem hiding this comment.
Can we please recreate this custom field and remove _field from its name?
|
|
||
| // Skip if camp status is "aborted". | ||
| if ($campStatus === 'aborted') { | ||
| \Civi::log()->info("Skipping camp ID $collectionCampId as it is marked as 'aborted'"); |
There was a problem hiding this comment.
Let's move this a where clause on the query
| $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; | ||
|
|
||
| if (!$newStatus || !$objectId) { | ||
| // Ensure valid status and camp details are returned. |
There was a problem hiding this comment.
these comments are not needed. the code itself speaks for it.
| // Skip if camp status is "aborted". | ||
| if ($campStatus === 'aborted') { | ||
| \Civi::log()->info("Skipping camp ID $collectionCampId as it is marked as 'aborted'"); | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
Let's move this check to API query where clause
| public static function updateCampStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) { | ||
| $statusDetails = self::checkCampStatusAndIds($objectName, $objectId, $objectRef); | ||
|
|
||
| // Ensure valid status and camp details are returned. |
There was a problem hiding this comment.
comment not needed as it does not add more value to readability
| // Check for status change. | ||
| if ($currentStatus !== $newStatus) { | ||
| if ($newStatus === 'authorized') { | ||
| // Access the subtype. |
| return; | ||
| } | ||
|
|
||
| // Access the id within the decoded data. |
| return; | ||
| } | ||
|
|
||
| // Update camp status to 'planned' when the camp is authorized. |
| $campStatus = $camp['Collection_Camp_Intent_Details.Camp_status_field']; | ||
|
|
||
| // Skip if camp status is "aborted". | ||
| if ($campStatus === 'aborted') { | ||
| \Civi::log()->info("Skipping camp ID $collectionCampId as it is marked as 'aborted'"); | ||
| return FALSE; | ||
| } |
| // Skip if camp status is "aborted". | ||
| if ($campStatus === 'aborted') { | ||
| \Civi::log()->info("Skipping camp ID $collectionCampId as it is marked as 'aborted'"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
1043-1049: Good addition, but let's improve that logging statement!The new check for aborted camps is a great addition! It prevents unnecessary emails and improves efficiency. However, the logging statement could be improved for better performance and security.
Consider using placeholders in the logging statement instead of string concatenation:
-\Civi::log()->info("Skipping camp ID $collectionCampId as it is marked as 'aborted'"); +\Civi::log()->info("Skipping camp ID %1 as it is marked as 'aborted'", [1 => $collectionCampId]);This approach is more efficient and safer, especially when dealing with user-supplied data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (6 hunks)
🧰 Additional context used
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (5 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampCron.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampCron.php
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php
🧰 Additional context used
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
56-62: LGTM! New event subscriptions added correctly.The new event subscriptions for 'updateCampStatusAfterAuth' and 'updateCampStatusOnOutcomeFilled' have been added consistently with the existing structure. Good job on maintaining the organization of the
getSubscribedEventsmethod!
1095-1108:⚠️ Potential issueRemove unused parameter and kudos on improved email content!
Great job on enhancing the email content with more detailed instructions! However, there's a small issue we should address:
The
$campStatusparameter has been added to the method signature but is not being used in the method body. Let's remove it to keep our code clean:-private static function getLogisticsEmailHtml($contactName, $collectionCampId, $campAttendedById, $collectionCampGoonjOffice, $campCode, $campAddress, $campStatus) { +private static function getLogisticsEmailHtml($contactName, $collectionCampId, $campAttendedById, $collectionCampGoonjOffice, $campCode, $campAddress) {Remember, unused parameters can lead to confusion and make the code harder to maintain. Always remove parameters that aren't being used in the method body.
Likely invalid or redundant comment.
| public static function updateCampStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) { | ||
| $statusDetails = self::checkCampStatusAndIds($objectName, $objectId, $objectRef); | ||
|
|
||
| // Ensure valid status and camp details are returned. | ||
| if (!$statusDetails) { | ||
| return; | ||
| } | ||
|
|
||
| $newStatus = $statusDetails['newStatus']; | ||
| $currentStatus = $statusDetails['currentStatus']; | ||
|
|
||
| // Check for status change. | ||
| if ($currentStatus !== $newStatus) { | ||
| if ($newStatus === 'authorized') { | ||
| // Access the subtype. | ||
| $subtypeId = $objectRef['subtype'] ?? NULL; | ||
| if ($subtypeId === NULL) { | ||
| return; | ||
| } | ||
|
|
||
| // Access the id within the decoded data. | ||
| $campId = $objectRef['id'] ?? NULL; | ||
| if ($campId === NULL) { | ||
| return; | ||
| } | ||
|
|
||
| // Update camp status to 'planned' when the camp is authorized. | ||
| $results = EckEntity::update('Collection_Camp', TRUE) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_status_field', 'planned') | ||
| ->addWhere('id', '=', $campId) | ||
| ->execute(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Nice addition, but let's make it DRYer!
The new updateCampStatusAfterAuth method is a great addition that improves the clarity of our camp status management. I love the use of the checkCampStatusAndIds helper function for input validation - that's a solid practice!
However, I've noticed some code duplication between this method and the generateCollectionCampCode method. Let's make our code DRYer!
Consider extracting the common logic into a separate method:
private static function handleAuthorizedCamp($objectRef, $campId) {
$subtypeId = $objectRef['subtype'] ?? NULL;
if ($subtypeId === NULL) {
return;
}
// Update camp status to 'planned'
EckEntity::update('Collection_Camp', TRUE)
->addValue('Collection_Camp_Intent_Details.Camp_status_field', 'planned')
->addWhere('id', '=', $campId)
->execute();
// Add any other common logic here
}Then, update both methods to use this new helper:
public static function updateCampStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) {
$statusDetails = self::checkCampStatusAndIds($objectName, $objectId, $objectRef);
if (!$statusDetails || $statusDetails['newStatus'] !== 'authorized') {
return;
}
self::handleAuthorizedCamp($objectRef, $objectId);
}
public static function generateCollectionCampCode(string $op, string $objectName, $objectId, &$objectRef) {
$statusDetails = self::checkCampStatusAndIds($objectName, $objectId, $objectRef);
if (!$statusDetails || $statusDetails['newStatus'] !== 'authorized') {
return;
}
self::handleAuthorizedCamp($objectRef, $objectId);
// Additional logic specific to generateCollectionCampCode
}This refactoring will make our code more maintainable and reduce the risk of inconsistencies when making future changes. Remember, DRY code is happy code! 😊
| public static function updateCampStatusOnOutcomeFilled($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(); | ||
|
|
||
| $collectionCampId = $collectionSourceVehicleDispatch['Camp_Vehicle_Dispatch.Collection_Camp']; | ||
|
|
||
| // Update camp status to 'completed' when the camp outcome form is submitted. | ||
| $results = EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Collection_Camp_Intent_Details.Camp_status_field', 'completed') | ||
| ->addWhere('id', '=', $collectionCampId) | ||
| ->execute(); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Great addition, but let's polish it up!
The new updateCampStatusOnOutcomeFilled method is a valuable addition that ensures our camp statuses are kept up-to-date. However, there are a few areas where we can make it even better:
-
The use of
findOfficeIdis a bit confusing here. It seems to be retrieving a vehicle dispatch ID, not an office ID. Let's rename this for clarity. -
We're missing some error handling and logging, which could make debugging tricky in the future.
-
The method could be more concise by directly fetching the collection camp ID.
Here's a suggested refactor that addresses these points:
public static function updateCampStatusOnOutcomeFilled($op, $groupID, $entityID, &$params) {
if ($op !== 'create') {
return;
}
try {
$collectionCampId = self::findCollectionCampIdFromOutcomeForm($params);
if (!$collectionCampId) {
\Civi::log()->warning('Unable to find collection camp ID from outcome form submission.');
return;
}
EckEntity::update('Collection_Camp', FALSE)
->addValue('Collection_Camp_Intent_Details.Camp_status_field', 'completed')
->addWhere('id', '=', $collectionCampId)
->execute();
\Civi::log()->info("Updated camp status to completed for camp ID: $collectionCampId");
} catch (\Exception $e) {
\Civi::log()->error('Error updating camp status on outcome filled: ' . $e->getMessage());
}
}
private static function findCollectionCampIdFromOutcomeForm($params) {
// Implement logic to find the collection camp ID directly from the outcome form parameters
}This refactored version improves error handling, adds logging for better debugging, and makes the method's purpose clearer. Remember, clear and robust code is happy code! 😊
2 participants
Remove the outcome link and change the status
Target Issue - https://github.com/coloredcow-admin/goonj-crm/issues/133
https://lab.civicrm.org/extensions/aws-ses
How to Test
Camp status fieldbecome PlannedCamp status fieldbecomes CompletedCron
Summary by CodeRabbit
New Features
Bug Fixes
Documentation