Page MenuHomePhabricator

Civi: Forget Me button yielding parameters errors
Closed, ResolvedPublicBUG REPORT


Spinning off from T321694: Civi: Forget Me button yields configuration error

Yamini and I are encountering errors when using the Forget Me button in Civi. She tested five times, and twice got the 'one of parameters is not of the type CommaSeparatedIntegers' error, and I get the same error about 1/4 of the time.

CID17949663on2022-11-09 at 3.57.38 PM.png (458×1 px, 407 KB)

Recent affected cids include 17949663, 13211031, and 22283764. Breadcrumb from prior Task:

"Looks like there is a trailing comma in the list there and thus it's seeing a non-int value."

The oldest of our backlog of requests are approaching the 30 day SLA recommended by Legal for data deletion requests, so this Task is somewhat time sensitive.

Event Timeline

I'm still trying to reproduce this locally. Those IDs in the error message correspond to the ids in the civicrm_contribution table for the affected CIDs, but I can't see any error in the logs that would indicate where in the code it's failing. I tried creating a donor locally with the same number of donations as CID 17949663, but the forgetme button works fine even so.

OK, I was able to get a backtrace with some ugly hackery on staging:

#3  CRM_Contribute_BAO_ContributionSoft::getSoftCreditContributionFields(Array ([0] => 20118726,[1] => 28606490,[2] => 33451523,[3] => 40456876,[4] => 52205974,[5] => 57517576,[6] => ), 1) called at [sites/all/modules/civicrm/api/v3/Contribution.php:255]
#4  civicrm_api3_contribution_get(Array ([contact_id] => Array ([IN] => Array ([0] => 17949663,[1] => 37260883)),[version] => 3,[return] => Array ([0] => custom_77,[1] => custom_78,[2] => custom_82,[3] => custom_83,[4] => currency,[5] => receive_date,[6] => total_amount,[7] => trxn_id))) called at [sites/default/civicrm/extensions/wmf-civicrm/api/v3/Contribution/Showme.php:35]
#5  civicrm_api3_contribution_showme(Array ([contact_id] => Array ([IN] => Array ([0] => 17949663,[1] => 37260883)),[version] => 3)) called at [sites/all/modules/civicrm/Civi/API/Provider/MagicFunctionProvider.php:89]

I can run the contribution.get for those contact IDs and those return fields from the api3 explorer just fine. With a small fix to require the forgetme file, I can run the contribution.showme for the contact IDs as well.

Looks like this line in api/v3/Contribution.php:

$contributions = _civicrm_api3_get_using_query_object('Contribution', $params, $additionalOptions, NULL, $mode, $returnProperties);

is returning a phantom extra contribution with a blank key and only the contact_id set, with the contact_id set to the merged/deleted contact that has no contributions:

  array(11) {
    string(8) "37260883"

I guess we should just get rid of the automatic Adyen GDPR calls for now and try to better test this behavior with merged contacts on dev environments before restoring it, so the Donor Relations team can fulfill these requests.

Change 855824 had a related patch set uploaded (by Ejegg; author: Ejegg):

[wikimedia/fundraising/crm@master] Remove Adyen GDPR calls for now

Change 855824 merged by Ejegg:

[wikimedia/fundraising/crm@master] Remove Adyen GDPR calls for now

@MBeat33 I've removed the new Adyen GDPR functionality for now to unblock these requests. On Monday I'll see if I can figure out a fix for it locally and restore it.

I guess my preferred approach would be to switch from apiv3 to apiv4 - the question is just how much we want to touch this code at this time of year

Thanks for the revert, @Ejegg and for the deep digging.

And if this code is risky to deploy we could definitely move this to a Q3 priority, @Eileenmcnaughton

Thanks for keeping this in Undefined Sprint, @greg If needed we can also push it to Q3.

Oh wow @Eileenmcnaughton, it really is api3 making a bad query:

SELECT, as contact_id, as contribution_id, civicrm_contribution.currency as `currency`, civicrm_contribution.receive_date as `receive_date`, civicrm_contribution.total_amount as `total_amount`, civicrm_contribution.trxn_id as `trxn_id`, as wmf_contribution_extra_id, wmf_contribution_extra.gateway as custom_117, wmf_contribution_extra.gateway_txn_id as custom_118, wmf_contribution_extra.original_amount as custom_121, wmf_contribution_extra.original_currency as custom_122
FROM civicrm_contact contact_a
LEFT JOIN civicrm_contribution ON civicrm_contribution.contact_id =
LEFT JOIN wmf_contribution_extra ON wmf_contribution_extra.entity_id = `civicrm_contribution`.id
WHERE  ( IN ("3", "22") );

The fact that it's doing the main select from civicrm_contact and just left-joining the contributions means it'll get the null row for the contact with no contributions (in this case, the mergee).

The switch to API4 for just the contribution::get in ShowMe is possible, but involves an annoying translation from the api3 params sent to ShowMe.

It looks like the fredge_get call, which similarly passes through contact_id, in, [x, y] to an api3 contribution_get is filtering out the null row by adding a condition on the contribution table, namely contribution.is_test=0. I could add a dummy condition like id IS NOT NULL to this request as a smaller fix for now.

Change 857230 had a related patch set uploaded (by Ejegg; author: Ejegg):

[wikimedia/fundraising/crm@master] Revert "Remove Adyen GDPR calls for now"

Change 857231 had a related patch set uploaded (by Ejegg; author: Ejegg):

[wikimedia/fundraising/crm@master] Fix Adyen GDPR forgetme calls for merged contacts

Change 857230 merged by jenkins-bot:

[wikimedia/fundraising/crm@master] Revert "Remove Adyen GDPR calls for now"

Change 857231 merged by jenkins-bot:

[wikimedia/fundraising/crm@master] Fix Adyen GDPR forgetme calls for merged contacts

@MBeat33 We've deployed a fix for this problem and restored the Adyen GDPR calls. Hopefully that's the end of problems with the ForgetMe button.

MBeat33 claimed this task.

Awesome, thank you @Ejegg I tested on a few and it looks great.

(just reopening so we can review during our sprint review next week, glad all is good!)

XenoRyet reassigned this task from Ejegg to MBeat33.
XenoRyet set Final Story Points to 2.