-
Notifications
You must be signed in to change notification settings - Fork 220
New Alvads Adapter #4194
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
base: master
Are you sure you want to change the base?
New Alvads Adapter #4194
Conversation
Hi @SamuelAlejandroNT why did you re-created the PR you've closed? Since you've done that the history of comments is left there, so please fix all the comments for the previous PR. Thank you |
Hi @AntoxaAntoxic , The reason was an issue with branches caused by our team—apologies for that. We've also fixed the errors now. Thank you. |
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.
Some the comments from the previous PR have been ignored (especially test-related), please double check and fix them
src/main/java/org/prebid/server/spring/config/bidder/AlvadsConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/alvads/AlvadsBidder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/alvads/AlvadsBidder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/alvads/AlvadsBidder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/alvads/AlvadsBidder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/alvads/AlvadsBidder.java
Outdated
Show resolved
Hide resolved
src/test/java/org/prebid/server/bidder/alvads/AlvadsBidderTest.java
Outdated
Show resolved
Hide resolved
Hi @AntoxaAntoxic, the previous PR comments have been reviewed, addressed, and the changes have been pushed. Thanks for your feedback. |
@SamuelAlejandroNT please add an integration test for the bidder |
Added an integration test |
🔧 Type of changes
[ x] new bid adapter
bid adapter update
new feature
new analytics adapter
new module
module update
bugfix
documentation
configuration
dependency update
tech debt (test coverage, refactorings, etc.)
✨ What's the context?
A new bid adapter AlvadsBidder has been implemented, and the bid type determination (BidType) logic has been corrected to properly distinguish between banner and video Imps. Test payloads were also updated to reflect separate banner and video Imps.
🧠 Rationale behind the change
Previously, the getBidType method always returned banner, even when the Imp was a video. This caused test failures and incorrect bid type mapping. The changes include:
Mapping each Bid to its corresponding AlvaAdsImp using impId.
Returning BidType.video when the Imp has a video object.
Updating test payloads to include separate banner and video Imps.
Ensuring lambda variables are final to comply with Java requirements.
No significant trade-offs were involved; the change is internal logic and test improvements.
🔎 New Bid Adapter Checklist
[x ] verify email contact works
[ x] NO fully dynamic hostnames
[ x] geographic host parameters are NOT required
[ x] direct use of HTTP is prohibited - implement an existing Bidder interface that will do all the job
[x ] if the ORTB is just forwarded to the endpoint, use the generic adapter - define the new adapter as the alias of the generic adapter
[ x] cover an adapter configuration with an integration test
🧪 Test plan
Bids were tested with both banner and video Imps. The results were verified to ensure:
Banner bids return BidType.banner.
Video bids return BidType.video.
Unit test makeBidsShouldReturnBidderBidsWithFullFields passes successfully.
🏎 Quality check
[x ] Are your changes following our code style guidelines?
[x ] Are there any breaking changes in your code?
[x ] Does your test coverage exceed 90%?
[x ] Are there any erroneous console logs, debuggers or leftover code in your changes?