Vonage / vonage-node-sdk

Vonage API client for Node.js. API support for SMS, Voice, Text-to-Speech, Numbers, Verify (2FA) and more.
Apache License 2.0
380 stars 181 forks source link

[TS] Property 'error_text' does not exist on VerifyCheckResponse interface #752

Closed SerajMuftah closed 1 year ago

SerajMuftah commented 1 year ago

Just updated the package to the latest version v3 and the first error I encountered was error_text missing.

Current Behavior

For instance, when I try to initiate a 2FA request, the only way to get the error text looks like this:

const res = await vonage.verify.start({
    number: phoneNumber,
    brand: brandName,
});

if (res.status != '0')
    console.log(res['error_text']);

If you try to use res.error_text instead, you get an error obviously.

Possible Solution

This is simple enough, just add error_text (or errorText to make it consistent with the latest package naming convention) to the response interface.

Your Environment

manchuck commented 1 year ago

@SerajMuftah The endpoint will only return the error_text on a non-200 response. If you run:

const res = await vonage.verify.start({
    number: phoneNumber,
    brand: brandName,
});

if (res.status !== 200)
    console.log(res['error_text']);

You should then see the error_text property. The res.status property will be the HTTP status code.

Can you let me know if that resolves the issue?

SerajMuftah commented 1 year ago

@manchuck I don't think the res.status gives the http request status, but rather predefined codes found in the API reference here.

This is a screenshot for successful verify check request: image

And this is when the request fails: image (Both taken from the API documentation)

I know that error_text will only return when the request does not succeed, but it doesn't exist on the response interface VerifyCheckResponse to begin with. It should be defined explicitly on the Interface like this:

export interface VerifyCheckResponse {
    // list of already existing properties
    error_text: string; // or errorText following JS/TS naming rule
}

As I don't think res['error_text'] is the best practice to handle it. For instance, Typescript wouldn't be able to throw an error if you updated the SDK for error_text to become errorText as I parse it from the response this way res['error_text'] and my application would break or at least I wouldn't be able to get the error until I anticipate the response manually and debug.

manchuck commented 1 year ago

@SerajMuftah Thanks for that. You should be able to see the full response from the API returned. I will make an update to have the error_text property as part of the result.

SerajMuftah commented 1 year ago

@manchuck Thank you. Let me know when this gets resolved.

manchuck commented 1 year ago

@SerajMuftah I just checked today and I cannot reproduce the error:

(async () => {
  const res = await vonage.verify.start({
    number: phoneNumber,
    brand: brandName, 
  });

  console.log(res);

  if (res.status !== '0') {
    console.log(res.error_text);
  }
})()

Ouputs:

{
  request_id: 'f8d6cf1a347a43bfbb1fa7b6bea6b962',
  status: '10',
  error_text: 'Concurrent verifications to the same number are not allowed'
}
Concurrent verifications to the same number are not allowed
SerajMuftah commented 1 year ago

@manchuck have you tried with TypeScript? I'd assume you're using JS which wouldn't produce such an error because the issue is missing the "error_text" property from Vonage's package type definitions.

manchuck commented 1 year ago

@SerajMuftah well, a can of worms was opened with this. I went through with a fine-toothed comb and found a number of incorrect types. I have updated the verify package in #794. Waiting for approval to merge in before publishing a release for you

SerajMuftah commented 1 year ago

@manchuck Thank you for working on fixing all the bugs, it's no surprise it took some time. I'm looking forward to testing out the new release and I hope all the issues have been ironed out.

manchuck commented 1 year ago

@SerajMuftah The latest version of the @vonage/verify package has been published. That version should resolve the issues you were having. Please feel free to reopen this bug if you continue to have issues