CRM-21580 - fix exporting financial batch fails when label of batch statuses changed

[JO0st]

Overview

Only batches with the exported staus could be exported. The check wether a batch had the status exported was done based on the label of the batch status. I changed this to use the name (which shouldn't change) of the status

Before

Batches couldn't be exported if the label of the exported state has changed.

After

Batches can be exported if the label of the exported state has changed.

---

• CRM-21580: exporting financial batch fails when label of batch statuses changed

[civicrm-builder]

Can one of the admins verify this patch?

[eileenmcnaughton]

test this please

[eileenmcnaughton]

This is fine (although we should not merge until 4.7.30 rc is cut since we are keeping short-form arrays out until then) but a shortcut would be to use 'getvalue' rather than 'getsingle' - you would need to add the 'return' => 'value' param.

Also, the 'sequential' => 1 param does nothing here - it causes the values to be re-indexed starting from 0 - but getsingle & getvalue return from a lower part of the array

[eileenmcnaughton]

email fails are on formatting / coding standards - see https://docs.civicrm.org/dev/en/latest/standards/ for what we use

[JO0st]

I followed your hints according to the getvalue and fixed the styling errors.Thanks for mentioning the getValue api call. I had never used that before.

[seamuslee001]

Test this please

[seamuslee001]

@JO0st you need to add a , to the end of this line

[colemanw]

@civicrm-builder add to whitelist

[colemanw]

@JO0st do you know how to do a rebase to squash these 3 commits into one?

git rebase -1 d08f40c

Then put s for squash by the two extra commits leaving the original commit alone and run it. Then do:

git push -f

[colemanw]

Unrelated test failure. @civicrm-builder retest this please.

[colemanw]

@civicrm-builder retest this please.

[colemanw]

@civicrm-builder retest this please.

[JO0st]

Squashed the commits into 1 commit

[monishdeb]

@JO0st there's a alternate way to fetch 'Exported' status ID, here's is it:

diff --git a/CRM/Financial/Form/Export.php b/CRM/Financial/Form/Export.php
index e5d19dd..b58e57b 100644
--- a/CRM/Financial/Form/Export.php
+++ b/CRM/Financial/Form/Export.php
@@ -94,8 +94,7 @@ class CRM_Financial_Form_Export extends CRM_Core_Form {
       $this->_batchIds = $this->_id;
     }
 
-    $allBatchStatus = CRM_Core_PseudoConstant::get('CRM_Batch_DAO_Batch', 'status_id');
-    $this->_exportStatusId = CRM_Utils_Array::key('Exported', $allBatchStatus);
+    $this->_exportStatusId = CRM_Core_PseudoConstant::getKey('CRM_Batch_DAO_Batch', 'status_id', 'Exported');
 
     // check if batch status is valid, do not allow exported batches to export again
     $batchStatus = CRM_Batch_BAO_Batch::getBatchStatuses($this->_batchIds);

This fn does the same thing and eventually makes use of the pseudo information associated with a table column (see here) and fetch the corresponding option value.

[JO0st]

@monishdeb That works indeed. Is there a reason to use that over the API?

[monishdeb]

@JO0st bcoz its just 1 LOC against 2 :) Also a standard approach to fetch the key( or option value) by using the pseudo information of a table column. Likewise, you can also fetch the label by using the key via CRM_Core_PseudoConstant::getLabel('CRM_Batch_DAO_Batch', 'status_id', <export option value>)

[JO0st]

I used @monishdeb 's way of getting the status id in stead of the api

[colemanw]

• (r-jira) Pass
• (r-test) Pass
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Skip: I feel confident enough about this code to merge it without running. We've had multiple eyes on it and it's a simple straightforward change to use a standard convention.
• (r-user) Pass
• (r-tech) Pass