Size Mapping V2 #4520
Size Mapping V2 #4520
Conversation
|
Modified checkAdUnitSetup function to check for the new sizeConfig object. |
|
This pull request introduces 1 alert when merging 4b876a6 into aba3689 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging c7b3816 into 7777359 - view on LGTM.com new alerts:
|
|
This pull request introduces 3 alerts when merging cbf2a61 into 7777359 - view on LGTM.com new alerts:
|
…hen bidder gets filtered out
|
This pull request introduces 3 alerts when merging 60847f0 into 7777359 - view on LGTM.com new alerts:
|
…last on incase there are multiple lables present
… of sizeConfig property in addition to doing normal adUnit checks
|
This pull request introduces 1 alert when merging eeb07e7 into 8e12e5f - view on LGTM.com new alerts:
|
…y, while keeping in place the essential sizeConfig checks
|
This pull request introduces 1 alert when merging bbb1888 into 8e12e5f - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 043626d into b615862 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 4c20b1b into b615862 - view on LGTM.com new alerts:
|
modules/sizeMappingV2.js
Outdated
| const keys = Object.keys(config); | ||
| if (!(includes(keys, 'minViewPort') && includes(keys, 'sizes'))) { | ||
| logError(`Ad Unit: ${adUnit.code}: mediaTypes.banner.sizeConfig is missing required property minViewPort or sizes or both. Removing the invalid mediaTypes.banner from Ad Unit.`); | ||
| return delete adUnit.mediaTypes.banner; |
There was a problem hiding this comment.
Given this block of code exists in a forEach, if one of the configs were 'bad' - it would still keep looping through even though the parent mediaTypes.banner object should have been deleted. If that's not expected, could you take another look at the logic here (and probably later in the function).
There was a problem hiding this comment.
@jsnellbaker Hi, I made the change.
I needed some way to display the log messages, and also delete the banner/video mediaType. I've made a change, let me know if that's fine.
|
This pull request introduces 1 alert when merging f0a29ca into 1a5c9c1 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 2165a8b into 927d36e - view on LGTM.com new alerts:
|
modules/sizeMappingV2.js
Outdated
| if (bid.sizeConfig) { | ||
| const relevantMediaTypes = getRelevantMediaTypesForBidder(bid.sizeConfig, activeViewport); | ||
| if (relevantMediaTypes.length === 0) { | ||
| logError(`SizeMappingV2:: Bidder: ${bid.bidder} in Ad Unit: ${adUnit.code} has not configured sizeConfig property correctly. This bidder won't be eligible for sizeConfig checks and will remain active.`); |
There was a problem hiding this comment.
This may be a minor thing, but could you flip the order of the adUnit and bidder in these log messages (and below this point)?
The majority of the messages earlier in the code use adUnit > bidder type of listing, so the switch here to bidder > adUnit is a bit distracting when looking at all the console messages together in the browser.
There was a problem hiding this comment.
Made the change. Please have a look.
modules/sizeMappingV2.js
Outdated
| return bids; | ||
| } | ||
| } else { | ||
| logInfo(`SizeMappingV2:: Bidder: ${bid.bidder} in Ad Unit: ${adUnit.code} is disabled due to failing sizeConfig check.`); |
There was a problem hiding this comment.
This message should only show when the bidder has the relevantMediaTypes field set to none for this size tier correct? If so, can we update the message to be more reflecting of that condition?
The current message feels like they did something wrong in their setup, rather than it be the result of a choice they made.
|
This pull request introduces 1 alert when merging 5afc8d1 into c147d0d - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 66a43c5 into 88b6a6d - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging b4cf490 into 88b6a6d - view on LGTM.com new alerts:
|
…lled only once per adUnit per auction
|
This pull request introduces 1 alert when merging 57c27e6 into c81ea5c - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 7a02628 into 8386a33 - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging 72b69df into c015d79 - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging ce36722 into c015d79 - view on LGTM.com new alerts:
|
The core feature has been implemented.
How to test it?
Edit the hello_world.html file, and replace it with an adUnit object containing the sizeConfig V2 specs. You can get hold of those objects from I2I, here. I2I - updates and new logic for sizeConfig feature #4129
Currently, the workflow (how to reach and activate this module) is not in place. I've currently created a hook on getBids function in adapterManager.js, and that hooked function is getting called for client side adapters only, at the present moment.
I've disabled the function checkAdUnitSetup temporarily in Prebid.js file to make this work.
The hello_world example has been modified with a simple adUnit setup with sizeMapping V2 specs to check the code. Later, will create a dedicated html example for this new feature.
Known Bugs
EDIT:
Q: How to test it?
Check the folder sizeMappingV2 under integrationExamples -> gpt, different scenarios has been already defined there. You can edit them to your liking to test edge cases and other scenarios.
The Workflow is working End-To-End. The feature is complete.
checkAdUnitSetup in prebid.js is hooked onto in sizeMappingV2 module to check the sizeConfig object.
All Known Bugs mentioned above has been resolved.
Pending Tasks:
Known Issue
If banner and video mediaTypes, both somehow gets filtered out, adUnits.sizes is set to an empty array. Due to this, some bidder adapters may complain.