-
Notifications
You must be signed in to change notification settings - Fork 753
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
New Adapter: Adverxo #4018
base: master
Are you sure you want to change the base?
New Adapter: Adverxo #4018
Conversation
Code coverage summaryNote:
adverxoRefer here for heat map coverage report
|
Code coverage summaryNote:
adverxoRefer here for heat map coverage report
|
Hi @dev-adverxo, 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:
adverxoRefer here for heat map coverage report
|
Hi @bsardo, thanks for the update. We've made the necessary changes and ensured that the tests passed. Let us know if there's anything else. |
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.
host cannot be a required parameter. It is incredibly awkward for publishers to have to enter a hostname on thousands of adunits when this is a global config.
For PBS, it's host should be specified in your YAML file. If you wish to have an optional impression-level host override, that's ok.
Hi @bretg, just to clarify, we don’t have a host parameter here. It’s only present in Prebid.js. |
Code coverage summaryNote:
adverxoRefer here for heat map coverage report
|
Hi @przemkaczmarek sorry for the changes after your review, but in the review process of our Prebid.js adapter we decide to introduce two aliases of our adapter. |
Code coverage summaryNote:
adverxoRefer here for heat map coverage report
|
Hi @bretg, sorry for tagging you again. I noticed the "required changes" tag is still present here, but it seems to be related to the optional host parameter, which we have already resolved. Could you please mark it as solved? Thanks! |
@przemkaczmarek and @bsardo , please review when you get a chance. |
@@ -0,0 +1,9 @@ | |||
endpoint: "https://adport.pbsadverxo.com/auction?id={{.AdUnit}}&auth={{.TokenID}}" |
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.
@@ -0,0 +1,22 @@ | |||
endpoint: "https://pbsadverxo.com/auction?id={{.AdUnit}}&auth={{.TokenID}}" |
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.
`null`, | ||
`{}`, | ||
`{"adCode": "string", "seatCode": 5, "originalPublisherid": "string"}`, | ||
`{ "adUnitId": 5}`, |
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.
add test with only "auth"
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 @przemkaczmarek, we already had a test for only "auth", it is on line 47, if it is not what you mean or need something else, please, let us know, thanks!
Summary
This PR introduces the Adverxo bidder adapter for Prebid Server. It supports the following mediatypes:
Maintainer Contact: [email protected]