-
Notifications
You must be signed in to change notification settings - Fork 190
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
Tradplus bid adapter #3508
Tradplus bid adapter #3508
Conversation
|
||
@Override | ||
public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest bidRequest) { | ||
final Map<ExtImpTradPlus, List<Imp>> extToImps = new HashMap<>(); |
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.
using a map here is an overhead since only one single imp is really utilized to retrieve the ext object, please get rid of it
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.
I have changed these codes,please review these codes.
private static List<Imp> removeFirstImpExt(List<Imp> imps) { | ||
imps.set(0, imps.getFirst().toBuilder().ext(null).build()); | ||
return imps; | ||
} |
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.
this looks redundant since the list of imps always has a single imp
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.
I have changed these codes,please review these codes.
public Result<List<BidderBid>> makeBids(BidderCall<BidRequest> httpCall, BidRequest bidRequest) { | ||
try { | ||
final BidResponse bidResponse = mapper.decodeValue(httpCall.getResponse().getBody(), BidResponse.class); | ||
return Result.of(extractBids(bidResponse, httpCall.getRequest().getPayload()), Collections.emptyList()); |
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.
Result.withValues()
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.
I have changed these codes,please review these codes.
import static org.prebid.server.proto.openrtb.ext.response.BidType.video; | ||
import static org.prebid.server.proto.openrtb.ext.response.BidType.xNative; | ||
|
||
public class TradPlusBidderTest extends VertxTest { |
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.
there is no any test that covers the outgoing http request:
- payload: the basic logic of using only a single imp ignoring others
- headers
- impIds object
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, please review these codes.
@@ -455,6 +455,8 @@ adapters.telaria.enabled=true | |||
adapters.telaria.endpoint=http://localhost:8090/telaria-exchange/ | |||
adapters.theadx.enabled=true | |||
adapters.theadx.endpoint=http://localhost:8090/theadx-exchange | |||
adapters.tradplus.enabled=true | |||
adapters.tradplus.endpoint=http://localhost:8090/tradplus-exchange |
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 the macros to the url to check them in the integration test
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.
private ExtImpTradPlus parseImpExt(ObjectNode extNode) { | ||
final ExtImpTradPlus extImpTradPlus; | ||
try { | ||
extImpTradPlus = mapper.mapper().convertValue(extNode, EXT_TYPE_REFERENCE).getBidder(); |
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.
just return here
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.
final MultiMap headers = HttpUtil.headers(); | ||
headers.set(HttpUtil.X_OPENRTB_VERSION_HEADER, X_OPENRTB_VERSION); | ||
return headers; |
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.
return HttpUtil.headers().set(HttpUtil.X_OPENRTB_VERSION_HEADER, X_OPENRTB_VERSION);
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.
|
||
@Override | ||
public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest bidRequest) { | ||
final List<HttpRequest<BidRequest>> httpRequests = new ArrayList<>(); |
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.
I don't see the point to have the list if it's always only a single one
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.
I have changed these codes,please review these codes.
try { | ||
final ExtImpTradPlus extImpTradPlus = parseImpExt(bidRequest.getImp().getFirst().getExt()); | ||
validateImpExt(extImpTradPlus); | ||
httpRequests.add(makeHttpRequest(extImpTradPlus, bidRequest.getImp(), bidRequest)); |
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.
return the request here
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.
.satisfies(headers -> assertThat(headers.get(X_OPENRTB_VERSION_HEADER)) | ||
.isEqualTo("2.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.
please assert the default headers as well
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.
assertThat(result.getValue()).hasSize(1) | ||
.extracting(HttpRequest::getPayload) | ||
.extracting(BidRequest::getImp) | ||
.extracting(List::size) | ||
.containsOnly(2); |
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.
imp ext clean-up isn't covered
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.
BidRequest.builder() | ||
.imp(singletonList(Imp.builder().id("123").build())) | ||
.build(), | ||
mapper.writeValueAsString( |
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 hide the mapper.writeValueAsString
call inside the givenBidResponse
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.
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.
it's not done
why do you serialize twice?
|
||
// then | ||
assertThat(result.getErrors()).isEmpty(); | ||
assertThat(result.getValue()) |
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.
assertThat(result.getValue())
.extracting(HttpRequest::getPayload)
.flatExtracting(BidRequest::getImp)
.extracting(Imp::getExt)
.containsOnlyNulls();
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.
} | ||
|
||
private static List<BidderBid> extractBids(BidResponse bidResponse, BidRequest bidRequest) { | ||
return bidResponse == null || CollectionUtils.isEmpty(bidResponse.getSeatbid()) ? Collections.emptyList() : bidsFromResponse(bidResponse, bidRequest.getImp()); |
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.
I suspect this line fails checkstyle, please run it locally to check
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.
Error: /home/runner/work/prebid-server-java/prebid-server-java/src/main/java/org/prebid/server/bidder/tradplus/TradPlusBidder.java:27:8: Unused import - java.util.ArrayList. [UnusedImports]
Error: /home/runner/work/prebid-server-java/prebid-server-java/src/main/java/org/prebid/server/bidder/tradplus/TradPlusBidder.java:56:37: Variable 'httpRequest' should be declared final. [FinalLocalVariable]
Error: /home/runner/work/prebid-server-java/prebid-server-java/src/main/java/org/prebid/server/bidder/tradplus/TradPlusBidder.java:79: Line is longer than 120 characters (found 123). [LineLength]
Error: /home/runner/work/prebid-server-java/prebid-server-java/src/main/java/org/prebid/server/bidder/tradplus/TradPlusBidder.java:80: Line is longer than 120 characters (found 135). [LineLength]
Error: /home/runner/work/prebid-server-java/prebid-server-java/src/main/java/org/prebid/server/bidder/tradplus/TradPlusBidder.java:106: Line is longer than 120 characters (found 167). [LineLength]
Error: /home/runner/work/prebid-server-java/prebid-server-java/src/main/java/org/prebid/server/bidder/tradplus/TradPlusBidder.java:110: Line is longer than 120 characters (found 243). [LineLength]
Error: /home/runner/work/prebid-server-java/prebid-server-java/src/main/java/org/prebid/server/bidder/tradplus/TradPlusBidder.java:125: Line is longer than 120 characters (found 134). [LineLength]
Error: /home/runner/work/prebid-server-java/prebid-server-java/src/test/java/org/prebid/server/bidder/tradplus/TradPlusBidderTest.java:324: Line is longer than 120 characters (found 131). [LineLength]
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.
The build is still red, please fix the checkstyle also please do not forget about this comment as well |
TradPlus is a leading, stable, and reliable ad monetization platform that is committed to providing global developers with fair, transparent and efficient monetization solutions. We also provide ADX and Saas ADX services. We serve 2,000+ global developers. Moreover, Our SDK has been certified by IAB Tech Lab and also joins Google Play SDK Index. And we have Information Security Management Certification.
Our advertising business spans the world. This is our official website: https://tradplusad.com/en. You can get more details about TradPlus.
this is TradPlus adapter version 1.0.
Thank!