Show contribution count on Outcome form#496
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new method, 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampCron.php (1)
Line range hint
77-83: Enhance error logging for better debuggingWhile the error handling captures basic information, it could be more helpful for debugging production issues.
Consider enhancing the error logging like this:
- \Civi::log()->info('Error processing camp', [ - 'id' => $camp['id'], - 'error' => $e->getMessage(), - ]); + \Civi::log()->error('Error processing collection camp', [ + 'camp_id' => $camp['id'], + 'camp_title' => $camp['title'], + 'error_message' => $e->getMessage(), + 'error_trace' => $e->getTraceAsString(), + 'operation' => 'cron_processing', + ]);wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
Line range hint
27-1456: Consider refactoring the service class to improve maintainabilityThe
CollectionCampServiceclass has grown too large and handles multiple responsibilities, including:
- Collection camp management
- Email notifications
- QR code generation
- Activity tracking
- Contribution management
- Form handling
This violates the Single Responsibility Principle and makes the code harder to maintain and test.
Consider breaking down this class into smaller, focused services:
CollectionCampManager- Core camp managementCollectionCampNotificationService- Email notificationsCollectionCampQRCodeService- QR code generationCollectionCampActivityTracker- Activity trackingCollectionCampContributionService- Contribution managementCollectionCampFormHandler- Form handlingExample refactoring for the contribution-related functionality:
class CollectionCampContributionService { private $logger; private $entityManager; public function __construct(LoggerInterface $logger, EntityManagerInterface $entityManager) { $this->logger = $logger; $this->entityManager = $entityManager; } public function updateContributionCount(CollectionCamp $collectionCamp): void { // Implementation moved from static method } public function updateContributorCount(CollectionCamp $collectionCamp): void { // Implementation moved from static method } }This refactoring would:
- Improve testability by allowing dependency injection
- Make the code more maintainable by separating concerns
- Make it easier to modify or extend individual components
- Reduce the risk of breaking unrelated functionality
📜 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/api/v3/Goonjcustom/CollectionCampCron.php(1 hunks)
| CollectionCampService::sendLogisticsEmail($camp); | ||
| CollectionCampService::updateContributorCount($camp); | ||
|
|
||
| CollectionCampService::updateContributionCount($camp); |
There was a problem hiding this comment.
💡 Codebase verification
The two count methods serve distinct purposes - no duplication concern
Looking at the implementations, these methods track different metrics:
updateContributorCount: Counts unique contributors (activities) for a collection campupdateContributionCount: Calculates the total monetary amount of contributions
The sequential calls are justified as they update different business metrics. However, there are a few suggestions for improvement:
- The
updateContributionCountmethod hasis_test = TRUEcondition which seems suspicious - this might be filtering out real contributions - Consider adding PHPDoc comments to document the distinct purpose of each method
- Consider combining these updates in a transaction if your database supports it
🔗 Analysis chain
Clarify the distinction between contributor and contribution counts
The code now calls both updateContributorCount and updateContributionCount sequentially. This raises several concerns:
- Are these counts measuring different things? If so, the distinction should be documented.
- If they're related, could they be combined into a single update operation?
- Multiple database updates in sequence might impact performance.
Let's check the implementation of both methods to understand their differences:
Consider the following improvements:
- If these are truly separate counts, add documentation explaining their purposes
- If they're related, consider combining them into a single update operation
- Consider wrapping related updates in a transaction for better data consistency
The previous ast-grep command had issues with some files, so let's try using ripgrep instead to find these method implementations. This will help us understand if they serve different purposes or if there's potential duplication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for both count update methods to compare their implementations
ast-grep --pattern 'function updateContributorCount($_) {
$$$
}'
ast-grep --pattern 'function updateContributionCount($_) {
$$$
}'
Length of output: 18682
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find these methods
rg -U "function updateContributor(Count|Count)" --type php -A 10
rg -U "function updateContribution(Count|Count)" --type php -A 10
# Also search for the class definition to understand the context
rg "class CollectionCampService" --type php -A 5
Length of output: 3199
| public static function updateContributionCount($collectionCamp) { | ||
| $contributions = Contribution::get(FALSE) | ||
| ->addSelect('total_amount') | ||
| ->addWhere('Contribution_Details.Source', '=', $collectionCamp['id']) | ||
| ->addWhere('is_test', '=', TRUE) | ||
| ->execute(); | ||
|
|
||
| // Initialize sum variable. | ||
| $totalSum = 0; | ||
|
|
||
| // Iterate through the results and sum the total_amount. | ||
| foreach ($contributions as $contribution) { | ||
| $totalSum += $contribution['total_amount']; | ||
| } | ||
|
|
||
| EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Camp_Outcome.Monitory_Contribution', $totalSum) | ||
| ->addWhere('id', '=', $collectionCamp['id']) | ||
| ->execute(); | ||
| } |
There was a problem hiding this comment.
Critical: Fix contribution query filter and add error handling
The implementation has several issues that need to be addressed:
- The query filter
is_test = TRUEwill only return test contributions, which is likely incorrect. Production contributions will be excluded. - Missing error handling for database operations.
- No validation of the input parameter.
- Direct floating-point arithmetic on monetary values could lead to precision issues.
Apply this diff to fix the issues:
public static function updateContributionCount($collectionCamp) {
+ if (empty($collectionCamp['id'])) {
+ \Civi::log()->error('updateContributionCount: Invalid collection camp');
+ return;
+ }
+
+ try {
$contributions = Contribution::get(FALSE)
->addSelect('total_amount')
->addWhere('Contribution_Details.Source', '=', $collectionCamp['id'])
- ->addWhere('is_test', '=', TRUE)
+ ->addWhere('is_test', '=', FALSE)
->execute();
// Initialize sum variable using BCMath for precision
- $totalSum = 0;
+ $totalSum = '0.00';
// Iterate through the results and sum the total_amount
foreach ($contributions as $contribution) {
- $totalSum += $contribution['total_amount'];
+ $totalSum = bcadd($totalSum, $contribution['total_amount'], 2);
}
EckEntity::update('Collection_Camp', FALSE)
->addValue('Camp_Outcome.Monitory_Contribution', $totalSum)
->addWhere('id', '=', $collectionCamp['id'])
->execute();
+ } catch (\Exception $e) {
+ \Civi::log()->error('Error updating contribution count: ' . $e->getMessage());
+ }
}📝 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.
| public static function updateContributionCount($collectionCamp) { | |
| $contributions = Contribution::get(FALSE) | |
| ->addSelect('total_amount') | |
| ->addWhere('Contribution_Details.Source', '=', $collectionCamp['id']) | |
| ->addWhere('is_test', '=', TRUE) | |
| ->execute(); | |
| // Initialize sum variable. | |
| $totalSum = 0; | |
| // Iterate through the results and sum the total_amount. | |
| foreach ($contributions as $contribution) { | |
| $totalSum += $contribution['total_amount']; | |
| } | |
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addValue('Camp_Outcome.Monitory_Contribution', $totalSum) | |
| ->addWhere('id', '=', $collectionCamp['id']) | |
| ->execute(); | |
| } | |
| public static function updateContributionCount($collectionCamp) { | |
| if (empty($collectionCamp['id'])) { | |
| \Civi::log()->error('updateContributionCount: Invalid collection camp'); | |
| return; | |
| } | |
| try { | |
| $contributions = Contribution::get(FALSE) | |
| ->addSelect('total_amount') | |
| ->addWhere('Contribution_Details.Source', '=', $collectionCamp['id']) | |
| ->addWhere('is_test', '=', FALSE) | |
| ->execute(); | |
| // Initialize sum variable using BCMath for precision | |
| $totalSum = '0.00'; | |
| // Iterate through the results and sum the total_amount | |
| foreach ($contributions as $contribution) { | |
| $totalSum = bcadd($totalSum, $contribution['total_amount'], 2); | |
| } | |
| EckEntity::update('Collection_Camp', FALSE) | |
| ->addValue('Camp_Outcome.Monitory_Contribution', $totalSum) | |
| ->addWhere('id', '=', $collectionCamp['id']) | |
| ->execute(); | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Error updating contribution count: ' . $e->getMessage()); | |
| } | |
| } |
| $contributions = Contribution::get(FALSE) | ||
| ->addSelect('total_amount') | ||
| ->addWhere('Contribution_Details.Source', '=', $collectionCamp['id']) | ||
| ->addWhere('is_test', '=', TRUE) |
There was a problem hiding this comment.
I don't think this is right.
We should do both IN , [TRUE, FALSE]
There was a problem hiding this comment.
For both ->addWhere('is_test', 'IS NOT NULL') this one gave all the results contains yes and no both,but when i trying to do ->addWhere('is_test', 'IN', ['true,false']) then it gave me only TRUE results.
2 participants
Show contribution count on Outcome form
Summary by CodeRabbit
Summary by CodeRabbit