-
Notifications
You must be signed in to change notification settings - Fork 749
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
Vrtcal: Added Video(VAST) Support #1965
Changes from 1 commit
48da8c9
fc72d74
dda79e4
40a71fe
25a44da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,9 +70,14 @@ func (a *VrtcalAdapter) MakeBids(internalRequest *openrtb2.BidRequest, externalR | |
|
||
for _, sb := range bidResp.SeatBid { | ||
for i := range sb.Bid { | ||
bidType, err := getReturnTypeForImp(sb.Bid[i].ImpID, internalRequest.Imp) | ||
if err != nil { | ||
return nil, []error{err} | ||
} | ||
|
||
bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{ | ||
Bid: &sb.Bid[i], | ||
BidType: "banner", | ||
BidType: bidType, | ||
}) | ||
} | ||
} | ||
|
@@ -87,3 +92,26 @@ func Builder(bidderName openrtb_ext.BidderName, config config.Adapter) (adapters | |
} | ||
return bidder, nil | ||
} | ||
|
||
func getReturnTypeForImp(impID string, imps []openrtb2.Imp) (openrtb_ext.BidType, error) { | ||
for _, imp := range imps { | ||
if imp.ID == impID { | ||
if imp.Banner != nil { | ||
vrtcal-dev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return openrtb_ext.BidTypeBanner, nil | ||
} | ||
|
||
if imp.Video != nil { | ||
return openrtb_ext.BidTypeVideo, nil | ||
} | ||
|
||
return "", &errortypes.BadServerResponse{ | ||
Message: fmt.Sprintf("Unsupported return type for ID: \"%s\"", impID), | ||
} | ||
} | ||
} | ||
|
||
//Failsafe default in case impression ID is not found | ||
return "", &errortypes.BadServerResponse{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add test coverage for this case here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've added supplemental test coverage for the failsafe unmatched impression ID. Thanks. |
||
Message: fmt.Sprintf("Failed to find impression for ID: \"%s\"", impID), | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
{ | ||
"mockBidRequest": { | ||
"id": "test-request-id", | ||
"imp": [ | ||
{ | ||
"id": "test-imp-id", | ||
"video": { | ||
"mimes": ["video/mp4"], | ||
"protocols": [2, 5], | ||
"w": 300, | ||
"h": 250 | ||
}, | ||
"ext": { | ||
"bidder": { | ||
"Just_an_unused_vrtcal_param": "unused_data_for_this" | ||
} | ||
} | ||
} | ||
], | ||
"app": { | ||
"id": "fake-app-id" | ||
} | ||
}, | ||
"httpCalls": [ | ||
{ | ||
"expectedRequest": { | ||
"uri": "http://rtb.vrtcal.com/bidder_prebid.vap?ssp=1804", | ||
"body": { | ||
"id": "test-request-id", | ||
"imp": [ | ||
{ | ||
"id": "test-imp-id", | ||
"video": { | ||
"mimes": ["video/mp4"], | ||
"protocols": [2, 5], | ||
"w": 300, | ||
"h": 250 | ||
}, | ||
|
||
"ext": { | ||
"bidder": { | ||
"Just_an_unused_vrtcal_param": "unused_data_for_this" | ||
} | ||
} | ||
} | ||
], | ||
"app": { | ||
"id": "fake-app-id" | ||
} | ||
} | ||
}, | ||
"mockResponse": { | ||
"status": 200, | ||
"body": { | ||
"id": "test-request-id", | ||
"seatbid": [ | ||
{ | ||
"seat": "vrtcal", | ||
"bid": [ | ||
{ | ||
"id": "8ee514f1-b2b8-4abb-89fd-084437d1e800", | ||
"impid": "test-imp-id", | ||
"price": 0.500000, | ||
"adm": "some-test-video-ad", | ||
"crid": "crid_10", | ||
"h": 250, | ||
"w": 300 | ||
} | ||
] | ||
} | ||
], | ||
"cur": "USD" | ||
} | ||
} | ||
} | ||
], | ||
"expectedBidResponses": [ | ||
{ | ||
"currency": "USD", | ||
"bids": [ | ||
{ | ||
"bid": { | ||
"id": "8ee514f1-b2b8-4abb-89fd-084437d1e800", | ||
"impid": "test-imp-id", | ||
"price": 0.5, | ||
"adm": "some-test-video-ad", | ||
"crid": "crid_10", | ||
"w": 300, | ||
"h": 250 | ||
}, | ||
"type": "video" | ||
} | ||
] | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,3 +4,4 @@ capabilities: | |
app: | ||
mediaTypes: | ||
- banner | ||
- video |
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.
Sorry, I missed this in my earlier review. In case there's an error getting the type of one of the bids, are you sure you want to return an error and discard the rest of the bids? It might be better to rather append the error in an error slice and move on to the next bid by adding a
continue
here.Thinking more about this, I realize that you mentioned your backend service only bids on a single impression for a single request so the response likely is gonna have a single bid as well but I'd still recommend adding a
continue
here and appending the error to an error slice as it is easy to do and this way you won't have to update this logic in the future even when your backend service does start supporting multiple impressions and returning multiple bidsThere 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.
Thanks Mansinahar. I've added a patch to allow the continuance of processing of further bids after ones with an unmatched Impression ID...