-
Notifications
You must be signed in to change notification settings - Fork 750
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
Required some size to exist on banner requests #646
Conversation
endpoints/openrtb2/auction.go
Outdated
@@ -355,6 +355,11 @@ func validateBanner(banner *openrtb.Banner, impIndex int) error { | |||
return fmt.Errorf("request.imp[%d].banner uses unsupported property: \"hmax\". Use the \"format\" array instead.", impIndex) | |||
} | |||
|
|||
hasRootSize := banner.H != nil && banner.W != nil |
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.
shouldn't this be banner.H != nil || banner.W != nil
? We need both for a size.
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 is defining hasRootSize
, in the positive. Like you said, we need both to have one.
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.
Doh, my bad.
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.
all good. It did point me to the fact that you could still set these to 0
, which isn't helpful either.
I added some logic for that & a few more tests.
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.
LGTM
73af6b1
to
066c7a2
Compare
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.
LGTM
* Required some size to exist on banner requests. * Added more tests. Fixed the code a bit.
* Required some size to exist on banner requests. * Added more tests. Fixed the code a bit.
* Required some size to exist on banner requests. * Added more tests. Fixed the code a bit.
In #644, I found this test which wasn't testing what it thought it was.
It intended to make sure we reject sizeless banners... but it was failing for other reasons.
Unfortunately, this is now a breaking change :(. Still probably better late than never. We can measure its true impact whenever we release it.