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

CRM-20987, added batch transaction column #10789

Merged
merged 2 commits into from
Jul 31, 2017

Conversation

pradpnayak
Copy link
Contributor


Overview

Financial batch listing page lists transaction with transaction date derived from civicrm_financial_trxn table(column name = Received). This PR fixes

  • Renaming column name from Received to Transaction Date
  • Adding column Received derived from civicrm_contribution.receive_date

Before

before

After

batch transaction listing

----------------------------------------
* CRM-20987: Add transaction date field to listings of transactions
  https://issues.civicrm.org/jira/browse/CRM-20987
@eileenmcnaughton
Copy link
Contributor

test failure looks like code just needs adjusting

@@ -255,6 +255,7 @@ public static function getFinancialTransactionsList() {
3 => 'amount',
4 => 'trxn_id',
5 => 'transaction_date',
5 => 'receive_date',
Copy link
Contributor

Choose a reason for hiding this comment

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

You have double indexed here. Might need to renumber?

@eileenmcnaughton
Copy link
Contributor

The changed function is only accessed from the BatchTransction.tpl file - which relates to financial batches. I haven't tested this yet (maybe @lcdservices has) but the code side of it appears safe & ok except for my comment about array indexing

----------------------------------------
* CRM-20987: Add transaction date field to listings of transactions
  https://issues.civicrm.org/jira/browse/CRM-20987

fixed indexes
@pradpnayak
Copy link
Contributor Author

@eileenmcnaughton i have fixed the indexing.

@lcdservices
Copy link
Contributor

tested and confirmed. looks good.

@eileenmcnaughton
Copy link
Contributor

Adding merge on pass based on Brian's feedback

@pradpnayak
Copy link
Contributor Author

jenkins, retest this please

@monishdeb
Copy link
Member

Merging as per tag

@monishdeb monishdeb merged commit f3df3c4 into civicrm:master Jul 31, 2017
@monishdeb monishdeb deleted the CRM-20987 branch July 31, 2017 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants