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

Update: Format Date type objects to permissible Iterable date string #2645

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheFacto-HH
Copy link

Hello 👋

We noticed when attempting to map the receivedAt property from Segment's UI to an Iterable destination field, Iterable was dropping the property (we used Iterable's Data-Schema-Management to define the property as a Date ahead of time). We discovered that this is very likely due to receivedAt being a Date which was not handled by the convertDatesInObject function.

We're looking for feedback on this update which would check against Date types and format them to be compatible with Iterable's permitted date string format.

Testing

Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
  • [Segmenters] Tested in the staging environment
  • [Segmenters] [If applicable for this change] Tested for regression with Hadron.

@joe-ayoub-segment
Copy link
Contributor

hi @TheFacto-HH thanks for raising this draft PR.
I had a quick look at the code (even though it's not ready for review) and can see that this line:

} else if (value instanceof Date) {
      obj[prop] = dateToIterableDateStringFormat(value.toISOString())
    } 

isn't going to do anything, as value will never be a Date object.

In order to help you with the issue you've been having, could you please tell me the following:

  1. Which field are you passing the value into?
  2. What value are you passing into the field?
  3. What expected outcome do you wish to happen?

Kind regards,
Joe

@TheFacto-HH
Copy link
Author

Hey @joe-ayoub-segment , thanks for the early review!

isn't going to do anything, as value will never be a Date object.

We noticed that if we mapped receivedAt to a destination property in Iterable, Iterable was dropping it. This seems to be because Segment was propagating an actual Date object to the destination instead of formatting it to an Iterable-supported date string. I noticed that receivedAt - per Segment's types - can, in fact, be a Date.

If you remove the line that you mentioned and re-run the unit test I added:

Screenshot 2024-12-13 at 10 43 51 AM

@joe-ayoub-segment
Copy link
Contributor

Thanks for explaining @TheFacto-HH.
I still can't see how a date object can be passed to a field in the Destination in Production though.
The JSON payload gets sent through a pipeline built from multiple microservices before it gets passed into the perform() function. At this stage the Date object will have been converted into an ISO 8601 string.

Could you show me reproduction steps for the issue you are having please, if possible with screenshots of the mapping fields and an example payload? I don't doubt your claim - I'm just trying to verify it.

The change looks safe, but I'd like to ensure that it actually addresses a fault before I merged it.

Thanks,
Joe

@TheFacto-HH
Copy link
Author

Sure thing!

Segment Mapping

Screenshot 2024-12-13 at 11 38 17 AM

(receivedAt is mapped to member360DeletedAt)

Iterable Data Schema

Screenshot 2024-12-13 at 11 33 40 AM

(member360DeletedAt is defined as a date and User field lock is on, meaning unrecognized fields will be dropped).

Iterable User Fields

Screenshot 2024-12-13 at 11 35 56 AM

(Notably, member360DeletedAt is not present after sending the test event [and the status was 200]).

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

Successfully merging this pull request may close these issues.

2 participants