[mis_builder] generalize auto expands

[TeoGoddet]

This is very far form mergability, it's purpose is to discuss an implementation of #115 (#392)

To do / to discuss :

• how to choose which details lines to show (typicically for accounts, we need only the accounts included in the kpi expression) We may have a domain field here :

• fix the drilldown (should not be a problem)

• optimisation issues Now the PR multiply the query number by the number of details line, to solve this we would need to add a group_by clause here : https://github.com/OCA/mis-builder/blob/90702af67af96d305b00eb11c5bf0bab2a42d6fe/mis_builder/models/aep.py#L355 but that would mean a details kind could not provide a domain for each line but a field for all line that would also automatically solve the problem of wich details line we display (We could get back wich lines to show from aep here instead having them from the report model here : https://github.com/OCA/mis-builder/blob/90702af67af96d305b00eb11c5bf0bab2a42d6fe/mis_builder/models/mis_report.py#L771)

• !! TEST !!

• genericity of the details line model in the PR, i make account_account and account_analytic_account extends RowDetailIdentifierInterface it prevent it to be compatible with account_move_line compatible model (budget, actuals, ..) the right approach would be to encapsulate the right aml fields in the RowDetailIdentifierInterface

• make it easy to extend : split the _get_row_detail_identifiers in many get_row_detail_identifiers_for{}

[sbidoul]

@TeoGoddet I don't have much time to look closely but this sounds promising!

Now the PR multiply the query number by the number of details line, to solve this we would need to add a group_by clause here :

This is probably the key thing to address as minimizing the number of db queries is a core design goal of MIS Builder (and what makes it complex - if we leave the many historical reason asides).

[TeoGoddet]

Another big thing that need to be discussed is expanding by many2many fields The problems is the same that exists with group_by and many2many, what to do when you have multiple tags, dupllicate the lines, split them, ... ? Maybe there is no generic solutions to this and we may have case by case solution (such as using analytic tag dimension for analytic tags)

[sbidoul]

Yes, expanding by m2m is impossible in the general case. Expanding on many2one is the only thing that can be made generic, and, as you, say, users can use analytic dimensions to fallback on m2o.

[TeoGoddet]

I just pushed a new version that is closer to the goal.

• It works, should be ok in performance optimal but I have not benchmark setup to test it.
• drilldown works !
• need to remove some legacy methods (or we should keep them for backward compatibility ?)
• still need to rewrite the test
• I renamed as least as possible field, maybe I should rename them and add a migration script ?
• Maybe reduce complexity : the RowDetailIdentifier class is a pretty useless abstraction if we do not make it an odoo ihneritable class.

[TeoGoddet]

Hi ! Thanks for the review, sorry for the noise on the CI, i could not get a working local CI. My progress so far are

• everything is working (need extensive testing)
• backward-compatible aep (according to the ci tests may need real use tests)
• code almost clean

I'm unsure about the name of the variables introduced, they are not the result of a good reflection but of the long dev process. You made good suggestions, big thanks !

[TeoGoddet]

In another MR, i could propose to have the expand field to be choosen dynamicaly using a many2one to ir.field

[TeoGoddet]

Ok, got the dev tools working better so the diff should be minimal @sbidoul it's now really close to ready, i'd love to have a review I'm affraid i need better names for row_detail_identifier and row_detail_model

Another thing have still in mind is to rewrite the expand field selection to be dynamic (maybe another MR)

[TeoGoddet]

@sbidoul the tests are now fixed, I think it's ready for review

[sbidoul]

Thanks a lot @TeoGoddet. I'll try to review and test this in depth soon.

[hitrosol]

This PR works well in runboat. Thanks for @TeoGoddet and @sbidoul for this nice new feature.

[hitrosol]

I try to combine this PR with https://github.com/OCA/mis-builder-contrib/pull/22 and MIS Budget (By Accounts), give us flexibility show the budget by AA, if we select the Auto Expand Detailed by AA. So we don't have to define line by line for each AA.

[AntoniRomera]

@sbidoul You have been able to check them, from the tests carried out it seems to work without any problems.

[sbidoul]

Yes I'll be reviewing this in details in the next days.

[pedrobaeza]

Note that I tried this in a branch mentioned in #115, but it's good to have this being considered finally. One of the problems I faced was the variables everywhere are mentioning account, and thus letting that names will lead to confusions, but that change made to conflict each time a change was introduced in main branch.

[sbidoul]

Note to self: check impact on mis_builder_budget.

Tested with budgets by account. This works well.

[sbidoul]

A few minor comments, but overall, this looks very good to me :+1: Thanks a ton for this contrib.

[TeoGoddet]

@sbidoul I'm not forgetting about this but need some time to fix everything, thanks a lot for the review !

[sbidoul]

No worries. Will you be at OCA days?

[TeoGoddet]

Normally Yes :)

[AntoniRomera]

@TeoGoddet If you agree, I can help you and make a PR in your repo with the proposed changes.

[TeoGoddet]

@AntoniRomera would be very appreciated !!

[AntoniRomera]

@sbidoul @TeoGoddet Is this at a standstill? Do you plan to do the merge?

[rafaelbn]

Could be great to have this merged in 14 to port for 15 . @TeoGoddet did you got @sbidoul in OCA Days? Do you agree to get this merged? Thank you!

[AntoniRomera]

@sbidoul @pedrobaeza @rafaelbn

This PR has been ready for months, I still don't see why it can't be integrated, if it is to keep the history, can it be done in version 16.0 or how do you see it?

[sbidoul]

@AntoniRomera I was waiting for answers to the questions I asked above. I now notice there has been an additional commit since. So I'll try to find the time to review this again. There were pre-commit and test issues, though so a rebase, and possibly some fixes, are necessary.

[github-actions[bot]]

There hasn't been any activity on this pull request in the past 4 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 PR to never become stale, please ask a PSC member to apply the "no stale" label.

[github-actions[bot]]

There hasn't been any activity on this pull request in the past 4 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 PR to never become stale, please ask a PSC member to apply the "no stale" label.

[OCA-git-bot]

Hi @sbidoul, some modules you are maintaining are being modified, check this out!

[TeoGoddet]

@AntoniRomera @FernandoRomera i wont be able to work on this for at leat another 6 months, feel free to taleover the PR