Skip to content
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

[mis_builder] generalize auto expands #397

Open
wants to merge 3 commits into
base: 14.0
Choose a base branch
from

Conversation

TeoGoddet
Copy link

@TeoGoddet TeoGoddet commented Dec 21, 2021

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 :
    image

  • 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 :

    accs = aml_model.read_group(

    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 :
    for rdi in kpi._get_row_detail_identifiers() or []:
    )

  • !! 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
Copy link
Member

sbidoul commented Dec 22, 2021

@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
Copy link
Author

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
Copy link
Member

sbidoul commented Jan 5, 2022

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
Copy link
Author

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 TeoGoddet force-pushed the expand-by-any branch 4 times, most recently from 6344a72 to 695c16c Compare March 14, 2022 21:18
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you are making good progress :) Let me know when you think it's ready for a first review.

mis_builder/models/kpimatrix.py Outdated Show resolved Hide resolved
mis_builder/models/expression_evaluator.py Outdated Show resolved Hide resolved
account = self.env["account.account"].browse(row.account_id)
self.assertEqual(row.label, "{} {}".format(account.code, account.name))
if row.row_detail_identifier:
account = self.env["account.account"].browse(row.row_detail_identifier)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this specific change, it might read better with something like this:

Suggested change
account = self.env["account.account"].browse(row.row_detail_identifier)
account = self.env[row.row_detail_model].browse(row.row_detail_res_id)

Not sure.

mis_builder/tests/test_mis_report_instance.py Outdated Show resolved Hide resolved
setup/_metapackage/setup.py Outdated Show resolved Hide resolved
@TeoGoddet
Copy link
Author

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 TeoGoddet force-pushed the expand-by-any branch 2 times, most recently from 42d10c9 to 91546b5 Compare March 14, 2022 22:51
@TeoGoddet
Copy link
Author

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

@TeoGoddet TeoGoddet force-pushed the expand-by-any branch 2 times, most recently from 3e21d12 to 61095b8 Compare March 15, 2022 12:24
@TeoGoddet
Copy link
Author

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 TeoGoddet changed the title WIP: [mis_builder] generalize auto expands [mis_builder] generalize auto expands Mar 25, 2022
@TeoGoddet TeoGoddet force-pushed the expand-by-any branch 2 times, most recently from 7004841 to 4e087c1 Compare April 18, 2022 20:48
@TeoGoddet
Copy link
Author

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

@sbidoul
Copy link
Member

sbidoul commented May 4, 2022

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

@pedrobaeza
Copy link
Member

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
Copy link
Member

sbidoul commented Jul 20, 2022

Note to self: check impact on mis_builder_budget.

Tested with budgets by account. This works well.

@@ -401,11 +432,11 @@ def f(mo):
return self._ACC_RE.sub(f, expr)

def replace_exprs_by_account_id(self, exprs):
"""Replace accounting variables in a list of expression
by their amount, iterating by accounts involved in the expression.
"""This method is depreciated and replaced by replace_exprs_by_row_detail.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is not used inside mis_builder anymore, let's remove it. It is unlikely that anyone is using that, and we'll do a major version bump anyway.

@sbidoul
Copy link
Member

sbidoul commented Jul 20, 2022

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

def get_rdi_name(self, rdi_id):
if rdi_id not in self._rdi_names:
self._load_rdi_names()
return self._rdi_names[rdi_id]
Copy link

@FernandoRomera FernandoRomera Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done a local test and with this change we avoid possible errors.

Suggested change
return self._rdi_names[rdi_id]
return self._rdi_names.get(rdi_id, _("Other"))

EDIT: This is necessary due to archived records, because apparently when the record is archived we don't get the id and name right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FernandoRomera can you provide an scenario to reproduce the problem ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible scenario is when you add the option to group by "product_id" and you are looking at a previous year and as you don't use it anymore you have archived the product to avoid problems, but you still have to be able to see it in previous years' reports.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the rest it works great, we have done a couple of tests and it makes it very easy to generate reports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so maybe it's better to search for inactive records too in _load_rdi_names() ?

rdi_id = UNCLASSIFIED_ROW_DETAIL
if not self._data[key].get(rdi_id, False):
self._data[key][rdi_id] = defaultdict(dict)
self._data[key][rdi_id][acc["account_id"][0]] = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: understand why we group by rdi_id and then account_id and not the contrary.


def _get_rdi_name(self, rdi):
result = rdi.name_get()[0][1]
if self._multi_company and rdi.company_id:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if rdi does not have a company_id field?

rdi_ids = list(rdi_ids)
if UNCLASSIFIED_ROW_DETAIL in rdi_ids:
rdi_ids.remove(UNCLASSIFIED_ROW_DETAIL)
rdis = self._rdi_model.search([("id", "in", rdi_ids)])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

search with active_test = False ?

@TeoGoddet
Copy link
Author

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

@sbidoul
Copy link
Member

sbidoul commented Aug 14, 2022

No worries. Will you be at OCA days?

@TeoGoddet
Copy link
Author

Normally Yes :)

@AntoniRomera
Copy link

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

@TeoGoddet
Copy link
Author

@AntoniRomera would be very appreciated !!

@AntoniRomera
Copy link

AntoniRomera commented Dec 29, 2022

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

@rafaelbn
Copy link
Member

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
Copy link

@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
Copy link
Member

sbidoul commented May 29, 2023

@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
Copy link

github-actions bot commented Oct 1, 2023

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 github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 1, 2023
@sbidoul sbidoul removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 1, 2023
Copy link

github-actions bot commented Feb 4, 2024

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 github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Feb 4, 2024
@github-actions github-actions bot closed this Mar 10, 2024
@sbidoul sbidoul reopened this Mar 10, 2024
@OCA-git-bot
Copy link
Contributor

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

@sbidoul sbidoul added no stale Use this label to prevent the automated stale action from closing this PR/Issue. and removed stale PR/Issue without recent activity, it'll be soon closed automatically. labels Mar 10, 2024
@TeoGoddet
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
14.0 enhancement needs review no stale Use this label to prevent the automated stale action from closing this PR/Issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants