Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with client_ref (probably should be named client-ref) #43

Closed
kodooy opened this issue Dec 18, 2020 · 6 comments
Closed

Problem with client_ref (probably should be named client-ref) #43

kodooy opened this issue Dec 18, 2020 · 6 comments

Comments

@kodooy
Copy link

kodooy commented Dec 18, 2020

  • Nexmo Notification Channel Version: 2.5
  • Laravel Version: 8.18.1
  • PHP Version: 7.4.10

Description:

When I'm sending NexmoMessage with clientReference() method a 'client_ref' parameter is added to my request, but according to Nexmo documentation this parameter should be called 'client-ref' (https://developer.nexmo.com/messaging/sms/guides/delivery-receipts#using-the-sms-api-in-campaigns).

With 'client_ref' parameter I have got an error 'Undefined index: client_ref' in payload I receive from Nexmo.
When I change in src/Channels/NexmoSmsChannel.php file 'client_ref' into 'client-ref' (line: 62 i 66) everything works OK (there is 'client-ref' property in payload form Nexmo).

Steps To Reproduce:

public function toNexmo($notifiable)
{
     return (new NexmoMessage)
            ->content($this->message->body)
            ->unicode()
            ->clientReference($this->message->id);
}
@driesvints
Copy link
Member

I'm confused myself here. It strikes me as odd that @Braunson wouldn't have tested his own implementation or anyone wouldn't have reported this before. Did something change on Nexmo's part or in one of the third party Nemxo libraries?

Original PR: #17

/cc @Braunson @dragonmantank

@Braunson
Copy link
Contributor

Braunson commented Dec 21, 2020

@driesvints It certainly is client-ref on the Nexmo docs, I don't see any versioning option for the docs if it did change to go back and see. Perhaps I had client-ref locally but with the underscore in the PR somehow, an oversight on my part. I've put in a new PR to resolve this issue.

I did find this https://metacpan.org/pod/Nexmo::SMS::Response::Message which references client_ref back in 1.x 🤷‍♂️

While I did run a quick test in the Nexmo sandbox and wasn't able to re-create that specific error from @kodooy, I did notice client_ref wasn't being returned where-as when I changed it to client-ref it was.

# With client-ref
[2020-12-21 14:33:59] local.ERROR: array (
  'msisdn' => '1xxxxxxxxxx',
  'to' => '1xxxxxxxxxx',
  // ...
  'client-ref' => 'abc-1234',
  'message-timestamp' => '2020-12-21 14:32:34',
)

# With client_ref
[2020-12-21 14:34:23] local.ERROR: array (
  'msisdn' => '1xxxxxxxxxx',
  'to' => '1xxxxxxxxxx',
  // ...
  'message-timestamp' => '2020-12-21 14:34:23',
)  

Update: Interestingly if you look in the Vonage SDK, specifically vendor/vonage/client-core/src/Network/Number/Request.php in __construct() you'll see this $this->params['client_ref']? I'm interested in seeing what @dragonmantank says.

@driesvints
Copy link
Member

Thanks for the PR!

@kodooy
Copy link
Author

kodooy commented Dec 21, 2020

Thank you! Now it works perfectly :-)

@dragonmantank
Copy link
Contributor

client-ref is definitely correct for SMS.

The Vonage\Network\Number\Request class is probably from an older API that's been long since deprecated (which I'll go through and get marked as such). That code hasn't been touched a long time, and from the looks of it was what turned into our Number Insights product.

There's a really good chance no one noticed it was wrong just because most customers don't use client-ref at all.

@driesvints
Copy link
Member

@dragonmantank thanks for the speedy answer Chris! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants