-
-
Notifications
You must be signed in to change notification settings - Fork 923
[18.0][IMP] purchase_order_secondary_unit: add vendor pricelist support #2932
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Copyright 2026 Quartile (https://www.quartile.co) | ||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | ||
|
|
||
| from odoo.tools.sql import column_exists | ||
|
|
||
|
|
||
| def migrate(cr, version): | ||
| if not column_exists(cr, "purchase_order_line", "secondary_uom_price"): | ||
| cr.execute(""" | ||
| ALTER TABLE purchase_order_line | ||
| ADD COLUMN secondary_uom_price double precision; | ||
|
|
||
| UPDATE purchase_order_line pol | ||
| SET secondary_uom_price = pol.price_unit * su.factor | ||
| FROM product_secondary_unit su | ||
| WHERE su.id = pol.secondary_uom_id; | ||
| """) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | ||
| from . import product_product | ||
| from . import product_supplierinfo | ||
| from . import product_template | ||
| from . import purchase_order |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,42 @@ | ||||
| # Copyright 2026 Quartile (https://www.quartile.co) | ||||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | ||||
|
|
||||
| from odoo import api, fields, models | ||||
|
|
||||
|
|
||||
| class ProductSupplierinfo(models.Model): | ||||
| _inherit = "product.supplierinfo" | ||||
|
|
||||
| secondary_uom_id = fields.Many2one( | ||||
| comodel_name="product.secondary.unit", | ||||
| string="Secondary Unit", | ||||
| domain="[('product_tmpl_id', '=', product_tmpl_id)]", | ||||
| ) | ||||
| secondary_uom_price = fields.Float( | ||||
| string="Secondary Price", | ||||
| digits="Product Price", | ||||
| compute="_compute_secondary_uom_price", | ||||
| inverse="_inverse_secondary_uom_price", | ||||
| store=True, | ||||
| ) | ||||
|
|
||||
| @api.depends("price", "secondary_uom_id", "secondary_uom_id.factor") | ||||
| def _compute_secondary_uom_price(self): | ||||
| for rec in self: | ||||
| if rec.secondary_uom_id: | ||||
| rec.secondary_uom_price = rec.price * rec.secondary_uom_id.factor | ||||
| else: | ||||
| rec.secondary_uom_price = 0.0 | ||||
|
|
||||
| @api.onchange("secondary_uom_price") | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we don't have to use @api.onchane for _inverse function.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the onchange myself and tested the behavior, and found that the value isn’t updated dynamically until the record is saved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you can directly update the price in _onchange_product_id_secondary_uom and skip adding the onchange in the inverse.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could add a new onchange method that replicates the logic of this inverse (I guess _onchange_product_id_secondary_uom is not the one to work on) if we should be strict about not using onchange with an inverse. I see a lot of these patterns, though, in the standard codebase, so I thought it was a widely accepted practice.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not reusing the onchange_product_id_secondary_uom? You update secondary_uom_id and price which is related with it in a single transaction
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could make it work but I don't feel there is a good reason to do so as the intents are different.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, not blocking the decision, let me know when we can move it forward, appreciate that the module has been functional tested deeply :)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! You can go ahead and merge this if you are okay with the current code, or should we add new onchange methods to remove the onchange from inverses? |
||||
| def _inverse_secondary_uom_price(self): | ||||
| for rec in self: | ||||
| if rec.secondary_uom_id: | ||||
| rec.price = rec.secondary_uom_price / rec.secondary_uom_id.factor | ||||
|
|
||||
| @api.onchange("product_tmpl_id", "product_id") | ||||
| def _onchange_product_id_secondary_uom(self): | ||||
| self.secondary_uom_id = ( | ||||
| self.product_id.purchase_secondary_uom_id | ||||
| or self.product_tmpl_id.purchase_secondary_uom_id | ||||
| ) | ||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -3,6 +3,15 @@ | |||
| from odoo import api, fields, models | ||||
|
|
||||
|
|
||||
| class PurchaseOrder(models.Model): | ||||
| _inherit = "purchase.order" | ||||
|
|
||||
| def _prepare_supplier_info(self, partner, line, price, currency): | ||||
| vals = super()._prepare_supplier_info(partner, line, price, currency) | ||||
| vals["secondary_uom_id"] = line.secondary_uom_id.id | ||||
| return vals | ||||
|
|
||||
|
|
||||
| class PurchaseOrderLine(models.Model): | ||||
| _inherit = ["purchase.order.line", "product.secondary.unit.mixin"] | ||||
| _name = "purchase.order.line" | ||||
|
|
@@ -25,12 +34,34 @@ class PurchaseOrderLine(models.Model): | |||
| product_packaging_id = fields.Many2one( | ||||
| compute="_compute_product_packaging_id", store=True, precompute=True | ||||
| ) | ||||
| secondary_uom_price = fields.Float( | ||||
| string="Secondary Price", | ||||
| digits="Product Price", | ||||
| aggregator="avg", | ||||
| compute="_compute_secondary_uom_price", | ||||
| inverse="_inverse_secondary_uom_price", | ||||
| store=True, | ||||
| ) | ||||
|
Comment on lines
+37
to
+44
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My next concern is that this compute store can slowdown the update of the module for big databases. Maybe we need a migration script where we create the column and fill it manually with a sql query.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the Héctor's concern is legit. About being computed or use onchanges, I usually prefer computed fields for not depending on UI, but sometimes the things get messy due to dependencies, and a simple onchange resolves the usability question.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @pedrobaeza , I think Héctor is concerned about adding
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I meant about making it computed stored. About reusing |
||||
|
|
||||
| @api.depends("secondary_uom_qty", "secondary_uom_id") | ||||
| def _compute_product_qty(self): | ||||
| self._compute_helper_target_field_qty() | ||||
| return super()._compute_product_qty() | ||||
|
|
||||
| @api.depends("price_unit", "secondary_uom_id", "secondary_uom_id.factor") | ||||
| def _compute_secondary_uom_price(self): | ||||
| for rec in self: | ||||
| if rec.secondary_uom_id: | ||||
| rec.secondary_uom_price = rec.price_unit * rec.secondary_uom_id.factor | ||||
| else: | ||||
| rec.secondary_uom_price = 0.0 | ||||
|
|
||||
| @api.onchange("secondary_uom_price") | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we don't have to use @api.onchane for _inverse function.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the onchange myself and tested the behavior, and found that the value isn’t updated dynamically until the record is saved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||||
| def _inverse_secondary_uom_price(self): | ||||
| for rec in self: | ||||
| if rec.secondary_uom_id: | ||||
| rec.price_unit = rec.secondary_uom_price / rec.secondary_uom_id.factor | ||||
|
|
||||
| @api.onchange("product_uom") | ||||
| def onchange_product_uom_for_secondary(self): | ||||
| self._onchange_helper_product_uom_for_secondary() | ||||
|
|
@@ -54,3 +85,12 @@ def onchange_product_id(self): | |||
| if self.secondary_uom_id: | ||||
| self.secondary_uom_qty = 1.0 | ||||
| return res | ||||
|
|
||||
| def _prepare_account_move_line(self, move=False): | ||||
| # Set secondary UoM values only when account_move_secondary_unit is installed | ||||
| # (i.e., the fields exist on account.move.line). | ||||
| res = super()._prepare_account_move_line(move) | ||||
| aml_fields = self.env["account.move.line"]._fields | ||||
| if "secondary_uom_id" in aml_fields and self.secondary_uom_id: | ||||
| res["secondary_uom_id"] = self.secondary_uom_id.id | ||||
| return res | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| For configuration of displaying secondary unit information in purchase reports and | ||
| the Purchase Order portal, see the guidelines provided in product_secondary_unit. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,8 @@ | ||
| This module extends the functionality of purchase orders to allow buy | ||
| products in secondary unit of distinct category. | ||
|
|
||
| Users can enter quantities and prices in secondary units on purchase order lines. Vendor | ||
| pricelist records are also extended to support secondary unit pricing. | ||
|
|
||
| Purchase reports and the Purchase Order portal are adjusted to display quantities and prices | ||
| in secondary units based on company configuration. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Updating existing vendor pricelist records from purchase order confirmation does not | ||
| currently support secondary UOM or secondary UOM pricing. This is not included in the | ||
| current scope and may be considered in future improvements. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,16 +6,64 @@ | |
| > | ||
| <!-- Header data --> | ||
| <th name="th_quantity" position="before"> | ||
| <th name="th_secondary_unit" class="text-end"> | ||
| <th | ||
| name="th_secondary_unit" | ||
| class="text-end" | ||
| t-if="not o.company_id.hide_secondary_uom_column(o)" | ||
| > | ||
| <strong>Second Qty</strong> | ||
| </th> | ||
| </th> | ||
| <!-- Content data --> | ||
| <xpath expr="//span[@t-field='line.product_qty']/.." position="before"> | ||
| <td id="secondary_unit" class="text-end"> | ||
| <td | ||
| id="secondary_unit" | ||
| class="text-end" | ||
| t-if="not o.company_id.hide_secondary_uom_column(o)" | ||
| > | ||
| <span t-field="line.secondary_uom_qty" /> | ||
| <span t-field="line.secondary_uom_id" /> | ||
| </td> | ||
| </xpath> | ||
| <!-- Quantity: hide primary when secondary is prioritized --> | ||
| <xpath expr="//span[@t-field='line.product_qty']" position="attributes"> | ||
| <attribute | ||
| name="t-if" | ||
| >line.get_secondary_uom_display_mode() != 'secondary'</attribute> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yostashiro Where is this function? We updated and getting errors as not existing, can't find anywhere.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR depends on OCA/product-attribute#2182, and the function is introduced in that PR , as I mentioned in #2932 (comment) . Unfortunately, this PR was merged before the dependency PR was merged. @yostashiro also explained the situation in the comment above: #2932 (comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, we have pinned to old version anyway after review as we really don't want these changes. In my opinion this should have been a seperate extension module, not because of the breakage, but it is for a feature that not everyone will want, its not really fixing an obvious oversight or error in functionality. In my experience this module is mostly used for straight conversions of qty's and adding price features just makes harder.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gdgellatly That's a fair point and we did consider a split. The tradeoff is four additional modules across the core apps (xxx_secondary_unit_price for product, purchase, sale, and account), and we thought the maintenance overhead would outweigh the benefit. Our clients who need the secondary unit conversion also tend to need pricing per secondary unit, and all the added features here are effectively opt-in. That said, we are happy to split the module if the PSC opts for it. You can also leave a comment on OCA/product-attribute#2182 as it gets harder to change direction after merge. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yostashiro Honestly it is no big deal, we have pinned now and that will get us through the v18 lifecycle which is only a few more months. For us a secondary unit is just a conversion to a primary unit, so price_subtotal is always identical and secondary_price_unit if we had would always just be price_subtotal/secondary_qty And then look again during migration to decide what to do. |
||
| </xpath> | ||
| <xpath expr="//span[@t-field='line.product_uom.name']" position="attributes"> | ||
| <attribute | ||
| name="t-if" | ||
| >line.get_secondary_uom_display_mode() != 'secondary'</attribute> | ||
| </xpath> | ||
| <!-- Quantity: add secondary qty ('secondary' mode) --> | ||
| <xpath expr="//span[@t-field='line.product_uom.name']" position="after"> | ||
| <t t-if="line.get_secondary_uom_display_mode() == 'secondary'"> | ||
| <span t-field="line.secondary_uom_qty" /> | ||
| <span t-field="line.secondary_uom_id.name" /> | ||
| </t> | ||
| </xpath> | ||
| <!-- Quantity: add secondary qty ('both' mode) --> | ||
| <xpath expr="//span[@t-field='line.product_qty']/.." position="inside"> | ||
| <t t-if="line.get_secondary_uom_display_mode() == 'both'"> | ||
| <br /> | ||
| <span class="text-muted"> | ||
| (<span t-field="line.secondary_uom_qty" /> | ||
| <span | ||
| t-field="line.secondary_uom_id.name" | ||
| groups="uom.group_uom" | ||
| />) | ||
| </span> | ||
| </t> | ||
| </xpath> | ||
| <!-- Unit price: hide primary ('secondary' mode) --> | ||
| <xpath expr="//span[@t-field='line.price_unit']" position="attributes"> | ||
| <attribute | ||
| name="t-if" | ||
| >line.get_secondary_uom_display_mode() != 'secondary'</attribute> | ||
| </xpath> | ||
| <xpath expr="//span[@t-field='line.price_unit']" position="after"> | ||
| <t t-call="purchase_order_secondary_unit.purchase_uom_price" /> | ||
| </xpath> | ||
| </template> | ||
| </odoo> | ||
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.
The bump should be "minor". i.e.
18.0.1.0.1->18.0.1.1.0