New Adapter: Insticator #3806
New Adapter: Insticator #3806
Conversation
* init insticator adaptor * update modules * add mediaType for bids * update insticator adaptor with tests * update insticator for test cases * fix test cases * update InstAdapter * update insticator adaptor * fix test cases for adUnitId * update insticator adapter type * Updates - add currency converter support - Add video validation support * remove default plcmt and placement * add more test cases * update for publisher ID * fix tests
* add multi-imps testJson * fix pubId parsing for request
Code coverage summaryNote:
insticatorRefer here for heat map coverage report |
Code coverage summaryNote:
insticatorRefer here for heat map coverage report |
|
Hi @SyntaxNode , did you get time to check the updates? #3806 (comment) Thanks! |
Cookie syncing LGTM. Verified the PBS setuid endpoint is being called, the cookie is being updated with the insticator user ID and no more than 10 DSPs are being called. |
| func RoundTo4Decimals(amount float64) float64 { | ||
| return math.Round(amount*10000) / 10000 | ||
| } |
There was a problem hiding this comment.
Please create mathutil_test.go with a unit test for this function.
There was a problem hiding this comment.
added unit test for it.
|
|
||
| func TestJsonSamples(t *testing.T) { | ||
| bidder, buildErr := Builder(openrtb_ext.BidderInsticator, config.Adapter{ | ||
| Endpoint: "https://ex.ingage.tech/v1/prebidserver"}, |
There was a problem hiding this comment.
We recommend using a fake URL for tests for maintenance reasons.
| func TestJsonSamples(t *testing.T) { | ||
| bidder, buildErr := Builder(openrtb_ext.BidderInsticator, config.Adapter{ | ||
| Endpoint: "https://ex.ingage.tech/v1/prebidserver"}, | ||
| config.Server{ExternalUrl: "https://ex.ingage.tech/v1/prebidserver", GvlID: 1, DataCenter: "2"}) |
There was a problem hiding this comment.
We recommend using a fake URL for tests for maintenance reasons.
adapters/insticator/insticator.go
Outdated
| if len(request.Device.UA) > 0 { | ||
| headers.Add("User-Agent", request.Device.UA) | ||
| } | ||
|
|
||
| if len(request.Device.IPv6) > 0 { | ||
| headers.Add("X-Forwarded-For", request.Device.IPv6) | ||
| } | ||
|
|
||
| if len(request.Device.IP) > 0 { | ||
| headers.Add("X-Forwarded-For", request.Device.IP) | ||
| headers.Add("IP", request.Device.IP) | ||
| } |
There was a problem hiding this comment.
You're missing some test coverage here. I suggest the following:
- Either update an existing exemplary test or add a new supplemental JSON test where
.device.ua,.device.ipv6and.device.ipare set - Add a supplemental JSON test where
.deviceis not nil but.device.ua,device.ipv6and.device.ipare of zero length.
There was a problem hiding this comment.
I have added a new adapters/insticator/insticatortest/supplemental/device-validation.json and added device object in one or two old files too.
adapters/insticator/insticator.go
Outdated
|
|
||
| // Unmarshal the imp extension to get the publisher ID | ||
| if err := jsonutil.Unmarshal(imp.Ext, &ext); err != nil { | ||
| return &errortypes.BadInput{Message: "Error unmarshalling imp extension"} |
There was a problem hiding this comment.
Please add a supplemental JSON test case where attempting to unmarshal imp.ext fails. You should be able to force this by simply setting imp.ext.insticator in your JSON test to an invalid type which would be something other than an object (e.g. a string).
There was a problem hiding this comment.
There was a problem hiding this comment.
Also this should never be non-empty. The ext definition you are using here is looking for the field name 'insticator' rather than bidder which is always translated to bidder by PBS core.
There was a problem hiding this comment.
This unmarshal step has been removed.
| floor, err := reqInfo.ConvertCurrency(bidFloor, bidFloorCur, "USD") | ||
| return mathutil.RoundTo4Decimals(floor), err |
There was a problem hiding this comment.
You're missing test coverage here. Please add the following supplemental JSON test cases:
- currency conversion is successful resulting in a bid floor that is greater than 0
- currency conversion fails
There was a problem hiding this comment.
|
|
||
| if len(request.Ext) > 0 { | ||
| if err := jsonutil.Unmarshal(request.Ext, &reqExt); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Please add a supplemental JSON test case where attempting to unmarshal request.ext fails. You should be able to force this by simply setting request.ext.insticator in your JSON test to an invalid type which would be something other than an object (e.g. a string).
There was a problem hiding this comment.
added adapters/insticator/insticatortest/supplemental/request-ext-unmarhal-fail.json
|
|
||
| func (a *adapter) MakeBids(request *openrtb2.BidRequest, requestData *adapters.RequestData, responseData *adapters.ResponseData) (*adapters.BidderResponse, []error) { | ||
| if adapters.IsResponseStatusCodeNoContent(responseData) { | ||
| return nil, nil |
There was a problem hiding this comment.
Please add a supplemental JSON test where your mock response returns a 204 status code.
There was a problem hiding this comment.
|
|
||
| var response openrtb2.BidResponse | ||
| if err := jsonutil.Unmarshal(responseData.Body, &response); err != nil { | ||
| return nil, []error{err} |
There was a problem hiding this comment.
Please add a supplemental JSON test where your mock response returns a bid response that cannot be successuflly unmarshalled. This can be accomplished by making one of the fields the wrong type.
There was a problem hiding this comment.
There was a problem hiding this comment.
What is this test testing? The filename says multi-imp but I only see one imp and I don't see anything failing validation.
There was a problem hiding this comment.
I have updated this test case, added both video and banner imps.
Code coverage summaryNote:
insticatorRefer here for heat map coverage report |
|
@bsardo Thanks a lot for your time. I have updated the PR with all the test cases. Please take a look :) |
adapters/insticator/insticator.go
Outdated
|
|
||
| // Populate site.publisher.id from imp extension | ||
| if requestCopy.Site != nil || requestCopy.App != nil { | ||
| if err := populatePublisherId(&impCopy, &requestCopy); err != nil { |
There was a problem hiding this comment.
This will unmarshal the imp.ext of the current imp and update the site/app object's publisher ID to match. It will repeat this process for every imp in the request. It would be much more efficient to remove this step from the loop, and just execute this for the last imp in the list, as it will have the same result and only consume 1/len(Imp) as many cycles.
There was a problem hiding this comment.
Yes, I know. I was doing the same previously but someone previously asked to update it.
adapters/insticator/insticator.go
Outdated
| return bidResponse, nil | ||
| } | ||
|
|
||
| func makeImps(imp openrtb2.Imp) (openrtb2.Imp, string, error) { |
There was a problem hiding this comment.
It may make sense to return the publisher ID here, since you unmarshal the imp.ext here. This would save you from unmarshalling it again in populatePublisherID(). Or possibly move the unmarshal into the Imp loop and pass the unmarshalled structure into this function.
There was a problem hiding this comment.
I see @hhhjort
what do you think of this ?
for i := 0; i < len(request.Imp); i++ {
// I will return publisherId from the makeImps, and use it in next step
impCopy, impKey, publisherId, err := makeImps(request.Imp[i])
if err != nil {
errs = append(errs, err)
continue
}
// Populate publisher.id from imp extension only once for the first imp
if (requestCopy.Site != nil || requestCopy.App != nil) && i == 0 {
// use publisherId here
if err := populatePublisherId(publisherId, &requestCopy); err != nil {
errs = append(errs, err)
continue
}
}
... rest code
and I will remove the Unmarshal from populatePublisherId
func populatePublisherId(imp *openrtb2.Imp, request *openrtb2.BidRequest) error {
// this will be removed
// var ext ext
// Unmarshal the imp extension to get the publisher ID
// if err := jsonutil.Unmarshal(imp.Ext, &ext); err != nil {
// return &errortypes.BadInput{Message: "Error unmarshalling imp extension"}
// }
// Populate site.publisher.id if request.Site is not nil
if request.Site != nil {
.... Rest code
Code coverage summaryNote:
insticatorRefer here for heat map coverage report |
|
@hhhjort updated the PR with your suggestions. Please take a look. Thanks :) |
| } | ||
| }, | ||
|
|
||
| "httpCalls": [ |
There was a problem hiding this comment.
You can verify the headers, specifically the device related ones, are set correctly given that you have a device object here by including the following:
"httpCalls": [
{
"expectedRequest": {
"headers": {
"Content-Type": [
"application/json;charset=utf-8"
],
"Accept": [
"application/json"
],
etc...
``
adapters/insticator/insticator.go
Outdated
| (video.H == nil || *video.H == 0) || | ||
| video.MIMEs == nil { | ||
|
|
||
| if (video.W == nil || *video.W == 0) || (video.H == nil || *video.H == 0) || video.MIMEs == nil { |
There was a problem hiding this comment.
Nitpick: you should be able to delete the parentheses
There was a problem hiding this comment.
Nitpick: file name typo -unmarhal-fail.json --> -unmarshal-fail.json
| "device": { | ||
| "ipv6": "2001:0db8:85a3:0000:0000:8a2e:0370:7334", | ||
| "ip": "1.22.22.1", | ||
| "ua": "Mozilla/5.0 (Linux; Android 8.0.0; SM-G960F Build/R16NW) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.84 Mobile Safari/537.36" | ||
| } | ||
| }, | ||
|
|
||
| "httpCalls": [ |
There was a problem hiding this comment.
Please verify the headers are set correctly in this test since some of them are derived from the device object. You can do so like this:
"httpCalls": [
{
"expectedRequest": {
"headers": {
"Content-Type": [
"application/json;charset=utf-8"
],
"Accept": [
"application/json"
],
etc...
``
adapters/insticator/insticator.go
Outdated
| if requestCopy.Site != nil || requestCopy.App != nil { | ||
| if err := populatePublisherId(&impCopy, &requestCopy); err != nil { | ||
| // Populate publisher.id from imp extension | ||
| if !isPublisherIdPopulated && (requestCopy.Site != nil || requestCopy.App != nil) { |
There was a problem hiding this comment.
You can simplify this to just if !isPublisherIdPopulated { since you've only declared support for site and app (not dooh) in your YAML file which means your adapter will only be called if site or app is present.
adapters/insticator/insticator.go
Outdated
| } | ||
|
|
||
| // populatePublisherId function populates site.publisher.id or app.publisher.id | ||
| func populatePublisherId(publisherId string, request *openrtb2.BidRequest) error { |
There was a problem hiding this comment.
This function does not return an error anymore so please remove that return parameter.
|
@bsardo Thanks for checking, I have updated the PR. |
Code coverage summaryNote:
insticatorRefer here for heat map coverage report |
DOCS PR: prebid/prebid.github.io#5571