-
Notifications
You must be signed in to change notification settings - Fork 187
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
FIX #3608 Port from PBS-Go: Equativ: SmartAdserver alias with update to use mtype #3678
base: master
Are you sure you want to change the base?
FIX #3608 Port from PBS-Go: Equativ: SmartAdserver alias with update to use mtype #3678
Conversation
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.
Q: I have a question here. The original PR adds gpp support: https://github.com/prebid/prebid-server/pull/4045/files#diff-aaf548993679435ffc95fdfea42cf46f7d38b55e91c34ea6d874379c6aa5e878R25-R26
All i've been able to find in regard to GPP in PBS-Java was gpp={{gpp}}&gpp_sid={{gpp_sid}}
in the query string under adapters.<BIDDER>.usersync.redirect.url
. In PBS-Go no changes were made to the URL when gpp-support was declared in the config. Should I do anything here to port this change as well?
@@ -1,16 +1,21 @@ | |||
adapters: | |||
smartadserver: | |||
endpoint: https://ssb-global.smartadserver.com | |||
endpoint-compression: gzip | |||
aliases: | |||
equativ: ~ |
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.
Q: Also not sure whether I should write something here or just leave a tilde. As reference, the corresponding change in the original PR: https://github.com/prebid/prebid-server/pull/4045/files#diff-91570c852a44292f55ba1a3c722b2c26d76eacc0c87a41a527a84bdf7b021e36R1
private static BidType getBidTypeFromMarkupType(Integer markupType) { | ||
if (markupType == null) { | ||
return BidType.banner; | ||
} | ||
return BidType.banner; | ||
return switch (markupType) { | ||
case 1 -> BidType.banner; | ||
case 2 -> BidType.video; | ||
case 3 -> BidType.audio; | ||
case 4 -> BidType.xNative; | ||
default -> BidType.banner; |
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.
Just as FYI: Taking into account this Go code, a response body that is missing mType
in PBS-Go will be parsed to 0 by default and to null
in PBS-Java. So I had to account for nullability of the Integer
and added a test case for both unhappy cases: null
and out-of-bounds (default -> BidType.banner
).
Theoretically this combination of null check + switch-case can be reworked into an chain of if
's. Let me know if you prefer that:
private static BidType getBidTypeFromMarkupType(Integer markupType) {
if (markupType == null) {
return BidType.banner;
}
if (markupType == 1) {
return BidType.banner;
}
if (markupType == 2) {
return BidType.video;
}
if (markupType == 3) {
return BidType.audio;
}
if (markupType == 4) {
return BidType.xNative;
}
return BidType.banner;
}
Hello everyone. I am going on vacation until February so I won't be able to react to any responses here. If this PR can wait until then, I will happily finish it in February. If not, I kindly ask @bretg to assign the Issue/PR to someone else, if any changes are required. |
🔧 Type of changes
✨ What's the context?
Implementing #3608, i.e. porting this change from PBS-Go
🧠 Rationale behind the change
Keeping PBS-Java up-to-date
🧪 Test plan
Made changes to the fixtures in the integration test (added markupType to the Bid- and Auction Response)
Expanded/reworked unit tests for the "makeBids" method, which was affected by the changes.
🏎 Quality check
mvn validate
is happy, but I might have missed something, pls let me know if I have to make any changes.