CRM-21751: Move SMS provider ID to 'Select Recipients' page from 'SMS content'

[monishdeb]

Overview

The need of this improvement is because of refactoring changes made in CRM_Mailing_BAO_Mailing::getRecipients() (under ticket CRM-21316) as per which we now rely on sms_provider_id to decide whether to fetch recipients for SMS or mailing. But this has caused regression to SMS compose UI where we used to fetch recipients before sms_provider_id is added to mailing record. The reason why getRecipients() operates as per mailing mode, which is wrong.

This ticket is about moving the sms_provider_id field 'Select Recipients' screen from 'SMS content'. This will allow us to set sms_provider_id before the getRecipients() got executed.

Before

'Select Recipient' screen:

'SMS content' screen:

After

'Select Recipient' screen:

'SMS content' screen:

Technical Details

There was an alternative solution #11628 by setting the default sms_provider_id at the backend to resolve the regression, but I think it won't be right to set the provider id without the consent of end-user.

---

• CRM-21751: Move SMS provider ID to 'Select Recipients' page from 'SMS content'
• CRM-21316: Refactor getRecipients fn by creating a new fn getMailRecipients, that will be used ONLY to fetch and store mail recipients

[monishdeb]

ping @JKingsnorth @eileenmcnaughton

[monishdeb]

Another simplest alternative to fix that regression would be to retain the $mode parameter of getRecipients fn. But that would be too manual approach in code pov as we always need to rely on an extra parameter to differentiate the mode rather than using mailing.sms_provider_id

[monishdeb]

If this PR is approved than I will create a user-doc PR to highlight the new changes in UI screen (that is make changes in https://github.com/civicrm/civicrm-user-guide/blob/master/docs/sms-text-messaging/everyday-tasks.md file)

[eileenmcnaughton]

The code looks logical on a quick look - I'm hoping @JKingsnorth can confirm since he is the one who has been working with you this

[JKingsnorth]

Jenkins, retest this please.

This seems like a reasonable approach. The reason I didn't do this before is because I think some extensions are tied in to this form process to provide custom functionality for SMS providers. I wanted to try to avoid making any significant changes to the process in order to fix the regression.

But it this is the preferred approach then OK [= I think @michaelmcandrew might have more experience with this area of the system than me, and I might not get a chance to look at this in detail today.

[JKingsnorth]

Also pinging @mattwire who's done work with SMS recently.

[eileenmcnaughton]

@monishdeb @JKingsnorth if there is doubt there is also the option of merging @JKingsnorth one in time for 4.7.31 & continuing on with this cleaner one to hit 4.7.32?

[michaelmcandrew]

I haven't created (and can't see any extensions published in the extension directory - at least not ones with SMS in the title) that change that screen so I would say that it is fine.

Tangent: I don't really think the option should be there anyway - how many people use more than one SMS provider?

[JKingsnorth]

I guess it's there because it is 'possible' to add more than one provider. We can still default this to the main provider (as per my code in the other PR - but in the setDefaults instead).

[monishdeb]

Tangent: I don't really think the option should be there anyway - how many people use more than one SMS provider?

I agree with @michaelmcandrew During the fix I was wondering what could have been the requirement of having multiple SMS providers and that you can set them all by default.
Two alternatives:

1. Allow only one sms provider which on submission will be available as
   Civi::settings->get('sms_provider_id') and we no longer need to provide option in SMS composer screen.
2. Allow multiple but consider only one as default and that one should be set as the default in 'Select Recipients' screen

[monishdeb]

Jenkins test this please

[JKingsnorth]

2. is my preference, as per my previous comment.

I don't see the benefit of stripping out the functionality that allows for multiple SMS providers since it's already in place. One use case might be if you're using different SMS providers for different world regions.

[monishdeb]

Implementing #2 (or either #1) is not relevant to this issue, rather can be treated as an improvement. In my opinion, we need to fix it under a separate JIRA ticket. As this PR fixes the regression I think we need to get this PR merged ASAP after review as 4.7.31-rc branch is soon going to be cut.

What do you say guys?

[seamuslee001]

Jenkins re test this please

[michaelmcandrew]

Implementing #2 (or either #1) is not relevant to this issue

Yes - agree that it is tangential - didn't mean to sidetrack the issue. I don't see any reason to not go with this change.

[eileenmcnaughton]

There seems to be soft consensus on this & as it addresses a immediate regression I'm going to merge it to the rc. Please continue the discussion on & if need be agree further patches for the rc or master

[monishdeb]

Thanks @eileenmcnaughton