OCA
[18.0][MIG] mis_builder: Migration to 18.0
- 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_namemethod inmis.report.kpi.expression. - in the
reporting-enginemodule, thereport_typefield inreport_typemodel has a new value xlsx. However, the test_report in odoo 18 can't examine it. I have to addwith self.assertLogs("odoo.tools.test_reports", level="WARNING"):to ignore the warning.
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.
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.
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.
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.
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?
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.
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.
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 -?)
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.
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.
@chaule97 I pushed a commit to your branch with the _read_group improvement.
There is still another one I'm working on right now.
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). 🤖
This is the test I just did, not successful unfortunately:
-
created a report template with 2 lines, crd[60%] and dbt[60%]
-
created a report for 2025 period
-
the database had the following entries on this account:
- MIS report shows:
30 for the credit line (not ok, should be 0 based on the entries)
30 for the debit line (ok)
When clicking on the 30 figures (on the 3 lines, same effect), the following error is shown:
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.
I removed a jquery remnant in the js code.
@vdewulf the drilldown issue should be fixed. The debit/credit calculation seems correct.
@sbidoul: Would it be possible to kindly merge this PR at your earliest convenience? We’re hoping to start using it in the production environment. Appreciate your support!
Thank you! 😊
We are also looking forward to this module. Any plans on merging any time soon?
Yes, it will be merged next week latest. In the meantime the data model should not change, so it is safe to use.
Alright, this one looks good now. We'll finish mis_builder_budget and mis_builder_demo separately.
/ocabot merge nobump
This PR looks fantastic, let's merge it! Prepared branch 18.0-ocabot-merge-pr-652-by-sbidoul-bump-nobump, awaiting test results.
Congratulations, your PR was merged at 4cdc69ba0034fbeab478fa1be7a7c843ccef2ec4. Thanks a lot for contributing to OCA. ❤️
I realised I did lose @chaule97 's authorship on two commit during my work here.
So since the merge is still very young I exceptionally did a rebase and force push to 18.0 to restore authorship on 39efa86fb1ff22563fa7dcc68095a4979a289699 and df5bbd74777fca57a95bd6af324ed31819704aa3.