-
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
feature(Budget Report) #7684
feature(Budget Report) #7684
Conversation
f788ce6
to
e9c6e0f
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.
@lomamech, I wasn't able to fully test this as there are some problems in the code. I've made suggestions below to improve the code clarity and the data.
Let me know when you have made the changes and please provide an example year that has data in it that I can check.
client/src/modules/reports/generate/budget_report/budget_report.html
Outdated
Show resolved
Hide resolved
client/src/modules/reports/generate/budget_report/budget_report.html
Outdated
Show resolved
Hide resolved
client/src/modules/reports/generate/budget_report/budget_report.config.js
Outdated
Show resolved
Hide resolved
server/controllers/finance/reports/budget_analytical/report.handlebars
Outdated
Show resolved
Hide resolved
}); | ||
}); | ||
|
||
dataFiscalsYear.forEach((fiscal, index) => { |
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 am not sure why you are treating index === 0 as different. I would advise you to just break it into two different functions/blocks though, for clarity:
const [firstYear, ...laterYears] = dataFiscalsYear;
tabFiscalreport.forEach(rep => {
if (firstYear.number === rep.number) {
// do whatever
}
}
// later:
tabFiscalreport.forEach(rep => {
let totalIncomeRealised = 0;
let totalExpenseRealised = 0;
/// etc
})
This saves you from a big .forEach()
loop that just calls if
first thing.
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 didn't quite understand what I should do, please give more guidance.
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 am not sure why you are treating index === 0 as different. I would advise you to just break it into two different functions/blocks though, for clarity:
FYI If you check the SQL definition of the budget
table, it indicates that period==0 is for the FY budget total for the account.
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.
client/src/modules/reports/generate/budget_report/budget_report.config.js
Show resolved
Hide resolved
e9c6e0f
to
6ddd727
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.
I did some basic testing and it seems to work.
However, before merging, please address the issues that @jniles brought up in his review.
Also update to the latest dependencies in master.
Also, a minor cosmetic thing.
- In the budget report, please center or right justify the "Realization" in the column headers.
- In Englsh, "Realization" should be "Actuals"
- In English, "Revenus" should be "Income"
- Implementation of the budget report, allowing the selection of the fiscal year and the number of previous years for result analysis. - Retrieving the values for the periods, excluding period 0, the closing period 13, as well as hidden and blocked accounts. - Correction of the restriction during the compilation of securities accounts for non-budgeted accounts. closes Third-Culture-Software#7683
b891cf0
to
4d0eb07
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.
Except for the coding issue that @jniles pointed out, I think this PR is ready to merge. I've tested it in the browser and it seems to work.
There are some improvements I would still like to see to the report, but I will add Issues for them so we can do them later.
The tests all pass locally.
closes #7683