-
Notifications
You must be signed in to change notification settings - Fork 769
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
Zeta Global SSP: Make sid param required #3849
base: master
Are you sure you want to change the base?
Zeta Global SSP: Make sid param required #3849
Conversation
Code coverage summaryNote:
zeta_global_sspRefer here for heat map coverage report
|
@@ -1,5 +1,8 @@ | |||
endpoint: https://ssp.disqus.com/bid/prebid-server?sid=GET_SID_FROM_ZETA | |||
enabled: false |
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.
Can you please help me understand why you have set enabled: false
? As per the Prebid Server Documentation, if you want to disable the bidder, you need to set disabled: true
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.
Seems like copy-pasted from somewhere. Fixed. Thank you!
@@ -9,12 +12,14 @@ capabilities: | |||
mediaTypes: | |||
- banner | |||
- video | |||
- audio |
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.
Please add json tests for audio
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.
Done
Code coverage summaryNote:
zeta_global_sspRefer here for heat map coverage report
|
@Sonali-More-Xandr @onkarvhanumante Could you please take a look again? thank you. |
With a field now being required, this is a breaking change so we should hold off on merging until the next major release so that host companies are prompted to re-review their stored requests so they can make the necessary adjustments. Tagging as 3.0. |
userSync: | ||
redirect: | ||
url: https://ssp.disqus.com/redirectuser?sid=GET_SID_FROM_ZETA&gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&r={{.RedirectURL}} | ||
url: https://ssp.disqus.com/redirectuser?sid={{.AccountID}}&gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&r={{.RedirectURL}} |
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.
AccountID
is not a supported macro for user sync endpoints. The information can be optionally provided, so I'm not opposed to adding the macro - but I wouldn't expect it to always be present. How does your user sync behave when the AccountID
is unknown?
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.
@SyntaxNode In case of user sync, sid
param is not critical for us and doesn't really impact the behavior. However, it would be great for us to know what publisher we do cookie sync with (for internal statistics and troubleshooting).
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.
Hi @abermanov-zeta, as @SyntaxNode mentioned, AccountID
is not a supported macro for cookie syncing at this time. If you need this, could you please open a separate PR that adds support for it? If you don't need it at this time, please remove this from your redirect URL.
If you choose to open a new PR to add support for this macro, it will obviously be a macro resolved at runtime. You'll probably want to pull the account ID from endpoints/cookie_sync.go#parseRequest
similar to how other runtime macro values are extracted for privacy (see privacyMacros
variable) in this function, ensure that it is passed to handleResponse
which then passes it to usersync/syncer.go#GetSync
where the runtime macros appear to be resolved.
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.
@bsardo Thank you! I removed the macro from the redirect url for now. I'll open a separate PR to add support for it once I have time (hopefully, this year).
Hi @abermanov-zeta, when you have time, please resolve conflicts. Also, we recently released PBS 3.0, more specifically v3.1.0, which updates Prebid Server package import references throughout the project from
As a result, please merge with master (no rebase) and then ensure all Prebid Server package import references in the files you’ve changed are |
Code coverage summaryNote:
zeta_global_sspRefer here for heat map coverage report
|
@bsardo Done. Thank you. |
@bsardo @Sonali-More-Xandr @SyntaxNode @onkarvhanumante @VeronikaSolovei9 Could you please review again? Thank you! |
Code coverage summaryNote:
zeta_global_sspRefer here for heat map coverage report
|
Zeta Global SSP Adapter update: 'sid' is a required parameter.