[mis_builder] generalize auto expands #397

TeoGoddet · 2021-12-21T21:41:42Z

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 :

mis-builder/mis_builder/models/aep.py

Line 355 in 90702af

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 :

mis-builder/mis_builder/models/mis_report.py

Line 771 in 90702af

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 · 2021-12-22T14:26:48Z

@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 · 2022-01-05T11:56:14Z

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 · 2022-01-05T12:16:21Z

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 · 2022-02-08T18:44:07Z

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.

sbidoul

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

mis_builder/models/expression_evaluator.py

sbidoul · 2022-03-14T21:17:32Z

mis_builder/tests/test_mis_report_instance.py

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


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

setup/_metapackage/setup.py

TeoGoddet · 2022-03-14T21:34:01Z

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 · 2022-03-15T09:10:39Z

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

TeoGoddet · 2022-03-15T14:15:39Z

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 · 2022-05-03T21:24:25Z

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

sbidoul · 2022-05-04T12:13:05Z

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

pedrobaeza · 2022-07-20T16:22:06Z

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 · 2022-07-20T16:44:11Z

Note to self: check impact on mis_builder_budget.

Tested with budgets by account. This works well.

sbidoul · 2022-07-20T16:59:14Z

mis_builder/models/aep.py

@@ -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.


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 · 2022-07-20T16:59:47Z

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

FernandoRomera · 2022-07-21T13:35:49Z

mis_builder/models/kpimatrix.py

+    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]


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.

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

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.

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

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

sbidoul · 2022-07-22T08:45:13Z

mis_builder/models/aep.py

+                    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]] = (


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

sbidoul · 2022-07-22T10:07:36Z

mis_builder/models/kpimatrix.py

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


What if rdi does not have a company_id field?

sbidoul · 2022-07-22T10:08:21Z

mis_builder/models/kpimatrix.py

+        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)])


search with active_test = False ?

TeoGoddet · 2022-08-14T17:14:06Z

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

sbidoul · 2022-08-14T19:07:39Z

No worries. Will you be at OCA days?

TeoGoddet · 2022-09-06T10:08:07Z

Normally Yes :)

AntoniRomera · 2022-09-12T08:28:31Z

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

TeoGoddet · 2022-09-24T12:12:59Z

@AntoniRomera would be very appreciated !!

[FIX] Add fixes

AntoniRomera · 2022-12-29T08:14:39Z

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

rafaelbn · 2023-02-14T17:56:20Z

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 · 2023-05-29T14:10:39Z

@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 · 2023-05-29T14:35:22Z

@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 · 2023-10-01T12:18:21Z

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 · 2024-02-04T12:17:30Z

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 · 2024-03-10T12:28:10Z

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

TeoGoddet · 2024-03-15T12:32:01Z

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

sbidoul added 14.0 work in progress labels Jan 7, 2022

TeoGoddet force-pushed the expand-by-any branch 2 times, most recently from 89ed20c to 08d07d0 Compare February 8, 2022 17:23

TeoGoddet force-pushed the expand-by-any branch 4 times, most recently from 6344a72 to 695c16c Compare March 14, 2022 21:18

sbidoul reviewed Mar 14, 2022

View reviewed changes

TeoGoddet force-pushed the expand-by-any branch 2 times, most recently from 42d10c9 to 91546b5 Compare March 14, 2022 22:51

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

TeoGoddet changed the title ~~WIP: [mis_builder] generalize auto expands~~ [mis_builder] generalize auto expands Mar 25, 2022

TeoGoddet force-pushed the expand-by-any branch 2 times, most recently from 7004841 to 4e087c1 Compare April 18, 2022 20:48

TeoGoddet force-pushed the expand-by-any branch from 4e087c1 to 6f14461 Compare May 3, 2022 21:08

sbidoul added needs review and removed work in progress labels May 4, 2022

sbidoul added the enhancement label May 4, 2022

AntoniRomera approved these changes May 25, 2022

View reviewed changes

sbidoul reviewed Jul 20, 2022

View reviewed changes

FernandoRomera reviewed Jul 21, 2022

View reviewed changes

sbidoul reviewed Jul 22, 2022

View reviewed changes

aromera and others added 2 commits September 24, 2022 14:38

[FIX] Add fixes

3732790

Merge pull request #1 from AntoniRomera/expand-by-any-improves

044c3eb

[FIX] Add fixes

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 1, 2023

sbidoul removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 1, 2023

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Feb 4, 2024

github-actions bot closed this Mar 10, 2024

sbidoul reopened this Mar 10, 2024

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

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

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

[mis_builder] generalize auto expands #397

Are you sure you want to change the base?

[mis_builder] generalize auto expands #397

Conversation

TeoGoddet commented Dec 21, 2021 • edited Loading

sbidoul commented Dec 22, 2021

TeoGoddet commented Jan 5, 2022

sbidoul commented Jan 5, 2022

TeoGoddet commented Feb 8, 2022

sbidoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TeoGoddet commented Mar 14, 2022

TeoGoddet commented Mar 15, 2022

TeoGoddet commented Mar 15, 2022

TeoGoddet commented May 3, 2022

sbidoul commented May 4, 2022 • edited Loading

pedrobaeza commented Jul 20, 2022

sbidoul commented Jul 20, 2022

Choose a reason for hiding this comment

sbidoul commented Jul 20, 2022 • edited Loading

FernandoRomera Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TeoGoddet commented Aug 14, 2022

sbidoul commented Aug 14, 2022 • edited Loading

TeoGoddet commented Sep 6, 2022

AntoniRomera commented Sep 12, 2022

TeoGoddet commented Sep 24, 2022

AntoniRomera commented Dec 29, 2022 • edited Loading

rafaelbn commented Feb 14, 2023

AntoniRomera commented May 29, 2023

sbidoul commented May 29, 2023

github-actions bot commented Oct 1, 2023

github-actions bot commented Feb 4, 2024

OCA-git-bot commented Mar 10, 2024

TeoGoddet commented Mar 15, 2024

TeoGoddet commented Dec 21, 2021 •

edited

Loading

sbidoul commented May 4, 2022 •

edited

Loading

sbidoul commented Jul 20, 2022 •

edited

Loading

FernandoRomera Jul 21, 2022 •

edited

Loading

sbidoul commented Aug 14, 2022 •

edited

Loading

AntoniRomera commented Dec 29, 2022 •

edited

Loading