search

activeprospect / leadconduit-types

This Node.JS module parses lead data and provides specialized functionality by lead data category.
1 stars 0 forks source link

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

Open alexkwolfe opened 6 years ago

alexkwolfe commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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. :-)