-
Notifications
You must be signed in to change notification settings - Fork 181
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 bidder bidmyadz #1352
New bidder bidmyadz #1352
Conversation
- video | ||
- native | ||
supported-vendors: | ||
vendor-id: 724 |
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 is between vendor id. BidMyAdz id is not present in go PR, and also 79.json does not contain it, so set it as zero,
Also pls fix it for bidmachine which id is 736
} | ||
} | ||
|
||
private boolean isValidRequest(BidRequest request, List<BidderError> errors) { |
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.
Can be static
And actually we can a bit update this method, not to check device for null
private static boolean isValidRequest(BidRequest request, List<BidderError> errors) {
if (request.getImp().size() > 1) {
errors.add(BidderError.badInput("Bidder does not support multi impression"));
}
final Device device = request.getDevice();
final String ip = device != null ? device.getIp() : null;
final String ipv6 = device != null ? device.getIpv6() : null;
if (StringUtils.isEmpty(ip) && StringUtils.isEmpty(ipv6)) {
errors.add(BidderError.badInput("IP/IPv6 is a required field"));
}
final String userAgent = device != null ? device.getUa() : null;
if (StringUtils.isEmpty(userAgent)) {
errors.add(BidderError.badInput("User-Agent is a required field"));
}
return errors.isEmpty();
}
Then hasValidIp()
method can be removed
.payload(request) | ||
.body(mapper.encode(request)) | ||
.build()); | ||
} catch (EncodeException e) { |
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 try/catch can be omitted
return Result.withValue(HttpRequest.<BidRequest>builder() | ||
.method(HttpMethod.POST) | ||
.uri(endpointUrl) | ||
.headers(HttpUtil.headers().add(HttpUtil.X_OPENRTB_VERSION_HEADER, "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.
Let's add method createHeaders()
|
||
private static List<BidderBid> bidsFromResponse(BidRequest bidRequest, BidResponse bidResponse) { | ||
final SeatBid seatBid = bidResponse.getSeatbid().get(0); | ||
return seatBid.getBid().stream() |
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.
From Go they also have error for empty bids
|
||
// then | ||
assertThat(result.getErrors()) | ||
.hasSize(1) |
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.
Can be omitted
.extracting(HttpRequest::getHeaders) | ||
.flatExtracting(MultiMap::entries) | ||
.extracting(Map.Entry::getKey, Map.Entry::getValue) | ||
.contains(tuple(HttpUtil.X_OPENRTB_VERSION_HEADER.toString(), "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.
Let's add check for every header
@AllArgsConstructor(staticName = "of") | ||
public class ExtImpBidmyadz { | ||
|
||
String placementId; |
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.
Seems here @JsonProperty
should be used
.bidderCreator(config -> new BidmyadzBidder(config.getEndpoint(), mapper)) | ||
.assemble(); | ||
} | ||
|
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.
Redundant \n
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.stream.Collectors; | ||
|
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.
/**
* BidMyAdz {@link Bidder} implementation.
*/
} | ||
|
||
@Test | ||
public void makeBidsShouldReturnBannerBidIfBannerIsPresent() throws JsonProcessingException { |
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.
IfMediaTypeEqualsBanner
} | ||
|
||
@Test | ||
public void makeHttpRequestsShouldReturnErrorsIfBidRequestHasMoreThan() { |
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.
HasMoreThan...
} | ||
|
||
@Test | ||
public void makeHttpRequestsShouldReturnErrorsIfBidRequestDoesNotHaveUa() { |
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.
DeviceDoesNotHaveUserAgent
} | ||
|
||
@Test | ||
public void makeHttpRequestsShouldReturnErrorsIfBidRequestDoesNotHaveIp() { |
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.
DeviceDoesNotHaveIpOrIpV6
} | ||
|
||
private static BidType getBidType(JsonNode ext) { | ||
final JsonNode mediaTypeNode = ext.get("mediaType"); |
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.
We should check that ext is not null to prevent NPE
No description provided.