[14.0][ADD]sale_order_general_discount_payment_term: Add sale order general discount on payment term #2660

odooNextev · 2023-08-30T10:00:20Z

This modulo allow to add sale_order general discount based on payment term discount field.
Also hide discount from partner to avoid conflicts

francesco-ooops

Functional ok!

aleuffre

You hide the original field, but for existing installations the value of sale_discount on res.partners will still exist and it will keep being applied to sale.orders, with no way for the user to change it.

There are a few options to deactivate it entirely, One of them could be a post_init_hook that just writes 0 on every partner's sale_discount field. I don't know if people would find it too aggressive, though.

Another option is to override onchange_partner_id, save the existing discount, call super() and then restore the discount that existed before the onchange was called, so as to nullify the effect of the change in sale_order_general_discount (every line's discount would be recalculated twice for nothing, but alas, I've seen worse in Odoo :D ).

Alternatively, since sale_order_general_discount is so small and you want to deactivate half of it, you could just replicate its functionality and not depend on it. (and maybe add it to the excludes key in the manifest so people don't accidentally install both).

I think option number 2 (overriding onchange_partner_id is by far the most preferable, though)

aleuffre · 2023-08-31T13:35:46Z

sale_order_general_discount_payment_term/models/account_payment_term.py

+    sale_discount = fields.Float(
+        store=True,
+        readonly=False,
+    )


No point restating obvious defaults IMO. (This is a new field, not an override, correct?)

Instead, I think it'd be good to add the same modifiers that the corresponding field has in sale_order_general_discount

sale_discount = fields.Float( digits=dp.get_precision('Discount'), string='Discount (%)', company_dependent=True, track_visibility="onchange", )

aleuffre · 2023-08-31T13:49:36Z

sale_order_general_discount_payment_term/views/res_partner.xml

+            <xpath
+                expr="//page[@name='sales_purchases']//label[@for='sale_discount']"
+                position="attributes"
+            >
+                <attribute name="invisible">1</attribute>
+            </xpath>
+            <xpath
+                expr="//page[@name='sales_purchases']//div[field[@name='sale_discount']]"
+                position="attributes"
+            >
+                <attribute name="invisible">1</attribute>
+            </xpath>


Suggested change

<xpath

expr="//page[@name='sales_purchases']//label[@for='sale_discount']"

position="attributes"

>

<attribute name="invisible">1</attribute>

</xpath>

<xpath

expr="//page[@name='sales_purchases']//div[field[@name='sale_discount']]"

position="attributes"

>

<attribute name="invisible">1</attribute>

</xpath>

<field name="sale_discount" position="attributes">

<attribute name="invisible">1</attribute>

</field>

is this not sufficient, or does it not work for some reason?

Hi @aleuffre, thanks for your review. Set "invisible" on field sale_discount it doesn't works properly because we need to hide a div with sale_discount and a string inside.

aleuffre · 2023-08-31T13:49:57Z

sale_order_general_discount_payment_term/models/sale_order.py

+    @api.depends("payment_term_id")
+    def _compute_general_discount(self):
+        for so in self:
+            so.general_discount = so.payment_term_id.sale_discount


api.depends and _compute functions are generally used if they're computing the value of a field.

I think this should be an api.onchange

Cf. the module sale_order_general_discount:

@api.onchange('partner_id') def onchange_partner_id(self): super().onchange_partner_id() self.general_discount = self.partner_id.sale_discount return

Hi @aleuffre, I think that is 12.0 version. In v14 this is the code in sale_order_general_discount:

@api.depends("partner_id")
def _compute_general_discount(self):
for so in self:
so.general_discount = so.partner_id.sale_discount

Ah, my bad. If you're overriding an existing compute and it works, then that's fine by me.

aleuffre · 2023-09-04T14:02:00Z

Sorry for my oversights during my first review. Trying to do too many things at once will do that to you.

Adding the field parameters to sale_discount seems to be the only thing left. After that I'll approve.

aleuffre · 2023-09-04T14:20:29Z

sale_order_general_discount_payment_term/models/account_payment_term.py

+        digits="Discount",
+        string="Discount (%)",
+        company_dependent=True,
+        track_visibility="onchange",


as you correctly spotted in the other comment, I was looking at v12 code during my previous review.

track_visibility was a v12 option, in v14 it's simply tracking=True

aleuffre · 2023-09-04T14:21:24Z

Code review looks good aside from that one small thing, please squash everything into a single commit.

francesco-ooops · 2023-09-04T15:49:20Z

@OCA/crm-sales-marketing-maintainers good for merge?

francesco-ooops · 2023-09-11T08:16:58Z

@pedrobaeza could this be merged please?

pedrobaeza · 2023-09-11T08:43:36Z

sale_order_general_discount_payment_term/views/res_partner.xml

+                expr="//page[@name='sales_purchases']//label[@for='sale_discount']"
+                position="attributes"
+            >
+                <attribute name="invisible">1</attribute>


This is not correct. This is out of scope of the module. And there's already a group for hiding this for those you don't want to see it.

@pedrobaeza the point of this change is that if there is a general discount set in partner and another general discount set in payment term they will be in conflict for setting the general discount on SO.

How else do you suggest we avoid this scenario?

OK, then hide it only if the payment term is set and it contains a discount.

Sorry, the flow is unclear to me. What happens if partner 1 has a default general discount set and in SO user selects a payment term with a general discount? Which general discount should be applied?

With this module installed, it's very likely that the only general discount that should be applied is the payment term one.

Shall we make it easier with a flag in settings allowing user to decide whether to hide the general discount in partner field?

You should clear that general discount on payment term assignment IMO.

The question is that you shouldn't remove the possibility of assigning directly a general discount for all cases. Just for those covered by this module (with payment term). If not, you will force to assign a payment term always if you want a general discount.

User has the possibility of assigning general discount in SO in 3 ways:

default from partner

manually from field "discount" in SO

default from payment term

base module "sale_order_general_discount" implements the first two options; this module implements the third.

I think if we add a "hide default general discount in partner" flag in settings to this module we will have the cleaner implementation as the user can decide:

whether to keep all 3 options,

to keep only 2 and 3 (flag enabled), or

to keep 1 and 2 by not installing this module.

Could that work?

I still think that you don't need another extra config. Just don't hide the field if you don't have a payment term selected.

Alright, @odooNextev can you implement that?

@pedrobaeza with the latest change by @odooNextev , if payment term discount = 0 and partner default discount is set, partner default is used in SO; otherwise payment term discount is used. Could this work?

…ent terms

pedrobaeza

/ocabot merge nobump

OCA-git-bot · 2023-09-15T16:25:33Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-2660-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot · 2023-09-15T16:52:27Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

OCA-git-bot · 2023-09-15T16:52:28Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

OCA-git-bot · 2023-09-15T16:55:48Z

Congratulations, your PR was merged at fca3a24. Thanks a lot for contributing to OCA. ❤️

francesco-ooops approved these changes Aug 31, 2023

View reviewed changes

aleuffre suggested changes Aug 31, 2023

View reviewed changes

odooNextev force-pushed the 14.0-add-sale_order_general_discount_payment_term branch from e2a82b1 to 3a64185 Compare September 4, 2023 14:15

aleuffre reviewed Sep 4, 2023

View reviewed changes

odooNextev force-pushed the 14.0-add-sale_order_general_discount_payment_term branch 2 times, most recently from e466597 to 1a04352 Compare September 4, 2023 14:41

aleuffre approved these changes Sep 4, 2023

View reviewed changes

OCA-git-bot added the approved label Sep 4, 2023

pedrobaeza added this to the 14.0 milestone Sep 11, 2023

pedrobaeza requested changes Sep 11, 2023

View reviewed changes

OCA-git-bot removed the approved label Sep 11, 2023

odooNextev force-pushed the 14.0-add-sale_order_general_discount_payment_term branch 3 times, most recently from e6c0a55 to f04fe42 Compare September 15, 2023 14:14

[ADD]sale_order_general_discount_payment_term: discount field on paym…

c43a9cc

…ent terms

odooNextev force-pushed the 14.0-add-sale_order_general_discount_payment_term branch from f04fe42 to c43a9cc Compare September 15, 2023 14:17

pedrobaeza approved these changes Sep 15, 2023

View reviewed changes

OCA-git-bot added the approved label Sep 15, 2023

OCA-git-bot added bot is merging ⏳ ready to merge labels Sep 15, 2023

OCA-git-bot merged commit f9cc5a7 into OCA:14.0 Sep 15, 2023
10 checks passed

OCA-git-bot removed the bot is merging ⏳ label Sep 15, 2023

OCA-git-bot added the merged 🎉 label Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[14.0][ADD]sale_order_general_discount_payment_term: Add sale order general discount on payment term #2660

[14.0][ADD]sale_order_general_discount_payment_term: Add sale order general discount on payment term #2660

odooNextev commented Aug 30, 2023

francesco-ooops left a comment

aleuffre left a comment •

edited

Loading

aleuffre Aug 31, 2023 •

edited

Loading

aleuffre Aug 31, 2023

odooNextev Sep 1, 2023

aleuffre Aug 31, 2023

odooNextev Sep 4, 2023 •

edited

Loading

aleuffre Sep 4, 2023

aleuffre commented Sep 4, 2023 •

edited

Loading

aleuffre Sep 4, 2023 •

edited

Loading

aleuffre commented Sep 4, 2023

francesco-ooops commented Sep 4, 2023

francesco-ooops commented Sep 11, 2023

pedrobaeza Sep 11, 2023

francesco-ooops Sep 11, 2023

pedrobaeza Sep 11, 2023

francesco-ooops Sep 11, 2023

pedrobaeza Sep 11, 2023

pedrobaeza Sep 11, 2023

francesco-ooops Sep 11, 2023

pedrobaeza Sep 11, 2023

francesco-ooops Sep 11, 2023

francesco-ooops Sep 15, 2023

pedrobaeza left a comment

OCA-git-bot commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

[14.0][ADD]sale_order_general_discount_payment_term: Add sale order general discount on payment term #2660

[14.0][ADD]sale_order_general_discount_payment_term: Add sale order general discount on payment term #2660

Conversation

odooNextev commented Aug 30, 2023

francesco-ooops left a comment

Choose a reason for hiding this comment

aleuffre left a comment • edited Loading

Choose a reason for hiding this comment

aleuffre Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odooNextev Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleuffre commented Sep 4, 2023 • edited Loading

aleuffre Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

aleuffre commented Sep 4, 2023

francesco-ooops commented Sep 4, 2023

francesco-ooops commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

OCA-git-bot commented Sep 15, 2023

aleuffre left a comment •

edited

Loading

aleuffre Aug 31, 2023 •

edited

Loading

odooNextev Sep 4, 2023 •

edited

Loading

aleuffre commented Sep 4, 2023 •

edited

Loading

aleuffre Sep 4, 2023 •

edited

Loading