-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[FIX] sale: Recompute global discount on order line updates #855
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
base: 18.0
Are you sure you want to change the base?
Conversation
db41525
to
f3138a5
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.
The current commit message doesn't clearly explain the purpose of the changes properly. Please update the commit message. It should ideally include:
The motivation behind this PR
The behavior before this PR
The desired behavior after this PR
This helps reviewers quickly understand the context and impact of the changes.
for line in lines: | ||
if ( | ||
line.product_id | ||
and line.order_id | ||
and line.order_id.company_id.sale_discount_product_id | ||
): | ||
if ( | ||
line.product_id.id | ||
!= line.order_id.company_id.sale_discount_product_id.id | ||
): | ||
if ( | ||
line.order_id.has_global_discount | ||
and line.order_id.global_discount_percentage | ||
): |
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.
Shouldn't we combine all these conditions into a single if
statement?
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.
Yes sir, actually i have done this to be easier to add print/logging at each level. will update it .
else: | ||
return super(SaleOrderDiscount, company_self).action_apply_discount() |
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't we return early and remove the unnecessary else block for better readability?
f3138a5
to
50f01a4
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.
Few comments
sale_global_discount/__manifest__.py
Outdated
"summary": "Manage global discounts in sale orders efficiently.", | ||
"category": "Tutorials/Sale Discount Management", | ||
"depends": ["sale_management"], | ||
"data": [], |
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.
Remove this if not required
has_global_discount = fields.Boolean( | ||
string="Has Global Discount", compute="_compute_has_global_discount", store=True | ||
) |
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.
has_global_discount = fields.Boolean( | |
string="Has Global Discount", compute="_compute_has_global_discount", store=True | |
) | |
has_global_discount = fields.Boolean(string="Has Global Discount", compute="_compute_has_global_discount", store=True) |
@api.depends( | ||
"order_line", "order_line.product_id", "company_id.sale_discount_product_id" | ||
) |
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.depends( | |
"order_line", "order_line.product_id", "company_id.sale_discount_product_id" | |
) | |
@api.depends( "order_line", "order_line.product_id", "company_id.sale_discount_product_id") |
|
||
def write(self, vals): | ||
result = super().write(vals) | ||
# If order lines were modified, update discount |
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.
Not needed
|
||
regular_lines = self._get_regular_lines() | ||
discount_lines = self._get_discount_lines() | ||
|
||
if not regular_lines: | ||
if discount_lines: | ||
discount_lines.unlink() | ||
self.global_discount_percentage = 0 | ||
return | ||
|
||
if discount_lines: | ||
discount_lines.unlink() | ||
|
||
self._create_global_discount_lines(self.global_discount_percentage / 100) |
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.
Why there are unnecessary lines left in between ??
return False | ||
|
||
lines = self.env["sale.order.line"].create(vals_list) | ||
return lines |
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.
comments are unnecessary and there are unnecessary lines left inbetween
@api.model_create_multi | ||
def create(self, vals_list): | ||
lines = super().create(vals_list) | ||
# Avoid triggering for discount product lines to prevent infinite loop |
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.
not needed
def unlink(self): | ||
orders = self.mapped("order_id") | ||
result = super().unlink() | ||
# After unlinking, update discount on affected orders |
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.
Not needed
|
||
# Update discounts for affected orders |
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.
not needed
Motivation : Global discounts in sale orders were not behaving as expected in multi-company and multi-tax setups. Reviewers reported that discount lines were not updated or removed when order lines changed, leading to incorrect totals and tax inconsistencies. Behavior Before: - Adding/removing/updating sale order lines did not update the global discount. - Global discount lines remained even when no regular order lines existed. - Discount lines were not grouped by tax combinations, causing tax mismatches. - Discount lines could appear in the middle of the order lines. Behavior After: - Global discount lines now update automatically when sale order lines are created, modified, or deleted. - Global discount is removed when no regular order lines remain. - Discount amounts are grouped by tax combinations for correct tax application. - Discount lines are positioned at the end of the order lines. - Sale order line hooks (`create`, `write`, `unlink`) trigger global discount recalculation. This improves accuracy of global discount behavior, ensures tax compliance, and aligns with expected user workflows.
50f01a4
to
acae4b4
Compare
added, updated, or removed from a sale order.
sequence=999
.This ensures accurate global discount behavior and maintains tax compliance.