-
Notifications
You must be signed in to change notification settings - Fork 744
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
Sonobi: add native support to sonobi adapter #3802
Conversation
Code coverage summaryNote:
sonobiRefer here for heat map coverage report
|
userSync: | ||
iframe: | ||
url: "https://sync.go.sonobi.com/uc.html?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&loc={{.RedirectURL}}" | ||
userMacro: "[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.
usersync works.
if imp.Banner == nil && imp.Video == nil && imp.Native != nil { | ||
mediaType = openrtb_ext.BidTypeNative |
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.
Code path is not covered. Should add json framework tests for Native
media type.
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.
@bansawbanchee Suggesting you to please use Mtype field set by your adapter response. Here is an example -
func getBidType(bid openrtb2.Bid) (openrtb_ext.BidType, error) {
switch bid.MType {
case openrtb2.MarkupBanner:
return openrtb_ext.BidTypeBanner, nil
case openrtb2.MarkupVideo:
return openrtb_ext.BidTypeVideo, nil
case openrtb2.MarkupNative:
return openrtb_ext.BidTypeNative, nil
}
return "", &errortypes.BadInput{
Message: fmt.Sprintf("Could not define media type for impression: %s", bid.ImpID),
}
}
04b0331
to
5933f60
Compare
Code coverage summaryNote:
sonobiRefer here for heat map coverage report
|
Went ahead and closed this on and combined it into our newest pull request: #3889 |
No description provided.