-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(account): report opening balances #1644
feat(account): report opening balances #1644
Conversation
@mbayopanda, can you give me a review of this code? |
@jniles I will review this code as soon as possible today. |
9116551
to
6e2264e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are very useful calculations that will be used by all reports. I've made some comments on the arithmetic used to combine balances, it would be useful to discuss the best practices for doing this going forward.
.then(periodId => | ||
getComputedAccountBalanceUntilDate(accountId, date, periodId) | ||
) | ||
.then(runningPeriodBalance => balance + runningPeriodBalance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These calculations are used here in financial reports - are they ever rounded? It looks like they would be subject to Javascript's rounding floating point issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, the currency filter takes care of that. However, I could make them into fixed numbers if you think it's best.
@@ -31,6 +31,8 @@ let frFileMissList = []; | |||
|
|||
const jsonFiles = buildJsonFileArray(); | |||
|
|||
const notSwapFile = (fname) => !fname.includes('.swp'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix 👍 . Truly professional swap file abusers end up with .sw* to the ends of the earth. This should work well though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually think this fix works, actually. You can test it out. I had limited success.
This commit builds a new feature for accounts that can discern their opening balance (from the General Ledger) given a date and account id. The major changes have been added in an "AccountExtra" file that should be merged into accounts/index.js as soon as the major refactoring is finished. This PR consists of two parts: the updated account report that now contains an opening balance and the utility to find the opening balance give a particular date and account_id. Closes Third-Culture-Software#1611. Closes Third-Culture-Software#1636.
6e2264e
to
8e5ad48
Compare
.then(periodId => | ||
getComputedAccountBalanceUntilDate(accountId, date, periodId) | ||
) | ||
.then(runningPeriodBalance => (balance + runningPeriodBalance).toFixed(4)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work well for now! It would be good to research a library like BigNumber
to make this a standard for reports going forward.
This commit builds a new feature for accounts that can discern their opening balance (from the
General Ledger) given a date and account id. The major changes have been added in an "AccountExtra"
file that should be merged into accounts/index.js as soon as the major refactoring is finished.
This PR consists of two parts: the updated account report that now
contains an opening balance and the utility to find the opening balance
give a particular date and account_id.
Closes #1611. Closes #1636.
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!