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

Make size mapping mediaType aware #3134

Merged
merged 4 commits into from
Oct 2, 2018
Merged

Conversation

snapwich
Copy link
Collaborator

@snapwich snapwich commented Sep 26, 2018

Type of change

  • Bugfix

Description of change

This makes the Prebid size mapping feature work on mediaTypes objects rather than size arrays. In other words, it is now mediaTypes aware and works specifically on mediaTypes.banner.sizes rather than adUnit.sizes (although the later is still supported, but deprecated).

This fixes issue #2462 since it only actively filters mediaType.banner.sizes while allowing non-banner (such as native and video) and even multi-mediaType bid requests to remain active without adUnit.sizes. However in the future, now that size mapping is aware of media types, we can use this feature to add sizeMapping support for filtering other non-banner media types if we want (we still need to have a discussion of how that would work).

@snapwich snapwich changed the title Native no size mapping Make size mapping mediaType aware Sep 26, 2018
@snapwich
Copy link
Collaborator Author

Added some additional console logging around size mapping (such as when an adUnit is deactivated and displaying the result of before and after filtering results) as well as fixed a bug (#3137) where an auction continued even if there were no valid bids.

@harpere harpere removed the question label Sep 27, 2018
@harpere harpere added the needs 2nd review Core module updates require two approvals from the core team label Sep 27, 2018
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

LGTM

@jsnellbaker
Copy link
Collaborator

Hi @snapwich Can you put together a docs PR with any documentation related changes in lieu of this PR?

http://prebid.org/dev-docs/publisher-api-reference.html#setConfig-Configure-Responsive-Ads still makes references to adUnits.sizes as the primary field we do the filtering based on what's defined in the sizeConfig object, so this likely should be updated. I'm not sure if there are any native examples that can also be updated to remove sizes field in the adUnit setup.

Thanks.

@jsnellbaker
Copy link
Collaborator

Thanks for putting the docs PR together. @jaiminpanchal27 gave his approval for this PR separately to me, so proceeding to merge.

@jsnellbaker jsnellbaker merged commit 298e41a into master Oct 2, 2018
@RBContent
Copy link

Hi All,

Do you guys confirm that the same things happen when an adunit array contains different mediatypes objects, I don't mean multi-format elements, but elements of different mediatypes, like a collection of native and banner mediatype.

It seems that the native part doesn't pass. I get the same message: 'label or size filtering'.

Regards

@jsnellbaker
Copy link
Collaborator

I'll take a look into this behavior; do you have a sample markup of the adunit array that didn't seem to work for you?

@jsnellbaker
Copy link
Collaborator

@RBContent I've setup a test page that has a banner adUnit and a native adUnit showing together. When I used the sizeMapping feature (and when I didn't), the delivery appeared to work as expected given the setup.

The only thing I saw (@snapwich please review), is when I had an adUnit.sizes defined in the native adUnit and it was using a set of sizes that were not covered by the sizeConfig - the new logic printed a message in the debug console saying that the placement was filtered. However it did not actually remove the bid object from being made (since it's a native type adunit).

If you're seeing a setup where this new logic doesn't appear to work, please share a snippet of your adunit/sizeConfig code (or provide a jsfiddle.net link) so we can review.

@snapwich
Copy link
Collaborator Author

snapwich commented Oct 3, 2018

@jsnellbaker that sounds correct. adUnit.sizes should not be set for a native adUnit, and if it is then the sizes are treated as banner sizes. Currently no size filtering is done for video or native (as we would have to define what that means and how it works, since sizing is not the same), so if an adUnit has multiple mediaTypes then the adUnit cannot be disabled (e.g. if you have a native and banner mediaType adUnit, the request will still be sent even if the banner sizes are all filtered because it's still possible to return a valid native bid).

@RBContent
Copy link

Hi,

The example I made for you on jsfiddle is available here: https://jsfiddle.net/tkugswd3/53/

You'll see one slot (top) dedicated for the native format and another one (bottom) for the banner format.
The native ad comes from Quantum (no test ads) and the banner one from Appnexus.

Normally you should only see the appnexus banner appearing. If you want to see the native format showing, please delete line 46 and 103 to 115.

@jsnellbaker
Copy link
Collaborator

jsnellbaker commented Oct 4, 2018

@RBContent I think there's some problem with the quantum placement itself rather than the logic.

I've tried several things with your test page:

  • replaced the quantum native ad unit with an appnexus test native ad unit while keeping/removing the sizeconfig code.
  • disabled the size config logic (the lines you noted) while running on the master branch, as well as on the 1.25.0 branch (using the old logic).

In all the cases when trying to use the quantum placement, I've never seen it return an ad (even with the old code and the sizeConfig disabled). When I tried to use the appnexus native test placement, it was returning an ad and rendering it fine in the manner I described above.

From what I'm seeing - it doesn't appear to be caused by the new code introduced by this PR.

If you're continuing to see a problem - can you open a new issue so we can discuss it there?

@RBContent
Copy link

Did you expect a visual asset to be returned by Quantum because the placement id is a live placement and no test ads are supposed to be returned.

Would you be so kind to share your working jsfiddle => appnexus with the sizeconfig logic.

Thank you.

@jsnellbaker
Copy link
Collaborator

@RBContent

Here is a jsfiddle setup with the appnexus banner and appenxus test native placements, using the 1.26.0 version of Prebid.js (that includes this PR).

When you load the page, you'll always see the native test ad render (regardless of the sizeConfig settings). If you resize the horitzontal viewport in the jsfiddle and re-run the page, you'll see the banner either show the blue prebid ad or the white fallback ad (following the logic of the sizeConfig).

In terms of the quantum placement, if you weren't expecting it to visually show an ad - how were you establishing the problem? If you were looking at the Prebid console messages and saw it that it said it filtered out the native placement - that's just because the native placement has a set of adUnit.sizes that do not fall into sizeConfig; the native adunit is still included in the auction and would return an ad (if available).

Native placements do not need to have sizes defined at all. It was only needed previously due to the bug that this PR fixed. If you remove the field now, the message won't show in the console log.

@RBContent
Copy link

I can't see the jsfiddle link in your last comment.

@jsnellbaker
Copy link
Collaborator

Oops sorry about that, here it is:
http://jsfiddle.net/fnuarsgp/

@RBContent
Copy link

Thanks and indeed, it's a lot better now comparing to version 1.25.0.
I'm in touch with Quantum right now and hoping to find out what's the issue.

AlessandroDG pushed a commit to simplaex/Prebid.js that referenced this pull request Oct 17, 2018
* Make size mapping mediaType aware (prebid#3134)

* make sizeMapping mediaTypes aware

* no sizeMapping if the only mediaType is not banner

* better logging for size mapping and no auction when no bids

* fix size mapping tests to ignore filter results (which are only for logging)

* Making targeting keys configurable (prebid#3140)

* moved NATIVE_KEYS to constants

* Changed TARGETING_KEYS from array to an array

- Now you can change key names
- Made changes in code at auction.js and targeting.js to handle the TARGETING_KEYS object than array

* fixed aauctionmanager test cases for dynamic targeting key names

* fixed native test cases for dynamic targeting key names

* fixed test cases of targeting for dynamic targeting key names

* in-dev changes

* replace old keys with new keys

* fixed a typo

* fixed a unit case in utils

* updated convertTargetingsFromOldToNew in fixtures

this will now replace partner specific keys as well as per config

* removed a log

* keep test case as it was

* improved convertTargetingsFromOldToNew in fixtures

* Capitalized key names in TARGETING_KEYS object; not values in object

* using computed properties

* using computed properties

* fixed eslint suggestion

* fixed eslint suggestions

* keeping unit tests more compliant with Mocha

* 4235 prebid endpoint return empty array instead of 204 when no bids returned (prebid#3136)

* corrected user sync type

* add alias pxyz
add version to endpoint
add validation for 204 bid response

* add validation with serverResponse is undefined
fix test for 204 (no bid response)

* remove package lock

* restore package-lock

* update gulp 4 notes on README (prebid#3154)

* Add teads bidder adapter (prebid#3135)

* Add teads bidder adapter

* Remove bidder code & tests arrow functions

* Added the option to pass a deal id instead of a partnership id (prebid#3148)

* increment Prebid version

* Prebid 1.26.0 release

* increment prebid version

* implement find polyfill in unit test (prebid#3156)

* InvibesBidAdapter - gdpr support (prebid#3151)

* removed `cookieSyncDelay` from `config` since we have more configuration options now with the `userSync` object (prebid#3142)

* Livewrapped bid and analytics adapter (prebid#3157)

* Livewrapped bid and analytics adapter

* Fixed some tests for browser compatibility

* Fixed some tests for browser compatibility

* Changed analytics adapter code name

* Fix double quote in debug message

* Adding appnexus debug via cookie/params (prebid#3152)

* adding appnexus debug via cookie/params

* removing nested object

* added documentation link and removed useless conditional

* fix lint error on documentation message

* Add RSA validation to Criteo FastBid (prebid#3110)

* Pass Prebid version to Criteo direct bidder

* Update Criteo profile IDs

* Add RSA verification to Criteo FastBid

* Update package-lock with jsencrypt and crypto-js

* Replacing all arrow functions in Mocha function calls

* Update Adapter Version to 14

* Added support for user syncing pixel (prebid#3092)

* Added Polymorph adapter

* Cleaned up code

* Updated var to let

* Updated with mediaType

* Updated tests

* Fixed tests

* Updated polymorph adapter to support cookie syncing and network key integration

* Fixed parens for lint

* Fixed a bug related to network_key approach

* Fixed double negation warning

* Updated tests for network_key

* Added bidId as cache buster and updated tests

* Fixed tests

* Small bugfix and cleanup for Prebid Server OpenRTB code (prebid#3113)

* change bid map to bid_id map and clean up openrtb in pbs

* add src to bids in bid request when known

* Revert "Adding appnexus debug via cookie/params" (prebid#3164)

* Revert "Small bugfix and cleanup for Prebid Server OpenRTB code (prebid#3113)"

This reverts commit a1f07e9.

* Revert "Added support for user syncing pixel (prebid#3092)"

This reverts commit 8fdf61d.

* Revert "Add RSA validation to Criteo FastBid (prebid#3110)"

This reverts commit e72e2dc.

* Revert "Adding appnexus debug via cookie/params (prebid#3152)"

This reverts commit 4797ea2.

* add bid ttl to cache call (prebid#3163)

* Render outstream safeframe  (prebid#3159)

* render outstream safeframe

* move code to Renderer module

* some more logic to move

* Prebid 1.27.0 Release

* increment Prebid version

* rubiconBidAdapter - Checking FPD values are defined before toString() (prebid#3165)

* rubiconBidAdapter - Checking FPD values are defined before toString()
                  - Added two tests for this behavior

* Removing unused variable

* changed to compare against null

added some null params to the tests to verify

* Adding mediaType param to parseSizes in order to ALWAYS get the correct parse Size method correct. (prebid#3166)

* PubMatic to support DigiTrust Id passing (prebid#3160)

* in-dev changes

* included config

* Unit test cases for DigitrustId passing in PubMatic bid adapter

* eslint fixes

* removed a comment

* replaced "() => {" with "() => {"

* OpenX Adapter: Added support for pubcid (prebid#3158)

* use version replace rather than package.json in widespace (prebid#3143)

* Trafficroots Resubmit (prebid#3141)

circleci is still failing, but I cannot find a workaround for this PR at the moment

* Rubicon skip video request in mutlti format when video is not setup (prebid#3167)

* Update rubiconBidAdapter.js

* OpenXOutstream Bid Adapter (prebid#3153)

* adds openxoutstreamadapter files

* cleans up url, adds bidder config and version, removes unused code

* updates template ad response

* remove final unused custom param code

* add openrtb to list of params again.

* add payload back so we can read bidId

* extra checks for cpm (pub_rev)

* adds unit tests

* adds unit tests

* update md page

* undo openx adapter space changes from autosave

* undo openx adapter space changes from autosave

* test cleanup

* test cleanup

* update md file

* update example page params

* remove sneaky console.log

* return false

* adds yieldmo to the md. remove useless docEl assignment

* remove useless docEl assignments

* move crid and adid to constants

* fix test

* remove another useless variable assignment caught by LGTM

* changes CR_ID. moves tdoc to within if scope

* updates crid on test

* use more constants and use crid for lfid

* remove superfluous trailing argument

* adds viewport meta tag to header. removes unnecessary string interpolation.

* move ad_id to a string

* undo changes for pacakge-lock

* fixes lfid to lfId, pID to pId

* fixes lfid to lfId, pID to pId

* remove rti:1

* adds openx maintainer email

* improves additional data passed to the handler in AuctionInit and AuctionEnd events (prebid#3168)

* removing cookieSet (prebid#3175)

* removing cookieSet

* removing blank line

* not including src/cookie anymore

* Removed deprecated priceType option (+tests) (prebid#3170)

* Improve Digital adapter: set dealID based on buying type (prebid#3182)

* Adding GDPR support

* Always drop user syncs when available

* Set dealID based on buying type

* disabling tests that are failing in safari (prebid#3186)

* Prebid 1.28.0 Release

* Increment pre version

* RVR-1124 Setup initial skeleton analytics adapter that can send something.

Approved-by: Alessandro Di Giovanni <alessandro.digiovanni@gamegenetics.de>

* Formatted auction/events data to fit needed schema.

* RVR-1135 fetched device data.

* Applied feedback.

* Applied feedback.

* Fetched core.

* Added click handler for reporting banners click events.

* Applied analyzer for reporting displayed impressions.

* Applied feedback.

* Merged in RVR-1214-invoke-handlers-on-rendering (pull request #7)

RVR-1214 Invoke handlers on rendering

* RVR-1214 Invoked handlers right after ad is displayed.

* Applied feedback.

Approved-by: Alessandro Di Giovanni <alessandro.digiovanni@gamegenetics.de>

* Merged in RVR-1192-configuration-global-parameters (pull request #8)

RVR-1192 Configuration/Global parameters

Approved-by: Alessandro Di Giovanni <alessandro.digiovanni@gamegenetics.de>

* Merged in RVR-1181-Prebid-js-unit-tests-setup (pull request #6)

RVR-1181 Prebid.js Unit tests setup

Approved-by: Alessandro Di Giovanni <alessandro.digiovanni@gamegenetics.de>

* Merged in RVR-1247-additional-data-to-impression-records (pull request #9)

RVR-1247 Additional data to impression records

Approved-by: Alessandro Di Giovanni <alessandro.digiovanni@gamegenetics.de>

* Merged in RVR-1249-add-requestedbids-to-auction (pull request #10)

RVR-1249 Add requested bids to auction object request.

Approved-by: Alessandro Di Giovanni <alessandro.digiovanni@gamegenetics.de>

* Merged in RVR-1261-fix-tests (pull request prebid#11)

RVR-1261 fix tests

* RVR-1261 Secured adapter from no containers configuration. And changed fetching URL.

* RVR-1261 Added event check for request and changed some names.

* Applied feedback.

Approved-by: Alessandro Di Giovanni <alessandro.digiovanni@gamegenetics.de>

* RVR-1352 analytics adapter bugs

Approved-by: Alessandro Di Giovanni <alessandro.digiovanni@gamegenetics.de>

* Fixed bug with geolocation notification.

* fixed missing bracket.

* one more fix.

* RVR-1357 Different optimisation responses & tracking into auction event

* RVR-1852 - Add content type and hardcoded auth headers

(cherry picked from commit 4def881)

* RVR-1852 - Change tracker host

* RVR-1852 - Override content type instead of adding header

* districtmDMX new adapter (prebid#2765)

* adding DMX

test @97%, two files added one updated

* Update districtm_spec.js

* Update districtmDMX.js

* adding all districtm needed file

* remove legacy file

* remove typo || 0 in the test method

* force default to return a valid width and height

* update unit test code for failing test

* changed class for an object

* remove package-lock.json

* upgrade to gulp 4 (prebid#2930)

* upgrade to gulp 4

* update circleci config

* removed some tasks and added notest flag

* update lint dependency for test task

* RVR-1914 Consistent data types in events

Also removes undefined and null properties in audience events

* Merged in RVR-1883-Add-Basic-Access-Authentication (pull request prebid#17)

RVR-1883 Add Basic Access Authentication

* RVR-1914 - Rename functions

* RVR-1914 - Set default total_duration to null in bid response

* RVR-1883 - Use RIVR_CLIENT_AUTH_TOKEN global variable for Auth token

* RVR-1883 - Restore stub after every test not just at the end

* RVR-1883 - Remove commented code

* Increase code coverage

* Fix for IE 11.0.0 and Safari 8.0.8 - includes()

Use core-js includes function for array
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* make sizeMapping mediaTypes aware

* no sizeMapping if the only mediaType is not banner

* better logging for size mapping and no auction when no bids

* fix size mapping tests to ignore filter results (which are only for logging)
pedrolopezmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Mar 18, 2019
* make sizeMapping mediaTypes aware

* no sizeMapping if the only mediaType is not banner

* better logging for size mapping and no auction when no bids

* fix size mapping tests to ignore filter results (which are only for logging)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants