fix https://issues.civicrm.org/jira/browse/CRM-19149 #8982

wahyukodar · 2016-09-04T10:50:38Z

Issue on JIRA https://issues.civicrm.org/jira/browse/CRM-19149

civicrm-builder · 2016-09-04T10:50:41Z

Can one of the admins verify this patch?

totten · 2016-09-05T19:56:02Z

jenkins, ok to test

JoeMurray · 2016-09-06T17:53:28Z

@wahyukodar please fix failing tests
@pradpnayak please QA

seamuslee001 · 2016-09-06T20:36:26Z

@wahyukodar Wahyu can you please do git rebase -i core repo alias/master and only pick commits for this PR

wahyukodar · 2016-09-07T03:37:52Z

@seamuslee001 Hi, can you help me what command to write on the terminal?

seamuslee001 · 2016-09-07T03:44:58Z

@wahyukodar Wahyu

What you want to do is in your terminal cd to the civicrm root.
Then do git fetch alias for civicrm core repo if you have it otherwise add another origin civicrm pointing to civicrm core

then do git rebase -i civirepo-alias/master and then follow the prompts. as per the docs here https://git-scm.com/docs/git-rebase

wahyukodar · 2016-09-07T04:21:07Z

@seamuslee001 Hi, I'm done git rebase, but it's still fail test.. Alert in jenkins "No report files were found. Configuration error".. What should I do?

seamuslee001 · 2016-09-07T04:22:04Z

@wahyukodar can you do another git rebase but when you get the list of commits ONLY include the ones relevant to this Pull and no merge commits.

wahyukodar · 2016-09-07T04:30:19Z

@seamuslee001 , when I run git rebase -i civirepo/master, I get

pick 122250e fix issue https://issues.civicrm.org/jira/browse/CRM-19149
pick 34f6cee CRM-19275 - DB Error on adding priceset when deferred revenue setting is enabled
pick b97c9c4 English grammar edit
pick b86587c CRM-19290 Fix date creation in Karma test
pick c151a36 fix issue https://issues.civicrm.org/jira/browse/CRM-19149
pick 0aa19c5 CRM-19290 Fix date creation in Karma test
pick 8d88ae4 fix https://issues.civicrm.org/jira/browse/CRM-19149
pick d84d83f fix https://issues.civicrm.org/jira/browse/CRM-19149
pick 6861dfe fix https://issues.civicrm.org/jira/browse/CRM-19149
pick 5db0520 fixed unit test
pick c67d5d7 fixed unit test

seamuslee001 · 2016-09-07T04:31:57Z

ok so what you want to do is to do

pick 122250e fix issue https://issues.civicrm.org/jira/browse/CRM-19149
#pick 34f6cee CRM-19275 - DB Error on adding priceset when deferred revenue setting is enabled
#pick b97c9c4 English grammar edit
#pick b86587c CRM-19290 Fix date creation in Karma test
pick c151a36 fix issue https://issues.civicrm.org/jira/browse/CRM-19149
#pick 0aa19c5 CRM-19290 Fix date creation in Karma test
pick 8d88ae4 fix https://issues.civicrm.org/jira/browse/CRM-19149
pick d84d83f fix https://issues.civicrm.org/jira/browse/CRM-19149
pick 6861dfe fix https://issues.civicrm.org/jira/browse/CRM-19149
pick 5db0520 fixed unit test
pick c67d5d7 fixed unit test

seamuslee001 · 2016-09-07T04:32:33Z

so add in the # where i have suggested and then do git push origin master -f

wahyukodar · 2016-09-07T04:38:16Z

I can't push, this log my terminal

Wahyu-MacBook-Air:civicrm-core wahyu$ git push origin master -f
Everything up-to-date
Wahyu-MacBook-Air:civicrm-core wahyu$ git status
interactive rebase in progress; onto 87881da
Last commands done (4 commands done):
pick 8d88ae4 fix https://issues.civicrm.org/jira/browse/CRM-19149
pick d84d83f fix https://issues.civicrm.org/jira/browse/CRM-19149
(see more in file .git/rebase-merge/done)
Next commands to do (3 remaining commands):
pick 6861dfe fix https://issues.civicrm.org/jira/browse/CRM-19149
pick 5db0520 fixed unit test
(use "git rebase --edit-todo" to view and edit)
You are currently rebasing branch 'master' on '87881da'.
(all conflicts fixed: run "git rebase --continue")

nothing to commit, working directory clean

seamuslee001 · 2016-09-07T04:42:20Z

ok do git commit --allow-empty and then git rebase --continue

wahyukodar · 2016-09-07T04:44:35Z

git rebase --continue error

git commit --allow-empty
[detached HEAD e723092] fix https://issues.civicrm.org/jira/browse/CRM-19149
Date: Sun Sep 4 17:47:21 2016 +0700
Wahyu-MacBook-Air:civicrm-core wahyu$ git rebase --continue
error: could not apply 6861dfe... fix https://issues.civicrm.org/jira/browse/CRM-19149

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
Could not apply 6861dfe... fix https://issues.civicrm.org/jira/browse/CRM-19149

seamuslee001 · 2016-09-07T04:56:06Z

ok now do git mergetool and you should see some errors like <<< etc and remove and fix merge and then do git rebase --continue

seamuslee001 · 2016-09-07T08:15:16Z

nice work @wahyukodar :-)

wahyukodar · 2016-09-07T08:17:05Z

@seamuslee001 Thank you very much.. :)

tamarmeir · 2016-09-08T17:41:03Z

Tested, looks good, see screen shot of accounting batch:

Note, however, that the financial transaction still shown with the contribution is the original one for check even after updating the payment method to credit card - not sure why I'm not seeing the other two financial transactions

Looking at civicrm_financial_trx via api explorer, I also noticed that the positive transaction (id = 94) with payment instrument id = 4 is still showing a negative net amount - not sure what that would impact but I'm thinking for consistency's sake, it would be good if the net amount was updated as well:

{

"is_error":0,
"version":3,
"count":3,
"values":[{
    "id":"94",
    "to_financial_account_id":"6",
    "trxn_date":"2016-09-08 17:18:00",
    "total_amount":"-100.00",
    "fee_amount":"0.00",
    "net_amount":"-100.00",
    "currency":"USD",
    "is_payment":"1",
    "status_id":"1",
    "payment_instrument_id":"4"
},
{
    "id":"95",
    "to_financial_account_id":"12",
    "trxn_date":"2016-09-08 17:18:00",
    "total_amount":"100.00",
    "fee_amount":"0.00",
    "net_amount":"-100.00",
    "currency":"USD",
    "is_payment":"1",
    "status_id":"1",
    "payment_instrument_id":"4"
},
{
    "id":"96",
    "to_financial_account_id":"12",
    "trxn_date":"2016-09-08 17:18:00",
    "total_amount":"-100.00",
    "fee_amount":"0.00",
    "net_amount":"-100.00",
    "currency":"USD",
    "is_payment":"1",
    "status_id":"1",
    "payment_instrument_id":"1"
}]

}

Thanks in advance for your time in working on this and happy to test once the above is remedied or if you feel that a separate ticket should be created, please let me now and I will do so.

Thanks!

…ty_transaction record is created

Fix up errors in to_account_id and ensure that financial_item is enti…

seamuslee001 · 2016-09-09T10:27:48Z

@tamarmeir with the latest commits i think your problems are fixed

tamarmeir · 2016-09-14T23:12:20Z

Almost there - financial transactions associated with contribution look good:

But api explore is still showing issues with the net amount of the financial transactions created upon editing - this time, it's the negative credit card transaction (id = 96) that shows a positive amount in the "net amount" field:

"is_error":0,
"version":3,
"count":3,
"values":[{
    "id":"94",
    "to_financial_account_id":"6",
    "trxn_date":"2016-09-14 23:02:00",
    "total_amount":"-100.00",
    "fee_amount":"0.00",
    "net_amount":"-100.00",
    "currency":"USD",
    "is_payment":"1",
    "status_id":"1",
    "payment_instrument_id":"4"
},
{
    "id":"95",
    "to_financial_account_id":"6",
    "trxn_date":"2016-09-14 23:02:00",
    "total_amount":"100.00",
    "fee_amount":"0.00",
    "net_amount":"100.00",
    "currency":"USD",
    "is_payment":"1",
    "status_id":"1",
    "payment_instrument_id":"4"
},
{
    "id":"96",
    "to_financial_account_id":"12",
    "trxn_date":"2016-09-14 23:02:00",
    "total_amount":"-100.00",
    "fee_amount":"0.00",
    "net_amount":"100.00",
    "currency":"USD",
    "is_payment":"1",
    "status_id":"1",
    "payment_instrument_id":"1"
}]

}

seamuslee001 · 2016-09-15T03:54:56Z

CRM/Contribute/BAO/Contribution.php

-        $params['trxnParams']['to_financial_account_id'] = $params['to_financial_account_id'];
-        $params['trxnParams']['payment_instrument_id'] = $params['contribution']->payment_instrument_id;
+        if ($params['total_amount'] < 0) {
+          $params['trxnParams']['net_amount'] = $params['trxnParams']['total_amount'];


@wahyukodar Wahyu could you move this line up to L3645 i.e. just after the elseif, That should then solve the last issue found by Tamar

Line 3645? are you sure?

@wahyukodar good pickup i mean L3465 :-) reads closer

Ok, I will move thi

$params['trxnParams']['net_amount'] = $params['trxnParams']['total_amount'];
@seamuslee001

to L3465? Right?

tamarmeir · 2016-09-15T16:14:44Z

Houston, we have lift off with the financial transactions! Not sure why there is a result of "All checks failed" in the last merge build - I will leave it to you, though since you have effectively resolved this issue with the last changes made.

Thanks and kind regards,
Tamar

seamuslee001 · 2016-09-15T21:04:21Z

@totten @monishdeb I think this can be merged now tested pretty thoroughly

yashodha · 2016-09-16T10:36:01Z

Jenkins test this please

yashodha · 2016-09-16T11:25:02Z

unrelated failure

pradpnayak · 2016-09-16T12:28:33Z

CRM/Contribute/BAO/Contribution.php

+          $prevFinancialItem = CRM_Financial_BAO_FinancialItem::getPreviousFinancialItem($fieldValues['id']);
+          if (!CRM_Utils_Rule::currencyCode($trxn->currency)) {
+            $trxn->currency = CRM_Core_Config::singleton()->defaultCurrency;
+          }


I think line 3611-3613 is not required.

pradpnayak · 2016-09-16T12:29:13Z

CRM/Contribute/BAO/Contribution.php

+            'entity_id' => $prevFinancialItem->id,
+            'financial_trxn_id' => $trxn->id,
+            'amount' => $trxn->total_amount,
+            'currency' => $trxn->currency,


civicrm_entity_financial_trxn doesn't have currency column.

this is as per

civicrm-core/CRM/Core/BAO/FinancialTrxn.php

Line 69 in 2fa84df

$entityFinancialTrxnParams

yeah, its not required in either of the place. Can you remove them as cruft?

sure I'll do a tidy up PR

pradpnayak · 2016-09-16T12:29:31Z

CRM/Contribute/BAO/Contribution.php

+          );
+          $entityTrxn = new CRM_Financial_DAO_EntityFinancialTrxn();
+          $entityTrxn->copyValues($entityFinancialTrxnParams);
+          $entityTrxn->save();


Why are we not using api here?

probably better idea

pradpnayak · 2016-09-16T12:30:48Z

CRM/Contribute/BAO/Contribution.php

@@ -3592,6 +3604,28 @@ public static function updateFinancialAccounts(&$params, $context = NULL, $skipT
        }
      }
    }
+    if ($context == 'changePaymentInstrument') {


Is this as per the dataflow for accounting?
https://wiki.civicrm.org/confluence/display/CRM/CiviAccounts+Data+Flow#CiviAccountsDataFlow-ChangePaymentInstrumentforaPartlyPaidorCompletedcontribution

@pradpnayak all this is doing is adding in a linkage back to the financial_item without it the display on the contribution tab does not show all financial_trxns

yeah, agreed. Can you please update the wiki and also inform @JoeMurray about the changes?

sure will give it a shot @pradpnayak

pradpnayak · 2016-09-16T13:11:07Z

CRM/Contribute/BAO/Contribution.php

+        else {
+          $params['trxnParams']['to_financial_account_id'] = $params['to_financial_account_id'];
+          $params['trxnParams']['payment_instrument_id'] = $params['contribution']->payment_instrument_id;
+        }
      }
    }


@seamuslee001 line 3474-3481 and 3484-3492 looks duplicate. Can we re-write line from 3472-3492 as
if (!empty($lastFinancialTrxnId['financialTrxnId'])) {
if ($params['total_amount'] != $params['trxnParams']['total_amount']) {
$params['trxnParams']['to_financial_account_id'] = CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_FinancialTrxn', $lastFinancialTrxnId['financialTrxnId'], 'to_financial_account_id');
$params['trxnParams']['payment_instrument_id'] = $params['prevContribution']->payment_instrument_id;
}
else {
$params['trxnParams']['to_financial_account_id'] = $params['to_financial_account_id'];
$params['trxnParams']['payment_instrument_id'] = $params['contribution']->payment_instrument_id;
}
}
}

@pradpnayak see followup pr #9055

civicrm-builder added the master label Sep 4, 2016

fix issue https://issues.civicrm.org/jira/browse/CRM-19149

122250e

fix https://issues.civicrm.org/jira/browse/CRM-19149

e723092

seamuslee001 and others added 2 commits September 9, 2016 20:13

Fix up errors in to_account_id and ensure that financial_item is enti…

bf4385c

…ty_transaction record is created

Merge pull request #1 from seamuslee001/wahyukodar-master

62cd50b

Fix up errors in to_account_id and ensure that financial_item is enti…

seamuslee001 reviewed Sep 15, 2016

View reviewed changes

fix net amount api explore

b001aae

yashodha merged commit c5d6e3d into civicrm:master Sep 16, 2016

pradpnayak reviewed Sep 16, 2016

View reviewed changes

fix https://issues.civicrm.org/jira/browse/CRM-19149 #8982

fix https://issues.civicrm.org/jira/browse/CRM-19149 #8982

Conversation

wahyukodar commented Sep 4, 2016

civicrm-builder commented Sep 4, 2016

totten commented Sep 5, 2016

JoeMurray commented Sep 6, 2016

seamuslee001 commented Sep 6, 2016 • edited Loading

wahyukodar commented Sep 7, 2016

seamuslee001 commented Sep 7, 2016

wahyukodar commented Sep 7, 2016

seamuslee001 commented Sep 7, 2016

wahyukodar commented Sep 7, 2016

seamuslee001 commented Sep 7, 2016

seamuslee001 commented Sep 7, 2016

wahyukodar commented Sep 7, 2016

seamuslee001 commented Sep 7, 2016

wahyukodar commented Sep 7, 2016

seamuslee001 commented Sep 7, 2016

seamuslee001 commented Sep 7, 2016

wahyukodar commented Sep 7, 2016

tamarmeir commented Sep 8, 2016

seamuslee001 commented Sep 9, 2016

tamarmeir commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamarmeir commented Sep 15, 2016

seamuslee001 commented Sep 15, 2016

yashodha commented Sep 16, 2016

yashodha commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradpnayak Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Sep 6, 2016 •

edited

Loading

pradpnayak Sep 16, 2016 •

edited

Loading