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

New Adapter: Adverxo #4018

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

dev-adverxo
Copy link

Summary

This PR introduces the Adverxo bidder adapter for Prebid Server. It supports the following mediatypes:

  • Banner
  • Video
  • Native

Maintainer Contact: [email protected]

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, fb3750c

adverxo

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:26:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:40:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:86:	MakeBids		95.7%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:134:	getAdUnitsParams	100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:153:	modifyImp		100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:170:	makeRequestCopyWithImp	100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:177:	buildEndpointURL	100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:186:	getMediaTypeForBid	100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:201:	getNativeAdm		90.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:221:	resolveMacros		100.0%
total:									(statements)		93.1%

@przemkaczmarek przemkaczmarek self-assigned this Oct 30, 2024
Copy link

github-actions bot commented Nov 4, 2024

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 913ea87

adverxo

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:26:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:40:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:86:	MakeBids		95.7%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:134:	getAdUnitsParams	100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:153:	modifyImp		100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:170:	makeRequestCopyWithImp	100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:177:	buildEndpointURL	100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:186:	getMediaTypeForBid	100.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:201:	getNativeAdm		90.0%
github.com/prebid/prebid-server/v2/adapters/adverxo/adverxo.go:221:	resolveMacros		100.0%
total:									(statements)		93.1%

@bsardo bsardo changed the title New bid adapter: Adverxo New Adapter: Adverxo Nov 4, 2024
@bsardo
Copy link
Collaborator

bsardo commented Nov 4, 2024

Hi @dev-adverxo, we recently released PBS 3.0, more specifically v3.1.0, which updates Prebid Server package import references throughout the project from v2 to v3.
For example:

import (
    "github.com/prebid/prebid-server/v3/adapters"
)

As a result, please merge with master (no rebase) and then ensure all Prebid Server package import references in the files you’ve changed are v3 such that the test suite passes so we can resume reviewing. Thanks!

Copy link

github-actions bot commented Nov 5, 2024

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 07b8d6b

adverxo

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:26:	Builder			100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:40:	MakeRequests		81.8%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:86:	MakeBids		95.7%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:134:	getAdUnitsParams	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:153:	modifyImp		100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:170:	makeRequestCopyWithImp	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:177:	buildEndpointURL	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:186:	getMediaTypeForBid	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:201:	getNativeAdm		90.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:221:	resolveMacros		100.0%
total:									(statements)		93.1%

@dev-adverxo
Copy link
Author

Hi @dev-adverxo, we recently released PBS 3.0, more specifically v3.1.0, which updates Prebid Server package import references throughout the project from v2 to v3. For example:

import (
    "github.com/prebid/prebid-server/v3/adapters"
)

As a result, please merge with master (no rebase) and then ensure all Prebid Server package import references in the files you’ve changed are v3 such that the test suite passes so we can resume reviewing. Thanks!

Hi @bsardo, thanks for the update. We've made the necessary changes and ensured that the tests passed.

Let us know if there's anything else.

przemkaczmarek
przemkaczmarek previously approved these changes Nov 5, 2024
Copy link
Contributor

@bretg bretg left a comment

Choose a reason for hiding this comment

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

host cannot be a required parameter. It is incredibly awkward for publishers to have to enter a hostname on thousands of adunits when this is a global config.

For PBS, it's host should be specified in your YAML file. If you wish to have an optional impression-level host override, that's ok.

@dev-adverxo dev-adverxo requested a review from bretg November 7, 2024 17:38
@dev-adverxo
Copy link
Author

host cannot be a required parameter. It is incredibly awkward for publishers to have to enter a hostname on thousands of adunits when this is a global config.

For PBS, it's host should be specified in your YAML file. If you wish to have an optional impression-level host override, that's ok.

Hi @bretg, just to clarify, we don’t have a host parameter here. It’s only present in Prebid.js.

Copy link

github-actions bot commented Nov 8, 2024

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 4fc858b

adverxo

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:26:	Builder			100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:40:	MakeRequests		81.8%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:86:	MakeBids		95.7%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:134:	getAdUnitsParams	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:153:	modifyImp		100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:170:	makeRequestCopyWithImp	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:177:	buildEndpointURL	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:186:	getMediaTypeForBid	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:201:	getNativeAdm		90.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:221:	resolveMacros		100.0%
total:									(statements)		93.1%

@dev-adverxo
Copy link
Author

Hi @przemkaczmarek sorry for the changes after your review, but in the review process of our Prebid.js adapter we decide to introduce two aliases of our adapter.

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 7b84702

adverxo

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:26:	Builder			100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:40:	MakeRequests		81.8%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:86:	MakeBids		95.7%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:134:	getAdUnitsParams	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:153:	modifyImp		100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:170:	makeRequestCopyWithImp	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:177:	buildEndpointURL	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:186:	getMediaTypeForBid	100.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:201:	getNativeAdm		90.0%
github.com/prebid/prebid-server/v3/adapters/adverxo/adverxo.go:221:	resolveMacros		100.0%
total:									(statements)		93.1%

@dev-adverxo
Copy link
Author

Hi @bretg, sorry for tagging you again. I noticed the "required changes" tag is still present here, but it seems to be related to the optional host parameter, which we have already resolved. Could you please mark it as solved? Thanks!

@bretg
Copy link
Contributor

bretg commented Nov 18, 2024

@przemkaczmarek and @bsardo , please review when you get a chance.

@@ -0,0 +1,9 @@
endpoint: "https://adport.pbsadverxo.com/auction?id={{.AdUnit}}&auth={{.TokenID}}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

@@ -0,0 +1,22 @@
endpoint: "https://pbsadverxo.com/auction?id={{.AdUnit}}&auth={{.TokenID}}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

`null`,
`{}`,
`{"adCode": "string", "seatCode": 5, "originalPublisherid": "string"}`,
`{ "adUnitId": 5}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

add test with only "auth"

Copy link
Author

@dev-adverxo dev-adverxo Dec 16, 2024

Choose a reason for hiding this comment

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

Hi @przemkaczmarek, we already had a test for only "auth", it is on line 47, if it is not what you mean or need something else, please, let us know, thanks!

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.

4 participants