New Adapter: ResetDigital #3452
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
|
PR tests are failing. Should push change to fix Additionally, bidder-params and bidder-info files are missing. These are required files. Refer prebid-server/static/bidder-params/appnexus.json Lines 1 to 134 in 225e31e prebid-server/static/bidder-info/appnexus.yaml Lines 1 to 21 in 06ac44b |
| @@ -0,0 +1,88 @@ | |||
| { | |||
There was a problem hiding this comment.
adapters/resetdigital/resetdigitaltest/examplary/simple-banner.json is not being referred by unit tests added in resetdigital_test.go
Additionally, should add supplemental tests as well. Refer appnexus adapter as example - https://github.com/prebid/prebid-server/blob/dd07431ef54c38a2617c04e76b3e7bda9c21f569/adapters/appnexus
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestMakeRequests(t *testing.T) { |
There was a problem hiding this comment.
Recommended to use JSON test framework to test MakeRequest
| assert.Equal(t, 1, len(reqs), "Unexpected number of HTTP requests. Got %d. Expected %d", len(reqs), 1) | ||
| } | ||
|
|
||
| func TestMakeBids(t *testing.T) { |
There was a problem hiding this comment.
Recommended to use JSON test framework to test MakeBids
openrtb_ext/imp_resetdigital.go
Outdated
| type ExtResetDigital struct { | ||
| cid string `json:"bid_id"` | ||
| crid string `json:"imp_id"` | ||
| adid string `json:"adid"` | ||
| } |
There was a problem hiding this comment.
should add corresponding bidder-params.json file for ExtResetDigital
|
@bruno-siira should create docs PR in https://github.com/prebid/prebid.github.io repo. This PR is needed for include Additionally, refer https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#prebid-server---new-bid-adapter-go for guidelines to add new adapter in PBS-GO repo |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
@bruno-siira Please add bidder-params and bidder-info files. Refer examples provided above. |
|
@bruno-siira ple
@bruno-siira PTAL at above comment to fix failing test case |
Hello, Were trying to push today a new fix, this is blocked on the auto test from the main, I'll check that link you sent. |
| if response.StatusCode == http.StatusNoContent { | ||
| return nil, nil | ||
| } | ||
| if response.StatusCode == http.StatusBadRequest { | ||
| return nil, nil | ||
| } | ||
| if response.StatusCode != http.StatusOK { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
could make use of response util here
prebid-server/adapters/response.go
Lines 10 to 28 in ec729e6
| //check no bids | ||
| jsonData := make(map[string]interface{}) | ||
|
|
||
| json.Unmarshal([]byte(response.Body), &jsonData) |
There was a problem hiding this comment.
missing error check for json.Unmarshal
| { | ||
| "mockBidRequest": { | ||
| "id": "12345", | ||
| "imp": [ |
There was a problem hiding this comment.
should update or add json test:
- to have request with multiple imps
- to have multi-format imp request
There was a problem hiding this comment.
By standard our solution works with single impressions. Should we adapt this? Or add a test to it to do it that way?
There was a problem hiding this comment.
By standard our solution works with single impressions. Should we adapt this? Or add a test to it to do it that way?
If solution works with single impressions then should explicitly mention this behaviour in bidder docs. So that request inclues only one impression.
@bruno-siira additionally lets us know your thoughts on adding json test for multi-format imp request
There was a problem hiding this comment.
So the solution here is to make it explicit on the docs that is single impression on docs? What should be the doc to use @onkarvhanumante ?
There was a problem hiding this comment.
So the solution here is to make it explicit on the docs that is single impression on docs? What should be the doc to use @onkarvhanumante ?
Raise docs PR. Refer prebid/prebid.github.io#5331 as example
| @@ -0,0 +1,21 @@ | |||
| adapters: | |||
| resetdigital: | |||
| endpoint: http://b-us-east14.resetdigital.co:9001 | |||
There was a problem hiding this comment.
@bruno-siira is it possible to use standard http or https port i.e http(s)://b-us-east14.resetdigital.co instead of 9001
There was a problem hiding this comment.
If necessary yes, but its a practice for this kind of our servers. It's needed for the approval?
There was a problem hiding this comment.
if possible then should use standard http or https port
There was a problem hiding this comment.
We have a situation where the only server that we have that supports the Forced Bid is not with the standard port. We have now server with that standard port but doesn't allow the forced bidder so it can't pass the auto test. Any idea to solve this @onkarvhanumante ?
There was a problem hiding this comment.
We have a situation where the only server that we have that supports the Forced Bid is not with the standard port. We have now server with that standard port but doesn't allow the forced bidder so it can't pass the auto test. Any idea to solve this @onkarvhanumante ?
I am not sure how to resolve it.
@Sonali-More-Xandr @gargcreation1992 any suggestions
no need to include `required` if its empty _Originally posted by @onkarvhanumante in prebid#3452 (comment)
|
@bruno-siira PR has few unaddressed comments. PTAL |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
|
@bruno-siira closing this PR. You can open PR after addressing the open comments |
no need to include `required` if its empty _Originally posted by @onkarvhanumante in prebid#3452 (comment)
New adapter for ResetDigital on Go