Skip to content

Conversation

@ManikMM
Copy link
Contributor

@ManikMM ManikMM commented Feb 6, 2025

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

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.

@ManikMM
Copy link
Contributor Author

ManikMM commented Feb 6, 2025

@joe-ayoub-segment Hey Joe, setting up a draft PR for our Reddit Pixel integration.
Let me know if you have any questions - otherwise we'll wait for your input.
Thanks!

@ManikMM
Copy link
Contributor Author

ManikMM commented Feb 21, 2025

@smultani Here is the PR for Reddit Pixel.
Could you review it for us and let me know how to proceed with any changes you suggest.
Also free to have a quick meeting if that is necessary.
Thanks!

@brennan
Copy link
Contributor

brennan commented Feb 24, 2025

@ManikMM, the team will review this week. Thanks for your patience. cc @smultani

description:
'Comma delimited list of Data Processing Modes for this conversion event. Currently only LDU (Limited Data Use) is supported.',
type: 'string',
choices: [{ label: 'Limited Data Use', value: 'LDU' }]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest adding a default: 'LDU' here so users don't even have to make a selection.
Also, are there plans in the future to add modes other than 'LDU'? If not then this field might not be necessary at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added empty string... this field is either there with that single option, or not included in the payload.

@@ -0,0 +1,3 @@
describe('Src.reportWebEvent', () => {
// TODO: Test your action
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest removing the test file entirely or adding unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add tests once we get the code working

console.log('Calling Reddit Pixel tracking function')
rdt.page() // Calling the available `page` method as an example.
} else {
console.error('rdt.page() is not defined.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest updating this to throw an error. Was this perform block just left behind from testing though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, perform block is to perform conversion pixel events.
We use the rdt('track', 'PageVisit') in intialization...

Then when a user action is taken (i.e. search, add to cart, etc.) - We want to trigger a
const addToCartButton = document.getElementById('addToCart');
addToCartButton.addEventListener('click', () => rdt('track', 'AddToCart'));

So, I'm assuming that the perform function will facilitate those downstream events, similar to other implementations I saw in this repo. lmk if that isn't accurate.

@nick-Ag
Copy link
Contributor

nick-Ag commented Mar 5, 2025

Hey @ManikMM - left some light comments, let me know when I should re-review! Thanks

@ManikMM
Copy link
Contributor Author

ManikMM commented Mar 7, 2025

Thanks for the review @nick-Ag, I made a few changes - The PageVisit pixel event type tracks just fine - we receive it on our end (assuming it's the one being fired from the init-pixel.ts).

However, when I use the dev-center/actions-tester to send other event types (Add To Cart, Search, etc.) - I do not receive those...

Would it be possible to jump on a quick call sometime next week to resolve this quickly? I could use a sanity check 😅

@brennan brennan added the eng-acknowledged This PR has been acknowledged by the engineering team. label Mar 7, 2025
@ManikMM
Copy link
Contributor Author

ManikMM commented Mar 18, 2025

@nick-Ag / @brennan any updates here?

@nick-Ag
Copy link
Contributor

nick-Ag commented Mar 18, 2025

@nick-Ag / @brennan any updates here?

Hi @ManikMM sorry for the delay here, please email me at naguilar@twilio.com and we can schedule a meeting to discuss this PR

@ManikMM
Copy link
Contributor Author

ManikMM commented Apr 2, 2025

@nick-Ag Any updates?

@akhsueh
Copy link
Contributor

akhsueh commented Apr 25, 2025

@nick-Ag ah sorry about that, I must've hit comment before it finished uploading. Here's the zip file attached now. Will also forward that email to you. Thank you!
Reddit Segment JS Pixel Tests.zip

@nick-Ag
Copy link
Contributor

nick-Ag commented Apr 28, 2025

Hi @akhsueh , @ManikMM - I think there should be some sanity unit tests at the very least for this destination. Was there a reason the empty unit testing files were removed?

@akhsueh
Copy link
Contributor

akhsueh commented Apr 29, 2025

Hi @akhsueh , @ManikMM - I think there should be some sanity unit tests at the very least for this destination. Was there a reason the empty unit testing files were removed?

Hey @nick-Ag , just added a couple of tests to it. Let us know if those suffice and look OK. Attaching a screenshot of the tests passing as well. Thanks!

image

@joe-ayoub-segment joe-ayoub-segment self-assigned this May 1, 2025
@joe-ayoub-segment
Copy link
Contributor

Hi @ManikMM - I'll provide feedback on this PR in the next 24h.

export const tracking_type: InputField = {
label: 'Tracking Type',
description:
"One of Reddit CAPI's standard conversion event types. To send a Custom event to Reddit use the Custom Event Action instead.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the description here actually reference Redit CAPI or Redit Pixel?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this, updated this to Reddit Pixel in the latest commit.

description: 'The number of items in the event. This should only be set for revenue-related events.',
type: 'integer'
},
value: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This field in the Redit CAPI Destination is called 'value_decimal'. Is the change in name deliberate? Any reason for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, it's just to align with the naming convention in the Reddit Javascript pixel (doc with references of what the pixel looks like for reference).

The Pixel uses "value" instead of "value_decimal" do to some legacy reasons. But "value" in the pixel is the same format as the "value_decimal" of the CAPI destination - they both look for float values such as 9.99.

Example:
rdt('track', 'AddToCart', { itemCount: 2, value: 19.99, conversionId: 'UniqueConversionID', currency: 'USD' });

}

export const user: InputField = {
label: 'User',
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that the Redit CAPI Destination's user field has slightly different property fields.
It has:

  1. external_id
  2. ip_address
  3. user_agent
  4. uuid

and the field in this Pixel Destination has:

  1. externalId
  2. idfa
  3. aaid
  4. phoneNumber

Are there good reasons why these are different?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes - for the Pixel Destination, the Reddit pixel automatically captures the ip address, user agent, and UUID directly from the browser, so we didn't need to add those to this Pixel Destination.

idfa and aaid are fields that need to be configured/passed by the advertiser though. And Phone Number is a new field as well that we'll also be adding to the CAPI Destination (here's the PR that I saw you picked up as well for it, thanks for that!) So I just added it here in the Pixel Destination as well to get ahead of it.

Here's some more info for reference on what configurable user values can be passed in the Reddit Pixel: https://business.reddithelp.com/s/article/advanced-matching-for-developers

required: false,
additionalProperties: false,
defaultObjectUI: 'keyvalue',
properties: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the 'modes' property has been moved to a top level setting, whereas in the CAPI Destination the 'modes' property is in the 'Data Processing Options' field. Is the reason for this because 'modes' should only have a single value per instantiation of the Pixel?

data_processing_options
},
perform: (rdt, { payload, settings }) => {
initPixel(rdt, payload, settings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it OK for the initPixel function to be called more than once on a single page load?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be called multiple times, but it could be better to just initialize it once per page load if possible. We can discuss in our call. Thanks!

export interface RedditPixel {
page: () => void
init: (pixelId: string, ldu?: any) => void
track: (eventName: string, eventMetadata?: any) => void
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @ManikMM let's define types for eventMetadata. We can't leave it as 'any' as the linter will complain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the heads up. Updated in the latest commit.

@@ -0,0 +1,37 @@
export interface RedditPixel {
page: () => void
init: (pixelId: string, ldu?: any) => void
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @ManikMM let's define a type for ldu. We can't leave it as 'any' as the linter will complain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the heads up, updated in the latest commit.

Comment on lines +16 to +42
export function trackCall(rdt: RedditPixel, payload: StandardEvent | CustomEvent) {
const custom_event_name = (payload as CustomEvent).custom_event_name

if (typeof custom_event_name === 'string' && custom_event_name.trim() !== '') {
;(payload as StandardEvent).tracking_type = 'Custom'
}

if ((payload as StandardEvent).tracking_type && Object.keys((payload as StandardEvent).tracking_type).length > 0) {
if (payload.user) {
if (payload.user?.device_type?.toLowerCase() === 'ios') {
payload.user.idfa = payload.user.advertising_id
} else {
payload.user.aaid = payload.user.advertising_id
}
}
const fullPayload = {
...payload.event_metadata,
products: payload.products,
conversionId: payload.conversion_id,
...payload.user,
customEventName: custom_event_name
}
rdt.track((payload as StandardEvent).tracking_type, fullPayload)
} else {
console.error(' No valid tracking type found in the Reddit Pixel.')
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @ManikMM I think we can optimize this function a bit.
Firstly let's defined a type for the 'fullPayload'.
Then we can remove idfa and aaid fields as they are redunant ( due to the advertising_id field).
Next, we can do something like this to figure out the eventName and customEventName values:

  const eventName = (payload as StandardEvent)?.tracking_type ?? 'Custom'
  const customEventName = (payload as CustomEvent)?.custom_event_name ?? undefined

This means we won't need the if/else block and we won't need to throw an error.

Let's discuss on a call.

name: 'Reddit Pixel',
slug: 'actions-reddit-pixel',
mode: 'device',

Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a description field here please? 1 line to explain what the Integration does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added in this commit.

idfa?: string | undefined
phoneNumber?: string | undefined
}) => void
metadata: ({
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @ManikMM , I can't see that this type is used anywhere. Can it be deleted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks for the catch. Updated here.

page: () => void
init: (pixelId: string, ldu?: any) => void
track: (eventName: string, eventMetadata?: any) => void
advanced_matching: ({
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @ManikMM , I can't see that this type is used anywhere. Can we remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks for the catch. Updated here.

Copy link
Contributor

@joe-ayoub-segment joe-ayoub-segment left a comment

Choose a reason for hiding this comment

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

Hi @ManikMM,
Thanks for this PR. I wen't through and left some inline comments. Please also see the attached video.

Can we schedule some time to make a few final updates to this Integration please?
There is an option for us to only use a single Action for this Integration. I can explain how when we meet.

Would you be free to meet on Tuesday next week? Please pick any time between 10:00 and 18:00 UK time, and send me an invite. Let's go with 1h if you can spare it.

Best regards,
Joe

Screen.Recording.2025-05-02.at.14.03.35.mov

akhsueh and others added 4 commits May 5, 2025 15:35
Co-authored-by: Joe Ayoub <45374896+joe-ayoub-segment@users.noreply.github.com>
Co-authored-by: Joe Ayoub <45374896+joe-ayoub-segment@users.noreply.github.com>
…e.json

Co-authored-by: Joe Ayoub <45374896+joe-ayoub-segment@users.noreply.github.com>
…e.json

Co-authored-by: Joe Ayoub <45374896+joe-ayoub-segment@users.noreply.github.com>
@ManikMM
Copy link
Contributor Author

ManikMM commented May 5, 2025

Hi @joe-ayoub-segment,

Glad to hear you're back :)
I went over you comments + video, and see that @akhsueh added a few changes as well...

Let's catch up soon - I've set up an hour for us on the 13th

@joe-ayoub-segment
Copy link
Contributor

Closed - moved to #2919

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.

5 participants