refactor: Trial Balance #1233

jniles · 2017-02-21T09:47:53Z

EDIT See #1233 (comment) for the most recent changes.

This PR refactors the Trial Balance to improve stability and usability. It addresses a few of the issues cited in multiple issues. It also fixes a bug with the Trial Balance calculation of account balance changes.

Details
This commit improves the Trial Balance UI and UX in the following ways:

The submit button now disables and enters a loading state after
posting to the GL.
The grid now right aligns the currency amounts.
The grid now uses currency rendering for all amounts.
The buttons now behave responsively.
The labels no longer bleed into the headercrumb.

There are also some performance improvements:

Use flat entity access on the grids
Use fastWatch
Use translate directive where possible

Addresses some of the points in #1163. Closes #1171. Addresses some of the points in #1042.

Bug Fix
This commit fixes a bug in the totals calculation for the trial balance
due to an incorrect LEFT JOIN. This has been broken out into multiple
subqueries. The performance of these queries should be investigated,
but there are no obvious optimizations to make.

Bug Fix
The $timeout hacks have been removed from ui-grid and replaced with events that redraw the grid when grouping changes. These sometimes caused flaky behavior in rendering.

Closes #868.

Feature
Finally, this PR includes a grid footer on the Trial Balance to give an overview of the changes. This is how the bug was discovered. See below on Vanga's dataset.

Closes #1189.

Feature
The Trial Balance finally has a loading indicator!

Fig 1: Trial Balance Improvements

Thank you for contributing!

Before submitting this pull request, please verify that you have:

Run your code through ESLint. Check out our styleguide.
Run integration tests.
Run end-to-end tests.
Accurately described the changes your are making in this pull request.

For a more detailed checklist, see the official review checklist that this PR will be evaluated against.

Ensuring that the above checkboxes are completed will help speed the review process and help build a stronger application. Thanks!

jniles · 2017-02-21T09:54:46Z

Interested in @imawh's feedback as well.

jniles · 2017-02-21T11:04:18Z

Received feedback from @imawh. He proposes the following changes:

Remove the total footer on avant and apres columns - these make no sense.
Only show the footer if debits/credits imbalance. They should be balanced normally.
Make the Voir les Details text smaller/more compact.
Include the account label with the account number.
Remove the "Group by Transaction" button.

DedrickEnc · 2017-02-21T11:13:45Z

We should also ask Patrick, what he is thinking.

jniles · 2017-02-21T11:54:20Z

@sfount, can you see if Patrick has any feedback?

sfount · 2017-02-21T11:57:43Z

Patrick is not at the office right now. I understand that he is not in good health.
@patsonjunior

jniles · 2017-02-21T12:43:04Z

The following changes have landed:

This commit incorporates feedback from our accountant.

Grid Footer is only shown when there are imbalances and is colored red.
Totals are only ever shown for debit/credit and not for before/after.
The grid is no longer padded - slightly more screen real estate for users.
The account now includes the account text.
Removed Group By Transaction
Cancel button on detail page.
"View Details" is translated just as "details".

Screenshots

Fig 1: Trial Balance Valid State

Fig 2: Trial Balance Error State

This commit improves the Trial Balance UI and UX in the following ways: 1. The submit button now disables and enters a loading state after posting to the GL. 2. The grid now right aligns the currency amounts. 3. The grid now uses currency rendering for all amounts. 4. The buttons now behave responsively. 5. The labels no longer bleed into the headercrumb. There are also some performance improvements: 1. Use flat entity access on the grids 2. Use fastWatch 3. Use translate directive where possible Addresses some of the points in Third-Culture-Software#1163. Closes Third-Culture-Software#1171.

This commit fixes a bug in the totals calculation for the trial balance due to an incorrect LEFT JOIN. This has been broken out into multiple subqueries. The performance of these queries should be investigated, but there are no obvious optimizations to make.

This commit encorporates feedback from our accountant. 1. Grid Footer is only shown when there are imbalances. 2. Totals are only ever shown for debit/credit and not for before/after. 3. The grid is no longer padded - slightly more screen real estate for users. 4. The account now includes the account text. 5. Removed Group By Transaction

sfount

The code here LGTM. The changes to the SQL will make this a very robust procedure.

I have made a few comments however I don't think any of these things should be blocking.

I also have a number of comments on the UX experience for this module:

Is there a reason that a different blue/ red colour is used for primary and error states? We have no other grids that have a blue header so when and where we use this should be considered. If these should have colours we should use the standard primary and danger colours used throughout the application.
The submit button turns red if there are errors. A red button is used throughout the application for a serious action (usually deleting an entity) however this button can never be pressed and won't delete anything - I think this is misleading.
The view errors button is also red, this is the same point as (2.)

@jniles the code here looks to me like it can be merged into master. If you would like to make any comments/ changes based on this review there is no problem. If this has to be in as soon as possible let me know and we can address the additional comments in an issue thread.

sfount · 2017-02-22T10:26:47Z

client/src/partials/journal/modals/trialBalanceDetail.body.js

+    treeRowHeaderAlwaysVisible : false,
+    appScopeProvider           : vm,
+    columnDefs                 : columns,
+    flatEntityAccess           : true,


sfount · 2017-02-22T10:28:46Z

client/src/partials/journal/modals/trialBalanceMain.body.html

+  id="main-grid"
+  ui-grid="TrialBalanceMainBodyCtrl.gridOptions"
+  ui-grid-auto-resize
+  ui-grid-grouping>


Does this UI grid still need ui-grid-grouping ?

Yes to make the uiGridGroupingConstants tree aggregation work. I might be able to migrate this to uiGridConstants instead, but for the moment, that's the way it was set up. The grouping aggregation doesn't work without the ui-grid-grouping directive.

sfount · 2017-02-22T10:30:05Z

client/src/partials/journal/modals/trialBalanceMain.body.js

-  'SessionService', 'TrialBalanceService', 'GridGroupingService', 'GridColumnService',
-  'NotifyService', '$state', '$timeout', 'uiGridConstants',
+  'SessionService', 'TrialBalanceService', 'NotifyService',
+  '$state', 'uiGridConstants', 'uiGridGroupingConstants', '$filter',


Are grouping constants used any more? I think that functionality was removed with the view as transaction button. If they're no longer used all of these imports can be removed.

They are used in the aggregation functions.

sfount · 2017-02-22T10:33:24Z

client/src/partials/journal/modals/trialBalanceMain.body.js

-  fetchDataByAccount()
-    .then(function (data) {
-      vm.gridOptions.data = data;
+        cssClass = TrialBalance.getCSSClass(vm.feedBack);


Is there a reason these CSS classes can't be assigned in the template like in many other modules using ng-class? I haven't looked into the specifics but this logic is very specific and unknown to me. If it is required it would be a good thing to standardise for these cases.

It would be fairly straightforward. You just have to make a header row template and switch the color based on that.

Header template:

https://github.com/angular-ui/ui-grid/blob/master/src/templates/ui-grid/ui-grid-header.html

sfount · 2017-02-22T10:37:31Z

client/src/partials/journal/modals/trialBalanceMain.body.html

+  ui-grid="TrialBalanceMainBodyCtrl.gridOptions"
+  ui-grid-auto-resize
+  ui-grid-grouping>
+  <bh-grid-loading-indicator


jniles · 2017-02-22T12:51:21Z

@sfount, let's just get this in. It will be useful to teach the team here without the previous bugs and with a loading button. The others will just be UI issues and/or performance that we can address through more training when we make the upgrades.

Note: Travis CI isn't reporting it, but these tests pass.

patsonjunior · 2017-02-22T13:16:26Z

A mon avis, il faudra d'abord revoir la configuration informatique de ce "Trial Balance" du fait qu'en principe toutes les écritures qu'elles soient générées automatiquement ou passées manuellement dans le journal de saisie dévraient toujours être équilibrées.
Si après revision, l'erreur persiste, être alors capable de retouver le document ou la pièce qui soutient la transaction à partir de la colonne "Référence" qui se trouve dans le" journal de saisie" afin d'équilibrer la transaction du vrai montant, au lieu de le faire de façon imaginaire avec risque de surestimation ou de sousestimation de la valeurs du compte.

jniles · 2017-02-22T13:48:44Z

@patsonjunior

En principe, le system marche avec les transactions équilibrées. Main il faut tous les temps faire la verification parce ce que le reseau, l'oridinateur, le courant, ou autres choses peuvent bouger les informations. Dans le fonctionnement normale, on n'aura pas des déquilibrées.
Vous avez raison! Si l'utilisateur clique sur le bouton "Voir Erreurs", ca va lui montrer les peices.

sfount · 2017-02-22T14:05:03Z

Okay, LGTM. We'll make issues to track UX improvements.

jniles added Bug Fix Refactor labels Feb 21, 2017

jniles force-pushed the refactor-trial-balance branch from a19e1d2 to 30f2c97 Compare February 21, 2017 09:48

jniles requested a review from sfount February 21, 2017 09:54

jniles force-pushed the refactor-trial-balance branch from 585fc82 to 41a17c0 Compare February 21, 2017 19:50

Jonathan Niles added 3 commits February 22, 2017 07:01

jniles force-pushed the refactor-trial-balance branch from 41a17c0 to 2ca505b Compare February 22, 2017 06:02

sfount reviewed Feb 22, 2017

View reviewed changes

sfount merged commit 3db6574 into Third-Culture-Software:master Feb 22, 2017

jniles deleted the refactor-trial-balance branch February 22, 2017 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Trial Balance #1233

refactor: Trial Balance #1233

jniles commented Feb 21, 2017 •

edited

Loading

jniles commented Feb 21, 2017

jniles commented Feb 21, 2017

DedrickEnc commented Feb 21, 2017

jniles commented Feb 21, 2017

sfount commented Feb 21, 2017

jniles commented Feb 21, 2017 •

edited

Loading

sfount left a comment

sfount Feb 22, 2017

sfount Feb 22, 2017

jniles Feb 22, 2017

sfount Feb 22, 2017

jniles Feb 22, 2017

sfount Feb 22, 2017

jniles Feb 22, 2017

jniles Feb 22, 2017

sfount Feb 22, 2017

jniles commented Feb 22, 2017 •

edited

Loading

patsonjunior commented Feb 22, 2017

jniles commented Feb 22, 2017 •

edited

Loading

sfount commented Feb 22, 2017

refactor: Trial Balance #1233

refactor: Trial Balance #1233

Conversation

jniles commented Feb 21, 2017 • edited Loading

jniles commented Feb 21, 2017

jniles commented Feb 21, 2017

DedrickEnc commented Feb 21, 2017

jniles commented Feb 21, 2017

sfount commented Feb 21, 2017

jniles commented Feb 21, 2017 • edited Loading

sfount 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

jniles commented Feb 22, 2017 • edited Loading

patsonjunior commented Feb 22, 2017

jniles commented Feb 22, 2017 • edited Loading

sfount commented Feb 22, 2017

jniles commented Feb 21, 2017 •

edited

Loading

jniles commented Feb 21, 2017 •

edited

Loading

jniles commented Feb 22, 2017 •

edited

Loading

jniles commented Feb 22, 2017 •

edited

Loading