GumGum: Collect the ad unit name for reporting #3912
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CTMBNara
changed the title
CTMBNara
requested changes
Apr 23, 2025
src/main/java/org/prebid/server/bidder/gumgum/GumgumBidder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/gumgum/GumgumBidder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/gumgum/GumgumBidder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/gumgum/GumgumBidder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/gumgum/GumgumBidder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/bidder/gumgum/GumgumBidder.java
Outdated
Show resolved
Hide resolved
|
Thank you for the review, I am looking into the comments. |
|
@ShayanK16GumGum any updates on this one? |
|
The code changes are done, will push. @osulzhenko |
|
|
@osulzhenko looking into it and fixing. |
|
@ShayanK16GumGum any updates on this one? |
|
@osulzhenko I have pushed the changes. Please have a look. |
CTMBNara
requested changes
Jun 3, 2025
Comment on lines
+118
to
+126
| private String extractAdUnitCode(Imp imp) { | ||
| final ExtPrebid<?, ExtImpGumgum> extPrebid = mapper.mapper() | ||
| .convertValue(imp.getExt(), GUMGUM_EXT_TYPE_REFERENCE); | ||
|
|
||
| return Optional.ofNullable(extPrebid.getPrebid()) | ||
| .map(prebid -> mapper.mapper().convertValue(prebid, ExtImpPrebid.class)) | ||
| .map(ExtImpPrebid::getAdUnitCode) | ||
| .orElse(null); | ||
| } |
There was a problem hiding this comment.
Instead of this method, use the following approach:
private static final TypeReference<ExtPrebid<ExtImpPrebid, ExtImpGumgum>> GUMGUM_EXT_TYPE_REFERENCE =
new TypeReference<>() {
};
private ExtPrebid<ExtImpPrebid, ExtImpGumgum> parseImpExt(Imp imp) {
try {
return mapper.mapper().convertValue(imp.getExt(), GUMGUM_EXT_TYPE_REFERENCE);
} catch (IllegalArgumentException e) {
throw new PreBidException(e.getMessage());
}
}
final ExtPrebid<ExtImpPrebid, ExtImpGumgum> extImp = parseImpExt(imp);
final ExtImpGumgum extImpGumgum = extImp.getBidder();
final String adUnitCode = Optional.ofNullable(extImp.getPrebid())
.map(ExtImpPrebid::getAdUnitCode)
.orElse(null);
modifiedImps.add(modifyImp(imp, extImpGumgum, adUnitCode));
final String extZone = extImpGumgum.getZone();
if (StringUtils.isNotEmpty(extZone)) {
zone = extZone;
}
final BigInteger extPubId = extImpGumgum.getPubId();
if (extPubId != null && !extPubId.equals(BigInteger.ZERO)) {
pubId = extPubId;
}
|
|
||
| @Test | ||
| public void testMakeHttpRequestsShouldNotSetTagIdFromZoneWhenAdUnitIdIsMissing() throws IOException { | ||
| //given:Imp without adUnitId but with zone |
There was a problem hiding this comment.
No explanation needed. The method name already does that. Just leave // given
Comment on lines
+56
to
+63
| //given | ||
| final BidRequest bidRequest = givenBidRequest(impBuilder -> | ||
| impBuilder.ext(mapper.valueToTree(ExtPrebid.of(null, mapper.createArrayNode())))); | ||
|
|
||
| //when | ||
| final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest); | ||
|
|
||
| //then |
There was a problem hiding this comment.
// given // when // then and fix it in other places
| //when | ||
| final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest); | ||
|
|
||
| //deserialize byte[] body into BidRequest |
Comment on lines
+85
to
+92
| final byte[] requestBody = result.getValue().get(0).getBody(); | ||
| final BidRequest modifiedRequest = mapper.readValue(requestBody, BidRequest.class); | ||
|
|
||
| //then | ||
| assertThat(modifiedRequest.getImp()).hasSize(1); | ||
| assertThat(modifiedRequest.getImp()) | ||
| .extracting(Imp::getTagid) | ||
| .containsExactly("adUnit123"); |
There was a problem hiding this comment.
Refactor other tests in the same style:
assertThat(result.getValue())
.extracting(HttpRequest::getPayload)
.flatExtracting(BidRequest::getImp)
.extracting(Imp::getTagid)
.containsExactly("adUnit123");
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🔧 Type of changes
✨ What's the context?
What's the context for the changes?
🧠 Rationale behind the change
Why did you choose to make these changes? Were there any trade-offs you had to consider?
🔎 New Bid Adapter Checklist
🧪 Test plan
How do you know the changes are safe to ship to production?
🏎 Quality check