[16.0][ADD] sms_alternative_provider, sms_messagebird

[hbrunn]

This module allows configuring multiple providers for sending SMS.

By restricting providers to country or area codes, you can ie have Odoo pick different providers for different countries.

[thomaspaulb]

@sebastienbeau This is an attempt to produce something a step up from what you have done with iap_alternative_provider, unfortunately the both cannot be reconciled. What do you think about it?

[hbrunn]

another discussion point: Given that basically all modules that mess with sms at all override sms.api#_send_sms_batch and/or sms.sms#_postprocess_iap_sent_sms, it's crucial that the overrides of sms_alternative_provider come after any other override.

I'm of a mind to patch into loading the module graph and inject a dependency on sms_alternative_provider for every module that depends on sms if sms_alternative_provider is installed. Do you agree, or should we leave this to glue modules to be done on a per case basis?

[thomaspaulb]

it's crucial that the overrides of sms_alternative_provider come after any other override.

I'm not quite sure of this TBH: let's say someone wanted to use sms_ovh_http in its current form next to this module, they could even without the patch, because the OVH messages will just be caught earlier but when super comes, ours will be sent.

That's not a very likely scenario, but in other cases, if I would override send_sms_batch for other reasons and I call super before I do some stuff, I would want the SMS already be sent when the super returns; your proposal would mess with that.

Could you give an example use case where someone would want this?

[hbrunn]

sms_ovh_http works in both cases, so that's not a good example. sms_masked_content would have a problem, but that's handled accidentally right because modules on the same level (=same dependencies, both depend just on sms) are sorted alphabetically.

So if you had another module also just depending on sms, but named anything_sorted_before_sms_alternative_provider, and it does something to messages that should affect all messages, currently this would only apply to messages sent via iap, because messages from other providers will be funneled away at this point already.

[thomaspaulb]

Ah... right. Yes. Well your dependency idea does sound like a nice solution, so IMO you can go for it!

[hbrunn]

ja hmm, i didn't think this through: at the point we can patch something, a module that we want to depend on ours might already be loaded, meaning its overrides are already incorporated into the models in the registry (odoo doesn't load the python code any more before actually loading the module). So we'd have to rebuild those models, remove the module from the module graph and have it reloaded afterwards.

that's a very heavy operation for a theoretical problem, so I suggest we stash this idea for now.

[thomaspaulb]

As for me I'm happy with the way it is now, we can work with it.

[thomaspaulb]

Hey, the message dicts are not passed here,

[hbrunn]

there are none at this point, because this function is called in sms.sms#_send, which operates on sms.sms records, and those you can access via results[i]['id']

actually I don't understand any more why you requested having the message dicts passed here in the first place. If a provider wants to do provider-specific stuff, why wouldn't it do that in _send_$provider after sending? And note that whatever a provider wants to write on an sms.sms record, it can just add to its result dictionaries.

[thomaspaulb]

So in some cases, handle_results=False, which does not call handle_results directly after sending; instead it's being called from postprocess_iap_results, but then without the original message dicts.

What are the scenarios for having handle_results=True or False?

[hbrunn]

we want to support just saying

self.env["ir.sms.gateway"]._send([some dicts, may or may not link to sms.sms records])

in which case the send function should call _handle_results, and here it shouldn't because _postprocess_iap_sent_sms does it. So this clumsiness is caused by this module trying to introduce a new kind of sms api, while staying compatible with overrides other modules might do for the original api. We could consider dropping this.

[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.

[thomaspaulb]

Applying no stale since I think this sms_alternative_provider approach we use is important to be included, as the current iap_alternative_provider approach is limited. @OCA/connector-telephony-maintainers

[NL66278]

👍 LGTM

[thomaspaulb]

/ocabot rebase

[OCA-git-bot]

Congratulations, PR rebased to 16.0.

[hbrunn]

@thomaspaulb since OCA/OCB@851f83f the tests need another patch to succeed. Then the sms_ovh_http code only succeeds because it creates its own sms account, if that doesn't exist we run into https://github.com/OCA/OCB/blob/16.0/addons/iap/models/iap_account.py#L55 which returns an account from another transaction, causing accessing it afterwards to fail. So I think it's prudent anyways to test for existence there.

If you agree with the latest two fixup commits, please give me the chance to squash commits before merging

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖