Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract payment function #31404

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Conversation

mattwire
Copy link
Contributor

Overview

Extraction of payment code in CRM_Contribute_Form_Confirm to make it easier to understand/refactor.

Before

processConfirm() is a big long function that is difficult to follow.

After

It's a bit easier to understand and a chunk of code is extracted to make it easier to see inputs/outputs.

Technical Details

Comments

I'm trying to debug a payment failure that leaves no trace on the server and got very lost in this function.

Copy link

civibot bot commented Oct 31, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Oct 31, 2024
@@ -2444,8 +2444,7 @@ protected function completeTransaction($result, $contributionID) {
*/
protected function bounceOnError($message): void {
CRM_Core_Session::singleton()
->setStatus(ts('Payment Processor Error message :') .
$message);
->setStatus(ts('Payment Processor Error message') . ': ' . $message);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just simplifies translation

@@ -123,7 +123,7 @@ public static function getMessages(&$error, $separator = '<br />') {
public static function displaySessionError(&$error, $separator = '<br />') {
$message = self::getMessages($error, $separator);
if ($message) {
$status = ts("Payment Processor Error message") . "{$separator} $message";
$status = ts('Payment Processor Error message') . "{$separator} $message";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor cleanup

* @throws \CRM_Core_Exception
*/
private function processConfirmPayment(\CRM_Contribute_DAO_Contribution $contribution, int $contactID): array {
$form = $this;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should do a find/replace in this function but want to do a straight extraction first.

Comment on lines +2664 to +2667
$result['is_payment_failure'] = TRUE;
$result['error'] = $e;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this at all and would prefer that an exception was thrown

@eileenmcnaughton
Copy link
Contributor

@mattwire this looks like a straight extraction but there is something going on in here I can't spot

api_v3_ContributionPageTest::testSubmitRecurMultiProcessorInstantPayment
Failure in api call for Contribution_Page submit: Property 'amount' has not been set.
#0 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php(465): Civi\Payment\PropertyBag->get('amount', 'default')
#1 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/CRM/Core/Payment/Dummy.php(208): Civi\Payment\PropertyBag->getAmount()
#2 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/CRM/Contribute/Form/Contribution/Confirm.php(2642): CRM_Core_Payment_Dummy->doPayment(Array)
#3 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/CRM/Contribute/Form/Contribution/Confirm.php(2600): CRM_Contribute_Form_Contribution_Confirm->processConfirmPayment(Object(CRM_Contribute_BAO_Contribution), 4)

@eileenmcnaughton
Copy link
Contributor

Also @mattwire FYI - out of scope here but about where things might be going - I generally try to pass in an array of the parameters a function uses when I can - rather than using $form->_params - because it makes it more visible / explicit what values are actually used & reduces the risk of random stuff being stuff in one array to get them out at some unpredictable later point

@mattwire
Copy link
Contributor Author

@eileenmcnaughton It was the $paymentParams array - that needed passing in too.
Agree about passing in array of parameters where possible, not sure when I'll get round to further refactoring but the nice thing about extracting this bit is that it removes a big chunk of code from the main function and it does an early return so we don't need to worry about anything that goes on in this bit of code affecting later down the function.

@eileenmcnaughton eileenmcnaughton merged commit db98500 into civicrm:master Nov 1, 2024
1 check passed
@mattwire mattwire deleted the extractpay branch November 1, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants