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

TL-36516: Add GPP signals to sync endpoint #80

Merged
merged 20 commits into from
Jun 23, 2023

Conversation

patrickloughrey
Copy link

Type of change

  • Feature

Description of change

Triplelift is adding GPP consent signals to our user sync endpoint when GPP consent data is available

patrickloughrey and others added 18 commits March 1, 2023 14:20
…kmethod

TL-35335: Cast playbackmethod as array
TL-36204: Copy tid to imp extension object
…py-TID

Revert "TL-36204: Copy tid to imp extension object"
@patrickloughrey patrickloughrey self-assigned this Jun 20, 2023
@patrickloughrey
Copy link
Author

@aikenstl per our discussion, breaking out gpp_sid into comma-separated values when adding it to our sync endpoint per IAB's recommendation

This prevents me from using the Prebid utils tryAppendQueryString to add gpp_sid to our sync endpoint because the comma-separated gpp_sid values become URI encoded when there is more than one value in the applicableSections array. cc: @nllerandi3lift

@@ -113,6 +122,13 @@ function _getSyncType(syncOptions) {
if (syncOptions.pixelEnabled) return 'image';
}

function _filterSid(sid) {
return sid.filter(element => {
return element !== null && element !== undefined;

Choose a reason for hiding this comment

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

I think we should make this
return Number.isInteger(element);
because that's all it should ever be anyway and will also exclude weird nulls/undefineds

Copy link
Author

Choose a reason for hiding this comment

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

I think I like the way I'm doing it here better. The applicableSections within the gppConsent object is an array that we need to do a join(',') on in order to transform it into a comma-separated string to append onto our sync pixel. The filter() is just to make sure the elements are valid

Choose a reason for hiding this comment

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

I meant keep the filter, but replace return element !== null && element !== undefined;
.isInteger will reject strings, etc which are also not valid, in addition to rejecting null/undefined. But this is a small suggestion so whatev you want

Copy link
Author

Choose a reason for hiding this comment

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

I misunderstood your original comment. I agree, the way you are suggesting is optimal. Pushing through.

syncEndpoint = tryAppendQueryString(syncEndpoint, 'gpp', gppConsent.gppString);
}
if (gppConsent.applicableSections && gppConsent.applicableSections.length !== 0) {
syncEndpoint = syncEndpoint + 'gpp_sid=' + _filterSid(gppConsent.applicableSections);

Choose a reason for hiding this comment

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

We need to url encode this so we don't end up with commas in the url
image

Copy link
Author

Choose a reason for hiding this comment

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

cc: @aikenstl see Nick's comment above - this will need to be URI encoded. I thought this may be the case - eng will need to account for this (decode this sid) in order to extract the correct gpp_sid values

@nllerandi3lift
Copy link

Approved as is. Up to you if you want to use isInteger on the filter or not. Otherwise feel free to merge and submit to Prebid as 1 PR (fledge and gpp stuff)

@patrickloughrey patrickloughrey merged commit c4a6dd7 into master Jun 23, 2023
@patrickloughrey patrickloughrey deleted the TL-36516-GPP-Sync-Endpoint branch June 23, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants