fixing batch search by payment method

[tanyabouman]

Overview

This issue describes my problem, https://issues.civicrm.org/jira/browse/CRM-17090, except that I haven't seen any problems with the Transaction ID. This issue was previously fixed by changing contribution_payment_instrument_id to payment_instrument_id (30d46f2) and now it's fixed by changing it back to contribution_payment_instrument_id.

Before

A search by payment method in an accounting batch returns all of the contributions, regardless of the payment method.

After

A search by payment method in an accounting batch only returns the contributions matching the payment method.

Comments

I found the problem with Backdrop CMS 1.10.1 and CiviCRM 5.2.2

[civicrm-builder]

Can one of the admins verify this patch?

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

@civicrm-builder add to whitelist

[colemanw]

Hey @tanyabouman thanks and congrats on your first pull-request.

This is a tricky issue because of the back-n-forth commits (it was broken one way, so we change it, then it broke the other way, so we change it back). Typically when we get into a push-me-pull-me situation like this we need a unit test to prevent further regressions. Do you have any experience with doing those?

[tanyabouman]

No, I don't have much experience with that. What's a good place to get started?

[monishdeb]

Use

$this->contributionCreate([
  'contact_id' => $contactId,
      'total_amount' => 1,
      'payment_instrument_id' => CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'payment_instrument_id', 'Credit Card'),
      'financial_type_id' => 1,
      'contribution_status_id' => 1,
      'receive_date' => '20080523000000',
      'receipt_date' => '20080523000000',
      'trxn_id' => '22ereerwww323323',
      'id' => NULL,
      'fee_amount' => 0,
      'net_amount' => 1,
      'currency' => 'USD',
      'skipCleanMoney' => TRUE,
]);

instead.

[tanyabouman]

Good, except it gave me an error.
Failure in api call for contribution create: Duplicate error - existing contribution record(s) have a matching Transaction ID or Invoice ID. Contribution record ID(s) are: 1
So I added unique invoice ids as well.

[monishdeb]

Optimise it further:

$result = CRM_Batch_BAO_Batch::getBatchFinancialItems($entityId, $returnvalues, $notPresent, $params, TRUE)->fetchAll();
$this->assertEquals(count($result), 1, 'In line' . __LINE__);
$this->assertEquals($result[0]['payment_method'], CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'payment_instrument_id', 'Check'), 'In line' . __LINE__);

And you will no longer need L100-113

[tanyabouman]

Nice!

[monishdeb]

Nice work @tanyabouman

So the regression was caused by https://github.com/civicrm/civicrm-core/pull/11611/files#diff-0a639f0cf282805e238274495195e6a4R933 where the PI filter name was changed to contribution_payment_instrument from payment_instrument. We cannot revert the original change as we still need that patch to fix CRM-21665: Check Number show hide not working and thus this PR does what is needed.

As per the added UT, I am impressed how you have managed to add required dummy data and put a assertion on the new filter, to ensure the patch works. There are two optimisation changes which I mentioned in the UT.

[monishdeb]

Jenkins test this please

[twomice]

@monishdeb Is this ready for a PR review? I can do that if/when it's ready.

[monishdeb]

@twomice yes absolutely. Sorry for the delay. I earlier checked the patch and it does fix the issue and also the PR got a new unit test. But I would definitely need you to review this PR to ensure the fix. I'll then merge it :)

[twomice]

Reviewing this now.

[twomice]

@monishdeb In general this looks good for what it is. See my comments below in r-run.

• (r-explain) Pass
• (r-test) Pass
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Undecided:
  • The original issue report describes two problems, "Neither [a] the Payment Instrument in the optional constraints when you first create the batch nor [b] the Payment Method filter in the Search Criteria above the list of transactions has any effect."
  • I observed both problems in the demo site at dmaster.demo.civicrm.org/.
  • On the Jenkins test site for this PR, I observed that the PR does fix the second problem ([b] Payment Method filter in the Search Criteria) but not the first ([a] Payment Instrument in the optional constraints when you first create the batch).
  • I'm not sure if we should hold out for a fix for both issues; this PR does at least allow one to filter by Payment Method; but it doesn't solve the entire problem described in the ticket.
  • Frankly, I wonder if we could just eliminate the field in [a] completely. Why does it need to be there?
• (r-user) Pass
• (r-tech) Pass

[tanyabouman]

@twomice , thanks for pointing that out. I fixed the rest of the bug.

[twomice]

@tanyabouman That's great; I looked at the testing site and indeed the payment method field in the New Financial Batch form is working well too. Great work.

That said, I think we should have a unit test that covers this part of the functionality. Probably would mean adding a second, separate test method to CRM_Batch_BAO_BatchTest. Would you be able to add that to the PR?

[twomice]

@monishdeb I've verified that the PR now fixes both of the issues I mentioned in my review above. I recommend we go ahead and merge. I'll then add the unit test that I mentioned for the second fix (see my comment: #12707 (comment))

[twomice]

@monishdeb See my review and comments above. Is it possible to merge this as-is?

[monishdeb]

Sorry for delay. Merging now.

[monishdeb]

Thanks guys for all your work and getting this PR merged 👍