[17.0][ADD] sale_financial_risk_exclude_order_type#561
Conversation
c38e764 to
b5b2e7b
Compare
rrebollo
left a comment
There was a problem hiding this comment.
From a technical perspective, it looks good. However, regarding the design or approach used for the implementation—why not just integrate the risk management directly into sale_order_type? I mean, the glue addon is fine, but I don't understand why you went with the company settings approach. Why not simply incorporate the feature into the sale order type itself?
b5b2e7b to
1338dbd
Compare
Good point, and I agree after revisiting the design. The initial implementation used a separate configuration model to keep the feature highly configurable and isolated from sale.order.type. The idea was to support company-specific rules without modifying the business model itself. However, after reviewing the use case more carefully, the additional configuration layer introduced unnecessary complexity for a behavior that is naturally tied to the order type definition itself. I have now simplified the implementation by moving the configuration directly into sale.order.type through dedicated boolean fields:
This approach is cleaner because it:
The current implementation preserves the same functional behavior while significantly simplifying the architecture. |
…der type This module extends sale_financial_risk to support financial risk behavior configuration directly on sale order types. It allows configuring sale order types to: - exclude orders from risk computation - bypass financial risk blocking validation The implementation intentionally integrates the behavior directly into sale.order.type in order to: - simplify the architecture - avoid unnecessary configuration models - reduce maintenance complexity - align configuration with business semantics Features: - risk computation exclusion by order type - blocking bypass support by order type - independent control of computation and blocking - multi-company compatible behavior - integration with sale_order_type Technical details: - extends sale.order.type - overrides evaluate_risk_message - extends risk computation domain - preserves standard sale_financial_risk behavior Includes automated tests for: - blocking bypass behavior - risk domain generation - separation of concerns - neutral order types - multi-company behavior - domain integrity
1338dbd to
029a921
Compare
There was a problem hiding this comment.
Do we really need two flag fields? There seems to be some common ground between the two behaviors. Could you explain this more clearly?
Based on the naming alone, "excluded" appears to result in no sale financial risk behavior, while "allow blocking" seems to do the same. Perhaps the issue is just a lack of clear documentation.
If both paths are legitimate and distinct, I would suggest implementing a single selection field with a range of choices. That said, it's possible that I simply lack enough functional knowledge of the addon.
rrebollo
left a comment
There was a problem hiding this comment.
Code Review. LGTM!
After reading the documentation, I understand that the two paths address distinct scenarios within the financial risk framework.
@BinhexTeam
Description
This module extends
sale_financial_riskto supportfinancial risk behavior configuration directly on
sale.order.type.It allows configuring sale order types to:
The implementation intentionally keeps the design
simple and business-oriented by integrating the
configuration directly into sale order types.
Features
sale_order_typeIncluded components
sale.order.typeextensionsDesign considerations
The implementation intentionally avoids introducing
additional configuration models.
The module:
sale_financial_riskbehaviorTechnical details
sale.order.typeevaluate_risk_message_get_risk_sale_order_domainDependencies
Testing
Includes automated tests for:
Checklist