-
Notifications
You must be signed in to change notification settings - Fork 759
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
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!
@bsardo @bretg @przemkaczmarek Hi, If there's anything else we can do from our side to facilitate the review process or address any concerns, please don't hesitate to let us know. |
New adapters must also open a PR against the Prebid Docs repository here: https://github.com/prebid/prebid.github.io/ Please see the User Documentation section of the new adapter developer guide. |
Hello @VeronikaSolovei9, we already had a JavaScript adapter; however, I’ve submitted a pull request with pbs set to true. Thanks for your feedback!: |
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.
Thank you, I overlooked the documentation exists already. Approved.
Hi @VeronikaSolovei9, What is missing to merge the adapter? Is there anything else we can do to help? Regarding the 'Changes Requested' tag that is still present here, it was something that never affected this adapter—it only impacted the JavaScript plugin. I mentioned this to Bretg, and he asked your teammates to double-check, but I see that no one has removed the 'Changes Requested' tag yet. You can read the entire pull-request conversation to understand. Thanks! |
Summary
This PR introduces the Adverxo bidder adapter for Prebid Server. It supports the following mediatypes:
Maintainer Contact: [email protected]