-
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
New adapter: Port Connatix adapter from PBS-Go #3781
base: master
Are you sure you want to change the base?
New adapter: Port Connatix adapter from PBS-Go #3781
Conversation
private final String endpointUrl; | ||
private final JacksonMapper mapper; | ||
|
||
private static final String BIDDER_CURRENCY = "USD"; |
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 do not mix up constants and dependencies, place constants first
CurrencyConversionService currencyConversionService, | ||
JacksonMapper mapper) { | ||
this.endpointUrl = HttpUtil.validateUrl(Objects.requireNonNull(endpointUrl)); | ||
this.currencyConversionService = currencyConversionService; |
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.
requireNonNull
@Override | ||
public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest request) { | ||
// Device IP required - bounce if not available | ||
if (request.getDevice() == null |
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'd extract device into a separate variable since it's used later as well
return headers; | ||
} | ||
|
||
// check display manager 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.
remove comments
} | ||
|
||
// check display manager version | ||
private String buildDisplayManagerVersion(BidRequest request) { |
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.
static
.filter(Objects::nonNull) | ||
.flatMap(Collection::stream) | ||
.filter(Objects::nonNull) | ||
.map(bid -> BidderBid.of(bid, getBidType(bid), bidResponse.getCur())) |
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.
BidderBid.of(bid, getBidType(bid), BID_CURRENCY))
cookie-family-name: connatix | ||
iframe: | ||
url: "https://capi.connatix.com/us/pixel?pId=53&gdpr={{gdpr}}&gdpr_consent={{gdpr_consent}}&us_privacy={{us_privacy}}&callback={{redirect_url}}" | ||
uid-macro: '$UID' |
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.
'[UID]'
redirect: | ||
url: "https://capi.connatix.com/us/pixel?pId=52&gdpr={{gdpr}}&gdpr_consent={{gdpr_consent}}&us_privacy={{us_privacy}}&callback={{redirect_url}}" | ||
uid-macro: '$UID' | ||
supportCors: false |
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.
support-cors:
ortb: | ||
multiformat-supported: false |
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
public class ConnatixTest extends IntegrationTest { | ||
|
||
@Test | ||
public void openrtb2AuctionShouldRespondWithBannerBidFromConnatix() throws IOException, JSONException { |
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 leave only one test, we test here only the correctness of the configuration
updates in response to comments are in progress |
.orElse(StringUtils.EMPTY); | ||
} | ||
|
||
private MultiMap resolveHeaders(Device device) { |
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.
static
} | ||
|
||
private static String buildDisplayManagerVersion(BidRequest request) { | ||
final String formatting = "%s-%s"; |
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 extract as a constant
iframe: | ||
url: "https://capi.connatix.com/us/pixel?pId=53&gdpr={{gdpr}}&gdpr_consent={{gdpr_consent}}&us_privacy={{us_privacy}}&callback={{redirect_url}}" | ||
uid-macro: '[UID]' | ||
supportCors: false |
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.
support-cors
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.
^^
final BidRequest bidRequest = givenBidRequest( | ||
request -> request.device(Device.builder().ip("deviceIp").build()), | ||
impBuilder -> impBuilder.ext(mapper.valueToTree(ExtPrebid.of(null, mapper.createArrayNode()))) | ||
); |
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.
according to the code style closing parenthesis should be on the previous line
please double-check everywhere in the PR
iframe: | ||
url: "https://capi.connatix.com/us/pixel?pId=53&gdpr={{gdpr}}&gdpr_consent={{gdpr_consent}}&us_privacy={{us_privacy}}&callback={{redirect_url}}" | ||
uid-macro: '[UID]' | ||
supportCors: false |
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.
^^
🔧 Type of changes
✨ What's the context?
What's the context for the changes?
New Connatix Adapter (port from Go)
#3601
🧠 Rationale behind the change
Why did you choose to make these changes? Were there any trade-offs you had to consider?
N/A
🔎 New Bid Adapter Checklist
🧪 Test plan
How do you know the changes are safe to ship to production?
Unit tests with decent coverage
🏎 Quality check