-
Notifications
You must be signed in to change notification settings - Fork 835
New Adapter: DXTech #4506
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
base: master
Are you sure you want to change the base?
New Adapter: DXTech #4506
Conversation
Code coverage summaryNote:
dxtechRefer here for heat map coverage report
|
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.
Your adapter is not passing our validation scripts because gofmt
needs to be run on the following files:
Run ./validate.sh --nofmt --cov --race 10
gofmt needs to be run, 6 files have issues. Below is a list of files to review:
adapters/dxtech/dxtech.go
adapters/dxtech/dxtech_test.go
adapters/dxtech/params_test.go
exchange/adapter_builders.go
openrtb_ext/bidders.go
openrtb_ext/imp_dxtech.go
Can you run the gofmt
command in your local folder before your next commit so validation passes and we can merge once approved?
"placementId": { | ||
"type": "string", | ||
"description": "The placement id" | ||
} |
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.
The documentation pull request New adapter - DXTech #6230 mentions two additional parameters, bidfloor
and bidfloorcur
, that are not listed here. According to file dev-docs/bidders/dxtech.md
:
Name | Scope | Description | Example | Type |
---|---|---|---|---|
placementId |
required | Placement Id | '1234abcd' |
string |
publisherId |
required | Publisher Id | '12345' |
string |
bidfloor |
optional | Bid Floor | 2.3 |
float |
bidfloorcur |
optional | Bid Floor Currency | 'USD' |
string |
Do we need to add them here? Or should they be removed from New adapter - DXTech #6230?
headers.Add("Accept", "application/json") | ||
headers.Add("X-Openrtb-Version", "2.5") | ||
|
||
if request.Site != nil { |
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.
Just to double-check: your adapter also supports app
according to your static/bidder-info/dxtech.yaml
file. If a given request comes with request.App
and not request.Site
, should any header, like maybe Origin
, be added?
"httpCalls": [ | ||
{ | ||
"expectedRequest": { | ||
"method": "GET", |
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.
Line 64 of adapters/dxtech/dxtech.go
sets "method": "POST"
to every outgoing request
63 adapterRequests = append(adapterRequests, &adapters.RequestData{
64 Method: http.MethodPost,
65 Uri: a.endpoint + "?" + params.Encode(),
66 Body: body,
67 Headers: getHeaders(request),
68 ImpIDs: openrtb_ext.GetImpIDs(request.Imp),
69 })
70 }
adapters/dxtech/dxtech.go
Therefore, the GET
value seems to be incorrect here and in the following files:
adapters/dxtech/dxtechtest/exemplary/banner.json|36 col 9-24| "method": "GET",
adapters/dxtech/dxtechtest/exemplary/empty-site-domain-ref.json|38 col 9-24| "method": "GET",
adapters/dxtech/dxtechtest/exemplary/ipv6.json|38 col 9-24| "method": "GET",
adapters/dxtech/dxtechtest/exemplary/video-test-request.json|41 col 9-24| "method": "GET",
adapters/dxtech/dxtechtest/exemplary/video.json|40 col 9-24| "method": "GET",
adapters/dxtech/dxtechtest/supplemental/invalid-response.json|40 col 9-24| "method": "GET",
adapters/dxtech/dxtechtest/supplemental/no-mtype.json|40 col 9-24| "method": "GET",
adapters/dxtech/dxtechtest/supplemental/status-code-bad-request.json|40 col 9-24| "method": "GET",
adapters/dxtech/dxtechtest/supplemental/status-code-no-content.json|40 col 9-24| "method": "GET",
adapters/dxtech/dxtechtest/supplemental/status-code-other-error.json|40 col 9-24| "method": "GET",
On a sidenote, your adapter helped us catch a bug in our JSON esting framework as it is not asserting this field. We plan to correct that in a future PR. In the meantime, can we modify?
No description provided.