don't allow multiple currencies in a batch

[MegaphoneJon]

https://lab.civicrm.org/dev/financial/-/issues/89

Overview

Adding multiple currencies to a batch results in dataTables errors when viewing a list of batches.

Steps to replicate

• Enable at least two currencies.
• Create two contributions of different currencies.
• Create a new accounting batch.
• Add both contributions to the batch.
• Close the batch.
• Go to Contributions menu » Accounting Batches » Closed Batches.

Before

dataTables error.

After

No dataTables error - but more importantly - you are no longer allowed to add a contribution that doesn't match the currency of existing transactions in the batch.

Comments

When I went to make this change, I saw a lot of code quality issues and ended up doing a fair amount of refactoring in the process:

• I extracted the SQL that gets a batch currency to its own method.
• The old SQL handled the possibility of multiple currencies, but in a way that generated an error. Since multiple currencies are no longer possible, I simplified the SQL.
• The validation specifying that the trxn's payment instrument must match the batch was being done at the AJAX level, and precluded other validations. I moved "payment instrument" validation to the BAO (alongside the new "currency" validation) and implemented exceptions. This also reduced the copy/paste between CRM_Financial_Page_AJAX::assignRemove() and CRM_Financial_Page_AJAX::bulkAssignRemove().

@JoeMurray you had this ticket assigned to yourself so I imagine you have some interest in this.

[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).

[MegaphoneJon]

Ugh - this test is broken, and likely never worked except by accident. api_v3_EntityBatchTest::setUp contains this as the default params for creating an EntityBatch record:

    $this->_entity = 'EntityBatch';
    $this->params = [
      'entity_id' => $this->contributionCreate($entityParams),
      'batch_id' => $this->batchCreate(),
      'entity_table' => 'civicrm_financial_trxn',
    ];

However, contributionCreate() returns a contribution ID. The entity_id in an EntityBatch record is a FinancialTrxn entity, not a Contribution.

[mlutfy]

jenkins, test this please

[eileenmcnaughton]

If you change this from [0] to ->first() at least one of the test fails will disappear (ie ->execute()->first()) trxn will be an empty array if nothing is returned - which may, or may not, cause e-notices lower down.

api\v4\Entity\ConformanceTest::testConformance with data set "EntityBatch" ('EntityBatch')
Undefined offset: 0

[eileenmcnaughton]

@MegaphoneJon at a high level I think the tests are failing because you assume the presence of required values - but for update that may not be true

[MegaphoneJon]

That makes a lot of sense, Eileen. I'll investigate that.

[eileenmcnaughton]

test this please

[MegaphoneJon]

test this please

[eileenmcnaughton]

@MegaphoneJon conflict :-(

[MegaphoneJon]

This finally passes tests.

One failure is because you can have entity_table equal civicrm_financial_trxn and entity_id equal x, where x isn't a valid record ID in civicrm_financial_trxn. I realize that's trickier to enforce, but is it impossible?

[eileenmcnaughton]

OK & I tested this & it works (after gaslighting myself that it didn't for a bit).

Note that the demo data set now has some non us currency transactions by default - #22438 makes it easier to spot the Canadian ones

[ufundo]

I think there's an issue with UK Gift Aid extension relating to this MR: https://lab.civicrm.org/extensions/ukgiftaid/-/issues/30#note_75288

The Gift Aid extension works by making batches of contributions - but the currency checks here seem to assume only ever adding financial_trxn to batches?

In particular this line https://github.com/MegaphoneJon/civicrm-core/blob/df33c0cd9bfa8c38aa8710a97f94ebb3da660a1a/CRM/Batch/BAO/EntityBatch.php#L42-L44 looks up a financial_trxn based on the entity_id -- but isn't it possible entity_table = civicrm_contribution and then the entity_id is a contribution_id? (And similarly in getBatchCurrency https://github.com/MegaphoneJon/civicrm-core/blob/df33c0cd9bfa8c38aa8710a97f94ebb3da660a1a/CRM/Batch/BAO/EntityBatch.php#L77 )

[MegaphoneJon]

Interesting @ufundo - I checked the entire core codebase and determined that EntityBatch only ever referenced FinancialTrxn records, but I've never used Gift Aid. Agreed that this code may need to be updated to account for this.

Admittedly, I suspect that it's Gift Aid that needs to be changed, not this code. It seems like batches of contributions rather than FinancialTrxn records is going to be inexact. E.g. how does Gift Aid handle a contribution with two (or more) line items of different financial types?

[ufundo]

Hi @MegaphoneJon - thanks for having a look!

Agree it feels like maybe Gift Aid should work on FinancialTrxn records rather than contributions, but may be a fair bit of work?

I don't know the logic of the design of EntityBatch with entity_table field, as opposed to a single-purpose " FinancialTrxnBatch "...

But as a stop-gap? #23741