Stop sending receipt when user don't have pan card#495
Merged
tarunnjoshi merged 2 commits intodevelopfrom Nov 25, 2024
Merged
Conversation
WalkthroughThe pull request introduces modifications 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 (
|
![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 (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
1466-1495: Consider adding tests and documentation.The email suppression feature would benefit from:
- Unit tests to verify the logic for different scenarios (with/without PAN card, invalid contribution ID, etc.)
- Documentation explaining the business rule and its implications
- Consider moving the PAN card validation logic to a separate service class for better separation of concerns
Would you like me to help create:
- Unit tests for the email suppression feature?
- Documentation template for the business rule?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(3 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
76-76: LGTM: Hook registration is correct.
The hook registration for mail parameter alteration follows CiviCRM conventions.
| * | ||
| */ | ||
| public function suppressEmailIfPanCardMissing(&$params, $context = NULL) { | ||
| if (!$params['valueName'] === 'contribution_online_receipt') { |
There was a problem hiding this comment.
Fix the condition syntax.
The condition has two issues:
- Using single
=for comparison instead of=== - Incorrect order of operands which could lead to accidental assignment
Apply this fix:
- if (!$params['valueName'] === 'contribution_online_receipt') {
+ if ($params['valueName'] !== 'contribution_online_receipt') {📝 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.
Suggested change
| if (!$params['valueName'] === 'contribution_online_receipt') { | |
| if ($params['valueName'] !== 'contribution_online_receipt') { |
Comment on lines
+1477
to
+1494
| try { | ||
| $contribution = Contribution::get(FALSE) | ||
| ->addSelect('Contribution_Details.PAN_Card_Number') | ||
| ->addWhere('id', '=', $contributionId) | ||
| ->execute() | ||
| ->single(); | ||
|
|
||
| $panCard = $contribution['Contribution_Details.PAN_Card_Number'] ?? ''; | ||
|
|
||
| if (empty($panCard)) { | ||
| // Modify params to suppress email (set the toEmail as null, or mark as test) | ||
| // Empty out the recipient email address. | ||
| $params['toEmail'] = ''; | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| \Civi::log()->error("Error retrieving contribution data for ID $contributionId: " . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging.
The current implementation has several areas for improvement:
- The catch block catches all exceptions without specific handling
- No logging when emails are suppressed
- Redundant comment on line 1489
Consider this improved implementation:
try {
$contribution = Contribution::get(FALSE)
->addSelect('Contribution_Details.PAN_Card_Number')
->addWhere('id', '=', $contributionId)
->execute()
->single();
$panCard = $contribution['Contribution_Details.PAN_Card_Number'] ?? '';
if (empty($panCard)) {
- // Modify params to suppress email (set the toEmail as null, or mark as test)
- // Empty out the recipient email address.
$params['toEmail'] = '';
+ \Civi::log()->info("Suppressed contribution receipt email for contribution ID $contributionId due to missing PAN card");
}
}
- catch (Exception $e) {
+ catch (\Civi\API4\Exception\ApiException $e) {
\Civi::log()->error("Error retrieving contribution data for ID $contributionId: " . $e->getMessage());
+ } catch (\Exception $e) {
+ \Civi::log()->error("Unexpected error while processing contribution ID $contributionId: " . $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.
Suggested change
| try { | |
| $contribution = Contribution::get(FALSE) | |
| ->addSelect('Contribution_Details.PAN_Card_Number') | |
| ->addWhere('id', '=', $contributionId) | |
| ->execute() | |
| ->single(); | |
| $panCard = $contribution['Contribution_Details.PAN_Card_Number'] ?? ''; | |
| if (empty($panCard)) { | |
| // Modify params to suppress email (set the toEmail as null, or mark as test) | |
| // Empty out the recipient email address. | |
| $params['toEmail'] = ''; | |
| } | |
| } | |
| catch (Exception $e) { | |
| \Civi::log()->error("Error retrieving contribution data for ID $contributionId: " . $e->getMessage()); | |
| } | |
| try { | |
| $contribution = Contribution::get(FALSE) | |
| ->addSelect('Contribution_Details.PAN_Card_Number') | |
| ->addWhere('id', '=', $contributionId) | |
| ->execute() | |
| ->single(); | |
| $panCard = $contribution['Contribution_Details.PAN_Card_Number'] ?? ''; | |
| if (empty($panCard)) { | |
| $params['toEmail'] = ''; | |
| \Civi::log()->info("Suppressed contribution receipt email for contribution ID $contributionId due to missing PAN card"); | |
| } | |
| } | |
| catch (\Civi\API4\Exception\ApiException $e) { | |
| \Civi::log()->error("Error retrieving contribution data for ID $contributionId: " . $e->getMessage()); | |
| } catch (\Exception $e) { | |
| \Civi::log()->error("Unexpected error while processing contribution ID $contributionId: " . $e->getMessage()); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(3 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
11-11: LGTM!
The import statement follows the existing pattern and is correctly placed with other imports.
76-76: LGTM!
The hook registration follows the existing pattern and correctly maps to the new method.
Comment on lines
+1466
to
+1495
| public function suppressEmailIfPanCardMissing(&$params, $context = NULL) { | ||
| if ($params['valueName'] !== 'contribution_online_receipt') { | ||
| return; | ||
| } | ||
|
|
||
| $contributionId = $params['tplParams']['contributionID'] ?? NULL; | ||
|
|
||
| if (!$contributionId) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| $contribution = Contribution::get(FALSE) | ||
| ->addSelect('Contribution_Details.PAN_Card_Number') | ||
| ->addWhere('id', '=', $contributionId) | ||
| ->execute() | ||
| ->single(); | ||
|
|
||
| $panCard = $contribution['Contribution_Details.PAN_Card_Number'] ?? ''; | ||
|
|
||
| if (empty($panCard)) { | ||
| // Modify params to suppress email (set the toEmail as null, or mark as test) | ||
| // Empty out the recipient email address. | ||
| $params['toEmail'] = ''; | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| \Civi::log()->error("Error retrieving contribution data for ID $contributionId: " . $e->getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve code structure and error handling
The method could benefit from several improvements:
- Use early returns to reduce nesting
- Add specific error handling for API exceptions
- Add logging when email is suppressed
- Remove redundant comments
Consider this improved implementation:
public function suppressEmailIfPanCardMissing(&$params, $context = NULL) {
- if ($params['valueName'] !== 'contribution_online_receipt') {
- return;
- }
+ if ($params['valueName'] !== 'contribution_online_receipt') return;
$contributionId = $params['tplParams']['contributionID'] ?? NULL;
+ if (!$contributionId) return;
- if (!$contributionId) {
- return;
- }
-
try {
$contribution = Contribution::get(FALSE)
->addSelect('Contribution_Details.PAN_Card_Number')
->addWhere('id', '=', $contributionId)
->execute()
->single();
$panCard = $contribution['Contribution_Details.PAN_Card_Number'] ?? '';
if (empty($panCard)) {
- // Modify params to suppress email (set the toEmail as null, or mark as test)
- // Empty out the recipient email address.
$params['toEmail'] = '';
+ \Civi::log()->info("Suppressed contribution receipt email for contribution ID $contributionId due to missing PAN card");
}
}
- catch (Exception $e) {
+ catch (\Civi\API4\Exception\ApiException $e) {
\Civi::log()->error("Error retrieving contribution data for ID $contributionId: " . $e->getMessage());
+ } catch (\Exception $e) {
+ \Civi::log()->error("Unexpected error while processing contribution ID $contributionId: " . $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.
Suggested change
| public function suppressEmailIfPanCardMissing(&$params, $context = NULL) { | |
| if ($params['valueName'] !== 'contribution_online_receipt') { | |
| return; | |
| } | |
| $contributionId = $params['tplParams']['contributionID'] ?? NULL; | |
| if (!$contributionId) { | |
| return; | |
| } | |
| try { | |
| $contribution = Contribution::get(FALSE) | |
| ->addSelect('Contribution_Details.PAN_Card_Number') | |
| ->addWhere('id', '=', $contributionId) | |
| ->execute() | |
| ->single(); | |
| $panCard = $contribution['Contribution_Details.PAN_Card_Number'] ?? ''; | |
| if (empty($panCard)) { | |
| // Modify params to suppress email (set the toEmail as null, or mark as test) | |
| // Empty out the recipient email address. | |
| $params['toEmail'] = ''; | |
| } | |
| } | |
| catch (Exception $e) { | |
| \Civi::log()->error("Error retrieving contribution data for ID $contributionId: " . $e->getMessage()); | |
| } | |
| } | |
| public function suppressEmailIfPanCardMissing(&$params, $context = NULL) { | |
| if ($params['valueName'] !== 'contribution_online_receipt') return; | |
| $contributionId = $params['tplParams']['contributionID'] ?? NULL; | |
| if (!$contributionId) return; | |
| try { | |
| $contribution = Contribution::get(FALSE) | |
| ->addSelect('Contribution_Details.PAN_Card_Number') | |
| ->addWhere('id', '=', $contributionId) | |
| ->execute() | |
| ->single(); | |
| $panCard = $contribution['Contribution_Details.PAN_Card_Number'] ?? ''; | |
| if (empty($panCard)) { | |
| $params['toEmail'] = ''; | |
| \Civi::log()->info("Suppressed contribution receipt email for contribution ID $contributionId due to missing PAN card"); | |
| } | |
| } | |
| catch (\Civi\API4\Exception\ApiException $e) { | |
| \Civi::log()->error("Error retrieving contribution data for ID $contributionId: " . $e->getMessage()); | |
| } catch (\Exception $e) { | |
| \Civi::log()->error("Unexpected error while processing contribution ID $contributionId: " . $e->getMessage()); | |
| } | |
| } |
pokhiii
approved these changes
Nov 25, 2024
This was referenced Nov 26, 2024
This was referenced Dec 11, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stop sending receipt when user don't have pan card
Summary by CodeRabbit
New Features
Bug Fixes