Update delivery report and message result objects

I'm doing some experimentation here https://github.com/dpi/smsframework/pull/51

The first commit adds the methods and simple stuff. Second commit onwards has some radical stuff. Did I go too far? Maybe. But I'm doing some real gateway development with this experimental report/result/status branch and it seems fine so far.

I'm working on the following gateways: Telstra (best example), Clickatell, and MailUp (not ready). And I may try Twilio if I have some time. This should give me an idea of how results and reports need to be implemented.

Just did a quick code check. The change from PHP arrays to objects with interfaces is a good one. So is the status constants move to a dedicated class.

Awesome.

However, I don't see the need why the Delivery report recipients property is an array. I don't see a use case for that. Even in your code, each delivery report is keyed by the same single recipient number.

The keying of Result::reports should probably be made unrelated to recipients. So far I see reports as an atomic operation, all recipients should have the same status. Otherwise create new reports if there are differing status.

I also still don't see the benefit of composing the SmsMessage object into the SmsResult object (especially if they are both serialized).

I added the message to the result purely for the benefit of post process hooks. There is no other reason, and yes I agree it is not ideal.

I would rather compose the SmsResult into the SmsMessage object.

Can give this a shot.

Pushed to the PR:

• Added getResult/setResult to message objects for use in post process events in #2786865.
  Removed Result::setMessages/getMessages
• Updated result::reports to have meaningless array keys.
• Updated result::getReport to iterate over reports, since keys no longer are for recipient.

Merged into #2786865: Replacing hooks with Symfony events. Updated provider to use message::setResult from #7

Changes pushed to PR

Ah, the tests exposed the reason why I chose the original design of tacking SMS message onto result:

the provider::send/incoming return an array of results, and there is no reference to the sent SMS messages (chunked, or otherwise).

1. We can change the @return signature of send/incoming to be an array of SMS messages. (preferred)
2. Or, revert to the original design of result->messages

Have you considered the case when delivery reports are updated at a later time

Considering reports themselves arn't persistent it didnt seam to matter...?

If we want to force reports to be for a single recipient that's OK I suppose.

I imagined if a status needed to be updated you can programmatically find the old report by using message Id + recipient.

Yeh don't worry about the patch, just using patches for test runner. Theres a bunch of random other junk in there too.

Just use PR for review.

And its not ready for review right now ;)

Probably this weekend sometime.

Since #16: Split status classes into two, updated interface documentation to refer to these classes instead of the old status class.

---

So as I see it now, to programmatically determine if a message was sent successfully:

• There must be *no* status set on result. The only status available for result are negative.
• There must be at least one delivery report.

Does this make sense?

Ignore the SmsDevelMessageForm code in the patch, just review the commits in PR. I was experimenting with how the devel message form determines if a message was sent successfully. See question in #24.

Patch will fail since I have not updated tests with new codes.

edit: codes, not return type

Per #5

However, I don't see the need why the Delivery report recipients property is an array.

and #14

force reports to be for a single recipient

Updating report object to have only one recipient.

Take a look if you like

https://github.com/dpi/smsframework/pull/51

Fixed and pushed nitpicks, ive posted some replies if you can take a look at those.

Theres a question you may have missed in #24 during the barrage of testbot results.

Are you online now?

Working on unit tests.

Includes nit fixes and unit tests, stricter variable type handling

• Added exception for incorrect variable type for setCreditsUsed and setCreditsBalance.
• Made the existing public variables in SmsMessageResult protected.
• Added unit test for message object and entity getResult and setResult methods.
• Added unit test for delivery report class.
• Added unit test for result class.

Fixed some @covers annotations from the new unit tests.

Pushed:

• Renamed protected credit balance variable to match getter and setter
• Added invalid parameters constant to result status class.

Going all in on the meaning of status messages for result. Per #2789237: Update delivery report and message result objects #24

Renaming result::*status* to result::*error*. Current head has getErrorMessage for result too, but it also has getStatus.

What do you think of:

We should really arrange a d.o documentation page for this ;) (edit: 22 nov 2016, created: https://www.drupal.org/node/2829641)

Updated devel form asserts

I would look at maybe two or three SMS providers to get an idea of things.

I'm working with three providers so far ;), however...

Just to be clear its just informational, the *why* isnt important since we are not acting apon the success or failure.

I had imagined something like $result->addTryAgain($recipient, 'some measure of time'); to resolve some types of failures. At least when using the queue system.

NULL status could mean success

The table I have has the error code == NULL as one of the variables to determine success. I think its reasonable to add the requirement of at least one report to be super sure about that. There would seem to be some kind of failure in some way if there are no reports?

I re-read your comment and see I can interpret it differently.

I think having setError instead of setStatus is better, gateways dont have to do anything on success. So we can easily determine if the whole transaction failed by checking if there is any error code. If it was binary/NULL, we have three states, where NULL == unknown status?. It makes it a lot harder to determine what happened this way.

count of reports == count of recipients

Im not sure you can qualify *success* from this equation, since reports now have their own status codes. We can assume all reports were *processed*.

This is where my aforementioned "try again?" idea comes from, since we cant tell easily from error codes whether its a good idea to retry the message (eg: such as flooded) or just dump it (eg: bad recipient).

Other than generating log data, what can we potentially automate from status codes?

1. Retry the message later
2. ...?

Updated. Should be fine.

oops, forgot about tests