Review gateway plugins [#2725247]

Ensure all methods on base gateway plugin:

Are tested
Have a core implementation

If they don't, then introduce an optional extra interface in SMS Framework core, or a separate module.

Configurable Plugin Interface

getConfiguration() ✅
setConfiguration() ✅
defaultConfiguration() ✅

Plugin form interface

buildConfigurationForm() ✅
validateConfigurationForm() ✅
submitConfigurationForm() ✅

Dependent Plugin Interface

calculateDependencies() ✅

Sms Gateway Plugin Interface

send ✅
balance
parseDeliveryReports ✅
getDeliveryReports

Sms Gateway Plugin Incoming Interface

incoming ✅

Comments

Comment #1

14 May 2016 at 10:16

dpi created an issue. See original summary.

Comment #2

almaudoh CreditAttribution: almaudoh commented 1 August 2016 at 17:06

Pulling this to the top of the queue

Comment #3

dpi

Perth, Australia

CreditAttribution: dpi as a volunteer commented 16 August 2016 at 10:01

Priority:

Normal

» Major

Comment #4

dpi

Perth, Australia

CreditAttribution: dpi as a volunteer commented 20 August 2016 at 07:07

Seems like its safe to remove:

validateNumbers
balance
getDeliveryReports
getError

Comment #5

almaudoh CreditAttribution: almaudoh commented 22 August 2016 at 00:07

We can remove validateNumbers, but the others are useful in some applications for me. getDeliveryReports is needed for some gateways where you need to pull delivery reports by http request from the server, balance provides a means to know the credit balance on that server while getError was a way of managing errors from the gateway.
Exceptions can be used to replace the getError functionality.

We should retain balance and getDeliveryReports.

Comment #6

dpi

Perth, Australia

CreditAttribution: dpi as a volunteer commented 8 September 2016 at 12:35

I understand the two methods can/could be useful but there is no first party usages or tests for them. I propose they can be added back later, considering the following:

`\Drupal\sms\Plugin\SmsGatewayPluginInterface::getDeliveryReports()`

The message ID's argument is not a standardised concept within the framework yet. This should be rethought... Yes they are used in delivery reports themselves but not yet elsewhere.

Perhaps add a pullDeliveryReports or similar to check for new reports.

Add some annotation to gateway plugin to say how it handles delivery reports: push, pull, both, none? This way we can provide a future UI to force pull reports. Or even set up a cronjob to automate this.

`\Drupal\sms\Plugin\SmsGatewayPluginInterface::balance()`

Balance is difficult because it is a currency: could be an officially traded currency or credit or token system.

To make this method usable within the framework there needs to be some standards.

Need to figure out what the purpose of the balance system; and why it benefits the Drupal install to know about the balance. Whether the balance is purely informational; or we can take action on available balance.

Balance cannot just be an int (current behaviour) or float...

I have the same reservations about balance in \Drupal\sms\Message\SmsMessageResultInterface but that is for another time.

The balance default of zero does not make sense, should be NULL...

edit: 8 Sept: balance base default