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

TypeScript typed statusCallbackEvent writes invalid attribute value #497

Closed
mpeltonen opened this issue Nov 6, 2019 · 7 comments · Fixed by #498
Closed

TypeScript typed statusCallbackEvent writes invalid attribute value #497

mpeltonen opened this issue Nov 6, 2019 · 7 comments · Fixed by #498
Labels
difficulty: medium fix is medium in difficulty status: help wanted requesting help from the community type: bug bug in the library type: community enhancement feature request not on Twilio's roadmap

Comments

@mpeltonen
Copy link
Contributor

As can be seen from the example code and its output below, statusCallbackEvent attribute ends up having wrong delimiter, comma instead of space, in the resulting XML, and this causes error on Twilio side.

Version: 3.37.0

Code Snippet (main.ts)

import * as twilio from "twilio";

const resp = new twilio.twiml.VoiceResponse()
const dial = resp.dial()

dial.number({ statusCallbackEvent: ["initiated", "ringing"] }, "123456567")

console.log(resp.toString())

Output

<?xml version="1.0" encoding="UTF-8"?>
<Response>
  <Dial>
    <Number statusCallbackEvent="initiated,ringing">123456567</Number>
  </Dial>
</Response>
@mpeltonen
Copy link
Contributor Author

A related observation is that NumberEvent type has "answered" as one of the options, but the callback URL actually seems to receive an event with CallStatus: "in-progress" instead of "answered".

@childish-sambino
Copy link
Contributor

For the second issue, this is by-design and the docs are here: https://www.twilio.com/docs/voice/twiml#callstatus-values

For the first issue, looks like in JS we're expecting the parameter to be a space-delimited string (see the example here), but in TS we're expecting an array of NumberEvents (similar to what we do in Java). I think it should work either way in JS (i.e., string or array in JS, and array in TS) so to be backwards-compatible and flexible to the clients.

So, the JS code needs to be updated to serialize the parameter into a space-delimited list.

Given that this exists in generated code, the best way to resolve is to submit a PR with very limited scope that illustrates how such cases should be handled. Using that as a template, we can then update the generator to use the new pattern.

A PR and +1s on the issue summary will help it move up the backlog.

@childish-sambino childish-sambino added code-generation issue deals with generated code difficulty: medium fix is medium in difficulty status: help wanted requesting help from the community type: bug bug in the library type: community enhancement feature request not on Twilio's roadmap labels Nov 6, 2019
@childish-sambino
Copy link
Contributor

Possible (un-tested) implementation:

Dial.prototype.number = function number(attributes, phoneNumber) {
  let element = this.dial.ele('Number', phoneNumber);

  for (let [key, value] of Object.entries(attributes)) {
    element.att(key, Array.isArray(value) ? value.join(' ') : value);
  }

  return new Number(element);
};

@mpeltonen
Copy link
Contributor Author

Something along the lines of this is also another possibility:

function TwiML() {
  this.response = builder.create('Response', {
    stringify: {
      attValue: function(val) {
        if (Array.isArray(val)) {
          return val.join(" ");
        }
        return val;
      }
    }
  }).dec('1.0', 'UTF-8');
}

This at least fixes my current case, and would probably be a good option if no atrributes exist which are Arrays and should be comma separated.

@childish-sambino
Copy link
Contributor

Ahhh. If that does indeed work, I LIKE IT!

I don't know of any arrays which are supposed to be comma-separated. Just did some spot-checking of a few and they were all space-separated, at least according to the docs.

@mpeltonen
Copy link
Contributor Author

Here's the documentation of those hooks: https://github.com/oozcitak/xmlbuilder-js/wiki/Value-Conversion

@childish-sambino
Copy link
Contributor

And TwilML.js is not generated code so a PR works even better.

@childish-sambino childish-sambino removed the code-generation issue deals with generated code label Nov 6, 2019
mpeltonen added a commit to mpeltonen/twilio-node that referenced this issue Nov 6, 2019
mpeltonen added a commit to mpeltonen/twilio-node that referenced this issue Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: help wanted requesting help from the community type: bug bug in the library type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants