Voucher cash transfer, generic income and expense tools #1586

mbayopanda · 2017-05-03T14:52:37Z

This PR add tools for journal voucher :

Generic income
Generic Expense
Cash Transfer

mbayopanda · 2017-05-04T14:16:29Z

@jniles could you review this PR

jniles · 2017-05-04T17:23:24Z

@mbayopanda this is ~900 lines of code to review + functionality .. it's a lot for me to review here at Vanga. You can either ask @sfount to do it, or I'll review it when I get back...

sfount · 2017-05-05T08:14:30Z

@mbayopanda @jniles I will look at this pull request this morning.

sfount

I will be reviewing the functionality of this pull request soon, I have made some comments on how the code here can be significantly reduced - this would help make it much easier to read!

sfount · 2017-05-05T08:38:37Z

client/src/modules/vouchers/complex-voucher.ctrl.js

+          lastRow.configure(rows[n]);
+        }
+
+        setTimeout(function () {


This should use the Angular js library $timeout which ensures that methods are called within $apply etc.

It would also be useful to leave a comment here explaining why this timeout of 0 seconds is needed!

sfount · 2017-05-05T08:50:39Z

client/src/modules/vouchers/complex-voucher.ctrl.js

@@ -105,6 +111,134 @@ function ComplexJournalVoucherController(Vouchers, $translate, Currencies, Sessi
      });
  }

+  // open generic income function
+  function openGenericIncomeTool() {


It seems to me that all of these tool chains do exactly the same thing which is to configure the rows - we could greatly reduce the code here by making all of these one function.

For example:

Toolkit.open(voucherTool) .then(processVoucherToolRows); function processVoucherToolRows(result) { // ... }

We also then may not even need the opening methods - I'm not sure exactly how this would work:

genericIncomeTool.action = openVoucherTool(genericIncomeTool); function openVoucherTool(voucherTool) { return function() { Toolkit.open(voucherTool) //... } }

There are a lot of patterns here that could be reduced into helper methods with clear comments!

sfount · 2017-05-05T09:00:16Z

client/src/modules/vouchers/complex-voucher.ctrl.js

+        // initial clear
+        vm.Voucher.clear();
+
+        while (n--) {


Could this be written as a helper method on the VoucherFormService? It seems like it may be used a lot.

For example:

// voucher-form.service.js VoucherForm.prototype.replaceFormRows = function replaceFormRows(rows) { this.clear(); rows.forEach(function (row) { //... }); }

I would also encourage using methods on arrays like forEach and map, while loops are useful for creating utilities that do not create an additional scope however methods like forEach and map are much easier to read and understand for other developers.

ghost · 2017-05-08T15:14:35Z

☔ The latest upstream changes (presumably 2ff9b91) made this pull request unmergeable. Please resolve the merge conflicts.

jniles · 2017-05-08T13:53:22Z

client/src/modules/templates/accountList.tmpl.html

@@ -0,0 +1,4 @@
+<a href>


What typeahead dropdown does Complex Vouchers use? Can this code be removed and your PR use the same one?

jniles · 2017-05-08T13:56:14Z

client/src/modules/vouchers/complex-voucher.ctrl.js

+   * @param {object} result
+   */
+  function updateView(result) {
+    $timeout(function () {


If you use:

vm.gridApi.core.notifyDataChange(uiGridConstants.dataChange.ROW);

does this eliminate the need to $timeout()?

no, it's doesn't fill the need.

jniles · 2017-05-08T13:59:32Z

client/src/modules/vouchers/complex-voucher.ctrl.js

+   *
+   * @description remove null rows
+   */
+  function removeNullRows() {


This is a well-named function. 👍

jniles · 2017-05-08T14:14:38Z

client/src/modules/vouchers/toolkit.service.js

@@ -41,7 +56,8 @@ function VoucherToolkitService($http, Modal, util) {
      controller   : option.controller,
      controllerAs : 'ToolCtrl',
      size         : 'md',
-      resolve      : { data : function () { return option; } }
+      backdrop     : 'static',


jniles · 2017-05-08T14:20:04Z

client/src/modules/vouchers/toolkit/cash_transfer/cash_transfer.js

+    };
+  }
+
+  // on select cashbox


What are these functions for and why are they empty?

jniles · 2017-05-08T14:23:55Z

client/src/modules/vouchers/toolkit/cash_transfer/cash_transfer.js

+      account : vm.account,
+    });
+
+    var msgTransfer = $translate.instant('VOUCHERS.GLOBAL.CASH_TRANSFER');


This will be much easier to maintain if you use one translate key and then do parameter substition as proposed in #408.

Like this:

var ms = $translate.instant('VOUCHER.TOOLS.SOME_DESCRIPTION', { cashbox: vm.cashbox.label, account: vm.account.label, amount: vm.amount, symbol : vm.cashbox.symbol });

jniles · 2017-05-08T14:27:02Z

client/src/modules/vouchers/voucher-form.service.js

+  VoucherForm.prototype.replaceFormRows = function replaceFormRows(rows) { 
+    this.clear();
+
+    var form = this;


Nice alias 👍

jniles · 2017-05-08T14:28:05Z

client/src/modules/vouchers/voucher-form.service.js

+      var lastRowIdx = form.store.data.length - 1;
+      var lastRow = form.store.data[lastRowIdx];
+
+      lastRow.configure(rows[--n]);


This is creative!

The reason I originally used while (n--) was to make sure we were adding the rows on to the end of the complex voucher. Since you are directly accessing the store, there is no need for this anymore.

Can you just do lastRow.configure(row)?

jniles · 2017-05-08T14:28:09Z

server/controllers/finance/reports/vouchers/receipt.handlebars

@@ -54,8 +54,8 @@
        <tr>
          <td>{{number}}</td>
          <td>{{label}}</td>
-          <td class="text-right">{{currency debit currency_id}}</td>
-          <td class="text-right">{{currency credit currency_id}}</td>
+          <td class="text-right">{{currency debit ../details.currency_id}}</td>


jniles · 2017-05-08T14:28:38Z

server/controllers/finance/reports/vouchers/receipt.handlebars

@@ -54,8 +54,8 @@
        <tr>
          <td>{{number}}</td>
          <td>{{label}}</td>
-          <td class="text-right">{{currency debit currency_id}}</td>
-          <td class="text-right">{{currency credit currency_id}}</td>
+          <td class="text-right">{{currency debit ../details.currency_id}}</td>


Closes #1595

jniles

Code looks good to me!

jniles · 2017-05-09T12:32:08Z

client/src/modules/vouchers/toolkit/cash_transfer/cash_transfer.js

+    Instance.close({
+      rows        : bundle,
+      description : msg,
+      type_id     : bhConstants.transactionType.TRANSFER, // Cash transfer


jniles · 2017-05-09T12:33:01Z

client/src/modules/vouchers/toolkit/cash_transfer/cash_transfer.js

+      account : vm.account,
+    });
+
+    var msg = $translate.instant('VOUCHERS.GLOBAL.TRANSFER_DESCRIPTION', {


jniles · 2017-05-09T12:35:15Z

client/src/modules/vouchers/toolkit/cash_transfer/cash_transfer.html

+      (<label class="control-label"  translate>FORM.LABELS.CREDIT</label>)
+
+      <!-- input typeahead -->
+      <input


At some point, we should formalize when to use an account typeahead versus an account ui-select. We could eliminate a lot of code from this PR if we used a component!

mbayopanda · 2017-05-09T14:06:46Z

@jniles could you review this PR please

jniles · 2017-05-09T14:13:25Z

LGTM!

Fix grid rows updating Test e2e for generic income and expense Cash transfer tool and test update test values Fix and enhancements Fix test e2e Fix and enhancements voucher tools Fix and enhancements

mbayopanda mentioned this pull request May 4, 2017

(bug) Journal Vouchers do not render the correct currency of the Transaction #1595

Closed

mbayopanda force-pushed the voucher_tools branch from 32acd7c to 72bb547 Compare May 4, 2017 12:09

sfount reviewed May 5, 2017

View reviewed changes

mbayopanda force-pushed the voucher_tools branch from bf7ad43 to 3db4bb3 Compare May 9, 2017 11:00

jniles reviewed May 9, 2017

View reviewed changes

mbayopanda force-pushed the voucher_tools branch from 3f927e0 to dd96dee Compare May 9, 2017 12:18

jniles approved these changes May 9, 2017

View reviewed changes

mbayopanda force-pushed the voucher_tools branch from dd96dee to 83dc301 Compare May 9, 2017 12:38

jniles merged commit 1a8d00f into IMA-WorldHealth:master May 9, 2017

mbayopanda added 4 commits May 9, 2017 12:25

Journal voucher tools : generic income, expense and cash transfer

23f5fc4

Fix grid rows updating Test e2e for generic income and expense Cash transfer tool and test update test values Fix and enhancements Fix test e2e Fix and enhancements voucher tools Fix and enhancements

Fix rebase errors

3db4bb3

Fix and enhancements

3fd7698

Fix e2e errors

83dc301

mbayopanda mentioned this pull request May 10, 2017

(chore) Add more tools for cash in the journal voucher #1550

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Voucher cash transfer, generic income and expense tools #1586

Voucher cash transfer, generic income and expense tools #1586

mbayopanda commented May 3, 2017

mbayopanda commented May 4, 2017

jniles commented May 4, 2017

sfount commented May 5, 2017

sfount left a comment

sfount May 5, 2017

sfount May 5, 2017

mbayopanda May 5, 2017

sfount May 5, 2017

ghost commented May 8, 2017

jniles May 8, 2017

jniles May 8, 2017

mbayopanda May 9, 2017

jniles May 8, 2017

jniles May 8, 2017

jniles May 8, 2017

jniles May 8, 2017

mbayopanda May 9, 2017

jniles May 8, 2017

jniles May 8, 2017

jniles May 8, 2017

jniles May 8, 2017

jniles left a comment

jniles May 9, 2017

jniles May 9, 2017

jniles May 9, 2017

mbayopanda commented May 9, 2017

jniles commented May 9, 2017

Voucher cash transfer, generic income and expense tools #1586

Voucher cash transfer, generic income and expense tools #1586

Conversation

mbayopanda commented May 3, 2017

mbayopanda commented May 4, 2017

jniles commented May 4, 2017

sfount commented May 5, 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

ghost commented May 8, 2017

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 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

mbayopanda commented May 9, 2017

jniles commented May 9, 2017