IX: Add multi-format ad unit support #2078
Conversation
| @@ -290,9 +292,28 @@ private static Response mergeNativeImpTrackers(Response response, List<EventTrac | |||
| .build(); | |||
| } | |||
|
|
|||
| private static BidType getBidType(String impId, List<Imp> imps) { | |||
| private static BidType getBidType(Bid bid, List<Imp> imps) { | |||
| Integer mType = bid.getMtype(); | |||
There was a problem hiding this comment.
According to project style, we prefer to use final keyword on variables declaration
| private static BidType getBidType(String impId, List<Imp> imps) { | ||
| private static BidType getBidType(Bid bid, List<Imp> imps) { | ||
| Integer mType = bid.getMtype(); | ||
| BidType bidType = mType != null ? switch (bid.getMtype()) { |
There was a problem hiding this comment.
Seems bidType resolving logic can be moved to separate function
| return bidType; | ||
| } | ||
|
|
||
| Optional<String> prebidType = Optional.ofNullable(bid.getExt()) |
There was a problem hiding this comment.
If you will move logic from 315 line to separate method, then you code will be easily simplified to
return Optional.ofNullable(bid.getExt())
...
.orElseGet(()->methodWithLogicFrom315Line());| @@ -636,6 +636,68 @@ public void makeBidsShouldReturnValidAdmIfNativeIsPresentInImpAndAdm12() throws | |||
| .containsExactly(mapper.writeValueAsString(expectedNativeResponse)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void makeBidsShouldReturnCorrectTypeMTypeInResponse() throws JsonProcessingException { | |||
There was a problem hiding this comment.
makeBidsShouldReturnVideoBidIfMtypeIsTwo
There was a problem hiding this comment.
Also pls create test for every mtype possible value
| .map(prebid -> prebid.get("type")) | ||
| .map(JsonNode::asText) | ||
| .map(BidType::fromString) | ||
| .orElse(getBidTypeFromImp(imps, bid.getImpid())); |
There was a problem hiding this comment.
If you are using orElse argument value will always be calculated, to had it calculated only if really needed
pls use (.orElseGet(() -> getBidTypeFromImp(imps, bid.getImpid())))
|
|
||
| // when | ||
| final Result<List<BidderBid>> result = ixBidder.makeBids(httpCall, null); | ||
| // then |
There was a problem hiding this comment.
Pls make 768 line empty
| .mtype(4)))); | ||
|
|
||
| // when | ||
| final Result<List<BidderBid>> result = ixBidder.makeBids(httpCall, null); |
There was a problem hiding this comment.
We prefer to have empty line betweengiven when then blocks
There was a problem hiding this comment.
Also pls check for similar occurences
| assertThat(result.getValue()) | ||
| .extracting(BidderBid::getBid) | ||
| .extracting(Bid::getExt) | ||
| .extracting(ext -> ext.get("prebid")) | ||
| .extracting(node -> mapper.treeToValue(node, ExtBidPrebid.class)) | ||
| .extracting(ExtBidPrebid::getType) | ||
| .containsExactly(BidType.video); |
There was a problem hiding this comment.
Why do we need to assert our input?
There was a problem hiding this comment.
Something could have altered the ext object while processing the incoming bid. We expect it to stay the same.
If it seems an unnecessary check I can take it out.
There was a problem hiding this comment.
Only the adapter can affect this result. If logic appears that somehow changes this object, the corresponding test will be written.
liliana-sortable
requested review from
CTMBNara
and removed request for
SerhiiNahornyi
This is a port of prebid/prebid-server#2315