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.

Sign up for GitHub

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

Jump to bottom

New Adapter: Adtargetme #2296

Merged
merged 6 commits into from
Aug 5, 2022
Merged

New Adapter: Adtargetme #2296

merged 6 commits into from
Aug 5, 2022

Conversation

grakholsky
Copy link
Contributor

@SyntaxNode
Copy link
Contributor

This PR is failing on the unit test:

--- FAIL: TestBidderUniquenessGatekeeping (0.00s)
    bidders_validate_test.go:62: 
        	Error Trace:	bidders_validate_test.go:62
        	Error:      	"9" is not less than or equal to "6"
        	Test:       	TestBidderUniquenessGatekeeping

This is because we require the first 6 characters of an bidder/adapter name to be unique unless both adapters refer to the same company.

Is there a relationship between Adtarget and Adtargetme?

@grakholsky
Copy link
Contributor Author

grakholsky commented Jul 8, 2022 •
edited
Loading

Hi.
We don’t have a relationship with Adtarget.
adtarget.me is a different company.

What name should be correct in our case?

@SyntaxNode
Copy link
Contributor

Please be aware that PR #2280 was recently merged. This PR updates the package used for the openrtb2 model. You will need to update this PR to use the github.com/mxmCherry/openrtb/v15/openrtb2 package.

@SyntaxNode
Copy link
Contributor

Hi. We don’t have a relationship with Adtarget. adtarget.me is a different company.

What name should be correct in our case?

You should choose your own name. You just need to a choose a more unique name to avoid conflicts with the existing adtarget bidder name. The first 6 characters must be unique.

@SyntaxNode SyntaxNode self-assigned this Jul 11, 2022
@hhhjort hhhjort requested a review from AlexBVolcy July 11, 2022 17:38
@grakholsky grakholsky mentioned this pull request Jul 12, 2022
Merged
8 tasks
@mansinahar mansinahar requested a review from SyntaxNode July 12, 2022 17:24
AlexBVolcy
AlexBVolcy reviewed Jul 13, 2022
View reviewed changes
adapters/adserver_adtarget/adserver_adtarget.go Outdated
Comment on lines 179 to 186
for _, imp := range imps {
if imp.ID == impId {
if imp.Video != nil {
mediaType = openrtb_ext.BidTypeVideo
} else if imp.Native != nil {
mediaType = openrtb_ext.BidTypeNative
}
return mediaType
Copy link
Contributor

@AlexBVolcy AlexBVolcy Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I might be confusing myself, but here it looks like you support Video and Native media types in addition to Banner. But in your docs PR and your adserver_adtarget.yaml file, it looks like you only support Banner. If you support video and native as well, could you update the docs PR and .yaml file? If not, could you explain what's being done here?

Copy link
Contributor Author

@grakholsky grakholsky Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We want to start with a banner, but we added the logic beforehand for the video and native. We will remove Video and Native support and return this logic later when we will be ready to work with Video and Native.

adapters/adserver_adtarget/adserver_adtarget.go Outdated
return mediaType
}
}
return mediaType
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that this return statement isn't covered by your current tests, could you add another JSON test to cover it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added returning an error for non-banner impressions.

@SyntaxNode
Copy link
Contributor

The new name you've chosen adserver_adtarget does indeed start with 6 unique characters, but those characters are adserv which is a bit generic and we discourage symbols in the name. Perhaps you can remove the vowels in target to get adtrgtme or something similar rather prepending adserver.

AlexBVolcy
AlexBVolcy previously approved these changes Jul 26, 2022
View reviewed changes
Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

LGTM

SyntaxNode
SyntaxNode reviewed Jul 26, 2022
View reviewed changes
adapters/adtrgtme/adtrgtme.go Show resolved Hide resolved
adapters/adtrgtme/adtrgtme.go Show resolved Hide resolved
adapters/adtrgtme/adtrgtme_test.go Outdated Show resolved Hide resolved
@bsardo
Copy link
Collaborator

bsardo commented Aug 2, 2022

@grakholsky just a friendly reminder to give @SyntaxNode's comments a look when you get a chance.

@grakholsky
adapters/adtrgtme: added supplemental tests
37caee8 
* added tests for headers ipv6
* added tests for handling another non-200 statuses
* removed port number
@SyntaxNode SyntaxNode merged commit b4e01dd into prebid:master Aug 5, 2022
@tarasmatokhniuk tarasmatokhniuk mentioned this pull request Aug 10, 2022
Merged
2 tasks
pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022
@grakholsky @pm-aadit-patil
New Adapter: Adtargetme (prebid#2296)
d6ff3fa
pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022
@grakholsky @pm-aadit-patil
New Adapter: Adtargetme (prebid#2296)
45e833f
pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022
@grakholsky @pm-aadit-patil
New Adapter: Adtargetme (prebid#2296)
4425c0e
jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022
@grakholsky @jorgeluisrocha
New Adapter: Adtargetme (prebid#2296)
bf0c574
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
@grakholsky @shunj-nb
New Adapter: Adtargetme (prebid#2296)
cfd924f
@bretg bretg mentioned this pull request Feb 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexBVolcy AlexBVolcy AlexBVolcy approved these changes

@SyntaxNode SyntaxNode SyntaxNode approved these changes

Assignees

@SyntaxNode SyntaxNode

@AlexBVolcy AlexBVolcy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@grakholsky @SyntaxNode @bsardo @AlexBVolcy