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

[17.0][MIG] stock_picking_analytic: Migration to 17.0 #676

Merged
merged 21 commits into from
Aug 22, 2024

Conversation

peluko00
Copy link
Contributor

@peluko00 peluko00 commented Aug 6, 2024

rousseldenis and others added 20 commits August 6, 2024 13:29
- Switch field from analytic_account_id to analytic_distribution.
- Assign a dummy search arg to move_ids_without_package field (stock.picking) to bypass
  the warning from https://github.com/odoo/odoo/blob/0f84366/odoo/fields.py#L808-L812.
- Add an invisible analytic distribution field without group assignment in the picking
  form, due to newly added check in odoo/odoo@0501bbd.
Currently translated at 100.0% (7 of 7 strings)

Translation: account-analytic-16.0/account-analytic-16.0-stock_picking_analytic
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-stock_picking_analytic/es/
Currently translated at 42.8% (3 of 7 strings)

Translation: account-analytic-16.0/account-analytic-16.0-stock_picking_analytic
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-stock_picking_analytic/pt_BR/
Currently translated at 100.0% (7 of 7 strings)

Translation: account-analytic-16.0/account-analytic-16.0-stock_picking_analytic
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-stock_picking_analytic/pt_BR/
Currently translated at 100.0% (7 of 7 strings)

Translation: account-analytic-16.0/account-analytic-16.0-stock_picking_analytic
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-stock_picking_analytic/it/
… distribution computation

Replaced the stock picking analytic distribution search and validations
with a query to improve performance when there are large amounts of
picking and move records. Also, updated test to handle a case where
there are different analytic accounts on moves related to a picking.
@peluko00 peluko00 changed the title [17.0][MIG] stock_picking_analyti: Migration to 17.0 [17.0][MIG] stock_picking_analytic: Migration to 17.0 Aug 6, 2024
Copy link

@mpascuall mpascuall left a comment

Choose a reason for hiding this comment

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

Working good, checked in runboat

@peluko00
Copy link
Contributor Author

peluko00 commented Aug 7, 2024

Hi @StefanRijnhart seems, it's ready to merge

@StefanRijnhart
Copy link
Member

@peluko00 Can you rebase and drop the test requirement?

@peluko00 peluko00 force-pushed the 17.0-mig-stock_picking_analytic branch from e5f2f06 to de7b376 Compare August 7, 2024 05:56
@peluko00
Copy link
Contributor Author

peluko00 commented Aug 7, 2024

@peluko00 Can you rebase and drop the test requirement?

Done!

# method would trigger a warning from
# https://github.com/odoo/odoo/blob/0f84366/odoo/fields.py#L808-L812
# pylint: disable=method-search
move_ids_without_package = fields.One2many(search="[]")
Copy link
Member

Choose a reason for hiding this comment

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

The field is no longer a computed field but instead a One2many field with a domain. Maybe this party trick that served us well in the past can and should be removed in this version? See https://github.com/odoo/odoo/blob/3b42467/addons/stock/models/stock_picking.py#L450-L451

Copy link
Contributor Author

@peluko00 peluko00 Aug 7, 2024

Choose a reason for hiding this comment

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

Yees, is not longer to be use. Thanks for the review

Copy link
Contributor

@BernatObrador BernatObrador left a comment

Choose a reason for hiding this comment

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

LGTM, tested in runboat

@peluko00 peluko00 force-pushed the 17.0-mig-stock_picking_analytic branch from de7b376 to c73fd2f Compare August 7, 2024 10:24
Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Alright! (code review)

Copy link

@ppyczko ppyczko left a comment

Choose a reason for hiding this comment

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

LGTM, tested in runboat

@peluko00
Copy link
Contributor Author

peluko00 commented Aug 7, 2024

Alright! (code review)

Seems like i'ts ready to merge

@StefanRijnhart
Copy link
Member

@peluko00 I don't think any of the PSC members of this repo have chimed in yet, so let's give it at least the 5 days until after creation of the PR that is conventional. Oca-bot will notify us when it's time.

@peluko00
Copy link
Contributor Author

peluko00 commented Aug 8, 2024

@rousseldenis can you review please

@StefanRijnhart
Copy link
Member

@peluko00 please note that reviews and other involvement in OCA projects are on a voluntary basis and that pings are not always appreciated.

@StefanRijnhart
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-676-by-StefanRijnhart-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 2890df2 into OCA:17.0 Aug 22, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at c3e4b12. Thanks a lot for contributing to OCA. ❤️

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.