-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add video support in Grid Bid Adapter #3545
Changes from 5 commits
57210b5
2a0bad2
12b9f52
f3a8ffe
8e2083c
bc9d9d4
14e9ed7
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 |
---|---|---|
|
@@ -7,6 +7,7 @@ Maintainer: grid-tech@themediagrid.com | |
# Description | ||
|
||
Module that connects to Grid demand source to fetch bids. | ||
Grid bid adapter supports Banner and Video (instream and outstream). | ||
|
||
# Test Parameters | ||
``` | ||
|
@@ -35,6 +36,19 @@ Module that connects to Grid demand source to fetch bids. | |
} | ||
} | ||
] | ||
} | ||
}, | ||
{ | ||
code: 'test-div', | ||
sizes: [[728, 90]], | ||
mediaTypes: { video: {} }, | ||
bids: [ | ||
{ | ||
bidder: "grid", | ||
params: { | ||
uid: 11 | ||
} | ||
} | ||
] | ||
} | ||
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. Thanks for good test parameters which return valid bid responses! This is great. |
||
]; | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,6 +163,7 @@ describe('TheMediaGrid Adapter', function () { | |
'ad': '<div>test content 1</div>', | ||
'bidderCode': 'grid', | ||
'currency': 'USD', | ||
'mediaType': 'banner', | ||
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. Thank you for > 80% Test Coverage! 👍 |
||
'netRevenue': false, | ||
'ttl': 360, | ||
} | ||
|
@@ -220,6 +221,7 @@ describe('TheMediaGrid Adapter', function () { | |
'ad': '<div>test content 1</div>', | ||
'bidderCode': 'grid', | ||
'currency': 'USD', | ||
'mediaType': 'banner', | ||
'netRevenue': false, | ||
'ttl': 360, | ||
}, | ||
|
@@ -233,6 +235,7 @@ describe('TheMediaGrid Adapter', function () { | |
'ad': '<div>test content 1</div>', | ||
'bidderCode': 'grid', | ||
'currency': 'USD', | ||
'mediaType': 'banner', | ||
'netRevenue': false, | ||
'ttl': 360, | ||
}, | ||
|
@@ -246,6 +249,7 @@ describe('TheMediaGrid Adapter', function () { | |
'ad': '<div>test content 2</div>', | ||
'bidderCode': 'grid', | ||
'currency': 'USD', | ||
'mediaType': 'banner', | ||
'netRevenue': false, | ||
'ttl': 360, | ||
} | ||
|
@@ -255,6 +259,51 @@ describe('TheMediaGrid Adapter', function () { | |
expect(result).to.deep.equal(expectedResponse); | ||
}); | ||
|
||
it('should get correct video bid response', function () { | ||
const bidRequests = [ | ||
{ | ||
'bidder': 'grid', | ||
'params': { | ||
'uid': '1' | ||
}, | ||
'adUnitCode': 'adunit-code-1', | ||
'sizes': [[300, 250], [300, 600]], | ||
'bidId': '659423fff799cb', | ||
'bidderRequestId': '5f2009617a7c0a', | ||
'auctionId': '1cbd2feafe5e8b', | ||
'mediaTypes': { | ||
'video': { | ||
'context': 'instream' | ||
} | ||
} | ||
} | ||
]; | ||
const response = {'bid': [{'price': 1.15, 'adm': '<VAST version=\"3.0\">\n<Ad id=\"21341234\"><\/Ad>\n<\/VAST>', 'auid': 1}], 'seat': '1'}; | ||
const request = spec.buildRequests(bidRequests); | ||
const expectedResponse = [ | ||
{ | ||
'requestId': '659423fff799cb', | ||
'cpm': 1.15, | ||
'creativeId': 1, | ||
'dealId': undefined, | ||
'width': undefined, | ||
'height': undefined, | ||
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. Same as above question. Is there a reason why the adapter does not just set these values? |
||
'bidderCode': 'grid', | ||
'currency': 'USD', | ||
'mediaType': 'video', | ||
'netRevenue': false, | ||
'ttl': 360, | ||
'vastXml': '<VAST version=\"3.0\">\n<Ad id=\"21341234\"><\/Ad>\n<\/VAST>', | ||
'adResponse': { | ||
'content': '<VAST version=\"3.0\">\n<Ad id=\"21341234\"><\/Ad>\n<\/VAST>' | ||
} | ||
} | ||
]; | ||
|
||
const result = spec.interpretResponse({'body': {'seatbid': [response]}}, request); | ||
expect(result).to.deep.equal(expectedResponse); | ||
}); | ||
|
||
it('handles wrong and nobid responses', function () { | ||
const bidRequests = [ | ||
{ | ||
|
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.
Hi this all looks great,
I have a question as to why when it is video there is no
bidResponse.width
orbidResponse.height
added?I see it is not sent or returned by your adserver.
You can see in the below screenshot that the KVP that gets set for the size turns out to be
undefinedxundefined
I have a feeling this may cause problems for certain publishers ad servers maybe?
Do you think it is a good idea to set the width and height based on the bidRequest object?
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.
According to Prebid Docs:
http://prebid.org/dev-docs/bidder-adaptor.html#interpreting-the-response
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.
width and height are required.
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.
Hi, thank you for the catch, we have updated code, please review again.