-
Notifications
You must be signed in to change notification settings - Fork 758
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
verizonmedia: override tag ID and site ID + split up multi-imp requests #1050
verizonmedia: override tag ID and site ID + split up multi-imp requests #1050
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.
We need tests to cover the new updates.
} | ||
/* Always override the tag ID and site ID of the request */ | ||
request.Imp[0].TagID = extension.Pos | ||
request.Site.ID = extension.Dcn |
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.
you are missing the banner size properties latest changes that we added.
err := json.Unmarshal(imp.Ext, &bidderExt) | ||
if err != nil { | ||
err = &errortypes.BadInput{ | ||
Message: "ext.bidder not provided", |
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.
the Imp range index can be added in the error message
Message: "Missing param dcn", | ||
if verizonMediaExt.Dcn == "" { | ||
err = &errortypes.BadInput{ | ||
Message: "Missing param dcn", |
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.
the Imp range index can be added in the error message
Message: "Missing param pos", | ||
if verizonMediaExt.Pos == "" { | ||
err = &errortypes.BadInput{ | ||
Message: "Missing param pos", |
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.
the same as above
16c1ed4
to
90ea09b
Compare
@oath-jac addressed all the comments |
dc73af9
to
f0a1c4b
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.
Quick question
headers.Add("x-openrtb-version", "2.5") | ||
if err := changeRequestForBidService(&reqCopy, &verizonMediaExt); err != nil { | ||
errors = append(errors, err) | ||
return nil, errors |
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.
@Aadeshp , are we sure we want to return because this particular Imp does not pass the checks found in changeRequestForBidService()
? Don't we want to continue
instead? The changeRequestForBidService()
function pretty much makes sure this Imp
is a banner and check for its dimensions. Are sure we want to exit at the first non banner
or non-properly dimensioned banner
we find?
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.
ah yes good find!
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.
fixed!
f0a1c4b
to
df24e68
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.
Looks pretty good
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 other than the one minor comment
headers.Add("x-openrtb-version", "2.5") | ||
|
||
if request.Device != nil { | ||
headers.Set("User-Agent", request.Device.UA) |
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.
Probably wanna check the value of Device.UA
before setting the header to make sure it's not empty
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.
fixed!
df24e68
to
c3643de
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
No description provided.