[18.0][ADD] sale_order_line_final_price: add new module#4468
[18.0][ADD] sale_order_line_final_price: add new module#4468eduezerouali-tecnativa wants to merge 1 commit into
Conversation
OCA-git-bot
added
series:18.0
mod:sale_order_line_price_lock_by_price_final
65eb84f to
2512440
Compare
|
Please simplify the name to Check also the CI. |
2512440 to
1b804e0
Compare
OCA-git-bot
added
mod:sale_order_line_final_price
1b804e0 to
f2035fd
Compare
|
Name changed, CONTEXT.md added and took care of CI. |
|
Ok, I understand the use case now. It seems to me there is already a module for that or at least a PR. I will check |
carlos-lopez-tecnativa
left a comment
carlos-lopez-tecnativa
left a comment
There was a problem hiding this comment.
Functional review: LGTM. Just one technical comment:
Please update the PR title to reflect the new module name.
| <field | ||
| name="price_final" | ||
| readonly="qty_invoiced > 0" | ||
| optional="show" | ||
| /> |
There was a problem hiding this comment.
I think this feature should only be visible when the discount feature is enabled. Otherwise, users will see two similar fields, even though discounts are not displayed.
What do you think about adding the sale.group_discount_per_so_line group?
There was a problem hiding this comment.
True. Done!
juancarlosonate-tecnativa
left a comment
juancarlosonate-tecnativa
left a comment
There was a problem hiding this comment.
LGTM, decimal precision behavior reviewed and working as documented, just rename the .pot file to the new module name and update the PR title accordingly.
08c4d35 to
695abf1
Compare
eduezerouali-tecnativa
changed the title
|
Changes done |
a1e3517 to
6777c26
Compare
|
We also added the case when |
6777c26 to
28310d1
Compare
|
What a great day to merge this nice PR. Let's do it! |
pedrobaeza
left a comment
pedrobaeza
left a comment
There was a problem hiding this comment.
Oh, wait, this is still doing 2 SQL operations (create and write), no matter if you do it directly or through modify. Please include everything needed in the vals dictionary for doing in one shot.
|
@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 18.0-ocabot-merge-pr-4468-by-pedrobaeza-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
28310d1 to
f5cd2b5
Compare
|
With new approach we should avoid doing 2 SQL operations. |
| self.env.add_to_compute(self._fields["price_final"], self) | ||
|
|
||
| @api.onchange("price_final") | ||
| def _onchange_price_final(self): |
There was a problem hiding this comment.
You should avoid onchange at all cost, because they only works on screen, but not by code. Isn't there any other option? Maybe the other formula didn't work because you didn't put force_save="1" on the view?
6 participants
cc @Tecnativa TT63696
@pedrobaeza @carlos-lopez-tecnativa