New Adapter: IQZone #1964
New Adapter: IQZone #1964
Conversation
There was a problem hiding this comment.
Left a few comments. Also, there is already a pull request to remove the old race condition test framework and an additional PR is under development to update the documents. We apologize for not having the documentation up to date yet. In the meantime, can we please remove the following files?
iqzone/iqzonetest/params/race/banner.json
iqzone/iqzonetest/params/race/native.json
iqzone/iqzonetest/params/race/video.json
These were required under the old data race tests and are no longer necessary. In regards of test case coverage your adapter looks file. It only seems to be missing the following scenarios inside MakeBids:
Can we please add a couple more JSON tests under iqzone/iqzonetest/supplemental/ to cover them?
openrtb_ext/imp_iqzone.go
Outdated
| @@ -0,0 +1,5 @@ | |||
| package openrtb_ext | |||
|
|
|||
| type ImpExtFoo struct { | |||
There was a problem hiding this comment.
Even if it's not being used, please rename to something less generic, ImpExtIqzone maybe
There was a problem hiding this comment.
This just looks like a copy/paste error from the docs. Easy fix. Thank you @IQZoneAdx for faithfully following our new bid adapter guide. :)
| } | ||
| }, | ||
|
|
||
| "required": ["placementId"] |
There was a problem hiding this comment.
Just to double-check: Are we sure every request will come with a "placementId" parameter? If it's set as "required", an incoming request with a missing "placementId" will get rejected and Prebid Server won't send a bid request to any bidder. So adding a required field can break the entire setup of a publisher.
| } | ||
|
|
||
| var invalidParams = []string{ | ||
| `{"placementId": 42}`, |
There was a problem hiding this comment.
If you decide to keep the "placementId" listed in the "required" array inside static/bidder-params/iqzone.json, can we please add a couple more test cases in order to assert the request will be invalid when "placementId" isn't found in the Ext?
40 var invalidParams = []string{
41 `{"placementId": 42}`,
+ `{}`,
+ `{"someOtherParam": "value"}`,
42 }
adapters/iqzone/params_test.go
adapters/iqzone/iqzone.go
Outdated
| bidResponse.Currency = response.Cur | ||
| for _, seatBid := range response.SeatBid { | ||
| for _, bid := range seatBid.Bid { | ||
| bid := bid // pin https://github.com/kyoh86/scopelint#whats-this |
There was a problem hiding this comment.
Good catch, we can remove the comment if you want
There was a problem hiding this comment.
This is a copy/paste from the docs. A clearer way of writing this loop could be:
for i := range seatBid.Bid {
b := &adapters.TypedBid{
Bid: &seatBid.Bid[i],
BidType: getMediaTypeForImp(bid.ImpID, request.Imp),
}
bidResponse.Bids = append(bidResponse.Bids, b)
}
This does look better IMHO and doesn't require extra explanation via the pin link. I'll update the docs soon with this approach.
| } | ||
| if imp.Banner == nil && imp.Video == nil && imp.Native != nil { | ||
| mediaType = openrtb_ext.BidTypeNative | ||
| } |
There was a problem hiding this comment.
Is the imp.Banner == nil && imp.Video == nil && imp.Native == nil scenario possible? If so, should getMediaTypeForImp return an error instead?
|
@guscarreon @SyntaxNode thanks for the review. We fixed all the problems |
There was a problem hiding this comment.
Looks good to me. @IQZoneAdx thank you for addressing the feedback and following the documentation steps so thoroughly.
… prebid-master * 'master' of https://github.com/prebid/prebid-server: Update Viper (prebid#1969) /cookie_sync Endpoint Rewrite (prebid#1879) New Adapter: IQZone (prebid#1964) Remove old race conidtion tests (prebid#1958) Remove time.Now() as the first parameter of NewRates() (prebid#1953) # Conflicts: # config/config.go # usersync/usersyncers/syncer_test.go
Doc - prebid/prebid.github.io#3210