-
-
Notifications
You must be signed in to change notification settings - Fork 363
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
[17.0][MIG] stock analytic: Migration to 17.0 #614
Conversation
… Stock analytic XML part is now migrated Add logo for generic modules
- migration point, put all the modules to not installable
This module allows the user to generate analytic information from stock moves. - Fixed flake8 and pylint errors.
Remove sale and purchase dependency. Add test Only assign analytic account if account != valuation acc Changing field name account_analytic_id Adjust to OCA latest guidelines. Add some usahe info on README
Remove remaining encoding hints. Correct lint in test Correct flake8 in test Fix documentation and test_flake8
Currently translated at 100.0% (2 of 2 strings) Translation: account-analytic-11.0/account-analytic-11.0-stock_analytic Translate-URL: https://translation.odoo-community.org/projects/account-analytic-11-0/account-analytic-11-0-stock_analytic/de/
Currently translated at 100.0% (2 of 2 strings) Translation: account-analytic-12.0/account-analytic-12.0-stock_analytic Translate-URL: https://translation.odoo-community.org/projects/account-analytic-12-0/account-analytic-12-0-stock_analytic/pt_BR/
Currently translated at 100.0% (2 of 2 strings) Translation: account-analytic-12.0/account-analytic-12.0-stock_analytic Translate-URL: https://translation.odoo-community.org/projects/account-analytic-12-0/account-analytic-12-0-stock_analytic/ca/
Currently translated at 100.0% (3 of 3 strings) Translation: account-analytic-12.0/account-analytic-12.0-stock_analytic Translate-URL: https://translation.odoo-community.org/projects/account-analytic-12-0/account-analytic-12-0-stock_analytic/hr/
Currently translated at 100.0% (4 of 4 strings) Translation: account-analytic-12.0/account-analytic-12.0-stock_analytic Translate-URL: https://translation.odoo-community.org/projects/account-analytic-12-0/account-analytic-12-0-stock_analytic/de/
Currently translated at 100.0% (4 of 4 strings) Translation: account-analytic-12.0/account-analytic-12.0-stock_analytic Translate-URL: https://translation.odoo-community.org/projects/account-analytic-12-0/account-analytic-12-0-stock_analytic/pt/
Currently translated at 100.0% (4 of 4 strings) Translation: account-analytic-12.0/account-analytic-12.0-stock_analytic Translate-URL: https://translation.odoo-community.org/projects/account-analytic-12-0/account-analytic-12-0-stock_analytic/es/
(cherry picked from commit 9255622)
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. Second thing: when odoo goes to odoo.models:
And after that I got a warning
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. |
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
and add to head section Also 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. |
@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 (OCA/odoo-pre-commit-hooks#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. |
So I can't create my own branch for cherry-picking @moitabenfdz can You consider make small changes from #614 (comment)? (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
|
@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 #651 |
Any update @moitabenfdz ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code review LGTM 👍🏿
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It's ready @StefanRijnhart? |
@peluko00 yes! /ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at 63dd863. Thanks a lot for contributing to OCA. ❤️ |
No description provided.