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

add Report Content Event to customerio destination #2650

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

karngyan
Copy link

@karngyan karngyan commented Dec 16, 2024

  • Add Report Content Event to customerio destination.
  • We also make sure it's ignored by Track Event.

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.

@karngyan karngyan marked this pull request as ready for review December 16, 2024 10:00
@karngyan karngyan requested a review from a team as a code owner December 16, 2024 10:00
@joe-ayoub-segment joe-ayoub-segment self-assigned this Dec 16, 2024
@joe-ayoub-segment
Copy link
Contributor

hi @karngyan thanks for raising this PR.

Do you work for Customer.io? If so could you please email me from a work email to [email protected] so I can verify your email address?

If you don't work for Customer.io I'll have to reach out to them to have them review the PR as well.

Kind regards,
Joe

@joe-ayoub-segment
Copy link
Contributor

Hi again @karngyan ,

The CI / Validate check is failing. Could you please run yarn types and commit the changes to your branch?

To help me understand the use-case for this PR a little, could you share information of the API docs for sending a "Report Content Event" request to Customer.io please? I want to ensure that any conventions with the other pre-existing Actions are continued with this new code.

Best regards,
Joe

@karngyan
Copy link
Author

Hi @joe-ayoub-segment 👋

Thank you for reviewing the PR! Yes, I work at Customer.io. I’ll send you an email from my work address shortly for verification.

I’ll run yarn types and update the PR. Regarding the documentation, this is a relatively new feature available to a select group of users. I’ll reach out to our docs team to get the link for you - please give me a little time. It's okay to delay this merge a little.

Thanks,
Karn

@joe-ayoub-segment
Copy link
Contributor

Just FYI we're going into a deploy freeze for the Xmas and New Years holidays, so this PR won't go out until the New Year.

title: 'Report Content Event',
description: 'Report a Viewed or Clicked Content event.',
defaultSubscription: 'event = "Report Content Event"',
fields: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @karngyan I just a had a quick look at some of the other Actions for this Destination.
There's an Action called 'Track Event' which looks similar to this new one you are adding. However it also has some additional fields which this new Action does not have.

  • Person ID
  • Event Name
  • Event ID

I don't know anything about Customer.io, but would like to ensure that changes to the Integration are done in a consistent way. Should this new Action also have the aforementioned fields?

Best regards,
Joe

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +142 to +148
{
name: 'Report Content Event',
subscribe: 'event = "Report Content Event"',
partnerAction: 'reportContentEvent',
mapping: defaultValues(reportContentEvent.fields),
type: 'automatic',
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth having 2 separate Presets, one for viewed_content and one for clicked_content.
Each preset would need to set the value of the 'Action Type' field accordingly.

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