Account groups expansion in reports

[pedrobaeza]

I have to add groups expansion in reports, the same as accounts. My plans are:

• Add the option for showing groups and which depth level at report level.
• Stack and indent these groups accordingly.

And this is needed for v10! So I need to create 3 modules:

• account_group: Module for backporting account groups on v10
• l10n_es_account_group: Module with Spanish localization account group data.
• mis_builder_account_group: With this development

What do you think about this?

[sbidoul]

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.

[pedrobaeza]

OK, let me know.

[sbidoul]

@pedrobaeza I thought about this a bit more. This is indeed far from trivial. It requires 1/ a generalization of the grouping/expansion mechanism which is currently hard coded to work on accounts; I've some sketchy ideas on how to do that (ie group/expand on other fields than account such as analytic dimensions or group_id), but it's a major surgery in MIS builder internals 2/ a second expansion mechanism to take into account the hierarchy of account groups

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 bale[('group_id.id', '=', ...)] or bale[code_prefix%]) from the account group definitions.

But I don't know the kind of report you want to produce. Do you have an example?

[pedrobaeza]

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 https://github.com/odoo/odoo/pull/23922

[sbidoul]

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()

[pedrobaeza]

so you want to show the account details by account group instead of showing it by account?

Yeah, that's it.

You need to start with a variant of AEP.replace_exprs_by_account_id()

OK, I'll check it next week that I start the development.

[pedrobaeza]

I have already created the base module for managing account groups in v10:

https://github.com/OCA/account-financial-tools/pull/690

And the group definition for Spain in:

https://github.com/OCA/l10n-spain/pull/919

Any additional advise before starting with MIS part?

[sbidoul]

Hi @pedrobaeza,

I re-read the code, and contrarily to what I said in https://github.com/OCA/mis-builder/issues/115#issuecomment-416292512 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.

[pedrobaeza]

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.

[pedrobaeza]

@sbidoul I don't understand how do you expect iter_parents should work in your proposal. No arguments? Which data should filled?

[sbidoul]

What I have in mind is something like this. Assuming a is an instance of RowDetailIdentifierInterface encapsulating an account id. Then list(a.iter_parents()) would return a list of RowDetailIdentifierInterface instances for each group the account is in (from lower level to top level).

I think this is needed around here to make sure we have all parents in _detail_rows and properly construct the KpiMatrixRow hierarchy.

[pedrobaeza]

Shouldn't be RowDetailIdentifierInterface KpiMatrixRowDetailIdentifierInterface? I'm saying because I'm trying hard to understand the structure of mis_builder, and I see several mixes from instance, matrix, aep, row... and I have to admit that I have a total mess in my head. I have very advanced the not abstracted approach, touching just the methods where I have detected the operations are done. I need a prototype very soon, so I continue with this (although we discard it) later.

[pedrobaeza]

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.

[sbidoul]

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.

[pedrobaeza]

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.

[sbidoul]

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.

[pedrobaeza]

OK, then I'm not the only one that have doubts, hehe. Well, if you want a talk on this, tell me.

[github-actions[bot]]

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. If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.