New Adapter: Displayio #3691
New Adapter: Displayio #3691
Conversation
Display.io prebid adapter
Code coverage summaryNote:
displayioRefer here for heat map coverage report |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@xdevel
|
fix single placement/inventory per imp rename DisplayioAdapter to Adapter
Code coverage summaryNote:
displayioRefer here for heat map coverage report |
Code coverage summaryNote:
displayioRefer here for heat map coverage report |
Code coverage summaryNote:
displayioRefer here for heat map coverage report |
|
@xdevel should update json test
|
Code coverage summaryNote:
displayioRefer here for heat map coverage report |
Code coverage summaryNote:
displayioRefer here for heat map coverage report |
|
@onkarvhanumante both tests are already added, please take a look on multi-format.json / multi-imp.json |
|
@onkarvhanumante any updates on this one? |
adapters/displayio/displayio.go
Outdated
| dioExt = reqDioExt{PlacementId: impressionExt.PlacementId, InventoryId: impressionExt.InventoryId} | ||
|
|
||
| err = json.Unmarshal(request.Ext, &requestExt) | ||
| if err != nil { | ||
| requestExt = make(map[string]interface{}) | ||
| } | ||
|
|
||
| requestExt["displayio"] = dioExt |
There was a problem hiding this comment.
any particular reason to unMarshal request.Ext on line 89? unMarshalled value is always overwritten by dioExt assignment on line 94
There was a problem hiding this comment.
@onkarvhanumante it's not overwritten but modified. We alter request.ext with dio-related data.
adapters/displayio/displayio.go
Outdated
| if len(bidderExt.Bidder) == 0 { | ||
| errs = append(errs, errors.New("bidder required")) | ||
| continue | ||
| } |
There was a problem hiding this comment.
no need to add this check. Prebid core code will make sure Bidder value is present
|
Code coverage summaryNote:
displayioRefer here for heat map coverage report |
Code coverage summaryNote:
displayioRefer here for heat map coverage report |
adapters/displayio/displayio_test.go
Outdated
| config.Adapter{Endpoint: "https://prebid.display.io/?publisher={{.PublisherID}}"}, | ||
| config.Server{ExternalUrl: "https://prebid.display.io/?publisher=101"}, |
There was a problem hiding this comment.
Nitpick: I suggest using a fake URL for these to decouple your tests. It'll be less maintenance if you ever need to change the URL.
adapters/displayio/displayio.go
Outdated
| "net/http" | ||
| "text/template" |
There was a problem hiding this comment.
Nitpick: I suggest organizing your imports so all standard packages are listed first, in which case you would move these after "fmt" and before all of the github imports.
| InventoryId string `json:"inventoryId"` | ||
| } | ||
|
|
||
| func (adapter *adapter) MakeRequests(request *openrtb2.BidRequest, requestInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { |
There was a problem hiding this comment.
You might want to consider making a copy of request even though MakeRequests is provided with a shallow copy of it. It looks like you're modifying request, particularly request.ext, for each impression with each subsequent impression reading a previously modified version of request.ext instead of the version that was originally passed into MakeRequests.
On second look, this might not be a problem right now since you're always writing placementId and inventoryId to request.ext and those are guaranteed to exist for every impression which means the previous imp values are always overwritten. However, maybe this a change you want to consider to make your adapter future proof?
I should also note that any changes made to request in MakeRequests will be part of the request passed into MakeBids as the adapter shares the same shallow request copy provided to your adapter by PBS core. In that case, MakeBids will see request.ext.displayio with the placementId and inventoryId fields set to whatever their values are in the last impression.
There was a problem hiding this comment.
Ok, thanks for suggestion. I changed the code to use a copy of request for each imp
There was a problem hiding this comment.
I'll leave it up to you to decide of course but I just wanted to point out that you could really simplify your JSON tests, perhaps cutting their size in half, by removing a lot of the noise. I understand that your JSON tests as currently written probably more accurately represent how your server processes bid requests, however, it is arguable as to whether that is of value here. A lot of the fields here can be deleted as they are not handled directly by your adapter. Other fields like adm can simply be just HTML tags with no markup between them.
I think such changes will make your tests a lot easier to follow and draw attention to the specific fields that your adapter cares about.
There was a problem hiding this comment.
Tests simplified. I didn't dive a lot into it but tried to clear some unnecessary fields
| "placementId": "1021", | ||
| "publisherId": "101", | ||
| "inventoryId": "1011" |
There was a problem hiding this comment.
Given my comment on your adapter MakeRequests function here, I suggest having different values for your two impressions to make sure everything works properly.
| "ext": { | ||
| "displayio": { | ||
| "placementId": "1011", | ||
| "inventoryId": "1011" | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it typical for this information to exist in the request.ext of the incoming request? It seemed like your adapter was getting this information from imp.ext.bidder and populating request.ext.displayio in the bid requests generated in MakeRequests.
There was a problem hiding this comment.
Is it typical for this information to exist in the request.ext of the incoming request?
No, no matter what will be send under request.ext.displayio, it will be replaced by adapter code.
I dropped it from incoming request tests to keep it straight
There was a problem hiding this comment.
Why is this one titled missing-bidfloorcur.json? Would currency-conversion.json be a better name?
There was a problem hiding this comment.
You're right. File renamed
use of request copy for makeRequests
Code coverage summaryNote:
displayioRefer here for heat map coverage report |
|
@onkarvhanumante looking forward to your final check. Thanks! |
|
@onkarvhanumante we've fixed all the blockers and are waiting for your approval |
Thanks for being patient with PR reviews. PR is merged |
authored by @xdevel
Display.io prebid server adapter
Docs PR: prebid/prebid.github.io#5330