Some SMS gateways (specifically, in my case, http://123-txt.com) require a "source" number to be specified which all SMS messages appear to be sent from. This bug proposes adding a SMSSendDriverFromNumber syspref, similar to the new EmailSMSSendDriverFromAddress (Bug 20356), to allow a source number to be specified.
Thinking more about it, naming the syspref SMSSendFrom is more consistent with the other SMS prefs
Created attachment 83132 [details] [review] Bug 21988: Add support for SMSSendFrom syspref This patch adds a SMSSendFrom syspref which allows a "from" number to be provided to SMS drivers that support it. It is passed to the driver as a "_from" parameter, in line with the formatting of the _username and _password parameters. Testing: Since this work depends on an SMS driver that supports it, it is virtually impossible to test. I have added the mocking of the SMSSendFrom preference to t/SMS.t, but can't really take it beyond that. Other than the adding of the syspref, a very tiny modification is made to C4/SMS.pm, but I don't anticipate this having an adverse effect. SMS drivers that do not support the _from paramter will simple ignore it.
Test plan: - Apply the patch and do the database update - Navigate to the SMS preferences - TEST => Observe that a new "Define a number from which SMS messages will be sent" preference exists - Enter a value in to the preference - Navigate away from the preferences - Navigate back to the SMS preferences - TEST => Observe that the value entered is preserved
Created attachment 83170 [details] [review] Bug 21988: (follow-up) Provide more open params Rather than specifying a very specific additional parameter to be passed to the provider, we now provide the ability for the user to specify a JSON object containing arbitrary additional parameters, which are then combined with the core parameters that are sent to the provider. Test plan: - Apply the patch and do the database update - Navigate to the SMS preferences - TEST => Observe that a new "Provide additional parameters to be passed to the SMS provider (as a JSON object)" preference is displayed - Enter a JSON object in to the preference - Navigate away from the preferences - Navigate back to the SMS preferences - TEST => Observe that the value entered is preserved
Created attachment 83175 [details] [review] Bug 21988: Add support for SMSSendFrom syspref This patch adds a SMSSendFrom syspref which allows a "from" number to be provided to SMS drivers that support it. It is passed to the driver as a "_from" parameter, in line with the formatting of the _username and _password parameters. Testing: Since this work depends on an SMS driver that supports it, it is virtually impossible to test. I have added the mocking of the SMSSendFrom preference to t/SMS.t, but can't really take it beyond that. Other than the adding of the syspref, a very tiny modification is made to C4/SMS.pm, but I don't anticipate this having an adverse effect. SMS drivers that do not support the _from paramter will simple ignore it. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 83176 [details] [review] Bug 21988: (follow-up) Provide more open params Rather than specifying a very specific additional parameter to be passed to the provider, we now provide the ability for the user to specify a JSON object containing arbitrary additional parameters, which are then combined with the core parameters that are sent to the provider. Test plan: - Apply the patch and do the database update - Navigate to the SMS preferences - TEST => Observe that a new "Provide additional parameters to be passed to the SMS provider (as a JSON object)" preference is displayed - Enter a JSON object in to the preference - Navigate away from the preferences - Navigate back to the SMS preferences - TEST => Observe that the value entered is preserved Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment on attachment 83175 [details] [review] Bug 21988: Add support for SMSSendFrom syspref Obsoleting, as https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83176&action=diff supercedes this patch
Comment on attachment 83175 [details] [review] Bug 21988: Add support for SMSSendFrom syspref Unobsoleting as it breaks application due to the missing ancestor. I'll recreate these patches properly reverting the first one
D'oh! Ignore me, I was misreading the second patch, it replaces the contents of the first one, so no obsoleting required.
Should not it be in the config (sms_send_config) instead?
I was completely unaware of sms_send_config and, reading C4::SMS::send_sms, it appears to already do what this patch was intending to do, albeit in a slightly less end-user friendly way. There is a general move towards making things more visible via sysprefs, so there could be an argument that this furthers that effort?
What you are trying to achieve here is exactly the point of the config file, and avoid 1 pref per custom parameter :) So we could move the config file to a pref, but I think we put it there to avoid to file to be modified from the interface.
...to avoid *the* file...
I'm happy either way to be honest. I guess ditching this and keeping it as it currently is avoids having to introduce a migration path. I guess it's whether having it as a syspref is sufficient benefit to warrant the hassle of users having to move their config to it.
We've decided that we're going to abandon this as it's replicating functionality that already exists. It's true that moving it to a syspref makes it more user friendly, but it also introduces a migration path that we don't have the time to tackle. Leaving as "In discussion" in case anyone at any point in the future thinks it's worth tackling.
(In reply to Andrew Isherwood from comment #15) > We've decided that we're going to abandon this as it's replicating > functionality that already exists. It's true that moving it to a syspref > makes it more user friendly, but it also introduces a migration path that we > don't have the time to tackle. > > Leaving as "In discussion" in case anyone at any point in the future thinks > it's worth tackling. Closing as WONTFIX for now.