Account statement - basic user interface #1672

mbayopanda · 2017-05-23T13:49:15Z

This PR is about the issue #1638

…into account_statement

sfount

This looks like a great start for the Account Statement module! I've made comments with some minor suggestions but none of them should be blocking if this needs to be merged.

sfount · 2017-05-24T10:03:25Z

client/src/modules/account_statement/account_statement.ctrl.js

+    .then(function (res) {
+      if (!res) { return; }
+      Notify.success('ACCOUNT_STATEMENT.SUCCESSFULLY_COMMENTED');
+      load(AccountStatement.filters.formatHTTP(true));


Is it necessary to reload all of the data in the account statement just because comments are added? I propose that on confirmation from the server we update the data locally so that it reflects these changes.

With editing financial information I think it is important to ensure the client and the server have the same data, however with comments I think an optimistic local update should work well.

What do you think?

sfount · 2017-05-24T10:04:38Z

client/src/modules/account_statement/account_statement.ctrl.js

+    GeneralLedger.read(null, options)
+    .then(function (data) {
+      vm.gridOptions.data = data;
+      vm.loading = false;


This should not be needed as it is included in the .finally

sfount · 2017-05-24T10:09:12Z

client/src/modules/account_statement/modals/comment.modal.js

+
+// DI
+CommentAccountStatementController.$inject = [
+  '$uibModalInstance', 'data', 'AccountStatementService', 'NotifyService',


What does data mean here?

I see from the context of the controller calling it that it contains the selected rows from the grid - I think the name could better reflect what is happening here!

Something like gridSelectionContext? Whatever makes sense to you.

sfount · 2017-05-24T10:10:42Z

client/src/modules/account_statement/modals/comment.modal.js

+    AccountStatement.commentAccountStatement(params)
+      .then(function (res) {
+        if (!res) { return; }
+        Instance.close(res);


Again I think .finally if this promise if resolved or rejected. Instance.close will be called twice in the case of success.

In this section the value returned is not false, in the finally clause we have .finally(Instance.close) which return a falsy value

sfount · 2017-05-24T10:12:23Z

server/controllers/finance/generalLedger/index.js

 */
-function list(req, res, next) {
+function find(options) {
+  const filters = new FilterParser(options, { tableAlias : 'gl', autoParseStatements : false });


sfount · 2017-05-24T10:16:35Z

client/src/modules/journal/modals/search.modal.html

-          required="0">
-          <bh-clear on-clear="ModalCtrl.clear('account_id')"></bh-clear>
-        </bh-account-select>
+        <div ng-if="!ModalCtrl.hasDefaultAccount">


A comment explaining when this switch is used would be useful for developers working on either the Journal or the Account Statement module.

sfount · 2017-05-24T10:18:16Z

client/src/modules/journal/modals/search.modal.js

@@ -22,11 +27,30 @@ function JournalSearchModalController(Instance, Projects, Notify, Store, filters
  // assign already defined custom filters to searchQueries object
  vm.searchQueries = util.maskObjectFromKeys(filters, searchQueryOptions);

+  // has default account
+  if (options.hasDefaultAccount) {


It would be useful to document the behaviour of hasDefaultAccount somewhere in a comment. The branching logic here could be difficult to follow if you didn't see that it was used by different modules with different needs.

jniles

@mbayopanda, there are some very nice features in this PR.

Sharing the same filter modal between the Posting Journal and the Account Statement is a really good idea. Your implementation looks fine to me.
Migrating the GL to FilterParser is a really nice improvement.

Some things to improve in the future, but are not necessarily blocking:

We already know that this component will be used on both the Posting Journal and the Account Statement. It would be ideal if we could design this feature so that it is easily transposed to each module, potentially as a component or directive.

I've made some other comments that may need to be addressed.

jniles · 2017-05-24T09:56:00Z

client/src/js/services/FilterService.js


  // replaces current filters with filters from cache
  FilterList.prototype.loadCache = function loadCache(storedCache) {
    Object.keys(storedCache).forEach(function (key) {
      var cached = storedCache[key];
      var currentFilter = this._filterIndex[key];
-      if(currentFilter) {
+      if (currentFilter) {


jniles · 2017-05-24T09:57:27Z

client/src/modules/account_statement/account_statement.ctrl.js

+
+  vm.gridOptions = {
+    enableColumnMenus        : false,
+    enableCellEdit           : false,


We don't need the enableCellEdit option for this module.

jniles · 2017-05-24T09:58:35Z

client/src/modules/account_statement/account_statement.ctrl.js

+      visible            : false,
+      footerCellTemplate : '<i></i>' },
+
+    { field            : 'uuid',


Do we ever need to see the uuid?

jniles · 2017-05-24T10:00:36Z

client/src/modules/account_statement/account_statement.html

+
+      <div class="toolbar-item">
+        <button data-method="comment" class="btn btn-default" ng-disabled="!AccountStateCtrl.selectedRows.length" ng-click="AccountStateCtrl.commentRows()">
+          <i class="fa fa-sticky-note-o"></i> <span class="hidden-xs" translate>FORM.LABELS.COMMENT</span>


jniles · 2017-05-24T10:01:00Z

client/src/modules/account_statement/account_statement.html

+          ui-grid-move-columns
+          ui-grid-selection
+          ui-grid-cellNav
+          ui-grid-edit


Do we need ui-grid edit?

jniles · 2017-05-24T10:11:19Z

client/src/modules/account_statement/modals/comment.modal.js

+  }
+
+  // get rows uuid
+  function uuids(rows) {


A better name for this function would be something like "getRowIds()" or something. It's confusing since uuid() on the server.

jniles · 2017-05-24T10:16:17Z

test/end-to-end/shared/GridUtils.js

+  * </pre>
+  *
+  */
+function expectCellValueMatch(gridId, row, col, value) {


jniles · 2017-05-24T10:17:31Z

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

+
+  <div class="row">
+    <div class="col-xs-4">
+      {{translate 'TABLE.COLUMNS.TOTAL'}} {{translate 'TABLE.COLUMNS.DEBIT'}}: <strong>{{currency (sum rows 'debit_equiv')}}</strong>


Could you move this sum function to SQL? It would be much more efficient.

jniles · 2017-05-24T10:17:40Z

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

+    </div>
+
+    <div class="col-xs-4">
+      {{translate 'TABLE.COLUMNS.RESULT'}}: <strong>{{currency (substract (sum rows 'debit_equiv') (sum rows 'credit_equiv'))}}</strong>


Could you move this sum function to SQL? It would be much more efficient.

jniles · 2017-05-24T10:18:26Z

server/controllers/finance/generalLedger/index.js

+      LEFT JOIN document_map dm2 ON dm2.uuid = gl.reference_uuid
+  `;
+
+  filters.period('period', 'trans_date');


Nice improvement with FilterParser 👍

…into account_statement

jniles

LGTM.

I've made one final comment to avoid an HTTP request and improve performance. Let me know what you think.

jniles · 2017-05-25T10:12:23Z

client/src/modules/journal/modals/search.modal.js

+   * if the search modal need to set account selection in default query panel we can send it
+   * as parameters
+   * @example
+   * <pre>


Very nice comments 👍

jniles · 2017-05-25T10:15:48Z

client/src/modules/journal/modals/search.modal.js

+    vm.defaultQueries.account_id = filters.account_id;
+  }
+
+  Account.read()


You don't need this .. I don't think. The onSelectAccount() callback should return the entire account object. Just do this:

vm.onSelectAccountCallback = function (account) { vm.currentAccountLabel = account.hrLabel; // other logic }); angular.forEach(vm.searchQueries, function (value, key) { if (angular.isDefined(value)) { if (key === 'account_id') { changes.post({ key : key, value : value, displayValue : vm.currentAccountLabel }); } // ... etc ... } });

This way, you don't have to download the entire account list.

Another way to reduce the data transfer is to do Account.read(filters.account_id) if it exists. That way, we only load one account if we need it's hrlabel and otherwise we do not load any accounts at all.

I'll merge this and try and implement it to see if it works.

jniles

LGTM.

I've made one final comment to avoid an HTTP request and improve performance. Let me know what you think.

jniles · 2017-05-25T12:02:58Z

bors r+

1672: Account statement - basic user interface r=jniles This PR is about the issue #1638

jniles · 2017-05-25T12:11:07Z

bors r-

bors · 2017-05-25T12:11:07Z

Canceled

jniles · 2017-05-25T12:27:18Z

@mbayopanda, the merge of #1667 seems to have broken this :(

…into account_statement

mbayopanda added 10 commits May 16, 2017 16:11

Account statement implementation

bae3872

Merge branch 'master' of https://github.com/IMA-WorldHealth/bhima-2.X …

d06e62e

…into account_statement

Add account filter

57cbbbe

Fix and enhancement

0bfc12d

Merge branch 'master' of https://github.com/IMA-WorldHealth/bhima-2.X …

0f17557

…into account_statement

Fix transaction filter

c935ce6

e2e test for account statement

ba1ba49

Fix mergin conflicts

b5ccc37

fix .only on e2e test

775099a

Fix test errors

5370c70

sfount mentioned this pull request May 24, 2017

(feature) Journal Comments #1437

Closed

sfount self-requested a review May 24, 2017 09:59

sfount reviewed May 24, 2017

View reviewed changes

jniles reviewed May 24, 2017

View reviewed changes

mbayopanda added 6 commits May 24, 2017 11:51

Merge branch 'master' of https://github.com/IMA-WorldHealth/bhima-2.X …

dae3248

…into account_statement

Fix environment variable

e0696fb

Fix and enhancements

fabae47

Merge branch 'master' of https://github.com/IMA-WorldHealth/bhima-2.X …

d0e69b2

…into account_statement

Use of aggregate for calculs

bfd4297

Fix data dependencies

f8fe839

jniles approved these changes May 25, 2017

View reviewed changes

bors bot added a commit that referenced this pull request May 25, 2017

Merge #1672

3a93f85

1672: Account statement - basic user interface r=jniles This PR is about the issue #1638

mbayopanda added 3 commits May 25, 2017 13:01

Merge branch 'master' of https://github.com/IMA-WorldHealth/bhima-2.X …

55f8427

…into account_statement

fix and enhancements

ca253f8

fix mergin conflicts

11ffba3

jniles merged commit 43a989e into Third-Culture-Software:master May 26, 2017

jniles mentioned this pull request May 26, 2017

Account Statement - basic use interface #1638

Closed

This was referenced Jun 9, 2017

(refactor) Income expense variance report #1781

Closed

(refactor) Aged debtor report #1788

Closed

mbayopanda mentioned this pull request Jun 13, 2017

(refactor) Balance sheet report #1791

Closed

This was referenced Jun 19, 2017

(Refactor) cashflow Report #1805

Closed

(Refactor) Cashflow By Service #1808

Closed

mbayopanda deleted the account_statement branch August 22, 2017 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account statement - basic user interface #1672

Account statement - basic user interface #1672

mbayopanda commented May 23, 2017

sfount left a comment

sfount May 24, 2017

sfount May 24, 2017

sfount May 24, 2017

sfount May 24, 2017

mbayopanda May 24, 2017

sfount May 24, 2017

sfount May 24, 2017

sfount May 24, 2017

jniles left a comment

jniles May 24, 2017

jniles May 24, 2017

jniles May 24, 2017

jniles May 24, 2017

jniles May 24, 2017

jniles May 24, 2017

jniles May 24, 2017

jniles May 24, 2017

jniles May 24, 2017

jniles May 24, 2017

jniles left a comment

jniles May 25, 2017

jniles May 25, 2017

jniles May 26, 2017

jniles left a comment

jniles commented May 25, 2017

jniles commented May 25, 2017

bors bot commented May 25, 2017

jniles commented May 25, 2017

Account statement - basic user interface #1672

Account statement - basic user interface #1672

Conversation

mbayopanda commented May 23, 2017

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

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

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

jniles left a comment

Choose a reason for hiding this comment

jniles commented May 25, 2017

jniles commented May 25, 2017

bors bot commented May 25, 2017

Canceled

jniles commented May 25, 2017