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

Refactor api3 Payment.Get API to support options + most fields in civicrm_financial_trxn #17071

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 14, 2020

Overview

Alternative to #16603 for fix ups on Payment.get

Before

See #16603

After

See #16603

Technical Details

@mattwire I took a look at #16603 & came to the conclusion that Payment.get is basically FinancialTrxn.get with 3 extras

  1. enforced is_payment = 1
  2. support for passing in contribution_id
  3. contribution_id added to results array.

Given that I was able to reach a similar but slightly different result. Note that I concluded it's not logical to pass in any entity_table other than contribution, so I made that a non-parameter.

Also I removed 'In Progress' from the test as that's not a status we support for contributions (it got into contribution statuses from pledge statuses but there is no core meaning for it - I switched to Pending'

Comments

@civibot
Copy link

civibot bot commented Apr 14, 2020

(Standard links)

@civibot civibot bot added the master label Apr 14, 2020
@eileenmcnaughton eileenmcnaughton force-pushed the payment branch 2 times, most recently from 3a35151 to 4ae5a04 Compare April 14, 2020 00:53
@eileenmcnaughton eileenmcnaughton changed the title Refactor api3 Payment.Get API to support options + most fields in civ… Refactor api3 Payment.Get API to support options + most fields in civicrm_financial_trxn Apr 14, 2020
@eileenmcnaughton eileenmcnaughton force-pushed the payment branch 2 times, most recently from c88c6f1 to c3fbf06 Compare April 15, 2020 01:33
@mattwire
Copy link
Contributor

@eileenmcnaughton We need to add limit=0 to the call to EntityFinancialTrxn like this otherwise only the first 25 results have contribution IDs:

-    $entityFinancialTrxns = civicrm_api3('EntityFinancialTrxn', 'get', ['financial_trxn_id' => ['IN' => array_keys($financialTrxn)], 'entity_table' => 'civicrm_contribution'])['values'];
+    $entityFinancialTrxnParams = [
+      'financial_trxn_id' => ['IN' => array_keys($financialTrxn)],
+      'entity_table' => 'civicrm_contribution',
+      'options' => ['limit' => 0],
+    ];
+    $entityFinancialTrxns = civicrm_api3('EntityFinancialTrxn', 'get', $entityFinancialTrxnParams)['values'];

(this was also broken in my PR #16603)

Once that change is made and tests are passing on this PR it is ok to merge

@eileenmcnaughton eileenmcnaughton force-pushed the payment branch 2 times, most recently from 224c95b to 44363cd Compare April 27, 2020 06:27
…icrm_financial_trxn

Unit test for PR#16603

Alternative for fix ups on Payment.get
@eileenmcnaughton
Copy link
Contributor Author

@mattwire - I've updated it with the fix you pointed out. Going forwards I'm starting to use apiv4 rather than v3 calls - even within v3 api - so we have to switch from remembering limit to checkPermissions :-)

@mattwire mattwire merged commit 089e2d1 into civicrm:master Apr 27, 2020
@eileenmcnaughton eileenmcnaughton deleted the payment branch April 27, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants