New krushmedia bid adapter #1504
Conversation
adapters/krushmedia/krushmedia.go
Outdated
| return &KrushmediaAdapter{endpoint: *template} | ||
| } | ||
|
|
||
| func (a *KrushmediaAdapter) CheckHasImps(request *openrtb.BidRequest) error { |
There was a problem hiding this comment.
Is there a reason why this method needs the KrushmediaAdapter receiver? It seems to be more of a helper method for MakeRequests. Also, any reason to have it as an exported/public method vs a private method?
adapters/krushmedia/krushmedia.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func GetHeaders(request *openrtb.BidRequest) *http.Header { |
There was a problem hiding this comment.
This seems like a helper method too. Any reason for it being an exported/public method vs keeping it private?
adapters/krushmedia/krushmedia.go
Outdated
|
|
||
| request := *openRTBRequest | ||
|
|
||
| if noImps := a.CheckHasImps(&request); noImps != nil { |
There was a problem hiding this comment.
I'd recommend replacing noImps with err here as the CheckHasImps function returns an error. noImps can be misleading as to what the function returns
adapters/krushmedia/krushmedia.go
Outdated
| var krushmediaExt openrtb_ext.ExtKrushmedia | ||
| if err := json.Unmarshal(bidderExt.Bidder, &krushmediaExt); err != nil { | ||
| return nil, &errortypes.BadInput{ | ||
| Message: "ext.bidder not provided", |
There was a problem hiding this comment.
This error is a little misleading as in this case ext.bidder would be present but there is a problem unmarshalling its value. Can you please update this error message to be more relevant?
adapters/krushmedia/krushmedia.go
Outdated
| var bidderExt adapters.ExtImpBidder | ||
| if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil { | ||
| return nil, &errortypes.BadInput{ | ||
| Message: "ext.bidder not provided", |
There was a problem hiding this comment.
Error here would not always mean a missing ext.bidder. It could also be a marshaling error. Can you please update this error message to be more relevant?
|
|
||
| "github.com/prebid/prebid-server/adapters/adapterstest" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Would recommend adding a simple unit test for NewKrushmediaBidder as well
| ) | ||
|
|
||
| func TestJsonSamples(t *testing.T) { | ||
| adapterstest.RunJSONBidderTest(t, "krushmediatest", NewKrushmediaBidder("http://example.com/?c=rtb&m=req&key=2")) |
There was a problem hiding this comment.
You need to have a macro in here for AccountID otherwise it will never be able to take the account ID in the bidder ext and replace it in the URL for the tests. The RunJSONBidderTest basically runs all the JSON tests you have defined below.
| "bidder": { | ||
| "host": "example.com", | ||
| "sourceid": "partner", | ||
| "accountid": "hash" |
There was a problem hiding this comment.
Since the JSON annotation for account id is key here: https://github.com/prebid/prebid-server/pull/1504/files#diff-724b5d286e85be13b490de0a88acfd4dR5 you need to use key here rather than accountid. This wasn't caught as an error because, 1. You are missing the macro in the test endpoint as mentioned above and 2. You need to update the uri in the expectedRequest below to have the account ID value mentioned in this bidder ext here.
Also, any reason why you have sourceid and host defined here? From the ExtKrushmedia struct it looks like you only support account ID
There was a problem hiding this comment.
Looks like you have it defined right in most of the tests. Can you please update it for the two tests that don't have it rightly defined and update the test endpoint have an Account ID macro?
| "0" | ||
| ] | ||
| }, | ||
| "uri": "http://example.com/?c=rtb&m=req&key=2", |
There was a problem hiding this comment.
As mentioned above, please update this uri to have the account ID mentioned in the ext for this mock request
| "ext": { | ||
| "bidder": { | ||
| "host": "example.com", | ||
| "sourceid": "partner", |
There was a problem hiding this comment.
Please update the bidder ext and the uri below as mentioned above
There was a problem hiding this comment.
This is invalid param set, should be object with 'key' property, this one left from another adapter used as example
|
Thanks for the PR! Really like how clean the code is |
One aditional commit inside colossus bid adapter due to failing test