feat(accounts reports) report on transtions for multiple accounts #3159

jeremielodi · 2018-09-07T10:23:53Z

sfount

This looks like a good first pass on the multiple accounts report, great work!

My main feedback here is on code that can be shared between this report and the account statement report. It looks like around ~200 lines of code could be removed from this and shared by the reports in a library.

The issue also mentions a 'combined' or 'separated' flag for the reports presentation:

Ideally, it should be available on two presentations: combined and separated. The "combined" presentation will put all the transactions in the same table and add an extra column for the "account" so that we can determine from which account the transaction came from. The "separated" view would basically repeat the "Account Statement Report" multiple times for each account selected, one table after the other

This feature can be discussed in the issue proposal thread if it is not appropriate, otherwise it would be good to implement it to properly close the feature.

sfount · 2018-09-07T10:52:36Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+}
+
+/**
+ * @function getGeneralLedgerSQL


It looks like this method already exists in reportAccounts https://github.com/IMA-WorldHealth/bhima-2.X/blob/59a150ec96c18fa053e877e047761d03f34311d1/server/controllers/finance/reports/reportAccounts/index.js#L106

There is a useful CS principle known as Don't repeat yourself (or DRY). Keeping code in one place means that when we update features, text or fix bugs we can just change the core code and not have to search around.

Can this be moved to a shared library so that all accounts that use this logic can share the code? (Something like AccountsExtra)

sfount · 2018-09-07T10:53:56Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+
+// @TODO define standards for displaying and rounding totals, unless numbers are rounded
+//       uniformly they may be displayed differently from what is recorded
+function getTotalsSQL(options) {


https://github.com/IMA-WorldHealth/bhima-2.X/blob/59a150ec96c18fa053e877e047761d03f34311d1/server/controllers/finance/reports/reportAccounts/index.js#L172

(Same principle as comment on getGeneralLedgerSQL)

sfount · 2018-09-07T10:54:24Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+ * @description
+ * This function returns all the transactions for an account,
+ */
+function getAccountTransactions(options, openingBalance = 0) {


https://github.com/IMA-WorldHealth/bhima-2.X/blob/59a150ec96c18fa053e877e047761d03f34311d1/server/controllers/finance/reports/reportAccounts/index.js#L205

(Same principle as comment on getGeneralLedgerSQL)

sfount · 2018-09-07T10:57:45Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+/**
+ * @method document
+ *
+ * @description


It would be good to update this comment description to explain the difference between this and the accounts report.

sfount · 2018-09-07T11:05:23Z

server/models/bhima.sql

@@ -98,7 +98,8 @@ INSERT INTO unit VALUES
  (204, 'Exchange Rate','TREE.EXCHANGE','',1,'/modules/exchange/exchange','/exchange'),
  (205, 'Account Reference Management','TREE.ACCOUNT_REFERENCE_MANAGEMENT','',1,'/modules/account_reference','/account_reference'),
  (206, '[OHADA] Bilan','TREE.OHADA_BALANCE_SHEET','',144,'/modules/reports/ohada_balance_sheet_report','/reports/ohada_balance_sheet_report'),
-  (207, 'Account Reference Report','TREE.ACCOUNT_REFERENCE_REPORT','',144,'/modules/reports/account_reference','/reports/account_reference');
+  (207, 'Account Reference Report','TREE.ACCOUNT_REFERENCE_REPORT','',144,'/modules/reports/account_reference','/reports/account_reference'),


@jniles How should changes to core BHIMA tables be tracked for upgrading production servers?

Is bhima.sql run whenever and upgrade is performed? It does include things that may have changed like transaction_type. Otherwise is there a method of tracking changes that don't require a schema update?

Ugh. Right now we just manually upgrade things. I'm not sure of a better way, or how to make sure people respect the better way when writing new code. As a reviewer, I find it hard to remember to remind people to make upgrade scripts.

jeremielodi · 2018-09-10T09:06:58Z

@sfount This PR is ready for a review

jniles

There is a lot of code here, and most looks good. I've made some comments on some confusion that needs to be cleared up.

jniles · 2018-09-10T12:01:21Z

.env.development

@@ -5,7 +5,7 @@ PORT=8080
 DB_HOST='localhost'
 DB_USER='bhima'
 DB_PASS='HISCongo2013'
-DB_NAME='bhima_test'
+DB_NAME='imck'


This should be reset to bhima_test.

jniles · 2018-09-10T12:09:14Z

client/src/i18n/en/report.json

+      "TITLE": "Accounts Report",
+      "DESCRIPTION": "This report shows all transactions for a group of accounts given a set time period.",
+      "WARN_MULTIPLE": "N.B. This report concerns an income/ expense account and spans multiple fiscal years. Income/ expense accounts are cleared to 0 at the beginning of every fiscal year, the cumulative balance displayed is for the transactions within this period of time. It is recommended to use this statement within a single fiscal year.",
+      "WARN_CURRENCY": "Attention! You are using a currency other than the enterprise currency.  Values on this report are subject to change with changes to the exchange rate and should not be considered final.  Please save a report in the enterprise currency for long term reference."


If these keys are the same as the previous report, you can just re-use them. There is no need to repeat ourselves.

jniles · 2018-09-10T12:12:05Z

client/src/modules/reports/generate/account_report_multiple/account_report_multiple.config.js

+  const reportUrl = 'reports/finance/account_report_multiple';
+  vm.previewGenerated = false;
+  vm.accountIds = [];
+  delete cache.reportDetails;


Why do we need to delete the cache.reportDetails? Doesn't that defeat the purpose of a cache?

jniles · 2018-09-10T12:25:05Z

client/src/modules/reports/generate/account_report_multiple/account_report_multiple.config.js

+  delete cache.reportDetails;
+  vm.reportDetails = {
+    currency_id : Session.enterprise.currency_id,
+    selectedAccounts : [],


Shouldn't we save the accounts so that when a user comes back to this screen, they find the same accounts? It's how the other account report works. See below.

It's already like that

jniles · 2018-09-10T12:25:49Z

client/src/modules/reports/generate/account_report_multiple/account_report_multiple.config.js

+  const cache = new AppCache('configure_account_report_multiple');
+  const reportUrl = 'reports/finance/account_report_multiple';
+  vm.previewGenerated = false;
+  vm.accountIds = [];


Why do we need both vm.accountIds and vm.reportDetails.selectedAccounts? We should keep all the data in one array.

jniles · 2018-09-10T12:37:40Z

client/src/modules/reports/generate/account_report_multiple/account_report_multiple.html

+            <bh-currency-select
+              currency-id="ReportConfigCtrl.reportDetails.currency_id"
+              on-change="ReportConfigCtrl.setCurrency(currency)"
+              validation-trigger="ConfigForm.$submitted">


It looks like validationTrigger was removed from bh-currency-select. Please remove it here.

jniles · 2018-09-10T12:41:03Z

client/src/modules/reports/generate/account_report_multiple/account_report_multiple.config.js

+
+    vm.reportDetails.selectedAccounts = vm.reportDetails.selectedAccounts.filter(ac => {
+      return vm.accountIds.indexOf(ac.id);
+    });


What is this statement doing? It isn't clear to me.

Here we are using a select multiple; when a selected account is removed , this snipet of code will filter vm.reportDetails.selectedAccounts then it will be reduced.
You can see at the screen shot bellow how accounts are displayed

You can notice that I need in the ui-select both then label and the account number but when the user remove the selected account, the on-remove callback return just the id of the account

jniles · 2018-09-10T12:42:11Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+      const { selectedAccounts } = params;
+      const accounts = [].concat(selectedAccounts);
+      return q.all(accounts.map(account => {
+        const parseAccount = JSON.parse(account);


This statement tells me that you are not sending account IDs to the server, but JSON objects. You should be sending an array of one or more account IDs to the server. Like this:

GET some/report?account_ids=1,2,3

That way, there won't be any need to call JSON.parse on each value.

jniles · 2018-09-10T12:42:55Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+    })
+    .then(results => {
+      _.extend(bundle, { Alltransactions : results }, { params });
+      return Fiscal.getNumberOfFiscalYears(params.dateFrom, params.dateTo);


It doesn't look like you ever use Fiscal.getNumberOfFiscalYears(). Why did you call it?

jniles · 2018-09-10T12:44:08Z

server/models/bhima.sql

@@ -98,7 +98,8 @@ INSERT INTO unit VALUES
  (204, 'Exchange Rate','TREE.EXCHANGE','',1,'/modules/exchange/exchange','/exchange'),
  (205, 'Account Reference Management','TREE.ACCOUNT_REFERENCE_MANAGEMENT','',1,'/modules/account_reference','/account_reference'),
  (206, '[OHADA] Bilan','TREE.OHADA_BALANCE_SHEET','',144,'/modules/reports/ohada_balance_sheet_report','/reports/ohada_balance_sheet_report'),
-  (207, 'Account Reference Report','TREE.ACCOUNT_REFERENCE_REPORT','',144,'/modules/reports/account_reference','/reports/account_reference');
+  (207, 'Account Reference Report','TREE.ACCOUNT_REFERENCE_REPORT','',144,'/modules/reports/account_reference','/reports/account_reference'),


Ugh. Right now we just manually upgrade things. I'm not sure of a better way, or how to make sure people respect the better way when writing new code. As a reviewer, I find it hard to remember to remind people to make upgrade scripts.

jniles · 2018-09-14T11:36:24Z

Looks like this isn't quite done:

The account dropdown should have the account numbers on it. This is how all other account dropdowns work.

Fig 1: Account Dropdown Needs Numbers

The currency symbol should be rendered in the account header:

The opening balance needs to be in the same currency as what the user chose.

jniles · 2018-09-18T14:25:59Z

@jeremielodi, what is the status of this report?

jeremielodi · 2018-09-18T18:54:12Z

Ready to be merged

jniles · 2018-09-19T06:35:12Z

Could you fix the conflicts first? Also, are you sure you fixed the issues in the last review? It doesn't look like any changes were made.

mbayopanda

The list of accounts doesn't display the account number

The translation here could just be "Rapport des comptes"

🔥 There is not opening balance

The "Rapport de compte (simple)" has the opening balance for the selected account but not the "Rapport des comptes (multiple)"

"Rapport de compte (simple)" : for 57110011
"Rapport de compte (multiple)" : for 57110011

mbayopanda · 2018-09-19T08:26:41Z

client/src/i18n/en/report.json

@@ -109,6 +109,10 @@
      "WARN_MULTIPLE": "N.B. This report concerns an income/ expense account and spans multiple fiscal years. Income/ expense accounts are cleared to 0 at the beginning of every fiscal year, the cumulative balance displayed is for the transactions within this period of time. It is recommended to use this statement within a single fiscal year.",
      "WARN_CURRENCY": "Attention! You are using a currency other than the enterprise currency.  Values on this report are subject to change with changes to the exchange rate and should not be considered final.  Please save a report in the enterprise currency for long term reference."
    },
+    "REPORT_ACCOUNTS_MULTIPLE": {
+      "TITLE": "Multiple Accounts Report",
+      "DESCRIPTION": "This report shows all transactions for a group of accounts given a set time period."


What about : This report displays all transactions of a group of accounts for a given period

mbayopanda · 2018-09-19T08:42:16Z

client/src/i18n/fr/tree.json

@@ -83,6 +83,7 @@
    "PURCHASE_REGISTRY":"Registre des achats",
    "REPORTS":"Rapports",
    "REPORT_ACCOUNTS":"Rapport de comptes",
+    "REPORTS_MULTIPLE_ACCOUNTS" : "Rapport de comptes mutiple",


You would say multiples instead of mutiple.

Also, i think the correct phrase could just be : "Rapport des comptes"

mbayopanda · 2018-09-19T08:43:19Z

client/src/i18n/fr/tree.json

@@ -83,6 +83,7 @@
    "PURCHASE_REGISTRY":"Registre des achats",
    "REPORTS":"Rapports",
    "REPORT_ACCOUNTS":"Rapport de comptes",


This translation could be updated to "Rapport de compte" in singular

Also seeing that Rapport de compte and Rapport des comptes are close in the navigation tree, it can be a bit confusing for users, so we can add a suffixes (simple and multiple) to differentiate them :

"Rapport de compte" => "Rapport de compte (simple)", in the report page we just keep "Rapport de compte"

"Rapport des comptes" => "Rapport des comptes (multiple)", in the report page we just keep "Rapport des comptes"

jniles

Just a few more things to clean up.

jniles · 2018-09-25T15:23:49Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+      }));
+    })
+    .then(openginBalances => {
+      openginBalances.forEach((balance, index) => {


It's easier to read if you change this variable name from openginBalances to openingBalances.

jniles · 2018-09-25T15:24:22Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+      }));
+    })
+    .then(_accountTransactions => {
+      accountTransactions = _accountTransactions;


This variable is useless. Do ahead and just call the parameter accountTransactions.

No it's used in several places.

Ah, I see it now. I wasn't expecting it to be used outside of it's context.

jniles · 2018-09-25T15:25:37Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+      openginBalances.forEach((balance, index) => {
+        accountTransactions[index].header = balance;
+      });
+      _.extend(bundle, { Alltransactions : accountTransactions }, { params });


Usually only class names start with a capital letter. Could you rename this variable allTransactions?

jniles · 2018-09-25T15:26:56Z

server/controllers/finance/reports/reportAccountsMultiple/index.js

+  let accountTransactions = []; // list of all account containing transactions
+  const accountIDs = [].concat(params.accountIds);
+
+  const accounts = await q.all(accountIDs.map(id => {


Nice use of await. We could apply it to the entire function, but it isn't necessary here.

jniles · 2018-09-25T15:33:03Z

server/controllers/finance/reports/reportAccountsMultiple/report.handlebars

+          <th></th>
+
+          <th class="text-right">
+            {{debcred header.exchangedBalance ../params.currency_id }}


For some reason, this doesn't reflect the same as the normal account statement in the "balance" column. Can you check what is going on?

This version works

This version isn't correct

jniles

Good enough!

bors r+

3159: feat(accounts reports) report on transtions for multiple accounts r=jniles a=jeremielodi closes #3103 Co-authored-by: jeremielodi <[email protected]>

bors · 2018-09-27T14:22:57Z

Build succeeded

continuous-integration/travis-ci/push

jeremielodi force-pushed the multiple_account_statement branch from 40787ef to 7288500 Compare September 7, 2018 10:25

jniles requested a review from sfount September 7, 2018 10:35

jeremielodi force-pushed the multiple_account_statement branch from 7288500 to 3a4b583 Compare September 7, 2018 10:55

sfount reviewed Sep 7, 2018

View reviewed changes

jeremielodi force-pushed the multiple_account_statement branch from 3a4b583 to 1aefd1e Compare September 10, 2018 07:54

jniles requested changes Sep 10, 2018

View reviewed changes

jeremielodi force-pushed the multiple_account_statement branch 3 times, most recently from d5b2834 to 26509fc Compare September 14, 2018 08:59

jniles requested a review from mbayopanda September 19, 2018 07:40

jeremielodi force-pushed the multiple_account_statement branch from 26509fc to c98220b Compare September 19, 2018 09:08

mbayopanda reviewed Sep 19, 2018

View reviewed changes

jeremielodi force-pushed the multiple_account_statement branch 3 times, most recently from f78574d to 9c46d1c Compare September 24, 2018 08:51

jniles reviewed Sep 25, 2018

View reviewed changes

jeremielodi force-pushed the multiple_account_statement branch from 9c46d1c to ed85bea Compare September 26, 2018 08:24

feat(accounts reports) report on transtions for multiple accounts

0fda952

jeremielodi force-pushed the multiple_account_statement branch from ed85bea to 0fda952 Compare September 27, 2018 10:32

jniles approved these changes Sep 27, 2018

View reviewed changes

bors bot added a commit that referenced this pull request Sep 27, 2018

Merge #3159

61e09ac

3159: feat(accounts reports) report on transtions for multiple accounts r=jniles a=jeremielodi closes #3103 Co-authored-by: jeremielodi <[email protected]>

jniles added the Report label Sep 27, 2018

bors bot merged commit 0fda952 into Third-Culture-Software:master Sep 27, 2018

jeremielodi deleted the multiple_account_statement branch October 1, 2021 09:59

feat(accounts reports) report on transtions for multiple accounts #3159

feat(accounts reports) report on transtions for multiple accounts #3159

Conversation

jeremielodi commented Sep 7, 2018

sfount left a comment • edited Loading

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

jeremielodi commented Sep 10, 2018 • edited Loading

jniles left a 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

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

jniles commented Sep 14, 2018

jniles commented Sep 18, 2018

jeremielodi commented Sep 18, 2018

jniles commented Sep 19, 2018

mbayopanda left a 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

jniles left a 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

jniles left a comment

Choose a reason for hiding this comment

bors bot commented Sep 27, 2018

Build succeeded

sfount left a comment •

edited

Loading

jeremielodi commented Sep 10, 2018 •

edited

Loading