Bug 21988 - Add SMSSendFrom system preference for specifying a source number for SMS messages
Summary: Add SMSSendFrom system preference for specifying a source number for SMS mess...
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-12 15:06 UTC by Andrew Isherwood
Modified: 2024-07-04 20:38 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 21988: Add support for SMSSendFrom syspref (4.33 KB, patch)
2018-12-12 15:53 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21988: (follow-up) Provide more open params (5.69 KB, patch)
2018-12-13 14:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21988: Add support for SMSSendFrom syspref (4.36 KB, patch)
2018-12-13 14:48 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 21988: (follow-up) Provide more open params (5.72 KB, patch)
2018-12-13 14:48 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2018-12-12 15:06:51 UTC
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.
Comment 1 Andrew Isherwood 2018-12-12 15:14:15 UTC
Thinking more about it, naming the syspref SMSSendFrom is more consistent with the other SMS prefs
Comment 2 Andrew Isherwood 2018-12-12 15:53:18 UTC
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.
Comment 3 Andrew Isherwood 2018-12-13 12:29:48 UTC
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
Comment 4 Andrew Isherwood 2018-12-13 14:25:41 UTC
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
Comment 5 Kyle M Hall (khall) 2018-12-13 14:48:25 UTC
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>
Comment 6 Kyle M Hall (khall) 2018-12-13 14:48:36 UTC
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 7 Andrew Isherwood 2019-01-09 10:39:14 UTC
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 8 Andrew Isherwood 2019-01-09 10:47:24 UTC
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
Comment 9 Andrew Isherwood 2019-01-09 10:49:55 UTC
D'oh! Ignore me, I was misreading the second patch, it replaces the contents of the first one, so no obsoleting required.
Comment 10 Jonathan Druart 2019-02-26 23:54:30 UTC
Should not it be in the config (sms_send_config) instead?
Comment 11 Andrew Isherwood 2019-02-27 09:58:55 UTC
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?
Comment 12 Jonathan Druart 2019-02-27 13:03:00 UTC
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.
Comment 13 Jonathan Druart 2019-02-27 13:03:24 UTC
...to avoid *the* file...
Comment 14 Andrew Isherwood 2019-02-28 08:35:57 UTC
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.
Comment 15 Andrew Isherwood 2019-03-13 09:58:09 UTC
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.
Comment 16 Katrin Fischer 2023-10-07 23:13:39 UTC
(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.