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

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

Open
wants to merge 15 commits into
base: 18.0
Choose a base branch
from

Conversation

chaule97
Copy link
Contributor

@chaule97 chaule97 commented Dec 2, 2024

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

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.

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.

self._account_model.with_company(company)
.search(acc_domain_with_company)
.ids
)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

@chaule97 chaule97 Dec 10, 2024

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

I'm not fan of this kind of marketing.

Copy link
Member

Choose a reason for hiding this comment

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

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

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.

image

@sbidoul
Copy link
Member

sbidoul commented Dec 10, 2024

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

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

sbidoul commented Dec 10, 2024

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

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

@chaule97
Copy link
Contributor Author

chaule97 commented Dec 10, 2024

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

sbidoul commented Dec 10, 2024

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

@chaule97 chaule97 force-pushed the 18.0-mig-mis_builder branch from 5035e05 to bc34ec2 Compare December 10, 2024 09:37
@chaule97 chaule97 requested a review from sbidoul December 10, 2024 09:57
@sbidoul
Copy link
Member

sbidoul commented Dec 10, 2024

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

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

pedrobaeza commented Dec 10, 2024

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

sbidoul commented Dec 10, 2024

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

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.

@sbidoul sbidoul added the 18.0 label Dec 11, 2024
@sbidoul
Copy link
Member

sbidoul commented Jan 31, 2025

@chaule97 I pushed a commit to your branch with the _read_group improvement.

There is still another one I'm working on right now.

"mis_builder.mis_report_instance_xlsx",
[self.report_instance.id],
report_type="xlsx",
)
Copy link
Member

Choose a reason for hiding this comment

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

We should test that the warning is the one we expect and nothing else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I have to rename this function name to test_xlsx_with_warning?

Copy link
Member

Choose a reason for hiding this comment

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

Do I have to rename this function name to test_xlsx_with_warning?

@chaule97 not necessarily. My point is that since we capture de logs, we should check that no other warning or error is logged than the one we expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated it

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@vdewulf
Copy link
Contributor

vdewulf commented Jan 31, 2025

This is the test I just did, not successful unfortunately:

  • created a report template with 2 lines, crd[60%] and dbt[60%]
    image

  • created a report for 2025 period

  • the database had the following entries on this account:

image

  • MIS report shows:
    30 for the credit line (not ok, should be 0 based on the entries)
    30 for the debit line (ok)
    image

When clicking on the 30 figures (on the 3 lines, same effect), the following error is shown:
image

@sbidoul
Copy link
Member

sbidoul commented Jan 31, 2025

Added a couple commits with the refactoring of prorata.read_group.mixin to use _read_group instead of read_group, and a basic test. Other tests for that are in the mis_builder_budget module.

@sbidoul
Copy link
Member

sbidoul commented Jan 31, 2025

I removed a jquery remnant in the js code.

@vdewulf the drilldown issue should be fixed. The debit/credit calculation seems correct.

@chaule97 chaule97 force-pushed the 18.0-mig-mis_builder branch 2 times, most recently from 3d80cd1 to fc979de Compare February 4, 2025 03:44
@chaule97 chaule97 force-pushed the 18.0-mig-mis_builder branch from fc979de to ab1f06c Compare February 4, 2025 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants