Add Clickatell integration to FaxSMS module. #7838

ChrisOstler · 2024-11-22T00:11:58Z

Short description of what this resolves:

This adds support for Clickatell as an SMS provider in the FaxSMS module. It has been tested to work both for sending using API actions (through the Secure Messaging Send SMS) and notifications (rc_sms_notification.php). The data tables in the Services/SMS page display, including entries for sent SMS reminder notifications.

Does your code include anything generated by an AI Engine? No

ChrisOstler · 2024-11-22T04:11:44Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/AppDispatch.php

@@ -49,6 +50,7 @@ public function __construct($type = null)
        if (empty(self::$_apiModule)) {
            self::$_apiModule = $_REQUEST['type'] ?? $_SESSION["oefax_current_module_type"] ?? null;
        }
+        $this->crypto = new CryptoGen();


Was there a reason instantiating this was deferred to the sub classes? It seemed odd not to just do it here, since all sub classes would need it for saveSetup().

ChrisOstler · 2024-11-22T04:13:33Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/AppDispatch.php

+    /**
+     * @return null
+     */
+    protected function index()


This was invoked as part of the dispatchActions(). I didn't take the time to figure it out, but implementations in other sub classes didn't seem to do anything, so I thought it would simplify new providers if there was a worked default to use.

I believe index was meant for the API use but unsure.

ChrisOstler · 2024-11-22T04:16:59Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/ClickatellSMSClient.php

+     */
+    public function sendSMS(string $toPhone = '', string $subject = '', string $message = '', string $from = ''): string
+    {
+        // $subject and $from do not have valid/meaningful values passed in


I was curious why there were parameters for subject and from, when there were not values passed in. The notification script generates the $db_sms_msg, but doesn't populate those fields and falls back to empty strings, instead.

message comes from dialogs or passed in. $db_sms_msg is a canned template in setups.

In the old scripts, $db_sms_msg was queried from the database. In the new script, it instead is generated in place in the script. Since we never set values for email_subject or email_sender, when we try to pull out values for parameters to sendSMS() we'll always fall back to the empty strings.

(Since an SMS doesn't have a separate subject I wouldn't expect to even have it as a parameter, unless it was to unify an interface with email. For emails, though, there aren't parameters for sender or subject, and instead those are hardcoded or global variables. Not anything that needs to be covered for this PR, but something I noticed on the way as a fresh set of eyes.)

If you don't need then don't use. I'm unsure why they're there and may go back to five years ago. Message/comment comes from send SMS dialog events. Some internal send events allow for sending to both email and SMS especially sending onetime tokens. Some dialogs were shared at one time to conserve on firing events.
Look at NotificationEventListener.php and interface/modules/custom_modules/oe-module-faxsms/openemr.bootstrap.php

ChrisOstler · 2024-11-22T19:30:21Z

Thanks for looking at this, @sjpadgett . Is there anything that you'd want to point in a different direction, or should I go ahead and publish this (and close #7823)?

sjpadgett · 2024-11-22T19:35:51Z

Close other and we'll get this in after I look over and do some testing.
Are you going to be on call tomorrow?

sjpadgett

I'm impressed, good job!
This will help others to add clients I hope.
There are some consolidations I see for later.
Once you're done I do a quick test then we can merge.
After doing these modifications the Send SMS button in Secure Messaging will work.

If planning on doing Pinpoint should be it's own PR which I'm very much looking forward.

Thanks for shifting gears and taking up the module. Doing so helps keep a single focus for all our notifications.

sjpadgett · 2024-11-23T20:10:50Z

interface/modules/custom_modules/oe-module-faxsms/openemr.bootstrap.php

+    $menuItem->label = xlt("SMS");
    $menuItem->url = "/interface/modules/custom_modules/oe-module-faxsms/messageUI.php?type=sms";
    $menuItem->children = [];
    $menuItem->acl_req = ["patients", "docs"];


Showing vendor helps advertise vendor and maintains legacy menus.
Add to top of method

$sms_label = match ($allowSMS) { '1' => xlt("RingCentral Messaging"), '2' => xlt("Twilio Messaging"), '5' => xlt("Clickatell Messaging"), default => xlt("SMS"), }; $fax_label = match ($allowFax) { '1' => xlt("RingCentral Fax"), '3' => xlt("Manage etherFAX"), default => xlt("FAX"), };

sjpadgett · 2024-11-23T20:12:19Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/AppDispatch.php

@@ -49,6 +50,7 @@ public function __construct($type = null)
        if (empty(self::$_apiModule)) {
            self::$_apiModule = $_REQUEST['type'] ?? $_SESSION["oefax_current_module_type"] ?? null;
        }
+        $this->crypto = new CryptoGen();


sjpadgett · 2024-11-23T20:19:23Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/AppDispatch.php

            case '3':
                return '_etherfax';
+            case '5':
+                return '_clickatell';
        }


To stay consistent can you add _email as I missed it and added in the email methods.

case '4':
return '_email';

sjpadgett · 2024-11-23T22:02:25Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/ClickatellSMSClient.php

+        $cleanup_chr = array ("+", " ", "(", ")", "\r", "\n", "\r\n");
+        $toPhone = str_replace($cleanup_chr, "", $toPhone);
+        if (!str_starts_with($toPhone, "1")) {


Because we don't know if this method is being called from API or a send dialog need to test for and resolve:

$authErrorMsg = $this->authenticate(); // for API calls at minimum if ($authErrorMsg !== 1) { return text(js_escape($authErrorMsg)); // goes to alert } $toPhone = $toPhone ?: $this->getRequest('phone'); $from = $from ?: $this->getRequest('from'); $message = $message ?: $this->getRequest('comments'); $toPhone = $this->formatPhoneForSave($toPhone);

sjpadgett · 2024-11-23T22:15:05Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/ClickatellSMSClient.php

+    public function __construct()
+    {
+        if (empty($GLOBALS['oefax_enable_sms'] ?? null)) {
+            throw new \RuntimeException(xlt("Access denied! Module not enabled"));
+        }
+        parent::__construct();
+    }
+


Check that user has permission via ACL in dispatch. Required by all dialogs or MessageUI

/**
* @param $acl
* @return int
*/

public function authenticate($acl = ['patient', 'doc']): int { list($s, $v) = $acl; return $this->verifyAcl($s, $v); }

I added this to AppDispatch, assuming that other vendor modules will need the same logic.

This method in client was originally meant for authenticating vendor API account, Twilio I believe.
I keep in client for security of bad actors and a must have for dialog use.

What needs to happen is implementing an alias for sendSMS/Fax regarding how called, API or internal. I think I did this somewhere but again old age memory fails me.

On another matter I'm thinking of adding the capability/option to use my Document Templates for notification messages instead of the canned message in setup. This would allow for better formatting and addition data points.

ChrisOstler · 2024-11-26T05:03:36Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/AppDispatch.php

@@ -49,6 +50,8 @@ public function __construct($type = null)
        if (empty(self::$_apiModule)) {
            self::$_apiModule = $_REQUEST['type'] ?? $_SESSION["oefax_current_module_type"] ?? null;
        }
+        $this->crypto = new CryptoGen();
+        $this->getCredentials();


Does this make sense? It seems like for API handling we were having to sprinkle calls to getCredentials() around to make sure that the config had been loaded (and triggered the corresponding side effects). This front loads it so that it's ready before calling dispatchAction().

What side effects? I prefer to call credentials from client on demand. Plus how is this helping if not setting returned credentials to a property? Also It is working the way it is now and to change would require going back and retesting all my vendors. Plus this is called from background tasks to collect counts. Shouldn't matter but I don't trust changing this current credential grabs.
Also I think at one time I was setting a property in dispatch and had to remove. Don't remember why.

@ChrisOstler I tested this and you can leave if you want. Still it seems I had issue in the past when I relied on set properties. Old age and I can't remember so if you use keep eye out.

I had originally seen/modeled on the Twilio integration, which sets fields as a side effect of loading the credentials. The email and EtherFAX integrations do the same thing, though the RingCentral one does not.

Looking more closely, many of those seem like they were just copy/paste and not needed. Also, all but email also call getCredentials() in the subclass constructor, and then store that, as well.

Given all of that, I'd lean towards adding a protected credentials field to AppDispatch, and populating it by calling getCredentials() in AppDispatch::_construct(). That may mean cleaning up all the others that have already declared credentials fields - I'm not a PHP dev so not sure how the language handles field hiding. (Then the side effects of setting fields in getCredentials() could also be removed.)

How would you like me to handle this?

Again like I said I had this in the beginning and had to change because of an issue I can't remember. Could have been contention or scope was lost.
You are stating the more logical way but I'm sure I did this by design. Why are you having a problem calling from your client?
Also RC used to be 2FA with caching tokens so was handled differently until I recently went JWT.

I tell you what. I have some refactors to do for our next release so I'll clean this issue up then when I can do a full test suite.
I'm just fearful of any patches until next release having this change done.

Why are you having a problem calling from your client?

I could certainly make a late call to getCredentials() in sendSMS(). I've been trying to take an eye to "how can the next integration be made simpler" as I've run into things. I'm quite happy to whichever approach you'd prefer; these are just my takes on potential simplifications.

This particular one (credentials) is interesting because of the timing of dispatchAction() - it's invoked at the end of AppDispatch::__construct(). That means that e.g. sendSms() will be called before parent::__construct() returns. I believe that this is why crypto was being populated in the sub classes __construct() - they also had to call getCredentials() (which depends on crypto) before invoking parent::__construct() to have them before sendSMS() was called, and so had to make sure that crypto was populated.

One alternative is to JIT load credentials where/when they are needed. This has the downside of having potentially multiple locations in an integration that do so. Another alternative is to populate a credentials field once in AppDispatch::__construct() so that it will be available always.

(I will note that the current approach of copying data to properties from credentials has the benefit of localizing the extraction/mapping of credential fields to named fields to a single place, rather than potentially multiple places that credentials are used. That can't be done without using side effects due to the timing of the parent::__construct() invocation.)

I understand your concern about keeping changes minimal for a patch. What's your preferred approach:

Keep current implementation of calling getCredentials() in AppDispatch::__construct() and use side effects to capture what's needed (this means that getCredentials() will be called twice for most current integrations)

Instantiate crypto in ClickatellSMSClient::__construct() and call getCredentials() directly (like other integrations)

Wait and call getCredentials() in sendSMS(), since for this integration that's the only place they will be needed

I'll happily take whichever approach you'd like.

I was thinking about this this morning and concluded I'd be foolish not to take advantage of an experience developer interested in improving module especially coming from the POV of new to adding new vendors.
Therefore I have setup a dev test environment for the module and will test as you go.

All of your observations are valid so go ahead and do what you think necessary for improving module. I'll worry about current bug fixes users run across and patches until v7.0.3 release.
Appreciate your efforts.

I did some testing: adding a credentials field to AppDispatch doesn't cause problems for subclasses that also define that field, as long as the subclasses define it with the same visibility or weaker. Given that, I went with the approach to call getCredentials() in the AppDispatch constructor and save it to a field. All the subclasses that are also saving it to a field should continue to work the same, with the downside that if they are making their own getCredentials() call in their constructor then the credentials will be retrieved twice.

This makes it so that any new integration will always have access to a populated credentials, and doesn't need to have any particular logic to handle that.

ChrisOstler · 2024-11-26T05:04:05Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/AppDispatch.php

-        $query = "SELECT fname, lname, phone_cell, email FROM Patient_data WHERE pid = ?";
+        $query = "SELECT fname, lname, phone_cell, email FROM patient_data WHERE pid = ?";


Turns out that on Linux, table names are case sensitive...

I think was a typo. SQL engine shouldn't care, at least for column names, unsure about table names.

Yeah, this was a surprise to me:

Database, table, table aliases and trigger names are affected by the systems case-sensitivity, while index, column, column aliases, stored routine and event names are never case sensitive.

https://mariadb.com/kb/en/identifier-case-sensitivity/

Easy enough of a fix!

sjpadgett

Once minor changes done we can merge if you want.

sjpadgett · 2024-11-26T17:51:09Z

interface/modules/custom_modules/oe-module-faxsms/openemr.bootstrap.php

    $menu = $event->getMenu();
    // Our SMS menu
    $menuItem = new stdClass();
    $menuItem->requirement = 0;
    $menuItem->target = 'sms';
    $menuItem->menu_id = 'mod0';
-    $menuItem->label = xlt("SMS");
+    $menuItem->label = xlt($sms_label);


Is already escaped and translated. I don't like using vars content to translate as I think it makes it difficult for translators.

Yeah. Good catch.

sjpadgett · 2024-11-26T17:54:24Z

interface/modules/custom_modules/oe-module-faxsms/src/Controller/AppDispatch.php

-        $query = "SELECT fname, lname, phone_cell, email FROM Patient_data WHERE pid = ?";
+        $query = "SELECT fname, lname, phone_cell, email FROM patient_data WHERE pid = ?";


I think was a typo. SQL engine shouldn't care, at least for column names, unsure about table names.

Add Clickatell integration to FaxSMS module.

630599c

ChrisOstler mentioned this pull request Nov 22, 2024

Fix SMS and email appointment reminder notifications #7823

Closed

ChrisOstler commented Nov 22, 2024

View reviewed changes

ChrisOstler marked this pull request as ready for review November 22, 2024 20:25

ChrisOstler added 2 commits November 22, 2024 14:17

Merge branch 'master' into clickatell

44fa1b6

Support viewing Clickatell notification results in SMS screen.

943a361

sjpadgett requested changes Nov 23, 2024

View reviewed changes

ChrisOstler added 2 commits November 25, 2024 21:57

Include type filter in notification query.

8e7033f

Add logic for API call authorization and address other feedback.

49838e9

ChrisOstler commented Nov 26, 2024

View reviewed changes

Fix ordering of label matching.

5a7b16d

sjpadgett approved these changes Nov 26, 2024

View reviewed changes

ChrisOstler added 3 commits November 26, 2024 14:18

Remove deplicate translation step.

96b49ff

Include 'Error' keyword to trigger caller behavior.

95aca39

Populate credential field in AppDispatch constructor.

1535815

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Clickatell integration to FaxSMS module. #7838

Add Clickatell integration to FaxSMS module. #7838

ChrisOstler commented Nov 22, 2024 •

edited

Loading

ChrisOstler Nov 22, 2024

ChrisOstler Nov 22, 2024

sjpadgett Nov 22, 2024

ChrisOstler Nov 22, 2024

sjpadgett Nov 22, 2024

ChrisOstler Nov 22, 2024

sjpadgett Nov 22, 2024

ChrisOstler commented Nov 22, 2024

sjpadgett commented Nov 22, 2024

sjpadgett left a comment

sjpadgett Nov 23, 2024

sjpadgett Nov 23, 2024

sjpadgett Nov 23, 2024

sjpadgett Nov 23, 2024

sjpadgett Nov 23, 2024

ChrisOstler Nov 26, 2024

sjpadgett Nov 27, 2024

ChrisOstler Nov 26, 2024

sjpadgett Nov 26, 2024 •

edited

Loading

sjpadgett Nov 26, 2024

ChrisOstler Nov 26, 2024

sjpadgett Nov 26, 2024

ChrisOstler Nov 27, 2024

sjpadgett Nov 27, 2024

ChrisOstler Nov 28, 2024

ChrisOstler Nov 26, 2024

sjpadgett Nov 26, 2024 •

edited

Loading

ChrisOstler Nov 26, 2024

sjpadgett left a comment

sjpadgett Nov 26, 2024

ChrisOstler Nov 26, 2024

sjpadgett Nov 26, 2024 •

edited

Loading

		$query = "SELECT fname, lname, phone_cell, email FROM Patient_data WHERE pid = ?";
		$query = "SELECT fname, lname, phone_cell, email FROM patient_data WHERE pid = ?";

Add Clickatell integration to FaxSMS module. #7838

Are you sure you want to change the base?

Add Clickatell integration to FaxSMS module. #7838

Conversation

ChrisOstler commented Nov 22, 2024 • edited Loading

Short description of what this resolves:

Does your code include anything generated by an AI Engine? No

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisOstler commented Nov 22, 2024

sjpadgett commented Nov 22, 2024

sjpadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjpadgett Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjpadgett Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjpadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjpadgett Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

ChrisOstler commented Nov 22, 2024 •

edited

Loading

sjpadgett Nov 26, 2024 •

edited

Loading

sjpadgett Nov 26, 2024 •

edited

Loading

sjpadgett Nov 26, 2024 •

edited

Loading