fix bug saving a new recurring contribution to the database #29005

patricklam · 2024-01-14T04:24:04Z

I experienced a bug in CiviCRM 5.69.2 with the Stripe plugin from mjw. Adding a new recurring contribution (for an annual membership), CiviCRM reports the error:

2024-01-12 04:49:44-0500 [error] CRM_Contribute_Form_Contribution_Confirm::completeTransaction CRM_Core_Exception: contribution_status_id is not a valid integer

and the error message "Failed to update contribution in database".

I tracked this down to the code patched here in contributionRecur, which attempts to store "In Progress" as the contribution_status_id, which is indeed not a valid integer. It looks like everywhere else in the code, CiviCRM is going to do a PseudoConstant lookup to store the relevant ID. I've modified the code here to match the convention elsewhere.

The patch appears to work on my own install of CiviCRM.

I don't have a CiviCRM account yet. I was planning to report it on the bug tracker, but my account hasn't been approved yet. Anyway, I can report it there once I have an account. Or not, if you just merge this patch. Thanks for all the work maintaining this software!

Overview

It looks like stale code dealing with recurring contributions is trying to put invalid values into the database, and getting rejected.

Before

Adding a new recurring contribution (for an annual membership), CiviCRM reports the error:

2024-01-12 04:49:44-0500 [error] CRM_Contribute_Form_Contribution_Confirm::completeTransaction CRM_Core_Exception: contribution_status_id is not a valid integer

and the error message "Failed to update contribution in database".

After

Adding a new recurring contribution appears to work.

I experienced a bug in CiviCRM 5.69.2 with the Stripe plugin from mjw. Adding a new recurring contribution (for an annual membership), CiviCRM reports the error: 2024-01-12 04:49:44-0500 [error] CRM_Contribute_Form_Contribution_Confirm::completeTransaction CRM_Core_Exception: contribution_status_id is not a valid integer and the error message "Failed to update contribution in database". I tracked this down to the code patched here in contributionRecur, which attempts to store "In Progress" as the contribution_status_id, which is indeed not a valid integer. It looks like everywhere else in the code, CiviCRM is going to do a PseudoConstant lookup to store the relevant ID. I've modified the code here to match the convention elsewhere. The patch appears to work on my own install of CiviCRM. I don't have a CiviCRM account yet. I was planning to report it on the bug tracker, but my account hasn't been approved yet. Anyway, I can report it there once I have an account. Or not, if you just merge this patch. Thanks for all the work maintaining this software!

civibot · 2024-01-14T04:24:07Z

🤖 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...

➡️ PR Demo site (can be rebuilt with the command test this please).
📖 Review standards
🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

civicrm-builder · 2024-01-14T04:24:07Z

Can one of the admins verify this patch?

civicrm-builder · 2024-01-14T04:24:07Z

Can one of the admins verify this patch?

colemanw · 2024-01-14T19:24:24Z

@civicrm-builder add to whitelist

colemanw · 2024-01-14T19:30:25Z

Thank you for this patch @patricklam.

For background, I think what's probably going on here is that some payment processors are switching from Civi's APIv3 over to APIv4. The two APIs have a crucial difference wrt these option value lookups: if you pass a string to APIv3 it will helpfully assume you meant to pass an id and convert it for you. APIv4 is also capable of doing those lookups but in order to reduce ambiguity it doesn't make assumptions - you have to explicitly tell it what to lookup by passing a key like $params['contribution_status_id:name'] = "In Progress".

Anyway, I think this is a good fix because it will work for both versions of the API.

Since this appears to be a regression I think this PR should be agains the 5.70 branch instead of master so we can backport it. Do you know how to rebase a PR in git?

demeritcowboy · 2024-01-14T21:05:05Z

CRM/Contribute/BAO/ContributionRecur.php

@@ -940,15 +940,15 @@ public static function updateOnNewPayment($recurringContributionID, $paymentStat

    if ($paymentStatus == 'Completed'
      && CRM_Contribute_PseudoConstant::contributionStatus($existing['contribution_status_id'], 'name') == 'Pending') {
-      $params['contribution_status_id'] = 'In Progress';
+      $params['contribution_status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'In Progress');


I'd say the tests are failing because contributionrecur has different statuses than contribution. But also, the fact that the tests were passing before means the api3 feature to use the name is still working. Is it possible you've disabled or are somehow missing the contribution_recur statuses in your db? If you go to admin - system settings - option groups do you have one for contribution_recur_status - does it have choices?

@demeritcowboy huh, my db was missing the contribution_recur_status options. (I ran into another problem where it wouldn't show me any types for adding option values; maybe there is another missing option value, but added it directly to the db). I've added contribution_recur_status now.

@colemanw I tried to rebase my PR but didn't quite seem to manage to do it. I'd thought that I had done it, but apparently not. I have created a rebased commit on 5.70 at 61cec1b but I don't know how to update this PR to point to that commit.

Since it sounds like there is no code bug and this PR is about futureproofing now, it could just be against master.

Hmm, maybe my assumption was incorrect and there's no bug here at all.

Well, the stripe plugin is using v3 almost everywhere that I saw. It was definitely throwing a type error trying to put "In Progress" as the contribution_status_id, but my db was also missing that option value. I stopped looking after this, but I don't know where "In Progress" would get looked up as a contribution_recur_status option value. I guess it does somewhere.

I do see that the BAO Contribution code does explicitly go through CRM_Core_PseudoConstant though; looks like ContributionRecur is older code that didn't also get updated.

It happens at

civicrm-core/api/v3/utils.php

Lines 2378 to 2382 in 3cab30b

$newValue = array_search((string) $value, $options);

if ($newValue !== FALSE) {

$value = $newValue;

return;

}

.

Here's two options:

Leave it as-is since the code is ok and the error seems related to missing config.

Use getKey but using the right BAO (ContributionRecur), but it's unnecessary because of the above - api3 will convert by itself.

By the way using $params['contribution_status_id:name'] = "In Progress" doesn't seem to work with api3 in a quick test. It gets ignored and returns all the records.

I see the lookup code, thanks.

I'll leave that up to the maintainers; it's not my codebase. The patch below does use the ContributionRecur BAO and it does work for me.

demeritcowboy · 2024-01-16T01:07:10Z

In the interest of getting you your first PR credit I think we could merge it if it passes tests.

Can you do a separate PR to add yourself to this file https://github.com/civicrm/civicrm-core/blob/master/contributor-key.yml for release notes credit?

demeritcowboy · 2024-01-16T02:02:26Z

Normally we ask to "squash" commits that aren't logically separate, but we've spent enough on this. FYI for future: https://docs.civicrm.org/dev/en/latest/tools/git/#rebasing-interactive

patricklam · 2024-01-16T02:03:45Z

Got it. Thanks!

…

On Tue, Jan 16, 2024, 3:02 PM demeritcowboy ***@***.***> wrote: Normally we ask to "squash" commits that aren't logically separate, but we've spent enough on this. FYI for future: https://docs.civicrm.org/dev/en/latest/tools/git/#rebasing-interactive — Reply to this email directly, view it on GitHub <#29005 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOKE5TWDAZXZGSZTZSMZI3YOXNT3AVCNFSM6AAAAABBZZ22AWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJSHE3DGMBVHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

civibot bot added the master label Jan 14, 2024

demeritcowboy reviewed Jan 14, 2024

View reviewed changes

use correct BAO to look up contribution_status_id

56f5e59

demeritcowboy added the merge on pass label Jan 16, 2024

patricklam added 2 commits January 16, 2024 14:11

Add contributor

94a37ad

Merge branch 'master' of github.com:patricklam/civicrm-core

60fc7d3

demeritcowboy merged commit 573e836 into civicrm:master Jan 16, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bug saving a new recurring contribution to the database #29005

fix bug saving a new recurring contribution to the database #29005

patricklam commented Jan 14, 2024

civibot bot commented Jan 14, 2024

civicrm-builder commented Jan 14, 2024

civicrm-builder commented Jan 14, 2024

colemanw commented Jan 14, 2024

colemanw commented Jan 14, 2024

demeritcowboy Jan 14, 2024

patricklam Jan 14, 2024

demeritcowboy Jan 14, 2024

colemanw Jan 15, 2024

patricklam Jan 15, 2024

demeritcowboy Jan 15, 2024

patricklam Jan 16, 2024

demeritcowboy commented Jan 16, 2024

demeritcowboy commented Jan 16, 2024

patricklam commented Jan 16, 2024 via email

	$newValue = array_search((string) $value, $options);
	if ($newValue !== FALSE) {
	$value = $newValue;
	return;
	}

fix bug saving a new recurring contribution to the database #29005

fix bug saving a new recurring contribution to the database #29005

Conversation

patricklam commented Jan 14, 2024

Overview

Before

After

civibot bot commented Jan 14, 2024

civicrm-builder commented Jan 14, 2024

civicrm-builder commented Jan 14, 2024

colemanw commented Jan 14, 2024

colemanw commented Jan 14, 2024

demeritcowboy Jan 14, 2024

Choose a reason for hiding this comment

patricklam Jan 14, 2024

Choose a reason for hiding this comment

demeritcowboy Jan 14, 2024

Choose a reason for hiding this comment

colemanw Jan 15, 2024

Choose a reason for hiding this comment

patricklam Jan 15, 2024

Choose a reason for hiding this comment

demeritcowboy Jan 15, 2024

Choose a reason for hiding this comment

patricklam Jan 16, 2024

Choose a reason for hiding this comment

demeritcowboy commented Jan 16, 2024

demeritcowboy commented Jan 16, 2024

patricklam commented Jan 16, 2024 via email