account-analytic
account-analytic copied to clipboard
OCA
[17.0][MIG] stock analytic: Migration to 17.0
@AndrzejGerasimukARCHIMEDES Well, it needs people to review it before it can be merged. That's what is currently blocking.
I've been using this code while migrating/developing other things. The one thing I noticed was missing was that the propagation of the analytic distribution does not actually work (I'm not sure if that is a version change or something else). Regardless, I made a fix, which the author is free to cherry-pick into this PR if that makes more sense: #645
Otherwise the tests at least are passing, and I haven't found anything that is in conflict with any version changes.
@moitabenfdz Could you cherry-pick the fix proposed by @aisopuro in #645 ?
Hi.
I have install this version to my test environment on which I have prepare upgrade from R14 to R17.
I want to report that this module needs pre-migration scripts.
Firstly there is a confilct on a view level. Before upgrade we need to do
delete from ir_ui_view where arch_fs ~ 'stock_analytic';
Second thing: when odoo goes to odoo.models: Prepare computation of stock.move.line.analytic_distribution I have a lot of notification such as
odoo.schema: Keep unexpected index stock_move__name_index on table stock_move
odoo.schema: Keep unexpected index stock_move__priority_index on table stock_move
odoo.schema: Keep unexpected index stock_move__create_date_index on table stock_move
odoo.schema: Keep unexpected index stock_picking__priority_index on table stock_picking
odoo.schema: Keep unexpected index stock_picking__date_index on table stock_picking
odoo.schema: Keep unexpected index stock_rule__company_id_index on table stock_rule
odoo.schema: Keep unexpected index stock_rule__auto_index on table stock_rule
And after that I got a warning
py.warnings: /home/odoo/src/odoo/odoo/addons/base/models/ir_module.py:178: DeprecationWarning: XML declarations in HTML module descriptions are deprecated since Odoo 17, stock_analytic can just have a UTF8 description with not need for a declaration.
File "/home/odoo/src/odoo/odoo-bin", line 8, in <module>
And most important: Historical data was lost. There are no premigration script that will migrate data from analytic_account_id and analitytic_tag_ids to analytic_distribution. Upgrade just add new field and delete old ones, So I have lost whole historical data from stock analytic.
I believe the warning about the deprecated XML can be fixed by rebasing this branch on top of 17. It is apparently caused by out-of-date tool settings in the repo, that should be updated in the latest 17.
@aisopuro thanks for reply. I found reason and how to resolve it
in: stock_analytic/static/description/index.html
We need to delete XML declarations
<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
and add to head section
<meta charset="UTF-8">
Also
In stock_analytic/views/stock_scrap_views.xml
There is a lack of XML declarations at the begining of code
<?xml version="1.0" encoding="UTF-8" ?>
And to be honest i think that in each view the UTF-8 sentence schould by in capital letters
This is my first project on which I collaborate with OCA. What is the etiquette of good practice? I propose changes in the comments and the authors correct them into the code, or should I commit the proposals myself?
@AndrzejGerasimukARCHIMEDES it depends. For small additions/improvements you can do as I did earlier in this PR and make a new branch/PR and let the author know they can cherry-pick your change if they think it works.
For this XML thing, it depends on the style guide and the project readme generation configurations (https://github.com/OCA/odoo-pre-commit-hooks/issues/94).
For me personally, my philosophy is "if they didn't make a lint for it, it probably isn't worth caring about, and fixing it will just add noise to the git log". So I wouldn't change the case of XML declarations unless a lint in the project said something about it.
Ultimately its up to the maintainers to weigh in on whether they think its worth it or not.
If you want to have it fixed, then I recommend my approach from earlier in this PR: make a separate branch where you include a separate commit that fixes the case. Then inform the author of this PR that they are free to cherry-pick it in here if they want to.
@aisopuro
So I can't create my own branch for cherry-picking
@moitabenfdz can You consider make small changes from https://github.com/OCA/account-analytic/pull/614#issuecomment-2082270200?
(sorry if i do anything wrong in giving a feedback - i am a newbie)
Also it would be great if You add pre-migration script in stock_analytic/migrations/17.0.0.0.0/pre-migration-cleanup.py
import logging
_logger = logging.getLogger(__name__)
# noinspection PyUnusedLocal
def migrate(cr, installed_version):
_logger.info(f'Start migration stock_analytic 17.0.0.0.0 from {installed_version} - pre cleanup')
with cr.savepoint():
cr.execute(
"""
delete from ir_ui_view where arch_fs ~ 'stock_analytic'
"""
)
_logger.info('migration stock_analytic finished')
@AndrzejGerasimukARCHIMEDES you will need to fork the project into some organization you have write access to, either your company's or your personal one. That can then be public, and can be referenced freely.
@aisopuro Thanks for help.
@moitabenfdz i have made PR https://github.com/OCA/account-analytic/pull/651 Can You cherry-pick changes from it? (https://github.com/OCA/account-analytic/pull/651/commits/79b498c003979ff066a3e2d6b16424b0c5e32718)
@AndrzejGerasimukARCHIMEDES Your migration script from #651 seems to belong primarily in the 16.0 branch, which is where the switch from analytic_account_id to analytic_distribution is made. I agree with @aisopuro that the other changes in your branch are less important and don't need to be included in here in. So I think we can proceed with merging this one first, and then go ahead with any other suggestions in new PRs.
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). 🤖
What a great day to merge this nice PR. Let's do it! Prepared branch 17.0-ocabot-merge-pr-614-by-StefanRijnhart-bump-nobump, awaiting test results.
Congratulations, your PR was merged at 63dd863e6a43c555b3d481d4e331da458b42cfef. Thanks a lot for contributing to OCA. ❤️