Add support for financial account custom fields

[MegaphoneJon]

Overview

The FinancialAccount entity almost but doesn't quite have support for custom fields.

Before

Can't save or display custom field data for financial accounts.

After

Works.

Technical Details

The BAO change is all that's necessary to support FinancialAccount custom fields in API4 - I almost put the rest in an extension, but the code is well-abstracted and so why not add support in core.

Comments

You'll need to add FinancialAccount to the cg_extend_objects option group. cv api OptionValue.create label='Financial Accounts' name='civicrm_financial_account' value='FinancialAccount' option_group_id='cg_extend_objects' will do the trick.

[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]

I think this minimal change to the BAO is ok and not a blocker to the PR, but FYI the more complete solution is the refactoring we've been doing to many (eventually all) of our BAOs which is to deprecate and gut the add() function, reducing it to one line that calls self::writeRecord() and move the rest of the pre/post logic to pre/post hooks.

[MegaphoneJon]

@colemanw Is there a simple entity that has implemented this change that can serve as a reference? I would also prefer to merge this now, but I'd be willing to redo it in a later PR, and ideally we can document it somewhere.

[colemanw]

This is a pretty simple example of such a refactor. The del() function contained some pre-logic which got moved to a self_pre hook:

42f0bf1

[eileenmcnaughton]

@MegaphoneJon possibly just this

diff --git a/CRM/Financial/BAO/FinancialAccount.php b/CRM/Financial/BAO/FinancialAccount.php
index 07198cb149..359f3b0de3 100644
--- a/CRM/Financial/BAO/FinancialAccount.php
+++ b/CRM/Financial/BAO/FinancialAccount.php
@@ -78,31 +78,7 @@ class CRM_Financial_BAO_FinancialAccount extends CRM_Financial_DAO_FinancialAcco
       $queryParams = [1 => [$params['financial_account_type_id'], 'Integer']];
       CRM_Core_DAO::executeQuery($query, $queryParams);
     }
-
-    // action is taken depending upon the mode
-    $financialAccount = new CRM_Financial_DAO_FinancialAccount();
-
-    // invoke pre hook
-    $op = 'create';
-    if (!empty($params['id'])) {
-      $op = 'edit';
-    }
-    CRM_Utils_Hook::pre($op, 'FinancialAccount', CRM_Utils_Array::value('id', $params), $params);
-
-    if (!empty($params['id'])) {
-      $financialAccount->id = $params['id'];
-      $financialAccount->find(TRUE);
-    }
-
-    $financialAccount->copyValues($params);
-    $financialAccount->save();
-
-    // invoke post hook
-    $op = 'create';
-    if (!empty($params['id'])) {
-      $op = 'edit';
-    }
-    CRM_Utils_Hook::post($op, 'FinancialAccount', $financialAccount->id, $financialAccount);
+    $financialAccount = self::writeRecord($params);
     CRM_Core_PseudoConstant::flush();
 
     return $financialAccount;

[MegaphoneJon]

Perfect, thank you. I'll bookmark this and try to implement it somewhere soon.

This makes me long for the in-person Civi events where this information would get shared, especially when we could use the sprint to give these techniques significant traction.

[colemanw]

I've tested this and it works without any errors. Agree that a followup is in order though.

[colemanw]

Here is a followup PR to cleanup the BAO: #23079

[eileenmcnaughton]

@MegaphoneJon it's starting to feel more feasible - there is at least a small sprint in Manchester in October...