Fix cashflow report improve budget report #7900

lomamech · 2024-11-28T09:53:17Z

Fixed display error in the Cashflow report.
Added a new summary section to improve budget report.

closes #7897

lomamech · 2024-11-28T10:17:53Z

Here is a PR correcting the Display error on the Cashflow report and adding the summary section on the Budget report

jniles · 2024-11-28T18:45:22Z

Can you remove the yarn.lock file?

Also, your migration SQL file refers to tables that do not exist.

jniles

I haven't tested this yet with any live data, but here is a review of the code.

There are a few things that should be caught before submitting a PR, like the missing translation tokens, the removal of yarn.lock, and the migration file hard coding the name of a database.

There are a lot of decisions I didn't understand, such as changing the transaction type "TRANSFER" to income. That shouldn't have been done without an issue explaining why all the sites (Vanga, Sona Bata, Bandunduville) need to change their transfer transaction types to income. Then we could all discuss why this makes sense or doesn't make sense, but I'm alarmed that it was just slipped in without even a mention in the PR description.

client/src/i18n/en/form.json

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

jniles · 2024-11-29T21:20:18Z

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


    const fiscalYear = await Fiscal.lookupFiscalYear(fiscalYearId);

+    if (includeSummarySection) {
+      const cashboxesIds = _.values(params.cashboxesIds);
+      const dataCashboxes = {};


Why does dataCashboxes need to be an object? It seems like you could just work with the values directly.

You are right

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

jniles · 2024-11-29T22:01:11Z

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

+    let firstIncomeConfigurationReferences = 0;
+    let secondIncomeConfigurationReferences = 0;
+
+    if (includeSummarySection) {


Can you help me understand this block of code? Why are the first two lines being treated differently?

And where is the ORDER BY or sort() command that guarantees the order they come out in? Because it could be random.

This section is really a hack, Summary section is presented as follows

This section is really a hack, Summary section is presented as follows, Result equals the difference between the total of Income Total - Expenses Total, Result Without Accounts: : 71, 75, 77 and Result Without Accounts: : 71 should evaluate the result without the sum of the balance of the following accounts 71, 75 and 77 but the challenge I had was to obtain the balance of these specific accounts, that's why I had opted for using the account references by adding the Budget Analysis type account references and when creating the references I made the first grouping 71, 75 and 77 and the second 71 (also equivalent to grants)

I am worried this will break in the future without a deterministic ordering. Is there any way we can make sure this data is sorted in the correct display order before merging?

As an example, suppose that MySQL gets an update that makes the shorter group_concat() strings be put first. Or suppose a production server crashes and we need to restore it from backup, and it reorders the data. I think this should have some sorting/ordering to make sure that doesn't happen.

To improve things, I will make the summary section dynamic, so that the analysis of the result will no longer be limited to two account references and but only in relation to the configuration of the account references

referen

@jniles After trying to make the summary section dynamic, I encountered the following constraint: how to distinguish the value of the subsidies represented by account 71 in the configuration?

Because in the current configuration I have configured the account references so that the subsidies are associated with the second reference, while accounts 71, 75 and 77 are grouped under the first reference. After creating a new transaction type, I make sure that this section remains compliant with the report template.

What do you think of this hack or trick, if not we will have to find another way to distinguish the subsidy accounts

@lomamech that seems fine to me.

jniles · 2024-11-29T22:04:09Z

server/models/06-bhima.sql

@@ -332,7 +332,7 @@ INSERT IGNORE INTO `transaction_type` (`id`, `text`, `type`, `fixed`) VALUES
  (2, 'VOUCHERS.SIMPLE.CASH_PAYMENT', 'income', 1),
  (3, 'VOUCHERS.SIMPLE.CONVENTION_PAYMENT', 'income', 1),
  (4, 'VOUCHERS.SIMPLE.SUPPORT_INCOME', 'income', 1),
-  (5, 'VOUCHERS.SIMPLE.TRANSFER', 'other',  1),
+  (5, 'VOUCHERS.SIMPLE.TRANSFER', 'income',  1),


This is a breaking change, and we should talk about it.

Why are all transfers considered income? I think the majority of account to account transfers at Vanga (for example, from the auxiliary cashbox to the primary cashbox) are marked as "transfers". Same for transfers to and from the bank.

Why is a transfer an income?

Regarding the change of transfer type transaction, I had chosen to modify the type to allow the analysis of transfers coming from the auxiliary cash registers of the receipts, but since this will have repercussions on the other production sites, I have the following proposal: add in the interface of the cashflow report the possibility of considering the transfers as being receipts and the default value will be false., @jniles @jmcameron What do you think about this proposal which would resolve the IMCK problem without disturbing other production sites?

I think I'm okay with that hack as a solution. Alternatively, we could create a custom "cash transfer" transaction type for IMCK that has the type_id "income" that IMCK could start to use for caisse-to-caisse transfers instead and then it would show up here.

Ultimately, I think this speaks to a larger issue in BHIMA - we don't separate "cash" and "unrealized revenue" very well. I thought we had an issue about this, but I cannot find it. We probably should model this in the account_type or account_category which would have greater refinement of the type of transaction. Instead of simply having "asset", "liability", etc, as our account types, we would have sub-categories, such as "bank", "credit card", "caisse", "mpesa", all of which are subcategories of the "cash" account. Then, instead of analyzing transaction types, we would be looking at flows into and out of the cash accounts, which I think is likely the spirit of the cashflow report.

Of course, this would mean that all institutions would need to classify their chart of accounts correctly, but that seems like a good requirement to have.

But regarding the cashflow report, I just think to add the option to consider the transfer in the income category, like that we will not need to add new types of transaction_type

Yes, I'm okay with this for IMCK.

server/models/migrations/next/migrate.sql

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

jniles

This is much better, thank you!

I am still concerned about the sorting issue. Is there anyway we can guarantee the output sort?

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

jniles · 2024-11-30T15:11:28Z

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

+    let firstIncomeConfigurationReferences = 0;
+    let secondIncomeConfigurationReferences = 0;
+
+    if (includeSummarySection) {


I am worried this will break in the future without a deterministic ordering. Is there any way we can make sure this data is sorted in the correct display order before merging?

As an example, suppose that MySQL gets an update that makes the shorter group_concat() strings be put first. Or suppose a production server crashes and we need to restore it from backup, and it reorders the data. I think this should have some sorting/ordering to make sure that doesn't happen.

jniles · 2024-11-30T15:45:31Z

server/models/06-bhima.sql

@@ -332,7 +332,7 @@ INSERT IGNORE INTO `transaction_type` (`id`, `text`, `type`, `fixed`) VALUES
  (2, 'VOUCHERS.SIMPLE.CASH_PAYMENT', 'income', 1),
  (3, 'VOUCHERS.SIMPLE.CONVENTION_PAYMENT', 'income', 1),
  (4, 'VOUCHERS.SIMPLE.SUPPORT_INCOME', 'income', 1),
-  (5, 'VOUCHERS.SIMPLE.TRANSFER', 'other',  1),
+  (5, 'VOUCHERS.SIMPLE.TRANSFER', 'income',  1),


I think I'm okay with that hack as a solution. Alternatively, we could create a custom "cash transfer" transaction type for IMCK that has the type_id "income" that IMCK could start to use for caisse-to-caisse transfers instead and then it would show up here.

Ultimately, I think this speaks to a larger issue in BHIMA - we don't separate "cash" and "unrealized revenue" very well. I thought we had an issue about this, but I cannot find it. We probably should model this in the account_type or account_category which would have greater refinement of the type of transaction. Instead of simply having "asset", "liability", etc, as our account types, we would have sub-categories, such as "bank", "credit card", "caisse", "mpesa", all of which are subcategories of the "cash" account. Then, instead of analyzing transaction types, we would be looking at flows into and out of the cash accounts, which I think is likely the spirit of the cashflow report.

Of course, this would mean that all institutions would need to classify their chart of accounts correctly, but that seems like a good requirement to have.

server/models/migrations/next/migrate.sql

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

lomamech · 2024-12-01T04:38:35Z

@jniles @jmcameron
I just made major changes in this PR, I just restored the transaction type transfer as other transaction and not as an income and I added the option Consider transfer movements as revenue. on the Cashflow report interface and it works very well

jniles

Code looks okay to me. I'll wait for @jmcameron to provide his feedback as well, since he is more familiar with the budget work at IMCK.

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

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

jniles · 2024-12-03T14:22:15Z

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

@@ -381,6 +383,22 @@ function report(req, res, next) {
      return db.exec(queryRun, paramsRun);
    })
    .then(rows => {
+      // When the isTransferAsRevenue option is enabled,


Maybe add a FIXME that this is an IMCK-specific hack.

@lomamech I encourage you to add an issue about this so we can generalize it later -- or base it on some environment variable that can be set for IMCK and ignored by other sites.

jmcameron

@lomamech I encountered an error in basic tests (npm run test or npm run build:db). While building the database, complains:

[build] default data
mysql: [Warning] Using a password on the command line interface can be insecure.
mysql: [Warning] Using a password on the command line interface can be insecure.
ERROR 1064 (42000) at line 351: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'INSERT IGNORE INTO `flux` VALUES
  (1,  'STOCK_FLUX.FROM_PURCHASE'),
  (2,  'STO' at line 11
failed to import default data into DB 2/2
failed to build DB

I'm about to merge more PRs, so please merge with master again after you fix this issue.

Thanks

lomamech · 2024-12-05T10:34:29Z

@lomamech I encountered an error in basic tests (npm run test or npm run build:db). While building the database, complains:
[build] default data
mysql: [Warning] Using a password on the command line interface can be insecure.
mysql: [Warning] Using a password on the command line interface can be insecure.
ERROR 1064 (42000) at line 351: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'INSERT IGNORE INTO `flux` VALUES
  (1,  'STOCK_FLUX.FROM_PURCHASE'),
  (2,  'STO' at line 11
failed to import default data into DB 2/2
failed to build DB
I'm about to merge more PRs, so please merge with master again after you fix this issue.

Thanks
@jmcameron I just encountered the same error, I will do an investigation

lomamech · 2024-12-05T13:19:02Z

@jmcameron I just updated my PR

jmcameron

I've started working through the code. I'll work on this more when the test issue below is resolved.

I encountered another testing problem. One of the end-to-end tests fails because you added an account reference type but did not update a related test. To reproduce the error, do this:
npx install playwright chromium
npm run test:e2e-1

lomamech · 2024-12-06T03:03:53Z

I encountered another testing problem. One of the end-to-end tests fails because you added an account reference type but did not update a related test. To reproduce the error, do this:

I just did it

jmcameron

I reviewed the code a couple of times. I'm satisfied with it, but I request that @jniles take one more look at it to see if all the issues he brought up have been addressed.

I tested it with the IMCK database and the budget report seems to work well. I was not completely sure how to reproduce the Cash flow report error, but I tested it and it seemed to work correctly.

All tests pass on my local test server.

@lomamech One suggestion for an improvement to the budget report.
At the end of the summary section, you list the Cashbox accounts but you use the phrase (English):
"This report concerns these accounts"
Since many accounts are shown in the report, this is confusing. I suggest you to change this to:
"This report concerns these cashbox accounts"

@lomamech I will be merging in other bump PRs, so I encourage you to merge with master once you have completed any changes.

jniles

Changes look good to me. Thanks for all your work on this @lomamech !

jniles · 2024-12-10T20:41:39Z

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

@@ -16,6 +16,8 @@ function BudgetReportController($sce, Notify, SavedReports, AppCache, reportData
    filter : 'default',
  };

+  vm.reportDetails.include_summary_section = 0;


jniles · 2024-12-10T20:41:59Z

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

  vm.numberYears = [
    { id : 1 }, { id : 2 }, { id : 3 }, { id : 4 }, { id : 5 },
  ];

  vm.preview = function preview(form) {
    if (form.$invalid) { return null; }

+    if (vm.reportDetails.include_summary_section === 0) {


jniles · 2024-12-10T20:43:51Z

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

+    let firstIncomeConfigurationReferences = 0;
+    let secondIncomeConfigurationReferences = 0;
+
+    if (includeSummarySection) {


@lomamech that seems fine to me.

lomamech · 2024-12-11T08:21:44Z

How to Perform the Test with the BHIMA Test Database

Execute all integration tests: npm run test:integration.
Post all the entries in the posting journal to the general ledger.

To do this, you will need to unlock the following debtor groups:

Locked Debtor Group (Test)
Updated Debtor Group (Test)

After posting the entries to the general ledger, proceed as follows:

Navigate to the Administration menu and access the Account References module.
Add the following two references:

These references will enable the analysis of financial performance by calculating the total revenue minus the total expenses. Afterward, proceed with a specific analysis:

Results excluding revenue from non-medical activities: This step provides a clear view of purely medical performance by isolating non-medical revenue.
Results excluding grants: This evaluation offers insight into the hospital’s financial independence by excluding external contributions to measure its ability to generate local income.

To integrate this evaluation into the BHIMA system, we have added a new reference titled "Account Type Reference – Budget Analysis".

This reference is configured to:

Identify and group revenue related to non-medical activities.
Analyze the relationship between local own revenue and grants while calculating the balance by subtracting the total expenses.

Once the two account references are created and ordered such that the second account reference represents the total grants (this is a workaround), it enables the evaluation of local revenue excluding grants, the total grants, and the total revenue.

The budget report interface is structured as follows:
(Provide the layout or additional details about the interface, such as sections, tables, or specific fields, if available.)

To view the summary section, simply click on "Yes".

This will allow you to select a cashbox, particularly the main cashbox.

An Overview of the Report

lomamech · 2024-12-11T14:19:31Z

To be able to test Cashflow
These modules with the test database, it will first be necessary to create the grouping of account references related to the cash flow.

Accounting analysis:
The creation of this grouping is essential to ensure the consistency of financial data and to facilitate the analysis of cash flows. Accounts related to cash flow must be properly associated to guarantee an accurate overview of cash inflows and outflows. This also helps identify potential discrepancies in cash flow forecasts and effectively manage working capital needs.

Simply go to the Administration Menu and configure the following account groupings.

For groupings with reference types Expense Cashflow

Achat marchandises with accounts 601
Charges Administratif with accounts 605, 61, 62, 65
Charges personnel with accounts 66

4.Impôt et taxes with accounts 64

For groupings with reference types Income Cashflow

Ventes biens et services with accounts 701, 706, 58511010

Subventions with accounts 71
Autre produits with accounts 75, 77

After configuring the various account references, the groupings enable the visualization of the cash flow report with specific parameters, such as selecting "This Month." We have also added the option "Consider transfer movements as revenue". This feature allows transactions of the type "Other" to be treated as revenue, facilitating the tracking of funds transferred from auxiliary cash registers to the main cash register.

Here is a preview of the report without considering transfers as revenue.

Here is a preview of the report with transfers considered as revenue.

lomamech · 2024-12-11T14:21:20Z

@jniles @jmcameron I have just completed the description on how to test my latest PR using the Test Database. For this, I had to make several additions to the test database.

jniles · 2024-12-11T16:47:53Z

It looks like this breaks the end to end tests for the posting journal edits, unfortunately. I'll try to look into this next week if you don't get to it before.

jmcameron · 2024-12-11T16:52:49Z

@lomamech Sorry this PR is taking a while to merge!

I think that some of the added items have caused some of the end-to-end tests to fail.

In npm run test:e2e-2:

  1) [chromium] › test/end-to-end/editJournal/edit_journal.spec.js:72:3 › Edit Posting Journal › edits a transaction to change the value of debit and credit

    Error: Expected locator #validation-errored-alert to not exist.

    expect(received).toBe(expected) // Object.is equality

    Expected: false
    Received: true

       at test/end-to-end/shared/TestUtils.js:215

      213 |     return expect(count > 0,
      214 |       `Expected locator ${selector} to ${bool ? '' : 'not '}exist.`,
    > 215 |     ).toBe(bool);
          |       ^
      216 |   },
      217 |
      218 |   /**
        at Object.exists (/home/jmcameron/Bhima/test/end-to-end/shared/TestUtils.js:215:7)
        at /home/jmcameron/Bhima/test/end-to-end/editJournal/edit_journal.spec.js:85:5

In npm run test:e2e-3:

  1) [chromium] › test/end-to-end/journal/SearchModal.tests.js:59:3 › Posting Journal › Search Tests › finds 0 rows for transfer transaction type

    Error: expect(received).toBe(expected) // Object.is equality

    Expected: 0
    Received: 2

       at test/end-to-end/shared/GridUtils.js:122

      120 |     expect(number.includes(rows.length), message);
      121 |   } else {
    > 122 |     expect(rows.length, message).toBe(number);
          |                                  ^
      123 |   }
      124 |
      125 |   // ADAPTED FROM PROTRACTOR CODE:
        at Object.expectRowCount (/home/jmcameron/Bhima/test/end-to-end/shared/GridUtils.js:122:34)
        at /home/jmcameron/Bhima/test/end-to-end/journal/SearchModal.tests.js:62:5

The rest of the end-to-end tests pass. Please fix these tests.

- Added a new summary section to improve budget report. closes Third-Culture-Software#7897

lomamech · 2024-12-13T18:25:01Z

@jmcameron @jniles all tests pass

jniles · 2024-12-14T14:01:58Z

I am still getting test errors using npm run test:e2e-all. I'll work on a fix for this PR today.

jniles · 2024-12-14T14:42:50Z

Okay, I think I fixed it.

@jmcameron see if it works on your machine.

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch 2 times, most recently from 10b12c4 to 3820350 Compare November 28, 2024 10:15

lomamech requested review from jmcameron and jniles November 28, 2024 10:15

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch 2 times, most recently from 7bcae6e to 19daa52 Compare November 29, 2024 03:15

jniles requested changes Nov 29, 2024

View reviewed changes

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch from 19daa52 to 8b6afb8 Compare November 30, 2024 13:40

jniles reviewed Nov 30, 2024

View reviewed changes

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch from 8b6afb8 to 273a302 Compare December 1, 2024 04:32

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch from 273a302 to 7abfeb1 Compare December 2, 2024 19:33

jniles reviewed Dec 3, 2024

View reviewed changes

jmcameron requested changes Dec 4, 2024

View reviewed changes

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch from 7abfeb1 to c490269 Compare December 5, 2024 10:16

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch 2 times, most recently from 8820f4f to fc5e060 Compare December 5, 2024 12:55

jmcameron requested changes Dec 5, 2024

View reviewed changes

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch from fc5e060 to a205d0c Compare December 6, 2024 03:01

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch from a205d0c to 077ef7e Compare December 6, 2024 03:06

jmcameron approved these changes Dec 10, 2024

View reviewed changes

jniles approved these changes Dec 10, 2024

View reviewed changes

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch 2 times, most recently from d05f4f5 to 305cb41 Compare December 11, 2024 09:11

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch 2 times, most recently from f8d65e0 to 0a80338 Compare December 11, 2024 20:14

- Fixed display error in the Cashflow report.

edb6294

- Added a new summary section to improve budget report. closes Third-Culture-Software#7897

lomamech force-pushed the fix_cashflow_report_improve_budget_report branch from 0a80338 to edb6294 Compare December 13, 2024 18:22

jniles added Report Feature Request labels Dec 14, 2024

test: ensure posting journal edit tests pass

b2ce01b

Fix cashflow report improve budget report #7900

Are you sure you want to change the base?

Fix cashflow report improve budget report #7900

Conversation

lomamech commented Nov 28, 2024

lomamech commented Nov 28, 2024

jniles commented Nov 28, 2024

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

lomamech Nov 30, 2024 • 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

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

lomamech commented Dec 1, 2024 • 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

jmcameron left a comment

Choose a reason for hiding this comment

lomamech commented Dec 5, 2024 • edited Loading

lomamech commented Dec 5, 2024

jmcameron left a comment

Choose a reason for hiding this comment

lomamech commented Dec 6, 2024

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

lomamech commented Dec 11, 2024 • edited Loading

lomamech commented Dec 11, 2024

lomamech commented Dec 11, 2024

jniles commented Dec 11, 2024

jmcameron commented Dec 11, 2024

lomamech commented Dec 13, 2024

jniles commented Dec 14, 2024

jniles commented Dec 14, 2024

lomamech Nov 30, 2024 •

edited

Loading

lomamech commented Dec 1, 2024 •

edited

Loading

lomamech commented Dec 5, 2024 •

edited

Loading

lomamech commented Dec 11, 2024 •

edited

Loading