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

[ready for core team review] CRM-20677, used generalized function to retrieve financial account id #10463

Merged
merged 3 commits into from Jul 14, 2017

Conversation

pradpnayak
Copy link
Contributor



return $paymentInstrumentValue ? self::$financialAccount[$paymentInstrumentValue] : self::$financialAccount;
public static function getInstrumentFinancialAccount($paymentInstrumentValue) {
$paymentInstrument = civicrm_api3('OptionValue', 'getsingle', array(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pradpnayak this is what is causing the test to fail would there be a reason why there is 2 payment_instruments with the same option value? Maybe its a test problem

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 you are right and I have fixed that test failure. Thanks for bringing that glitch into notice :)

@monishdeb
Copy link
Member

Jenkin test this please

Copy link
Member

@monishdeb monishdeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, working fine. There's enough unit test coverage in core to track any unintended regression due to this optimization.

@monishdeb monishdeb changed the title CRM-20677, used generalized function to retrieve financial account id [ready for core team review] CRM-20677, used generalized function to retrieve financial account id Jul 4, 2017
pradpnayak and others added 3 commits July 11, 2017 18:08
----------------------------------------
* CRM-20677: Use generalised function to retrieve financial account
  https://issues.civicrm.org/jira/browse/CRM-20677
----------------------------------------
* CRM-20677: Use generalised function to retrieve financial account
  https://issues.civicrm.org/jira/browse/CRM-20677

CRM-20677. more changes

----------------------------------------
* CRM-20677: Use generalised function to retrieve financial account
  https://issues.civicrm.org/jira/browse/CRM-20677
@colemanw colemanw merged commit 100d147 into civicrm:master Jul 14, 2017
*/
public static function getInstrumentFinancialAccount($paymentInstrumentValue = NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small note -- this was technically a change in the function signature (dropping the NULL default). Not a huge deal because it's an internal function, but there was a stale reference in sql/GenerateData.php (which is used for regen.sh) where it calls:

 $financialAccountId = CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount();

To work around it, I grepped forgetInstrumentFinancialAccount and saw a test written with the hard-coded value getInstrumentFinancialAccount(4). Not sure if it's the right thing, but using 4 does seem to get regen.sh running.

totten added a commit to totten/civicrm-core that referenced this pull request Jul 20, 2017
In [civicrm#10463](civicrm#10463), the
function signature of `getInstrumentFinancialAccount()` changed slightly -- in that
the parameter became mandatory.

Grepping for other calls to `getInstrumentFinancialAccount()` shows that one of the unit-tests
works with example input `4`, so this does the same.

Before
======

Running "regen.sh" fails -- because `getInstrumentFinancialAccount` is called without a parameter.

After
=====

Running "regen.sh" completes -- because `getInstrumentFinancialAccount` is called with `4`.

Acceptance Prompts
==================

 * The example data produced by `regen.sh` should contain suitable/similar values for financial records.
 * Determine whether `4` is a suitable value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants