-
-
Notifications
You must be signed in to change notification settings - Fork 369
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
[14.0][MIG] stock_analytic: Migration to V14. #353
Conversation
There is an issue in another module : #354 |
9d7a5bf
to
3149069
Compare
Now test are OK 👍 @rousseldenis Could you help me to review? |
@@ -93,7 +93,7 @@ def _create_picking( | |||
"location_id": location_id.id, | |||
"location_dest_id": location_dest_id.id, | |||
"date": datetime.now().strftime("%Y-%m-%d %H:%M:%S"), | |||
"date_expected": datetime.now().strftime("%Y-%m-%d %H:%M:%S"), | |||
"date_deadline": datetime.now().strftime("%Y-%m-%d %H:%M:%S"), |
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.
I think you can do fields.Datetime.now()
if ( | ||
line[2]["account_id"] | ||
!= self.product_id.categ_id.property_stock_valuation_account_id.id | ||
): |
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.
@alan196 @rousseldenis It's not really a review, but I'm willing to use this module and I don't catch why there is this condition on the product category's stock valuation account.account.
Do you know the reason ?
Thank you very much ! 🙏
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.
@clementmbr The same condition is in previous versions: 9.0, 10.0, 11.0, (module is not migrated to 12.0) and 13.0, mabe older versions too. I'd say it's fine.
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.
Yes we have checked this with @clementmbr it is fine indeed.
@alan196 Are you going to put any more effort into this PR? I see you have two reviews with suggested changes. Have you considered them? I have a bunch of custom modules that depends on stock_analytic so either this PR gets pushed or else this more recent one does: #391 . I'd rather help with these than create yet another PR with a proposed migration ;) But first I want to make sure one of them is still maintained... |
Hi @alan196, Can you make this PR ready to merge? |
… 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/
(cherry picked from commit 9255622)
Currently translated at 100.0% (5 of 5 strings) Translation: account-analytic-13.0/account-analytic-13.0-stock_analytic Translate-URL: https://translation.odoo-community.org/projects/account-analytic-13-0/account-analytic-13-0-stock_analytic/pt_BR/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: account-analytic-13.0/account-analytic-13.0-stock_analytic Translate-URL: https://translation.odoo-community.org/projects/account-analytic-13-0/account-analytic-13-0-stock_analytic/
3149069
to
f21c84a
Compare
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.
All tests pass. Looks good to me!!!
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.
Thanks for contributing, LGTM
This PR has the |
@dreispt @pedrobaeza If that's ok, would it be possible for you to merge this PR please? I can do without but it'd be really nice to have this module ready in the main repo for our migration to Odoo 14 this weekend. |
@rousseldenis do you agree on merging? |
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 620b568. Thanks a lot for contributing to OCA. ❤️ |
Thank you!! See you for Odoo 15 ;) |
Proposed changes
I have migrated the module stock_analytic I also make a little improvement and add analytic groups in the field, in order to show analytic accounts in stock moves only to users that have analytic group.
Types of changes
Checklist