add phone number support to reddit-conversions-api

[akhsueh]

A summary of your pull request, including the what change you're making and why.

Adding the support of passing phone numbers into the Reddit Conversions API destination (destinations/actions-reddit-conversions-api).

We have configured it to the following default mapping, please let us know if this is not the correct default value from Segment that we should use:

default: {
  '@if': {
    exists: { '@path': '$.properties.phone' },
    then: { '@path': '$.properties.phone' },
    else: { '@path': '$.context.traits.phone' }
  }
}

Testing

• [ X] Added unit tests and they have passed
• [X ] Tested end-to-end using the local server / Actions Tester and checked the final payload in the request that was being sent
• [ X] [If destination is already live] Tested for backward compatibility of destination: didn't notice any issues in the unit tests or end to end test using actions tester
• [Segmenters] Tested in the staging environment
• [Segmenters] [If applicable for this change] Tested for regression with Hadron.

[varadarajan-tw]

@akhsueh - Is this PR ready for review?

[joe-ayoub-segment]

hi @akhsueh , looks like @codeyanker indicated that phone number needs to be 'smart' hashed.
Please let me know when you add the code for this and I'll review.
I think it would be worth adding a unit test to test the outputs for multiple different phone number inputs.
I assume we'll need to implement something similar for the Pixel Destination also?

[joe-ayoub-segment]

function canonicalizePhoneNumber(value: string): string {
  const digits = value
    .trim()
    .split(/[a-zA-Z]/)[0]
    .replace(/\D/g, '');
  return digits ? `+${digits}` : ''
}

[joe-ayoub-segment]

phone_number: processHashing( 
  user.phone_number ?? '', 
  'sha256', 'hex', 
  features, 
  'actions-reddit-conversions-api', 
  canonicalizePhoneNumber
)

[akhsueh]

Hi @joe-ayoub-segment , thanks for taking a look here. I connected internally with the team and we're OK without implementing the phone number hashing piece if Segment is OK with it too. Our API will always defensively hash - so if it sees a request that has an unhashed value being passed in the phone number field, it will automatically hash it before we process it and store it anywhere.

If hashing before the request is required by Segment though, we can add that function in there. Is this something you typically require to implement or is it OK to rely on our API to hash the values automatically?

[joe-ayoub-segment]

Hi @akhsueh , I think it would be better to smart-hash it on the Segment side so that the field behaves the same as other related fields. It's possible our docs already indicate that smart-hashing occurs, so it's simpler to just do it here.

We will need to test it though, but that can be done locally using the testing tool. Is this something you are comfortable doing? We can get on a call to do it if that works best for you? Here is my Calendly: https://calendly.com/joe_ayoub

[akhsueh]

Hi @joe-ayoub-segment , thanks for touching base on this in the call the other day. I updated this to include the cleaning function and smart-hash function to the phone numbers before sending. Also updated the tests for it.

[joe-ayoub-segment]

hi @akhsueh I'm thinking we should actually leave this in, and update the signature of the function to an optional phoneNumber string value or undefined value. We would then also return undefined if there is no phone number.

What do you think ?

[akhsueh]

Hi @joe-ayoub-segment sounds good, I added that back into the latest commit and changed the smartHash a bit to try to resolve an error when I inserted that line back into cleanPhoneNumber.

[joe-ayoub-segment]

Thanks for getting this done @akhsueh . I only had one query about the clearPhoneNumber function. Wonder if it should handle when a null or undefined value is passed in.