Showheroes bidder #4190
Open
Showheroes bidder #4190
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AntoxaAntoxic
requested changes
Sep 17, 2025
Comment on lines
57
to
60
| public ShowheroesBidder(String endpointUrl, | ||
| CurrencyConversionService currencyConversionService, | ||
| PrebidVersionProvider prebidVersionProvider, | ||
| JacksonMapper mapper) { |
There was a problem hiding this comment.
please fix formatting
| } | ||
|
|
||
| private BidderError validate(BidRequest bidRequest) { | ||
| // request must contain site object with page or app object with bundle |
There was a problem hiding this comment.
please remove comments here and further
| } | ||
|
|
||
| @Override | ||
| public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest request) { |
There was a problem hiding this comment.
please place the methods in the class in a calling order, according to the style guide of the repo
| this.pbsVersion = prebidVersionProvider.getNameVersionRecord(); | ||
| } | ||
|
|
||
| private BidderError validate(BidRequest bidRequest) { |
| return null; | ||
| } | ||
|
|
||
| private ExtRequestPrebidChannel getPrebidChannel(BidRequest bidRequest) { |
Comment on lines
33
to
37
| BidderDeps showheroesBidderDeps(BidderConfigurationProperties showheroesConfigurationProperties, | ||
| @NotBlank @Value("${external-url}") String externalUrl, | ||
| CurrencyConversionService currencyConversionService, | ||
| PrebidVersionProvider prebidVersionProvider, | ||
| JacksonMapper mapper) { |
| .banner(Banner.builder().build()) | ||
| .build())) | ||
| .build(), | ||
| mapper.writeValueAsString( |
There was a problem hiding this comment.
please hide mapper.writeValueAsString inside the givenHttpCall
Comment on lines
381
to
383
| assertThat(result.getValue()) | ||
| .extracting(BidderBid::getType) | ||
| .containsExactly(video); |
There was a problem hiding this comment.
please check the whole bid, not just a type, here and further
Comment on lines
414
to
418
| BidRequest.builder() | ||
| .imp(List.of( | ||
| Imp.builder().id("123").banner(Banner.builder().build()).build(), | ||
| Imp.builder().id("456").video(Video.builder().build()).build())) | ||
| .build(), |
There was a problem hiding this comment.
passing BidRequest is redundant, because it's never used by the makeBids method, so I believe we can just remove this parameter from the givenHttpCall
Comment on lines
+4
to
+6
| aliases: | ||
| showheroes-bs: ~ | ||
| showheroesBs: ~ |
There was a problem hiding this comment.
separate integration tests should be added for all the hard aliases to make sure the configuration is correct
AntoxaAntoxic
requested changes
Sep 18, 2025
Comment on lines
185
to
196
| @Override | ||
| public Result<List<BidderBid>> makeBids(BidderCall<BidRequest> httpCall, BidRequest bidRequest) { | ||
| final BidResponse bidResponse; | ||
|
|
||
| try { | ||
| bidResponse = mapper.decodeValue(httpCall.getResponse().getBody(), BidResponse.class); | ||
| } catch (DecodeException | PreBidException e) { | ||
| return Result.withError(BidderError.badServerResponse(e.getMessage())); | ||
| } | ||
|
|
||
| return Result.of(extractBids(bidResponse), Collections.emptyList()); | ||
| } |
There was a problem hiding this comment.
@Override
public Result<List<BidderBid>> makeBids(BidderCall<BidRequest> httpCall, BidRequest bidRequest) {
try {
final BidResponse bidResponse = mapper.decodeValue(httpCall.getResponse().getBody(), BidResponse.class);
return Result.withValues(extractBids(bidResponse));
} catch (DecodeException e) {
return Result.withError(BidderError.badServerResponse(e.getMessage()));
}
}
Comment on lines
6
to
8
| /** | ||
| * Defines the contract for bidRequest.imp[i].ext.showheroes | ||
| */ |
There was a problem hiding this comment.
please remove the comment
|
|
||
| @BeforeEach | ||
| public void setUp() { | ||
| // set always 'test_version' as Prebid version for testing |
There was a problem hiding this comment.
please remove the comment
| import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; | ||
| import static java.util.Collections.singletonList; | ||
|
|
||
| public class ShowheroesBSTest extends IntegrationTest { |
There was a problem hiding this comment.
you've missed one alias
Comment on lines
217
to
222
| assertThat(result.getValue()).hasSize(1); | ||
|
|
||
| final BidRequest outgoingRequest = result.getValue().get(0).getPayload(); | ||
| assertThat(outgoingRequest.getImp()).hasSize(2) | ||
| .extracting(Imp::getId) | ||
| .containsExactly("imp1", "imp2"); |
There was a problem hiding this comment.
assertThat(result.getValue()).hasSize(1)
.extracting(HttpRequest::getImpIds)
.containsOnly(Set.of("imp1", "imp2"));
Comment on lines
298
to
312
| @Test | ||
| public void makeBidsShouldReturnVideoBidIfVideoIsPresentInImp() throws JsonProcessingException { | ||
| // given | ||
| final BidderCall<BidRequest> httpCall = givenHttpCall( | ||
| givenBidResponse(bidBuilder -> bidBuilder.impid("123").mtype(2))); | ||
|
|
||
| // when | ||
| final Result<List<BidderBid>> result = target.makeBids(httpCall, null); | ||
|
|
||
| // then | ||
| assertThat(result.getErrors()).isEmpty(); | ||
| assertThat(result.getValue()) | ||
| .extracting(BidderBid::getBid, BidderBid::getType) | ||
| .containsExactly(tuple(Bid.builder().impid("123").mtype(2).build(), video)); | ||
| } |
There was a problem hiding this comment.
This test is redundant because there is no such test case as this test covers.
Comment on lines
282
to
296
| @Test | ||
| public void makeBidsShouldReturnBannerBidIfBannerIsPresentInImp() throws JsonProcessingException { | ||
| // given | ||
| final BidderCall<BidRequest> httpCall = givenHttpCall( | ||
| givenBidResponse(bidBuilder -> bidBuilder.impid("123").mtype(1))); | ||
|
|
||
| // when | ||
| final Result<List<BidderBid>> result = target.makeBids(httpCall, null); | ||
|
|
||
| // then | ||
| assertThat(result.getErrors()).isEmpty(); | ||
| assertThat(result.getValue()) | ||
| .extracting(BidderBid::getBid, BidderBid::getType) | ||
| .containsExactly(tuple(Bid.builder().impid("123").mtype(1).build(), banner)); | ||
| } |
There was a problem hiding this comment.
This test is redundant because there is no such test case as this test covers.
Comment on lines
235
to
240
| assertThat(result.getValue()).hasSize(1); | ||
|
|
||
| final Map<String, String> headers = result.getValue().get(0).getHeaders().entries().stream() | ||
| .collect(java.util.stream.Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); | ||
| assertThat(headers).containsEntry("Content-Type", "application/json;charset=utf-8") | ||
| .containsEntry("Accept", "application/json"); |
There was a problem hiding this comment.
assertThat(result.getValue()).hasSize(1).first()
.extracting(HttpRequest::getHeaders)
.satisfies(headers -> assertThat(headers.get(CONTENT_TYPE_HEADER))
.isEqualTo(APPLICATION_JSON_CONTENT_TYPE))
.satisfies(headers -> assertThat(headers.get(ACCEPT_HEADER))
.isEqualTo(APPLICATION_JSON_VALUE));
src/test/java/org/prebid/server/bidder/showheroes/ShowheroesBidderTest.java
Show resolved
Hide resolved
src/test/java/org/prebid/server/bidder/showheroes/ShowheroesBidderTest.java
Show resolved
Hide resolved
|
@AntoxaAntoxic I think I addressed all your comments. Thanks for your review. |
Comment on lines
122
to
142
| private Imp modifyImp(BidRequest bidRequest, Imp imp, ExtRequestPrebidChannel prebidChannel) { | ||
| final ExtImpShowheroes extImpShowheroes = parseImpExt(imp); | ||
|
|
||
| final Imp.ImpBuilder impBuilder = imp.toBuilder(); | ||
|
|
||
| if (prebidChannel != null && imp.getDisplaymanager() == null) { | ||
| impBuilder.displaymanager(prebidChannel.getName()); | ||
| impBuilder.displaymanagerver(prebidChannel.getVersion()); | ||
| } | ||
|
|
||
| impBuilder.ext(modifyImpExt(imp, extImpShowheroes)); | ||
|
|
||
| if (!shouldConvertFloor(imp)) { | ||
| return impBuilder.build(); | ||
| } | ||
|
|
||
| return impBuilder | ||
| .bidfloorcur(BID_CURRENCY) | ||
| .bidfloor(resolveBidFloor(bidRequest, imp)) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
I'd like to suggest the following refactoring
private Imp modifyImp(BidRequest bidRequest, Imp imp, ExtRequestPrebidChannel prebidChannel) {
final ExtImpShowheroes extImpShowheroes = parseImpExt(imp); // I'd prefer to move this out of the modifyImp method
final boolean shouldSetDisplayManager = prebidChannel != null && imp.getDisplaymanager() == null;
final boolean shouldConvertFloor = shouldConvertFloor(imp);
return imp.toBuilder()
.displaymanager(shouldSetDisplayManager ? prebidChannel.getName() : imp.getDisplaymanager())
.displaymanagerver(shouldSetDisplayManager ? prebidChannel.getVersion()) : imp.getDisplaymanagerver())
.bidfloorcur(shouldConvertFloor ? BID_CURRENCY : imp.getBidfloorcur())
.bidfloor(shouldConvertFloor ? resolveBidFloor(bidRequest, imp) : imp.getbidloor())
.ext(modifyImpExt(imp, extImpShowheroes));
.build();
}| try { | ||
| final BidResponse bidResponse = mapper.decodeValue(httpCall.getResponse().getBody(), BidResponse.class); | ||
| return Result.of(extractBids(bidResponse), Collections.emptyList()); | ||
| } catch (DecodeException | PreBidException e) { |
There was a problem hiding this comment.
PreBidException is impossible here
| .toList(); | ||
| } | ||
|
|
||
| private BidType getBidType(Bid bid) { |
| final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest); | ||
|
|
||
| // then | ||
| assertThat(result.getErrors()).hasSize(1); |
There was a problem hiding this comment.
what kind of error is expected?
| final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest); | ||
|
|
||
| // then | ||
| assertThat(result.getErrors()).hasSize(1); |
There was a problem hiding this comment.
what kind of error is expected?
| tuple(Bid.builder().impid("456").mtype(2).price(BigDecimal.TEN).build(), video)); | ||
| } | ||
|
|
||
| private static BidRequest givenBidRequest(Function<Imp.ImpBuilder, Imp.ImpBuilder> impCustomizer) { |
There was a problem hiding this comment.
This method utilizes the impCustomizer only once in the makeHttpRequestsShouldReturnErrorIfImpExtCouldNotBeParsed. All other tests call the givenBidRequest(identity()), so I believe we don't need the impCustomizer parameter
Comment on lines
227
to
230
| final BidRequest outgoingRequest = result.getValue().get(0).getPayload(); | ||
| assertThat(outgoingRequest.getImp()).hasSize(2) | ||
| .extracting(Imp::getId) | ||
| .containsExactly("imp1", "imp2"); |
There was a problem hiding this comment.
Here is the suggestion
assertThat(result.getValue()).hasSize(1)
.extracting(HttpRequest::getPayload)
.flatExtracting(BidRequest::getImp)
.extracting(Imp::getId)
.containsExactly("imp1", "imp2");
Comment on lines
325
to
327
| assertThat(result.getValue()) | ||
| .extracting(BidderBid::getBid, BidderBid::getType) | ||
| .containsExactly(tuple(Bid.builder().impid("123").mtype(99).build(), video)); |
Comment on lines
+347
to
+369
| @Test | ||
| public void makeBidsShouldReturnMultipleBids() throws JsonProcessingException { | ||
| // given | ||
| final BidderCall<BidRequest> httpCall = givenHttpCall( | ||
| BidResponse.builder() | ||
| .seatbid(singletonList(SeatBid.builder() | ||
| .bid(List.of( | ||
| Bid.builder().impid("123").mtype(1).price(BigDecimal.ONE).build(), | ||
| Bid.builder().impid("456").mtype(2).price(BigDecimal.TEN).build())) | ||
| .build())) | ||
| .build()); | ||
|
|
||
| // when | ||
| final Result<List<BidderBid>> result = target.makeBids(httpCall, null); | ||
|
|
||
| // then | ||
| assertThat(result.getErrors()).isEmpty(); | ||
| assertThat(result.getValue()).hasSize(2) | ||
| .extracting(BidderBid::getBid, BidderBid::getType) | ||
| .containsExactlyInAnyOrder( | ||
| tuple(Bid.builder().impid("123").mtype(1).price(BigDecimal.ONE).build(), banner), | ||
| tuple(Bid.builder().impid("456").mtype(2).price(BigDecimal.TEN).build(), video)); | ||
| } |
AntoxaAntoxic
approved these changes
Sep 25, 2025
|
Hi @AntoxaAntoxic and @CTMBNara , can we know the next steps to do for this bidder to be merged? Do we need a second approval? We'd like to push this forward; thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
🔧 Type of changes
✨ What's the context?
Adding support for the
showheroesbid adapter. Copy of the: prebid/prebid-server#4533Docs PR: prebid/prebid.github.io#6256
🔎 New Bid Adapter Checklist
🧪 Test plan
How do you know the changes are safe to ship to production?
🏎 Quality check