Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TL-36516: Add GPP signals to sync endpoint #80
Changes from 18 commits
25664ce
7307d1a
ee90d99
3ee1e9b
0ff06ab
40dbe99
4291c1e
feecf24
a709f3f
49ba6a9
cd65aba
b362e3a
5574ef5
13ee09b
35a7286
3836830
c13200c
0c5d586
15540f7
a46248e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
valuesThere was a problem hiding this comment.
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
There was a problem hiding this comment.
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 thegppConsent
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 validThere was a problem hiding this comment.
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 wantThere was a problem hiding this comment.
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.