Conversation
454fb94 to
85196f2
Compare
85196f2 to
4999a6c
Compare
4999a6c to
3034469
Compare
I didn't fully understand your question, but I think you are asking if it is possible to base the price tiers on the margin rather than the product price. We decided to take this approach because cost price variability (when using FIFO) can be confusing for the Commercial User, and also because our client requested it this way (and we agreed with the decision). The idea behind this module is to give Commercial User some flexibility to set prices and earn commissions based on that. We never considered the scenario where the discount drops below the cost price, because the Company must always make a profit regardless of the applied discount. Example: A product has a price of €100 and a cost of €50. We would never create a 50% tier (adjusted to the cost price, even if variable) because the company wouldn't make money on that transaction. We would apply tiers of 5%, 15%, 25% (for example) so that in the worst-case scenario, the company still gains 25%. If we were to provide a configuration option to define the base for the tiers (Product Base Price vs. Margin), it wouldn't be as simple as just enabling a checkbox. The tiers would change completely because they would be compared against a much lower value (the margin). Similarly, we would still need to calculate the unit prices that the Commercial User choose based on the widget, so the computational load would remain practically the same (though I might save on unit conversion). I hope this answers your question. I'm open to further questions. |
3034469 to
3c7bdee
Compare
|
This PR has the |
3c7bdee to
88a1ddf
Compare
|
Upgraded. Now you can customize Text and Icon via system parameter. Also Pricelist tier is checked only if Final Price is below Non Compliant. Price between last tier (T3 for example) and Pricelist, will be considered Pricelist (since you are improving the margin for the company). |
7a35740 to
dd97569
Compare
|
Now labels on the views shows the custom text if System Parameter is used |
sale_price_compliance/static/src/widgets/price_compliance_level_widget.xml
Outdated
Show resolved
Hide resolved
dd97569 to
889fe64
Compare
|
@chienandalu separated selection field into a mixin and inherited the mixin in the other modules of the PR series |
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for this well-designed module, Eduardo. The mixin-based architecture with Product > Category > Company fallback is clean, and the SQL constraints are thorough. I went through the full diff and have a few observations that are all non-blocking.
Overall
The code is solid. Good use of pre_init_hook to avoid expensive recomputation on install, proper ensure_one() guards, and the tier customization via system parameters is a nice touch. Tests cover the main flows well including constraint validation and parameter customization.
A few minor notes:
-
Threshold ordering is not enforced -- the SQL constraints check positivity, max 1.0, and no gaps, but nothing prevents
t1=0.30, t2=0.10, t3=0.20(i.e. t2 < t1). The tier computation in_compute_price_compliance_tierwould still work because it iterates the tiers sequentially, but it might confuse users configuring the thresholds since the discount ranges would overlap. Might be worth a constraint liket1 <= t2 <= t3or at minimum a note in the docs. Not blocking since the computation handles it correctly. -
_get_price_compliance_thresholdsusesif not threshold-- this means a threshold of exactly0.0is treated as "not set", which is consistent with the gap constraint logic. Just flagging it as intentional sincefloat(0.0)is falsy in Python. -
Minor typo in docstring --
_get_price_compliance_thresholdssays "thresolds" instead of "thresholds" (line in config mixin). -
f-stringin logger warning -- in_get_tier_selections, the line_logger.warning(f"Wrong parameter value for {param}")uses an f-string. OCA guidelines generally prefer_logger.warning("Wrong parameter value for %s", param)with lazy formatting to avoid string formatting when the log level is disabled.
Nice work on the widget and the popover UX.
| "Price Compliance Thresholds Percentage should be positive.", | ||
| ), | ||
| ( | ||
| "check_price_compliance_le_1", |
There was a problem hiding this comment.
Nit: the SQL constraints enforce positivity, max 1.0, and no gaps, but they don't enforce monotonic ordering (t1 <= t2 <= t3). A user could set t1=30%, t2=10%, t3=20% which would lead to overlapping ranges in the widget. Not blocking, but worth considering as a future improvement or documenting the expected behavior.
| icp_st = self.env["ir.config_parameter"].sudo().get_param(param) | ||
| try: | ||
| st_dict = safe_eval(icp_st) | ||
| except Exception: |
There was a problem hiding this comment.
Minor: OCA convention prefers lazy logger formatting over f-strings:
| except Exception: | |
| _logger.warning("Wrong parameter value for %s", param) |
| ), | ||
| ( | ||
| "check_price_compliance_le_1", | ||
| """CHECK ( |
There was a problem hiding this comment.
Tiny typo: "thresolds" -> "thresholds" in the docstring.
alexey-pelykh
left a comment
There was a problem hiding this comment.
Nice module, thanks @Shide for the contribution!
The threshold fallback chain (product -> template -> category -> company) is well designed, and the SQL constraints for gap/overlap prevention are solid. The pre_init_hook for pre-creating columns is a good touch for install performance. OWL widget looks clean too.
A couple of minor things I noticed:
-
Copyright year in
__manifest__.py: It says 2025 while all other files use 2026. Probably just an oversight from when the module was started. -
f-string in logger (
price_compliance_threshold_tier_mixin.py): The_logger.warning(f"Wrong parameter value for {param}")should use lazy formatting (_logger.warning("Wrong parameter value for %s", param)) per Python logging best practices. -
No test for the confirmation blocking flow: The
_action_confirm/_check_compliant_pricingpath is core to the module's value -- blocking non-compliant orders for regular users and the manager bypass withmessage_post. Would be great to have a test covering both theUserErrorraise and the manager bypass scenario. Could be a follow-up.
None of these are blockers given the existing approvals and the overall quality of the module. Good work!

This PR introduces a new Price Compliance mechanism to help sales users adhere to pricing policies.
It provides visual feedback via a widget and flexible configuration options to define discount/price thresholds.
Key Features
Price Compliance Widget
Flexible Configuration Hierarchy
Pricelist Compatibility
https://www.loom.com/share/b6d97d85617747b1a708a539af4a9d94
PR Series
MT-12373 MT-13322 @moduon @rafaelbn @fcvalgar @chienandalu @yajo @EmilioPascual @Gelojr please review if you want 😄