-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[ADD] sale_order_priority #657
Conversation
eb86696
to
6086863
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.
Only minor nitpicking otherwise 👍
sale_order_priority/i18n/it.po
Outdated
#. module: sale_order_priority | ||
#: model:ir.model.fields,help:sale_order_priority.field_sale_order_priority | ||
msgid "Priority for this sale order. Setting manually a value here would set it as priority for all the order lines" | ||
msgstr "Prioritdi questo ordine di vendita. Impostando qui un valore, verrà impostato anche in tutte le righe" |
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.
typo
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 the review, done!
ecb6563
to
7149b03
Compare
The diff is showing changes in other modules |
@aheficent I kept them in a separate commit, they fix the errors that are blocking from OCA/maintainer-quality-tools#554 |
I think it's better to say in the readme that the priority is inherited by the procurement not the picking |
d98d07e
to
93ad09c
Compare
@aheficent all in all, it's cleaner to move the fix of README errors to another PR -> #658 |
@aheficent fixed the README too, in order to explain the procurements' role |
93ad09c
to
c2a3e6a
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.
Code review
sale_order_priority/models/sale.py
Outdated
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | ||
|
||
from odoo import models, fields, api | ||
from odoo.addons.procurement.models import procurement |
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.
@SimoRubi As you depends on procurement addon, it is better to reflect it in the manifest.
sale_order_priority/models/sale.py
Outdated
@api.depends('order_line.priority') | ||
def _compute_priority(self): | ||
for order in self: | ||
order.priority = order.mapped('order_line') and \ |
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.
@SimoRubi Better to store order.mapped(...) in a variable before:
priority = order.mapped('order_line.priority')
order.priority = priority and max(priority) or '1'
sale_order_priority/models/sale.py
Outdated
_inherit = 'sale.order' | ||
|
||
@api.depends('order_line.priority') | ||
def _compute_priority(self): |
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.
@SimoRubi Please decorate @api.multi see https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#models
Here under too
sale_order_priority/models/sale.py
Outdated
for order in self: | ||
order.order_line.write({'priority': order.priority}) | ||
|
||
priority = fields.Selection( |
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.
@SimoRubi Please put fields definition on top
sale_order_priority/models/sale.py
Outdated
# Copyright 2018 Simone Rubino - Agile Business Group | ||
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | ||
|
||
from odoo import models, fields, api |
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.
api, fields, models
sale_order_priority/models/sale.py
Outdated
|
||
def _inverse_priority(self): | ||
for order in self: | ||
order.order_line.write({'priority': order.priority}) |
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.
@SimoRubi You cannot call write in an inverse. Affect recordset values directly
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.
@rousseldenis why can't I call write
in an inverse method? I took it from https://github.com/odoo/odoo/blob/0780f10c5474ae28e768b1bbba0245b0fb46e990/addons/stock/models/stock_picking.py#L341 and tested it, it works.
Moreover I can't order.order_line = order.priority
because order.order_line
is One2Many
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.
@SimoRubi You cannot trust everything in code but go to https://github.com/odoo/odoo/blob/0780f10c5474ae28e768b1bbba0245b0fb46e990/odoo/fields.py#L206
The problem is that inverse methods are called by write, see https://github.com/odoo/odoo/blob/0780f10c5474ae28e768b1bbba0245b0fb46e990/odoo/models.py#L3586
If you write in inverse, that can trigger some unwanted recomputations.
You have to loop into order_line and affect values.
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.
@rousseldenis now I understand what you meant, done, thanks.
Anyway, I checked that assigning directly to the model (https://github.com/odoo/odoo/blob/0780f10c5474ae28e768b1bbba0245b0fb46e990/odoo/models.py#L5238) actually seems to call the write
method too (https://github.com/odoo/odoo/blob/0780f10c5474ae28e768b1bbba0245b0fb46e990/odoo/fields.py#L954), is it correct or I am mixing up already?
Runbot still affected by #630 cannot create a sale order line. I'll test in local later |
@rousseldenis thanks for the accurate review, everything done but I have this little question |
@aheficent actually runbot is green |
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.
Tested successfully on runbot. It works.
However, can anybody explain what is the priority in the picking used for?
If I have two pickings for the same item 1 unit. Only 1 unit in stock. One picking is priority normal and the other is priority urgent. However, the first picking is already reserved, so the second picking remains waiting availability. Is there a way to automatically unreserve the first picking when confirming the second one? Thank you!
PS: I know my question refers to standard Odoo, just asking.
@aheficent I think it's just used in the _order property. So, procurement and picking are 'just' ordered (in tree views too). But in case of automatic assignation (scheduler or...), the picking with higher priority will reserve products before other with smaller one. In normal cases, we never see a difference because of equal values. |
@rousseldenis all right. So confirming a SO is manual assignation. We have to manually unreserve the SO with lower priority if we need another more urgent SO to take over. |
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.
👍
@OCA/crm-sales-marketing-maintainers is this ok to be merged? |
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
* [ADD] sale_order_priority * Code improvements * [FIX] Better not to call write in inverse
Module to manage the priority of sale orders and link it with the generated picking.