Skip to content
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

Add Adpone adapter #1033

Merged
merged 18 commits into from
Oct 1, 2019
Merged

Add Adpone adapter #1033

merged 18 commits into from
Oct 1, 2019

Conversation

seergiioo6
Copy link
Contributor

No description provided.

@guscarreon guscarreon self-assigned this Sep 20, 2019
guscarreon
guscarreon previously approved these changes Sep 20, 2019
adapters/adpone/adpone.go Show resolved Hide resolved

headers := http.Header{}
headers.Add("Content-Type", "application/json;charset=utf-8")
requestToBidder := &adapters.RequestData{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think more headers might be helpful? Other adapters include headers such as x-openrtb-version. Do you think it'll help to include more header info?

headers.Add("Accept", "application/json")
headers.Add("x-openrtb-version", "2.5")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely yes; I will add more headers. Thanks a lot

exchange/adapter_map.go Outdated Show resolved Hide resolved
Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there no bidder JSON parameters to expect and validate for?

- banner
site:
mediaTypes:
- banner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a newline in here?

"title": "Adpone Adapter Params",
"description": "A schema which validates params accepted by the adpone adapter",
"type": "object",
"properties": {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we don't have any parameters we want to check for? If not, do you think you'll need them in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably in the future we will need some parameters so I have added one (placementId). Thanks!

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null check issue on line 29

requestsToBidder []*adapters.RequestData,
errs []error,
) {
var imp = &openRTBRequest.Imp[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a request with an empty openRTBRequest.Imp array comes in, your adapter fails with an "index out of range" error in line 29. Can we check for nil before dereferencing the Imp array and check its length before accessing any of its elements?

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for your patience

@hhhjort hhhjort merged commit 2de7bb6 into prebid:master Oct 1, 2019
mansinahar pushed a commit to mansinahar/prebid-server that referenced this pull request Nov 1, 2019
* adpone adapter

* adpone adapter

* go fmt files

* fix typo

* remove extra information from req

* fix testing

* fix usersync_test

* add adpone json schema

* add adpone to syncer_test.go

* add adpone to syncer.go

* Add extra headers + return error if no impresison in the bid request

* add optional placementId param

* remove imp_adpone.go

* fix adpone.json

* remove some headers

* added placementId as optional parameter

* go fmt files

* fix index out of ragne
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 1, 2020
* adpone adapter

* adpone adapter

* go fmt files

* fix typo

* remove extra information from req

* fix testing

* fix usersync_test

* add adpone json schema

* add adpone to syncer_test.go

* add adpone to syncer.go

* Add extra headers + return error if no impresison in the bid request

* add optional placementId param

* remove imp_adpone.go

* fix adpone.json

* remove some headers

* added placementId as optional parameter

* go fmt files

* fix index out of ragne
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 2, 2020
* adpone adapter

* adpone adapter

* go fmt files

* fix typo

* remove extra information from req

* fix testing

* fix usersync_test

* add adpone json schema

* add adpone to syncer_test.go

* add adpone to syncer.go

* Add extra headers + return error if no impresison in the bid request

* add optional placementId param

* remove imp_adpone.go

* fix adpone.json

* remove some headers

* added placementId as optional parameter

* go fmt files

* fix index out of ragne
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 4, 2020
* adpone adapter

* adpone adapter

* go fmt files

* fix typo

* remove extra information from req

* fix testing

* fix usersync_test

* add adpone json schema

* add adpone to syncer_test.go

* add adpone to syncer.go

* Add extra headers + return error if no impresison in the bid request

* add optional placementId param

* remove imp_adpone.go

* fix adpone.json

* remove some headers

* added placementId as optional parameter

* go fmt files

* fix index out of ragne
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants