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

The phone type (and probably other types) throw an exception when parsing an array #110

Open
alexkwolfe opened this issue May 30, 2018 · 2 comments
Labels

Comments

@alexkwolfe
Copy link
Contributor

There was a batch of leads submitted where an array of phone numbers was provided in the phone_1 field. On the inbound side, the array was not parsed by the phone type, and no error occurred.

But on the outbound side, an integration declared a request variable as a phone type. This caused an unhandled error and therefore the handler to crash.

TypeError: string.match is not a function
    at stripType (/srv/leadconduit/app/releases/20180525205842/node_modules/leadconduit-types/lib/types/phone.js:52:20)
    at Object.parse (/srv/leadconduit/app/releases/20180525205842/node_modules/leadconduit-types/lib/types/phone.js:27:11)
    at Object.module.exports.parse (/srv/leadconduit/app/releases/20180525205842/node_modules/leadconduit-types/lib/index.js:35:26)
    at parseTypes (/srv/leadconduit/app/releases/20180525205842/lib/handler/util/send.js:443:22)
    at module.exports (/srv/leadconduit/app/releases/20180525205842/lib/handler/util/send.js:172:5)
    at /srv/leadconduit/app/releases/20180525205842/lib/handler/steps/recipient-step.js:9:14
    at /srv/leadconduit/app/releases/20180525205842/lib/handler/middleware/steps.js:52:16
    at /srv/leadconduit/app/releases/20180525205842/node_modules/@activeprospect/leadconduit-flow-steps/lib/steps.js:21:18
@alexkwolfe alexkwolfe added the bug label May 30, 2018
@alexkwolfe
Copy link
Contributor Author

The lead handler has code like this:

    # Parse the value, if necessary
    parsed =
      if _.isArray(value)
        value
      else
        types.parse(typeName, value, req)

We could move the _.isArray() test into the parse function exported by this module. Then we could remove it from the handler. This would make the behavior consistent on the inbound and outbound side.

My concern, though, is that if an integration declares the type as phone it may be hardcoded to expect a phone object. So solving it this way will just push the error into the integration.

Maybe a better solution would be to only parse the first value and discard the second. What do you think @cgrayson?

@cgrayson
Copy link
Contributor

cgrayson commented Jun 4, 2018

I think I like the idea of parsing only the first value and discarding others best. It's simple, consistent, and non-breaking. From a user standpoint, it seems totally sensible that if I send an array of values into a field that accepts one, that something has to be lost. If I were the user, I'd consider myself lucky to have it keep the first intact instead of blowing up or doing something else weird. :-)

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

No branches or pull requests

2 participants