[16.0][IMP] account_payment_partner: add hook to get recipient bank

[LauraCForgeFlow]

This improvement adds a hook to be able to inherit easily the logic in which the partner bank is assigned. In this module, if there is no payment mode, the partner bank is set to False, but that may not be desirable in all cases.

[pedrobaeza]

Please give more details about the change... As is, it doesn't seem a correct or desirable change

[pedrobaeza]

If you need to change the behavior, you shouldn't do a hook, but override the compute method, filter the records you want to modify the behavior and assign the value, and call super with the rest of the records.

[LauraCForgeFlow]

Hi @pedrobaeza, we already thought about doing what you suggest, but the problem is that this module leaves empty the partner bank in different conditions depending on whether we have a payment mode, whether it is an invoice or a bill, etc.

If we override the compute method, we need to replicate again all these evaluations on the new specific module. By using the hook, we can simply check if the logic on this module is leaving the result empty or not, and then apply our desired custom behavior.

Moreover, calling the super() method with only a subset of the records, as you also suggest, can be problematic, because if there are other modules applying a different logic to assign the partner bank, we need to be able to call the super() method with all of the records.

What do you think?

[pedrobaeza]

I understand that it seems more complicated, and it's in some cases, but at the end, it's the winning strategy, as you don't always have the option to add these hooks, and you develop a strategy for how to deal with any computed field. Anyway, even having the option, this complicates the code, making it less readable.

For both cases you have said, the solution is to call super with all the records before doing your code, and then acting in the cases you like. Check after calling super which ones are empty, and then what you pretend to do. In other cases, it's better to not call super with records that you want to have predictable results. It's up to you to decide how to behave.

And about the performance, of course these strategies causes a bit of overhead, but they are barely appreciable in the general computation.

[LauraCForgeFlow]

Our main issue revolves around the compute method resetting the field to False, which overrides the value set by the super() method.

If you have a specific case where you want to use this module's logic in case there is a payment mode, as well as the base module's logic when there is no payment mode, you cannot do it anymore, since the value computed in the base module will have been overridden by this method (and set to False in case there is no payment mode).

Still, we will try to implement it by overriding the compute method, as you suggested.

[pedrobaeza]

For such case, you can do:

    def _compute_partner_bank_id(self):
        prev_data = {x: x.partner_bank_id for x in self}
        res = super()._compute_partner_bank_id()
        for record in prev_data:
            if not record.partner_bank_id and prev_data[record]:
                record.partner_bank_id = prev_data[record]
        return res

[LauraCForgeFlow]

This won't work for us, since we want to use the standard logic for setting the partner_bank_id in case there we do not have payment mode.

With the method you suggested, after the field is set in the base module, it gets overwritten by the method in the account_payment_partner module, and set to False, so we have no way of knowing what value was computed originally (in the base module). That's why we thought of using the hook, to make sure that the value set by the standard logic of this method was not overwritten and set to False by the account_payment_partner module.

I hope this explanation clarifies the issue. :)

[pedrobaeza]

OK, got it, but I think in this case the best is that you overwrite the compute method and re-add the same logic as in core in your custom. Although copy/paste is not the recommended technique for most of the things, in this case, you are assuring that you get the desired behavior no matter if there are upstream changes here on in Odoo core, and being very few lines of code, it's not a big burden.

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.