[ADD] sale_pricelist_sale_margin module

[angelmoya]

Adds module that allows to set a sale margin on the pricelist.
The margin is calculated as a percentage of the cost price, list price or other price list.

This is usefull when company wants to define pricelist by sale margins... you can do "haking" formula pricelist, but is no human friendly... for example:

• if you want 40% of margin on sales, you can put in formula discount = - ((1 / (1 - (40/100))) - 1) x 100 = -66,6666666 so discount of -66,66666% is equal to margin 40%

With this module you can put directly margin = 40%

[pedrobaeza]

On the name, the second sale can be removed.

[legalsylvain]

Hi @angelmoya
Thanks a lot for your contribution. your need is legit.
however I think that the implementation could be optimized.

altervative :

• Base your feature on the existing "discount" feature.
• add the sale_margin field, as you did, and make it a compute readonly=False, store = True, and make price_discount computed on sale_margin.
• So when you write 40% in the margin field, it will write -66% in the discount field. And the inverse also.

Pro:

• lighter code.
• doesn't add a new key in the compute_price

In fact discount and margin are the same concept, but seen in a different way.

what do you think ?

thanks !

[angelmoya]

Hi @legalsylvain , I did not do this way because I was thinking that I could have problems with rounding... but testing it I can not reproduce rounding issues that I spect... so I will do this way

[angelmoya]

@pedrobaeza module name changed!
@legalsylvain work done!

[legalsylvain]

Hi. looks lighter !
I wonder if it's possible to :

• remove discount_type :
• keep visible price_discount AND sale_margin and make them computable depending on the other.

only a remarks. not a blocking point.

thanks !

[angelmoya]

Hi @legalsylvain,

IMO this functionality is better for user, it's more clean. By default it works as standard, and if you set Discount type: Sale Margin, you can put margin, and discount is useless for you.

Also mantain two fields showed require mantain two computed fields related one by other.

But if someone else think that is better to show both fields maybe we can work on that.

[legalsylvain]

Hi. thanks @angelmoya. Well, it was just a suggestion to make ligther changes in database. in fact, it's a matter of taste. maybe some user want to see the conversion shown. (I mean see -66% AND 40% displayed). but maybe other users prefer to see only one field.
Never mind, as said, not a blocking point !

[alexey-pelykh]

Thanks for this module, the use case is clear and practical.

I have a few concerns with the current implementation:

1. price_discount compute runs unconditionally -- The _compute_price_discount method always overwrites price_discount from sale_margin, regardless of discount_type. When a user selects "Discount" type and enters a discount percentage, the compute will override it (since sale_margin defaults to 0, price_discount becomes 0). The compute should be conditional:

@api.depends("sale_margin", "discount_type")
def _compute_price_discount(self):
    for record in self:
        if record.discount_type == "sale_margin":
            record.price_discount = (
                -1 * ((1 / (1 - (record.sale_margin / 100))) - 1) * 100
            )

Without the condition, installing this module breaks standard discount behavior on all pricelist items.

2. Division by zero when sale_margin is 100 -- The formula divides by (1 - sale_margin/100). Setting margin to 100% causes a ZeroDivisionError. A guard is needed:

if record.sale_margin >= 100:
    record.price_discount = 0  # or raise a validation error

Arguably a @api.constrains preventing sale_margin >= 100 would be cleaner since a 100% margin is economically meaningless (infinite price).

3. Missing store=True on the redefined price_discount -- The original price_discount on product.pricelist.item is a stored field. Redefining it as computed without store=True means the value will not be persisted and will be lost on cache invalidation / server restart. This needs store=True to match the original field behavior.

4. License mismatch in test file -- The copyright header in test_pricelist_margin.py references LGPL but the manifest declares AGPL-3. Should be:

# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html)

5. Minor: the PR title still references the old module name sale_pricelist_sale_margin -- could be updated to sale_pricelist_margin to match the rename.

Items 1-3 are functional issues that should be addressed before merge.

[legalsylvain]

Hi @alexey-pelykh. Could you stop spamming OCA PRs ? Thanks a lot.

Did you saw the according mailing list that talk about you and your behavior ?

[alexey-pelykh]

@legalsylvain I was asked to stop, so I did. I did not bother to check the discussion, neither I have intent to participate in it.

[legalsylvain]

@legalsylvain I was asked to stop, so I did. I did not bother to check the discussion, neither I have intent to participate in it.

thanks. could you update your review ? your bot is blocking the PR, for the time being.

neither I have intent to participate in it.

No problem if you don't want to participate. However, an apology would be welcome, because activating a feature that spams lots of users on hundreds of PRs is a bit annoying !

You see!

[alexey-pelykh]

an apology would be welcome, because activating a feature that spams lots of users on hundreds of PRs is a bit annoying !

I have no regrets of stirring up some activity in PRs, even if it has false-positives and at the expense of some annoyance. As a bonus (for me) - the false-positives were factored in so that the tooling is better. That is anyway better than having PRs expire because nobody ever did review, even semi-automated one.

could you update your review ?

Absolutely, what points from the review are inaccurate or inappropriate?

[legalsylvain]

Absolutely, what points from the review are inaccurate or inappropriate?

If you think some part of the bot message are accurate, please rewrite review as usual. I mean add comments inline, when you are commenting part of code. otherwise, it's a mess to review your review.

(Note: In other PR when you make bot posting reviews, other people said the same thing.)

if you have no time, no problem, but just remove the "requested change" the bot put.

thanks.

[alexey-pelykh]

@legalsylvain I do think that those points are legit.

I mean add comments inline, when you are commenting part of code. otherwise, it's a mess to review your review.

Please explain why your request to redo the review should be honored? Do we have rules on the review format that I'm not aware of? If so, please point to them - I will gladly correct this mistake so that rules are followed.

Otherwise it's subjective. Not liking review format does not make it less valid, you know.

[legalsylvain]

I give up ! you won.