-
-
Notifications
You must be signed in to change notification settings - Fork 983
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_price_recalculation: Module to allow to recalculate on demand sale order line prices #54
[ADD] sale_order_price_recalculation: Module to allow to recalculate on demand sale order line prices #54
Conversation
Thanks @pedrobaeza! I see this module calls product_id_change while the other that was mentioned on the mailing list calss price_get. Therefore this one does a bit more than updating the price. I'm not sure which of the two approaches is better, although I would lend towards a change of the price only, which better correspond to the button name. What do people think? |
Well, in fact, call product_id_change is the same as get the price, but avoiding possible side effects if the way the price is calculated changes. The same method is called when you change quantity, so you are not going to find things in the line that you don't want to get. I do it this way because you can have other fields like discount modified (with module product_visible_discount), and the other method doesn't change that. |
Code LGTM at first glance. I'll try to test on Monday |
I just want to add that such code will be extremely slow on large orders line with 50+ lines because of the write on every line that will trigger a recomputation of order fields for each line. This is okay as a 1st approach but would probably be better refactored later to collect the prices and then call a single write at the order level (with nested lines values) while disabling field recomputation and enabling them back after. We did that kind of trick for a customer in 6.1 for purchase and typically went from 15+ minutes to 20 secs on 70+ order lines (disclaimer: our localization adds a few computed fields; but still...) |
Hi, @rvalyi. Thanks for the suggestion. I will implement your approach, because indeed it scalates better. I let you know when I have it ready. |
This module is very similar as our module, so I approve this 👍 |
@rvalyi Can you point me to which context key defers the computation of fields?, because I can't remember it. |
@vrenaville do you think you could backport this to v7 so that we have a smooth migration path please? |
👍 |
True, record.date_order, False, | ||
record.fiscal_position.id, False, | ||
context=self.env.context) | ||
line.write(res['value']) |
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.
Can you add the names of the keyword arguments in this call to product_id_change, in order to preserve compatibility with overrides that change the order of the keyword arguments?
Doesn't the API compatibility layer take care of the context argument?
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.
For information, we encountered some issues with the same kind of code.. odoo/odoo#4698
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.
Done!
Is this still work in progress, as the tag implies? |
@StefanRijnhart, do you have any clue about what @rvalyi mentions? |
@@ -0,0 +1,8 @@ | |||
Recalculation of sale order lines prices on demand |
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.
Can we prettify this using OCA template ?
@pedrobaeza: looks like you could look at setting 'recompute' to False in the context, if you think that is safe. |
3a5e5e8
to
1ad0c1e
Compare
This one was one of my old pendings. @StefanRijnhart, I have tried the context argument, but it seems it's not working. Anyway, I prefer not to put it, because it can have side effects on other added computed fields on sale order lines. I have put a note on known issues. I have also added some tests. Please review. |
1ad0c1e
to
c801f40
Compare
{ | ||
"name": "Price recalculation in sales orders", | ||
"version": "8.0.1.0.0", | ||
"depends": [ |
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 licence miss
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.
Done
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.
(fixed below)
+1 with @gurneyalex concern |
c801f40
to
8046370
Compare
Changes done! |
All changes done! |
👍 |
@lmignon if it's ok with you I'll merge. |
qty=line.product_uom_qty, uom=line.product_uom.id, | ||
qty_uos=line.product_uos_qty, uos=line.product_uos.id, | ||
name=line.name, partner_id=order.partner_id.id, lang=False, | ||
update_tax=True, date_order=order.date_order, packaging=False, |
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.
IMO, you should also provide the packaging from the line even if it's not used by default but if the method is redefined by others modules, the params is part of the 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.
But this will add an unneeded dependency to sale_stock module. Are you sure this is needed?
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.
@pedrobaeza sorry for the noise. You are right. The filed packaging is not defined on the sale.order.line in this module. Strange to see a parameter for a field defined in an optional module...
8046370
to
03dbbd4
Compare
…n demand sale order line prices ================================================== Recalculation of sale order lines prices on demand ================================================== This module adds a button on sale orders below pricelist that recalculates the prices of the order lines that contain a product in them. It is launched manually as a button to get the user decide if he/she wants to recalculate prices when pricelist is changed. Usage ===== Inside a sale order, you can click in any moment a button called "(Recalculate prices)", that is next to the pricelist selection, to launch a recalculation of all the prices of the lines, losing previous custom prices. Known issues / Roadmap ====================== * In a sale order with lot of lines, the recalculation may slow down, because sale general data (amount untaxed, amount taxed...) are recalculated for each line.
03dbbd4
to
9081bd4
Compare
👍 |
Please merge this old one that it's already completed. |
[ADD] sale_order_price_recalculation: Module to allow to recalculate on demand sale order line prices
Recalculation of sale order lines prices on demand
This module adds a button on sale orders below pricelist that recalculates the
prices of the order lines that contain a product in them.
It is launched manually as a button to get the user decide if he/she wants to
recalculate prices when pricelist is changed.
Usage
Inside a sale order, you can click in any moment a button called
"(Recalculate prices)", that is next to the pricelist selection, to launch
a recalculation of all the prices of the lines, losing previous custom prices.
Known issues / Roadmap
sale general data (amount untaxed, amount taxed...) are recalculated for
each line.