[14.0][ADD] sale_commission_delivery_partner: Use Delivery Address Agents

[Chionne27]

Superseeds #537

[OCA-git-bot]

Hi @PicchiSeba, @renda-dev, @ilyasProgrammer, @aleuffre,
some modules you are maintaining are being modified, check this out!

[francesco-ooops]

Functional ok!

@pedrobaeza what do you think?

[aleuffre]

Partial review. See comment

[aleuffre]

issue: overwriting functions without calling super() is an anti-pattern and should be avoided at all costs, especially in Odoo where several different configurations of modules could be installed. In this case, _compute_agent_ids is redefined by multiple modules, and I'm not at all sure about what the final result would be (even if the tests are currently green).

There are indeed some extreme cases where it's unavoidable, and those cases should at least be clearly signposted with comments explaining how and why this choice was made.

I think there's a fairly clean solution here that would allow us to avoid having to overwrite the function:

1. in the sale_commission module, in sale.commission.mixin, introduce a hook function: _get_partner_for_commission or something like that. That function would return the correct partner to use to calculate commissions. In sale.commission.mixin that function would just raise a NotImplementedError().
2. In the sale_commission module, in the account.move.line and sale.order.linemodels, implement the "concrete" function _get_partner_for_commission. They would return self.move_id.partner_id and self.order_id.partner_id respectively.
3. In the sale_commission module, make a small change to _compute_agent_ids so that your new function _get_partner_for_commission is called instead of having partner_id hardcoded in the function. Do this also in other modules that override _compute_agent_ids.
4. In your new module, overwrite only _get_partner_for_commission to return self.move_id.partner_shipping_id and self.order_id.partner_shipping_id. You can still do a simple override of _compute_agent_ids to add the field you need to api.depends like this

    @api.depends("move_id.partner_shipping_id")
    def _compute_agent_ids(self):
        super()._compute_agent_ids()

What do you think?

[Chionne27]

Hi @aleuffre. Thank you fo the suggestion. This approach actually improves both maintainability and flexibility of code. Let me know if everything is right

[aleuffre]

I think the current implementation is missing point 4 and part of point 3.

[aleuffre]

See other comment