Report aged of Debts #1475

lomamech · 2017-04-07T09:34:35Z

Report aged of Debts

Update translate files
Creation of user interface
Update data for bhima_test.sql
Adding test integration and E2E
Updating some tests

jniles

Looks like a useful report!

Your grid code is good, as is your updates to the i18n keys!

I'm confused about why you needed the agedCreditor.html file, it seems unnecessary at best and a duplication of work at worst. If you really don't need those files, then you can also remove the AgedCreditorsReportService and other files that were created to support those files. Please remove all unused files and code.

jniles · 2017-04-07T09:39:11Z

client/src/i18n/en/form.json

@@ -271,7 +271,7 @@
 "IS_REPORT":"Is report ?",
 "IS_TITLE":"Is title ?",
 "IS_WAREHOUSE":"Is warehouse ?",
-"INCLUDE_ZEROES":"Inclure des lignes avec des valeurs zéro",
+"INCLUDE_ZEROES":"Include lines with zero values",


jniles · 2017-04-07T09:40:32Z

client/src/partials/finance/reports/agedCreditors/agedCreditors.html

+<div class="flex-header static">
+  <div class="bhima-title">
+    <ol class="headercrumb">
+      <li class="static">{{ "TREE.FINANCE" | translate }}</li>


All new client-side modules should use the translate directive instead of the translate filter. Please update this code (and all other translate filters) to be <element translate>KEY</element>.

jniles · 2017-04-07T09:40:55Z

client/src/partials/finance/reports/agedCreditors/agedCreditors.js

+
+  // basic grid options
+  vm.gridOptions = {
+    fastWatch: true,


jniles · 2017-04-07T09:42:35Z

server/controllers/finance/reports/debtors/aged.handlebars

-            <th class="text-center">{{translate "REPORT.AGED_DEBTORS.OVER_NINETY_DAYS"}}</th>
+            <th class="text-center">{{translate "REPORT.ZERO_TO_THIRTY_DAYS"}}</th>
+            <th class="text-center">{{translate "REPORT.THIRTY_TO_SIXTY_DAYS"}}</th>
+            <th class="text-center">{{translate "REPORT.SIXTY_TO_NINETY_DAYS"}}</th>


👍 This was a good change.

jniles · 2017-04-07T09:43:42Z

client/src/partials/finance/reports/agedCreditors/agedCreditors.html

@@ -0,0 +1,33 @@
+<div class="flex-header static">


I don't think this file should exist. Normally, for a report, all you need is two client file:

report.config.js

report.modal.html

Why does this file exist?

jniles

This is almost there!

Two more updates and then this will be perfect to get into master 👍

Please use the translate directive instead of the translate filter. This was mentioned in the first review, but it looks like you missed a few.
You use "Aged Creditors" and "Aged Suppliers" a couple times ... it is better to pick one name and use it consistently. "Aged Creditors" makes sense (since we may include banks or other creditors in here). I propose you rename variables, files, comments, and functions to reflect the new name "Aged Creditors".

Apart from those two things, this looks good to go!

jniles · 2017-04-07T17:16:36Z

client/src/partials/reports/modals/agedCreditors.modal.html

+  <div class="modal-body">
+    <div class="form-group"
+      ng-class="{ 'has-error' : ConfigForm.$submitted && ConfigForm.label.$invalid }">
+      <label class="control-label">{{ "FORM.LABELS.LABEL" | translate }}</label>


These should use the translate directive.

jniles · 2017-04-07T17:17:47Z

server/controllers/finance/reports/creditors/aged.handlebars

@@ -0,0 +1,77 @@
+{{> head title="REPORT.AGED_SUPPLIERS"}}


You are mixing "Aged Suppliers" and "Aged Creditors" a lot. I propose you stick to the name "Aged Creditors", to avoid confusing in file names and variable names.

jniles · 2017-04-07T17:20:05Z

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

+ *
+ * @description
+ * This report displays the amounts owed by creditor groups broken down by age of
+ * their debt.  The report highlights clients who have long overdue debts, so


Some of this comment doesn't make any sense since it was lifted from the Aged Debtors report. You can just remove everything after the first sentence.

jniles · 2017-04-11T13:27:51Z

@lomamech, these changes look good! There are some conflicts to resolve, but afterwards this will be a really great report to have. 👍

jniles · 2017-04-12T16:06:06Z

@lomamech, it seems like the creditor group report tests fail... do you fail on your local machine?

jniles · 2017-04-14T19:46:49Z

@kwilu try

ghost · 2017-04-14T19:46:56Z

⌛ Trying commit a7c1378 with merge fda8425...

Report aged of Debts Report aged of Debts - Update translate files - Creation of user interface - Update data for bhima_test.sql - Adding test integration and E2E - Updating some tests closes #1387

ghost · 2017-04-15T06:24:14Z

💥 Test timed out

ghost · 2017-04-15T07:55:08Z

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

- Remove Unnecessary folder reports/agedCreditors

- Using creditors instead suppliers - Fix Comment

jniles · 2017-04-20T08:59:08Z

Looks good to me 👍

lomamech requested a review from jniles April 7, 2017 09:34

jniles suggested changes Apr 7, 2017

View reviewed changes

jniles added the Report label Apr 7, 2017

jniles reviewed Apr 7, 2017

View reviewed changes

lomamech force-pushed the agedCreditors branch from 76cafb1 to e908461 Compare April 11, 2017 07:50

jniles approved these changes Apr 11, 2017

View reviewed changes

lomamech force-pushed the agedCreditors branch 2 times, most recently from 245ddb3 to a7c1378 Compare April 11, 2017 14:16

lomamech added 3 commits April 19, 2017 16:02

Resolve conflict in test integration of general ledger

82b706a

- Fix translation key in aged.handlebars

91e093c

- Remove Unnecessary folder reports/agedCreditors

- Using translate directive

9dd89e8

- Using creditors instead suppliers - Fix Comment

lomamech force-pushed the agedCreditors branch 3 times, most recently from 871e261 to 9dd89e8 Compare April 19, 2017 15:04

Move report files of agedCreditors from old src/partial to src/modules

ce40fe6

jniles merged commit 2f2cb6d into IMA-WorldHealth:master Apr 20, 2017

lomamech deleted the agedCreditors branch May 15, 2017 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report aged of Debts #1475

Report aged of Debts #1475

lomamech commented Apr 7, 2017 •

edited

Loading

jniles left a comment

jniles Apr 7, 2017

jniles Apr 7, 2017

jniles Apr 7, 2017

jniles Apr 7, 2017

jniles Apr 7, 2017

jniles left a comment

jniles Apr 7, 2017

jniles Apr 7, 2017

jniles Apr 7, 2017

jniles commented Apr 11, 2017

jniles commented Apr 12, 2017

jniles commented Apr 14, 2017

ghost commented Apr 14, 2017

ghost commented Apr 15, 2017

ghost commented Apr 15, 2017

jniles commented Apr 20, 2017

Report aged of Debts #1475

Report aged of Debts #1475

Conversation

lomamech commented Apr 7, 2017 • 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

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 commented Apr 11, 2017

jniles commented Apr 12, 2017

jniles commented Apr 14, 2017

ghost commented Apr 14, 2017

ghost commented Apr 15, 2017

ghost commented Apr 15, 2017

jniles commented Apr 20, 2017

lomamech commented Apr 7, 2017 •

edited

Loading