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

update logic of ad_types field in appnexusBidAdapter #4065

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

jsnellbaker
Copy link
Collaborator

Type of change

  • Feature

Description of change

This PR changes how the ad_types field of the appnexus request is populated in relation to banner media type.

Previously the field was populated automatically with 'banner' if the mediaType(s) field of the bid object was empty or if either of those fields were set to banner. The latter case actually always happened since Prebid.js treats any adUnit request without a defined mediaTypes object as banner and creates a mediaTypes field in the bid with a banner object in this scenario.

Now with this change, the banner ad_type is only populated if the original adUnit contained a mediaTypes.banner object. If the adUnit.mediaTypes object is missing, then the field is left out of the appnexus request. In terms of the delivery decision for the bid, this scenario would defer to the types that are setup within the placement and use that to choose the type of bid to find/return.

@jaiminpanchal27 jaiminpanchal27 added LGTM needs 2nd review Core module updates require two approvals from the core team and removed needs review labels Aug 6, 2019
@mkendall07
Copy link
Member

placing on hold for now due to some concerns about multi format.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

on hold

@jsnellbaker
Copy link
Collaborator Author

@mkendall07 Per the internal feedback, this change should be good to implement. Can you take another look when you have the chance? Thanks!

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM

@mkendall07 mkendall07 merged commit 2bdbb12 into master Sep 6, 2019
@mkendall07 mkendall07 deleted the appnexus_media_types branch September 6, 2019 17:39
@lsv1
Copy link
Contributor

lsv1 commented Sep 13, 2019

Hey all,

When I define adUnits without mediaTypes I get a parsing error returned, it looks like it's related to missing sizes in the request payload to AppNexus.

I think sizes must still be defined and sent over to avoid the parse error. Check out my Fiddle for a working example - http://jsfiddle.net/nfm0ahbk/2/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants