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

Fixes #2276 - video.playerSize and Size Mapping not working together #2311

Merged
merged 3 commits into from
Apr 3, 2018

Conversation

jsnellbaker
Copy link
Collaborator

@jsnellbaker jsnellbaker commented Mar 23, 2018

Type of change

  • Bugfix

Description of change

Currently, using Size Mapping functionality with a video adUnit that specifies mediaTypes.video.playerSize would not pass the sizeMapping.js validation checks causing the ad unit to not be included in the bidRequest object.

As a quick recap, the validation check in the sizeMapping (located here https://github.com/prebid/Prebid.js/blob/master/src/sizeMapping.js#L41) does the following:

  • uses a list of arrayed sizes (ie [[300, 250], [300, 600]]) as a list of acceptable sizes (this is derived from the matching mediaQuery params).
  • It reads the adUnit's size params and cross checks that array against the mediaQuery list of sizes
  • if it finds a match, the size passes the filter
  • otherwise if no sizes are matched, the bid doesn't get added to the request

Because the filtering operates on a list of arrayed sizes, the adUnits.sizes has to use the same format. Otherwise, the filter would treat the single array of sizes ie [300, 250] as two integers and the size wouldn't pass the check.

To meet this requirement, I changed the adUnits.mediaTypes.video.playerSize field to accept either format (ie [[640, 480]] or [640, 480]) but it will automatically format the latter syntax into the former.

A note - while the playerSize field will expect an array of an array, there should only 1 set of sizes in the field. The changes in the checkBidRequestSizes() function check for this exact type of setup.

Other information

fixes issue reported in #2276

return Array.isArray(val) && val.length === 2 && utils.isInteger(val[0]) && utils.isInteger(val[1]);
}

// Array.prototype.forEach.call(adUnits, adUnit => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can drop this line if no longer needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it.

@matthewlane matthewlane added the needs 2nd review Core module updates require two approvals from the core team label Mar 30, 2018
@jsnellbaker
Copy link
Collaborator Author

@matthewlane Per your separate feedback, I added support for the mediaTypes.video.playerSize field to be able to take either [[640, 480]] or [640,480] syntax styles. When the code detects the latter style, it automatically converts it to the former style to ensure it works with sizeMapping functionality.

I'll update the original description in lieu of the functionality change.

@mkendall07 mkendall07 merged commit a6b42d4 into master Apr 3, 2018
@mkendall07 mkendall07 deleted the bugfix/issue_2276 branch August 17, 2018 15:10
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