[18.0][MIG] mis_builder: Migration to 18.0 #652

chaule97 · 2024-12-02T05:17:28Z

When searching for accounts by code, we only search for accounts by code within a single company. I will add a TODO note to improve this query in future updates.
In Odoo 18, name_search already searches by display_name, so I override the _search_display_name method in mis.report.kpi.expression.
in the reporting-engine module, the report_type field in report_type model has a new value xlsx. However, the test_report in odoo 18 can't examine it. I have to add with self.assertLogs("odoo.tools.test_reports", level="WARNING"): to ignore the warning.

And refine a defaultdict declaration in AEP. The previous version was not giving wrong results, but this one will be more robust.

Currently translated at 100.0% (280 of 280 strings) Translation: mis-builder-17.0/mis-builder-17.0-mis_builder Translate-URL: https://translation.odoo-community.org/projects/mis-builder-17-0/mis-builder-17-0-mis_builder/zh_CN/

sbidoul

Hello, thanks for this migration.

Lots of nice things! I need some time to look closer at the multi company part.

My main worry is the search_fetch which I think make the code less readable for little performance benefit.

As a side note (no need to change), in general I prefer to see mechanical changes such as replacing _ by self.env._ in separate commits to make review a bit easier.

sbidoul · 2024-12-08T17:48:52Z

mis_builder/models/aep.py

+                        self._account_model.with_company(company)
+                        .search(acc_domain_with_company)
+                        .ids
+                    )


Why is this necessary?

When searching for accounts by code, we only search for accounts by code within a single company

Example: We have two accounts in different companies with the same code. When we search by code, even though we add [("company_ids", "in", self.companies.ids)], the result includes only one account per company when with_company is used.

sbidoul · 2024-12-08T17:56:10Z

mis_builder/models/aep.py

@@ -341,21 +350,21 @@ def do_queries(
            # fetch sum of debit/credit, grouped by account_id
            _logger.debug("read_group domain: %s", domain)
            try:
-                accs = aml_model.read_group(
+                accs = aml_model.with_context(
+                    allowed_company_ids=self.companies.ids


Not sure we want to use the context for this. Maybe we should and company_id in self.companies.ids in the domain instead.

I have tried it, but its testcase failed:

domain = list(domain) + domain_by_mode[mode] domain.append(("account_id", "in", self._map_account_ids[key])) domain.append(("company_id", "in", self.companies.ids)) if additional_move_line_filter: domain.extend(additional_move_line_filter) # fetch sum of debit/credit, grouped by account_id _logger.debug("read_group domain: %s", domain) try: accs = aml_model.read_group( domain, ["debit", "credit", "account_id", "company_id"], ["account_id", "company_id"], lazy=False, )

2024-12-10 04:32:45,052 42839 ERROR odoo odoo.addons.mis_builder.tests.test_aep: FAIL: TestAEP.test_aep_basic
Traceback (most recent call last):
File "/home/chau/code/oca/oca-18.0/mis-builder/mis_builder/tests/test_aep.py", line 166, in test_aep_basic
self.assertEqual(self._eval("balp[][('account_id.code', '=', '400AR')]"), 100)
AssertionError: AccountingNone != 100

Thanks for the explanation.

I started investigating this. And oh my... the SQL query that is now emitted by this read_group is very complex and I have serious doubts about its performance. Need to find time to dig deeper.

sbidoul · 2024-12-08T17:59:17Z

mis_builder/models/mis_report_instance.py

@@ -896,7 +911,15 @@ def drilldown(self, arg):
        expr = arg.get("expr")
        account_id = arg.get("account_id")
        if period_id and expr and AEP.has_account_var(expr):
-            period = self.env["mis.report.instance.period"].browse(period_id)
+            period = self.env["mis.report.instance.period"].search_fetch(


I see the performance benefit of search_fetch but we should balance it's use against maintainability. For small tables such as this one, the performance benefit is not worth the reduced maintainability and readability. Can you please revert these?

sbidoul · 2024-12-08T18:01:36Z

mis_builder/models/mis_report_style.py

@@ -133,6 +132,10 @@ def check_positive_val(self):
    hide_always_inherit = fields.Boolean(default=True)
    hide_always = fields.Boolean(default=False)

+    _sql_constraints = [
+        ("style_name_event_uniq", "unique(name)", "Style name should be unique")
+    ]


This constraint seems unrelated to the migration? Better have it in a separate PR.

sbidoul · 2024-12-08T18:02:34Z

mis_builder/readme/CREDITS.md

@@ -0,0 +1 @@
+The migration of this module from 17.0 to 18.0 was financially supported by Camptocamp.


I'm not fan of this kind of marketing.

I think it's legit in this case to promote that funding, and we created that section for this goal. C2C is contributing in this case this way. It's true that we need to be aware when migrating to another version to remove it, but it's fair enough to put it.

chaule97 · 2024-12-10T05:20:11Z

search _fetch is a new feature, sometime we are not not familiar with it. but you are right when we don't need to over optimize. I think if code that can't be overridden or queries that can only be used once then it is still good for modules.

sbidoul · 2024-12-10T08:53:19Z

I'm aware of the optimization that search_fetch brings, but it comes at a cost of code that is harder to read, and also the optimization benefit is easily lost when later modifying the code to read another field as it's easy to forget to add the new field to the search_fetch call. So I think we should reserve these for places where it really matters.

It is anyway unrelated to the migration. Could you remove these search_fetch changes, and possibly open another PR if you think it is important in some places, so we can discuss it separately from the migration?

Could you also move the constraint on style name to a separate PR? It is also unrelated to the migration and it will be easier for me to track the backports if it is separated.

Thanks for moving the self.env._ part to a separate commit!

I'll look closer at the multicompany parts soon.

pedrobaeza · 2024-12-10T08:57:43Z

I think self.env._ is part of the migration itself, as it's something of 18.0 version that comes with the migration (we have included in fact in the migration guide). It's not a different feature. And it can't be backported.

sbidoul · 2024-12-10T08:59:37Z

I think self.env._ is part of the migration itself, as it's something of 18.0 version that comes with the migration (we have included in fact in the migration guide). It's not a different feature. And it can't be backported.

Yes, it is in a separate commit, that is fine. I'm not asking to have it in a separate PR.

pedrobaeza · 2024-12-10T09:07:16Z

I don't think it should go in a separate commit as said.

chaule97 · 2024-12-10T09:30:08Z

Could you also move the constraint on style name to a separate PR? It is also unrelated to the migration and it will be easier for me to track the backports if it is separated.

Should I create PR for 17.0 or 18.0?

sbidoul · 2024-12-10T09:31:29Z

@chaule97 as you prefer. Even 16.0 if you like.

sbidoul · 2024-12-10T10:35:38Z

I don't think it should go in a separate commit as said.

My reasoning is that it's similar to the changes done by pre-commit. Mechanical changes done by odoo-module-migrator are best in a separate commit and keep the changes done by a human in a distinct commit.

pedrobaeza · 2024-12-10T10:42:08Z

My reasoning is that it's similar to the changes done by pre-commit. Mechanical changes done by odoo-module-migrator are best in a separate commit and keep the changes done by a human in a distinct commit.

Well, I do that changes by hand, not using odoo-module-migrator, but then let's normalize this into a commit [MIG] <module>: odoo-module-migrator auto-changes, but not specific commits for each of the changes.

pedrobaeza · 2024-12-10T10:42:38Z

Anyway, thinking twice, at the end, odoo-module-migrator is doing the same job as you as human, so we differentiate them? What we want is to have all the changes needed for a migration in one place (or is it only me that this is what I want to check while reviewing a migration - among other things, of course -?)

sbidoul · 2024-12-10T10:50:15Z

odoo-module-migrator is doing the same job as you as human, so we differentiate them

By that reasaoning, why then requiring a separate commit for pre-commit changes?
When there are massive mechanical changes like that it makes the review much easier when they are isolated from the rest.

pedrobaeza · 2024-12-10T10:52:34Z

pre-commit changes are only syntactic or tool-related ones. They can be mostly ignored. These other changes involves framework changes and other modifications that may affect with more probability with the features/functioning of the module.

jguenat and others added 9 commits November 19, 2024 11:22

[FIX] mis_builder: branche and matrix value computation

1828e15

[IMP] simplify when same account used by multiple companies

f377e75

Add news

34427f1

[BOT] post-merge updates

6739621

[IMP] add a few more tests

435a36e

And refine a defaultdict declaration in AEP. The previous version was not giving wrong results, but this one will be more robust.

[BOT] post-merge updates

bfb9f5d

[TST] add more tests for branches

079c48e

[BOT] post-merge updates

8951aa5

Translated using Weblate (Chinese (Simplified) (zh_CN))

b98b2da

Currently translated at 100.0% (280 of 280 strings) Translation: mis-builder-17.0/mis-builder-17.0-mis_builder Translate-URL: https://translation.odoo-community.org/projects/mis-builder-17-0/mis-builder-17-0-mis_builder/zh_CN/

chaule97 mentioned this pull request Dec 3, 2024

[18.0][MIG] mis_builder_budget: Migration to 18.0 #653

Open

henrybackman approved these changes Dec 9, 2024

View reviewed changes

sbidoul reviewed Dec 9, 2024

View reviewed changes

chaule97 force-pushed the 18.0-mig-mis_builder branch 3 times, most recently from 1862431 to 5035e05 Compare December 10, 2024 05:12

chaule97 added 2 commits December 10, 2024 16:33

[MIG] mis_builder: Migration to 18.0

dc52bcd

[UDP] Replace _() to self.env._()

bc34ec2

chaule97 force-pushed the 18.0-mig-mis_builder branch from 5035e05 to bc34ec2 Compare December 10, 2024 09:37

chaule97 requested a review from sbidoul December 10, 2024 09:57

sbidoul mentioned this pull request Dec 11, 2024

[17.0][IMP] mis_builder: add support for branch companies #646

Merged

sbidoul added the 18.0 label Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[18.0][MIG] mis_builder: Migration to 18.0 #652

[18.0][MIG] mis_builder: Migration to 18.0 #652

chaule97 commented Dec 2, 2024

sbidoul left a comment

sbidoul Dec 8, 2024

chaule97 Dec 10, 2024 •

edited

Loading

sbidoul Dec 8, 2024

chaule97 Dec 10, 2024

sbidoul Dec 11, 2024

sbidoul Dec 8, 2024

sbidoul Dec 8, 2024

sbidoul Dec 8, 2024

pedrobaeza Dec 10, 2024

chaule97 commented Dec 10, 2024

sbidoul commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024

sbidoul commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024

chaule97 commented Dec 10, 2024 •

edited

Loading

sbidoul commented Dec 10, 2024

sbidoul commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024 •

edited

Loading

sbidoul commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024

		@@ -0,0 +1 @@
		The migration of this module from 17.0 to 18.0 was financially supported by Camptocamp.

[18.0][MIG] mis_builder: Migration to 18.0 #652

Are you sure you want to change the base?

[18.0][MIG] mis_builder: Migration to 18.0 #652

Conversation

chaule97 commented Dec 2, 2024

sbidoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaule97 Dec 10, 2024 • 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

chaule97 commented Dec 10, 2024

sbidoul commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024

sbidoul commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024

chaule97 commented Dec 10, 2024 • edited Loading

sbidoul commented Dec 10, 2024

sbidoul commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024 • edited Loading

sbidoul commented Dec 10, 2024

pedrobaeza commented Dec 10, 2024

chaule97 Dec 10, 2024 •

edited

Loading

chaule97 commented Dec 10, 2024 •

edited

Loading

pedrobaeza commented Dec 10, 2024 •

edited

Loading