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

[18.0][MIG] purchase_order_supplierinfo_update: Migration to 18.0 #2500

Open
wants to merge 14 commits into
base: 18.0
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Dec 26, 2024

No description provided.

Copy link

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

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

Tested locally and working. I left a few comments to improve the code, mainly.
Additionally, please squash some of the administrative commits according to this guide
TT54247
@Tecnativa @pedrobaeza @sergio-teruel

("product_id", "=", line.product_id.id),
("date_order", ">", line.date_order),
]
if not self.env["purchase.order.line"].search(domain, limit=1):

Choose a reason for hiding this comment

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

I think this can be done with search_count instead of search. It's more efficient if you don't need any fields from the search result.

Suggested change
if not self.env["purchase.order.line"].search(domain, limit=1):
if not self.env["purchase.order.line"].search_count(domain):

Comment on lines 7 to 12
from odoo.tests import Form, TransactionCase

from odoo.addons.base.tests.common import DISABLED_MAIL_CONTEXT


class TestPurchaseOrderSupplierinfoUpdate(TransactionCase):

Choose a reason for hiding this comment

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

Please inherit from BaseCommon and remove DISABLED_MAIL_CONTEXT because it is already included in BaseCommon

Example OCA/account-invoicing@caebebd#diff-2c01e63cb4e7571a7422b3b88bb4d648c677020e4d9bd39de046e36e970d4497L10-L24

Comment on lines 119 to 128
"order_line": [
(
0,
0,
{
"product_id": self.product_2.id,
"product_uom": self.product_2.uom_po_id.id,
"price_unit": 20.00,
},
)
]

Choose a reason for hiding this comment

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

Coud you use Command.create It is more readable. Example here

Suggested change
"order_line": [
(
0,
0,
{
"product_id": self.product_2.id,
"product_uom": self.product_2.uom_po_id.id,
"price_unit": 20.00,
},
)
]
"order_line": [
Command.create({
"product_id": self.product_2.id,
"product_uom": self.product_2.uom_po_id.id,
"price_unit": 20.00,
})
]

Choose a reason for hiding this comment

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

ping @jdidderen-nsi

Copy link
Contributor

@sergio-teruel sergio-teruel left a comment

Choose a reason for hiding this comment

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

Functional ok
Waiting for @carlos-lopez-tecnativa's comments to be resolved

@carlos-lopez-tecnativa
Copy link

ping @jdidderen-nsi could you please check my previous comment

OCA-git-bot and others added 6 commits March 3, 2025 19:48
Currently translated at 100.0% (2 of 2 strings)

Translation: purchase-workflow-16.0/purchase-workflow-16.0-purchase_order_supplierinfo_update
Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-16-0/purchase-workflow-16-0-purchase_order_supplierinfo_update/pt_BR/
- Include context keys for avoiding mail operations overhead.
Currently translated at 100.0% (2 of 2 strings)

Translation: purchase-workflow-16.0/purchase-workflow-16.0-purchase_order_supplierinfo_update
Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-16-0/purchase-workflow-16-0-purchase_order_supplierinfo_update/it/
@jdidderen jdidderen force-pushed the 18.0-mig-purchase_order_supplierinfo_update branch from 9c7b66a to 1ddcc9d Compare March 3, 2025 18:50
@ghost
Copy link
Author

ghost commented Mar 3, 2025

@carlos-lopez-tecnativa We should be good to go
Sorry for the delay and thanks for the review 👍

Copy link

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

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

LGTM. Please let me know what you think about my comment. Is it applicable or not?

Comment on lines +71 to +72
if new_seller_price != seller.price:
seller.sudo().price = new_seller_price

Choose a reason for hiding this comment

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

Now that Odoo supports discounts, I think it's necessary to update the discount on supplier info when it changes, similar to how the supplier info is created upon order confirmation. What do you think @sergio-teruel?
https://github.com/odoo/odoo/blob/b0c9d6c36c8c6c68e6c0fdf42dab1723744f0508/addons/purchase/models/purchase_order.py#L559

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!!... I agree with @carlos-lopez-tecnativa

Choose a reason for hiding this comment

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

@carlos-lopez-tecnativa @sergio-teruel The discount is added in the logic. I applied all the comments in 17.0 too #2499 👍

@jdidderen jdidderen force-pushed the 18.0-mig-purchase_order_supplierinfo_update branch from 0f81d1a to 32f12f1 Compare March 15, 2025 13:28
@jdidderen jdidderen force-pushed the 18.0-mig-purchase_order_supplierinfo_update branch from 32f12f1 to b577ab5 Compare March 15, 2025 13:34
Comment on lines +29 to +30
if vals.get("price_unit"):
self.update_supplierinfo_price()

Choose a reason for hiding this comment

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

In a confirmed purchase order, if you change the prices, it will update the supplier_info. It is also possible to evaluate the discount.

Suggested change
if vals.get("price_unit"):
self.update_supplierinfo_price()
if vals.get("price_unit") or "discount" in vals:
self.update_supplierinfo_price()

@@ -0,0 +1,5 @@
This module extends the functionality of purchase to allow the price of

Choose a reason for hiding this comment

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

I think it needs to be updated in the README to mention that the discount will be updated too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.