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

error incorrectly updating line item due to pending/declined transaction #301

Closed
lcdservices opened this issue Mar 19, 2020 · 5 comments
Closed

Comments

@lcdservices
Copy link
Contributor

The error presents as "Failure, Error message: DB Error: already exists" in the scheduled job log (Iatsrecurringcontributions job), and with a duplicate key error in the Civi logs, like this:

UPDATE civicrm_line_item SET contribution_id = 363000 , price_field_id = 2 , label = 'My Label' , qty = 1 , unit_price = 50.00 , line_total = 50.00 , participant_count = 0 , price_field_value_id = 14 , financial_type_id = 2 , tax_amount = 0 WHERE ( civicrm_line_item.id = 339372 ) [nativecode=1062 ** Duplicate entry 'civicrm_membership-13756-363000-14-2' for key 'UI_line_item_value']

Tracing the code:

  • in the API function, around line 180, we do a Contribution getsingle API call to see if there is an existing pending contribution. if so, we add the 'id' to the parameter array so that we are updating the existing contrib instead of creating a new one. per the comments, this appears to be because at times a pending contrib is created and then updated and marked completed when the transaction completes.
  • the error is triggered downstream because that UPDATE statement is changing the contribution ID, and line items already exist for that ID

But I think the problem is actually with the mechanism to retrieve existing pending contributions. In our case, the error was triggered when hitting a pending contribution tied to the recurring contribution that had been declined. The error code from iATS is:

[result] => Array
        (
            [payment_status_id] => 1
            [auth_code] =>
            [result] => Array
                (
                    [auth_result] => REJECT: 15
                    [remote_id] => AC5EE4CB
                    [status] => 0
                    [reasonMessage] => General decline code.
                )
            [success] =>
            [message] => General decline code.
        )

I removed the return statement on the getsingle call (to get all values) and added a condition to exclude pending contribs with "decline" in the subject line. That caused a new, pending contribution to be created (declined again), but allowed the script to proceed with processing. That's a lousy way to handle the condition, and I'm not sure if it's the correct way to address the problem. But if I understand the comments and code properly, it wasn't intended to retrieve pending/declined contributions and update them. It was intended to retrieve previously created pending contribs that were only created with the intent of delaying completion until the transaction was fully processed.

Either we need to condition that code more intelligently, or maybe the issue is with the status, where a decline code from iATS should result in the contribution being given a Failed status rather than a Pending status.

@adixon
Copy link
Contributor

adixon commented Mar 19, 2020

Thanks for this. Agreed that grabbing a random-ish pending contribution to complete is a bit edgy, as I recall it's a work around to support recurring sequences with a future start date. In such a case, no transaction runs but you want to create a pending contribution because recurring sequences without at least one contribution are bad (because, at least in the past, the code assumed all recurring sequences had at least one). When the job that triggers recurring contribution runs, the code doesn't know if it's the first one so it's always checking.

Note that this isn't the ACH strategy (which also generates pending contributions for later completion) because in that case, there's a matching invoice id that helps.

In general, failed recurring contributions will be marked as Failed and not left pending, so this is usually a good strategy, though it's clearly not working for your setup. I suspect there's something else going on that's preventing the recurring job from setting the failed contribution to "Failed"? I notice (

// So far so, good ... now use my utility function process_contribution_payment to
) that a contribution is normally left in pending only in case of server failure, so perhaps your server isn't completing the transaction? Or is there something else that's breaking the completion of the contribution?

@lcdservices
Copy link
Contributor Author

There's nothing to indicate a server issue in our logs. I think the issue is around here:

$contribution['contribution_status_id'] = empty($auth_code) ? 2 : 4;

  • process_contribution_payment() calls self::process_payment()
  • that attempts the transaction with iATS. the result is a failure, resulting in the reasonMessage returned as $result['message'] (around line 369).
  • back in process_contribution_payment() that message is appended to the subject line (General Decline code) -- which we are seeing in the Contribution record (line 96).
  • but the line cited above (94) retains a status of Pending if there is no auth_code (which there is not -- see my example above of the response)

So basically, because a General decline response does not include an auth_code, and is not a success, it is stored with a Pending status. I don't know the details of how/when an auth_code is assigned, but I suspect the absence of that value does not necessarily mean there was a server/system failure.

Per: https://home.iatspayments.com/developer-info/reject-codes/credit-card-reject-codes-north-america/ -- it definitely sounds like that's a legitimately declined card -- i.e. a failure (code 15).

@adixon
Copy link
Contributor

adixon commented Mar 20, 2020

Thanks for digging into this so thoroughly. I believe you are correct about the origin of the problem.

I've created a branch with a small fix, here:
https://github.com/iATSPayments/com.iatspayments.civicrm/tree/recur-failure-handling-301

The key error as you identified it is that the code is not populating the $result['auth_code'] on failure.

It used to, this is a result of my refactoring of this bit of code for the new processor in the 1.7.x series.

Are you able to try patching your code with this to see if it fixes it?
https://github.com/iATSPayments/com.iatspayments.civicrm/compare/recur-failure-handling-301?expand=1#diff-0c3cf93d138a223fa4b558283b452008)

I was able to reproduce your experience and the above patch resolved it. It reclaimed the pending contributions as new failed ones, so you might end up with undocumented failed transactions (not the worst thing in the world).

@lcdservices
Copy link
Contributor Author

Thanks -- I've implemented that and will monitor and report back once we see some General decline trxns get processed.

@adixon
Copy link
Contributor

adixon commented Oct 27, 2020

Patch merged.

@adixon adixon closed this as completed Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants