fix(report): account report uses ent currency #936
Conversation
jniles
force-pushed
the
fix-account-report-currency
branch
from
7e618b8
to
ebdb798
Compare
|
@sfount, this can be reviewed anytime. |
This commit fixes account report to use the enterprise currency in rendering totals. It also migrates the account selection to a ui-select for good measure. Finally, the loading indicator on the form is now hooked up. Partially fixes IMA-WorldHealth#924.
jniles
force-pushed
the
fix-account-report-currency
branch
from
ebdb798
to
b6af238
Compare
|
@sfount, can I get a review? |
| if(source === 1){ | ||
|
|
||
| let sql = ` | ||
| if (source === 1) { |
There was a problem hiding this comment.
There is no indication at all here as to what source is supposed to mean or refer to - it looks like it's taking values from either the posting journal, general ledger or both however this is not at all clear given the code.
Should this be refactored here or do you think it is inappropriate for this pull request?
There was a problem hiding this comment.
In my view, I would do that as a separate PR. I have no idea if these queries even produce correct data - a separate PR could do a full audit.
| let sql = ` | ||
| ORDER BY posting_journal.trans_date ASC; | ||
| `; | ||
| } else if (source === 3) { |
There was a problem hiding this comment.
Is there not a MySQL view that combines the posting journal/ general ledger without manually doing that each time?
There was a problem hiding this comment.
Yup! It's called the combined_ledger. Would you like me to do a refactor on this more than just the currency?
There was a problem hiding this comment.
Not in this pull request - as you mentioned this report could use a refactor/ audit. Could you make an issue to track this?
This commit fixes account report to use the enterprise currency in
rendering totals. It also migrates the account selection to a ui-select
for good measure. Finally, the loading indicator on the form is now
hooked up.
Partially fixes #924.
Thank you for contributing!
Before submitting this pull request, please verify that you have:
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!