-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
Account groups expansion in reports #115
Comments
Ouch, I'm not sure adding another expansion mechanism than accounts is feasible in an extension module for mis builder. This requires a refactoring first to generalize the expansion mechanism. I'll need to think about it. |
OK, let me know. |
@pedrobaeza I thought about this a bit more. This is indeed far from trivial. It requires So if you are on a hurry or with restricted budget, it's better to seek alternatives. For instance, a relatively easy approach to generate a trial balance based on account groups, is to generate KPIs from the groups (eg But I don't know the kind of report you want to produce. Do you have an example? |
I'm in a hurry I'm afraid, but I can develop fast also, hehe. This is going to be used with Spanish financial reports: https://github.com/OCA/l10n-spain/tree/11.0/l10n_es_mis_report, allowing to select on the moment they want to see the report, which account group level (1, 2, 3, etc), and using account group definition from odoo/odoo#23922 |
Ah I see better, so you want to show the account details by account group instead of showing it by account? That is quite easier to implement than the generalized grouping mechanism. Not in an extension module though. But if you backport account_group I see no problem to have mis_builder depend on it. You need to start with a variant of AEP.replace_exprs_by_account_id() |
Yeah, that's it.
OK, I'll check it next week that I start the development. |
I have already created the base module for managing account groups in v10: OCA/account-financial-tools#690 And the group definition for Spain in: Any additional advise before starting with MIS part? |
Hi @pedrobaeza, I re-read the code, and contrarily to what I said in #115 (comment) I now think it's necessary to start by the other end. So I suggest to start with a refactoring of kpi_matrix.py. Currently the KpiMatrix's concept of a detail line is intimately linked to account_id. It's a leaky abstraction that must be removed first. So in a nutshell, implementing this TODO. account_id everywhere in kpi_matrix.py must be replaced by an instance of some abstract class. That abstract class' interface must be hashable, be able to compute a label for the row, and be sortable. In other words, detail rows of a kpi are currently identified by account_id. That must be replaced by a more generic concept which will encapsulate either an account group or account (for your use case). But it could also encapsulate something else in the future (eg an analytic account). The interface of that thing passed to KpiMatrixRow in place of account_id could be class RowDetailIdentifierInterface:
def get_label(self):
""" return the row label """
def iter_parents(self):
""" yield all parent RowDetailIdentifierInterface
(eg if self references an account, yield parent groups from bottom to top)
This is important so detail rows can be added in any order and the KpiMatrix
can create placeholders for parent rows.
"""
def __hash__(self):
pass
def __lt__(self):
pass Then there is the question of styles of detail rows, but that can be addressed in a second step and start with one style for all detail row, to do one small change at a time. |
Yeah, in my investigation I have already deduced that it's not so simple. I was adding the abstraction through other mechanism (renaming methods and classes, and adding the condition for the group), but I will add the abstraction through classes as you propose. I have exhausted a lot of the budget in improving account_chart_update (which I think it's worth - look at the last PR, you'll love it), so full development with tests is not going to be possible I'm afraid, but I'm going to try at least to have the abstraction plus the group implementation. |
@sbidoul I don't understand how do you expect |
What I have in mind is something like this. Assuming I think this is needed around here to make sure we have all parents in |
Shouldn't be |
You can see here my dirty approach: https://github.com/Tecnativa/mis-builder/commits/10.0-mis_builder-group_aggreg. I know it's not what you want and without the abstraction you want, but it's working now and I'm not able to continue with more development hours I'm afraid. |
Thanks Pedro. I propose we keep this open then, because merging it will make the code even more complex to maintain and evolve. I'll see if I can find some time/budget later to put in place a proper detail/grouping abstraction. |
Yeah, that's why I don't even propose a PR. Maybe on OCA days you can explain me a bit to see if I can implement it. |
Not sure I can explain :) It's not very clear in my head either. Need to spend time on it to get a better view. |
OK, then I'm not the only one that have doubts, hehe. Well, if you want a talk on this, tell me. |
There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
I have to add groups expansion in reports, the same as accounts. My plans are:
And this is needed for v10! So I need to create 3 modules:
What do you think about this?
The text was updated successfully, but these errors were encountered: