-
Notifications
You must be signed in to change notification settings - Fork 220
SmileWanted endpoint now supports dynamic zoneId and integrates prebid server technology #4109
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
base: master
Are you sure you want to change the base?
Conversation
772729c
to
2ab6d26
Compare
@QuentinGallard Hi, is it still a draft? |
885f378
to
60ef209
Compare
Hi @osulzhenko, The same pull request exists on Prebid Server Go project : prebid/prebid-server#4468 |
adapters.smartyads.endpoint=http://localhost:8090/smartyads-exchange | ||
adapters.smilewanted.enabled=true | ||
adapters.smilewanted.endpoint=http://localhost:8090/smilewanted-exchange | ||
adapters.smilewanted.endpoint=http://localhost:8090/smilewanted-exchange/java/ |
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 change is unnecessary
public void makeHttpRequestsShouldReturnSingleRequest() { | ||
// given | ||
final BidRequest bidRequest = BidRequest.builder() | ||
.imp(singletonList(givenImp("zone123"))) | ||
.build(); | ||
|
||
// when | ||
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest); | ||
|
||
// then | ||
assertThat(result.getErrors()).isEmpty(); | ||
assertThat(result.getValue()).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.
I would also assert the payload and impIds sent to the bidder, also I'd created a bid request with at least two imps
@Test | ||
public void makeHttpRequestsShouldCorrectlyAddHeaders() { | ||
public void creationShouldFailOnNullEndpointUrl() { | ||
assertThatNullPointerException().isThrownBy(() -> new SmileWantedBidder(null, jacksonMapper)); | ||
} | ||
|
||
@Test | ||
public void creationShouldFailOnNullMapper() { | ||
assertThatNullPointerException().isThrownBy(() -> new SmileWantedBidder(ENDPOINT_URL, 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.
those are kind of redundant, can be removed
adapters: | ||
smilewanted: | ||
endpoint: https://prebid-server.smilewanted.com | ||
endpoint: https://prebid-server.smilewanted.com/java/ |
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 would add a {{ZoneId}} macro instead of just appending zone Id to the end of the endpoint, please consider that
if (CollectionUtils.isEmpty(request.getImp())) { | ||
return Result.withError(BidderError.badInput("No impressions in 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.
this check is redundant
return Result.withValue(HttpRequest.<BidRequest>builder() | ||
.method(HttpMethod.POST) | ||
.uri(endpointUrl) | ||
.uri(url) | ||
.headers(createHeaders()) | ||
.payload(outgoingRequest) | ||
.body(mapper.encodeToBytes(outgoingRequest)) | ||
.build()); | ||
} |
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 replace with the BidderUtil.defaultRequest()
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.
I would hide mapper.writeValueAsString
inside the givenHttpCall
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.
^^
@QuentinGallard any update? |
1f73f2f
to
e664dc5
Compare
} | ||
|
||
final BidRequest outgoingRequest = request.toBuilder().at(DEFAULT_AT).build(); | ||
final String url = endpointUrl.replace("{{ZoneId}}", extImpSmilewanted.getZoneId()); |
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 {{ZoneId}}
as a constant
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.
don't forget to add this macro to the URL in the smilewanted.json
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.
^^
e664dc5
to
8d691c4
Compare
Hi @AntoxaAntoxic , the test that failed does not appear to be related to my changes and seems unstable to me. What should I do now to get this Pull Request approved? Thanks a lot for your time :) |
Thank you for the great PR. Yes, the failed test is not related to your changes. The PR will be merged after the 2nd approval closer to the next release date. Thanks! |
final String body = bidResponse != null | ||
? mapper.writeValueAsString(bidResponse) | ||
: mapper.writeValueAsString(null); | ||
return givenHttpCall(bidRequest, body); |
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 givenHttpCall(bidRequest, mapper.writeValueAsString(bidResponse));
} | ||
|
||
final BidRequest outgoingRequest = request.toBuilder().at(DEFAULT_AT).build(); | ||
final String url = endpointUrl.replace(ZONE_ID_MACRO, extImpSmilewanted.getZoneId()); |
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.
add url encoding for zoneId
44cb1d7
to
4ba0336
Compare
…d server technology
4ba0336
to
a97150e
Compare
🔧 Type of changes
✨ What's the context?
SmileWanted adapter needs to support dynamic endpoint URLs based on the zoneId parameter to align with our server infrastructure requirements.
🧠 Rationale behind the change
The SmileWanted bidder endpoint needs to be constructed dynamically using the zoneId from the impression extension. This change:
🧪 Test plan
🏎 Quality check