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

Module - Size Mapping V2 #4690

Merged
merged 45 commits into from
Mar 11, 2020
Merged

Module - Size Mapping V2 #4690

merged 45 commits into from
Mar 11, 2020

Conversation

Fawke
Copy link
Contributor

@Fawke Fawke commented Jan 4, 2020 •
edited by idettman
Loading

Type of change

  • Feature

Description of change

Implementation of the new size mapping spec. Find details of the spec here.
#4129 (comment)

  • In short, this spec introduces ad unit and bidder level sizeConfig for displaying responsive ads.
  • Behaviour of Labels stays unchanged. You can declare them in pbjs.requestBids.

Please read the docs PR to understand how to setup Ad Units using the new sizeConfig.

To load and test the module in isolation, you can run this command.

gulp serve --modules=sizeMappingV2,<insert bidder adapters you want to test>

There is an example to test the module located at integrationExamples/gpt/responsiveAds_sizeMappingV2.html. You can modify the parameters and verify.

Link to docs PR: prebid/prebid.github.io#1729

@Fawke
implement size bucket filtration logic
3d5760a
@Fawke
Merge branch 'master' of https://github.com/prebid/Prebid.js into adv…
d8c96fc 
…-size-config
@Fawke
finish implmenation of getFilteredMediaTypes
240cfce
@Fawke
implement getBids function
8d7d35d
@Fawke
log useful information to the console
5264373
@Fawke
enable test workflow
ac0007d
@Fawke
modify checkAdUnitSetup function to account for the new sizeConfig ob…
4b876a6 
…ject
@Fawke
remove unrelated example
c7b3816
@Fawke
add decider function to choose between sizeMapping v1 and sizeMapping v2
751ceff
@Fawke
add getBidsHook for s2s bidders
cbf2a61
@Fawke
handle edge case where all mediaTypes get filtered out and the case w…
60847f0 
…hen bidder gets filtered out
@Fawke
add test examples for banner media type
5c35aed
@Fawke
update label check to pick up the fist label operator instead of the …
ca521ba 
…last on incase there are multiple lables present
@Fawke
added example for label checks with banner ad
9ca28cf
@Fawke
add checkAdUnit setup hook on sizeMappingV2 modules to check presence…
eeb07e7 
… of sizeConfig property in addition to doing normal adUnit checks
@Fawke
restore old hello world example
0d6e27c
@Fawke
give free pass to video mediaTypes configured with sizeConfig propert…
208861a 
…y, while keeping in place the essential sizeConfig checks
@Fawke
fixing minor bus and enchancing bidder level sizeConfig checks
bbb1888
@Fawke
bugfix: checkBidderSizeConfigFormat
72b5607
@Fawke
add more scenarios for testing sizeMapping V2
d554866
@Fawke
small docs changes
ff0d348
@Fawke
feedback1 changes
0c30a75
@Fawke
modify logic for bailing out
613a830
@Fawke
add module description
4040fad
@Fawke
refactor isUsingNewSizeMapping function by making it a pure function
1efc71c
@Fawke
add unit test case for isUsingNewSizeMapping function
723c298
@Fawke
sync with master
043626d
@Fawke
made adUnit checks more robusts and fully make adUnit.mediaTypes mand…
4c20b1b 
…atory
@Fawke
remove redundancy in checAdUnitSetupHook
f0a29ca
@Fawke
add banner units test cases for checkAdUnitSetupHook function
2165a8b
@lgtm-com

This comment has been minimized.

Sign in to view
idettman
idettman suggested changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@idettman idettman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you address the LGTM error "1 for Useless assignment to local variable"

Also, can you remove changes to files that that only change formatting (such as adding/removing spaces
in test/spec/unit/pbjs_api_spec.js)? This is a large PR to review and it would help reviewers so they can focus on the required changes only. I would recommend opening another PR after this that specifically addresses the formatting changes.

@Fawke
Copy link
Contributor Author

Fawke commented Jan 8, 2020

@idettman LGTM error should be gone now, and reverted back changes to pbjs_api_spec.js.

@benjaminclot
Copy link
Contributor

I tested this module locally (with a production setup) and the results are very good. Is there an intent to merge this with Prebid 2.x too?

@Fawke
Copy link
Contributor Author

Fawke commented Jan 20, 2020

I tested this module locally (with a production setup) and the results are very good. Is there an intent to merge this with Prebid 2.x too?

Thanks for checking it out @benjaminclot. AFIK, we will merge this with 2.0 as well.

@stale

This comment has been minimized.

Sign in to view
@stale stale bot added the stale label Feb 3, 2020
@Fawke Fawke added needs review pinned won't be closed by stalebot and removed stale labels Feb 3, 2020
@benjaminclot
Copy link
Contributor

Is this still planned for merging? (I sure do hope so)

@jaiminpanchal27 jaiminpanchal27 added LGTM needs 2nd review Core module updates require two approvals from the core team and removed needs review labels Mar 3, 2020
@jaiminpanchal27 jaiminpanchal27 removed their assignment Mar 3, 2020
idettman
idettman reviewed Mar 6, 2020
View reviewed changes
Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

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

Cook updates look good, but trying to get a passing circleci test, can you try to update your branch with any changes from the remote master?

@idettman idettman added the docs label Mar 6, 2020
@Fawke Fawke merged commit 6005a29 into master Mar 11, 2020
redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Mar 13, 2020
@redaguermas
Merge branch 'master' of https://github.com/prebid/Prebid.js
36fab76 
* 'master' of https://github.com/prebid/Prebid.js: (49 commits)
  Submitting Clicktripz bid adapter (prebid#4929)
  add UNICORN bid adapter (prebid#4917)
  3.12.0-pre
  3.11.0 release
  Eids liveintent ext fix (prebid#4944)
  add mediaforce bid adapter (prebid#4933)
  update logic in adpod module for playersize (prebid#4953)
  Module - Size Mapping V2 (prebid#4690)
  Lifestreet adapter 3.0 (prebid#4927)
  IX Adapter - Increase banner TTL to 300s (prebid#4957)
  assert string returned not that we break things (prebid#4962)
  added option to url parser to ignore decoding entire url (prebid#4938)
  adding user-id support in medianet adapter (prebid#4925)
  removing the log (prebid#4960)
  add import extensions (prebid#4959)
  Add 7xbid adapter to compatible with prebid 3.0 (prebid#4908)
  Fix failing code-coverage command (prebid#4892)
  enable no-console eslint rule for project (prebid#4802)
  update audigent tests to fix larger test suite issue (prebid#4952)
  use bidId or bidIds in the payload (prebid#4903)
  ...
redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Mar 13, 2020
@redaguermas
Merge branch 'master' into NoBid-Version-1.2.4
031ebf1 
* master: (49 commits)
  Submitting Clicktripz bid adapter (prebid#4929)
  add UNICORN bid adapter (prebid#4917)
  3.12.0-pre
  3.11.0 release
  Eids liveintent ext fix (prebid#4944)
  add mediaforce bid adapter (prebid#4933)
  update logic in adpod module for playersize (prebid#4953)
  Module - Size Mapping V2 (prebid#4690)
  Lifestreet adapter 3.0 (prebid#4927)
  IX Adapter - Increase banner TTL to 300s (prebid#4957)
  assert string returned not that we break things (prebid#4962)
  added option to url parser to ignore decoding entire url (prebid#4938)
  adding user-id support in medianet adapter (prebid#4925)
  removing the log (prebid#4960)
  add import extensions (prebid#4959)
  Add 7xbid adapter to compatible with prebid 3.0 (prebid#4908)
  Fix failing code-coverage command (prebid#4892)
  enable no-console eslint rule for project (prebid#4802)
  update audigent tests to fix larger test suite issue (prebid#4952)
  use bidId or bidIds in the payload (prebid#4903)
  ...
@benjaminclot benjaminclot mentioned this pull request Mar 30, 2020
Closed
bmwcmw pushed a commit to criteo-forks/Prebid.js that referenced this pull request Mar 31, 2020
@Fawke
Module - Size Mapping V2 (prebid#4690)
8d205fc 
* implement size bucket filtration logic

* finish implmenation of getFilteredMediaTypes

* implement getBids function

* log useful information to the console

* enable test workflow

* modify checkAdUnitSetup function to account for the new sizeConfig object

* remove unrelated example

* add decider function to choose between sizeMapping v1 and sizeMapping v2

* add getBidsHook for s2s bidders

* handle edge case where all mediaTypes get filtered out and the case when bidder gets filtered out

* add test examples for banner media type

* update label check to pick up the fist label operator instead of the last on incase there are multiple lables present

* added example for label checks with banner ad

* add checkAdUnit setup hook on sizeMappingV2 modules to check presence of sizeConfig property in addition to doing normal adUnit checks

* restore old hello world example

* give free pass to video mediaTypes configured with sizeConfig property, while keeping in place the essential sizeConfig checks

* fixing minor bus and enchancing bidder level sizeConfig checks

* bugfix: checkBidderSizeConfigFormat

* add more scenarios for testing sizeMapping V2

* small docs changes

* feedback1 changes

* modify logic for bailing out

* add module description

* refactor isUsingNewSizeMapping function by making it a pure function

* add unit test case for isUsingNewSizeMapping function

* made adUnit checks more robusts and fully make adUnit.mediaTypes mandatory

* remove redundancy in checAdUnitSetupHook

* add banner units test cases for checkAdUnitSetupHook function

* add video and native mediaTypes units test for checkAdUnitSetupHook function

* rewrite some of the log messages

* redefine log messages to make it simple to the end user

* code optimization done so that getFilteredMediaTypes function gets called only once per adUnit per auction

* add code comments

* code refactorization and more unit test cases

* more unit tests for getBids function

* add sizeMapping usage example

* delete sizeMappingV2 directory

* add doctype declaration

* fix LGTM alert and revert changes to pbjs_api.spec.js

* fix LGTM alerts in sizeMappingV2_spec file

* add file extension for imports
rjvelicaria pushed a commit to openx/Prebid.js that referenced this pull request Apr 9, 2020
@Fawke @rjvelicaria
Module - Size Mapping V2 (prebid#4690)
43df67b 
* implement size bucket filtration logic

* finish implmenation of getFilteredMediaTypes

* implement getBids function

* log useful information to the console

* enable test workflow

* modify checkAdUnitSetup function to account for the new sizeConfig object

* remove unrelated example

* add decider function to choose between sizeMapping v1 and sizeMapping v2

* add getBidsHook for s2s bidders

* handle edge case where all mediaTypes get filtered out and the case when bidder gets filtered out

* add test examples for banner media type

* update label check to pick up the fist label operator instead of the last on incase there are multiple lables present

* added example for label checks with banner ad

* add checkAdUnit setup hook on sizeMappingV2 modules to check presence of sizeConfig property in addition to doing normal adUnit checks

* restore old hello world example

* give free pass to video mediaTypes configured with sizeConfig property, while keeping in place the essential sizeConfig checks

* fixing minor bus and enchancing bidder level sizeConfig checks

* bugfix: checkBidderSizeConfigFormat

* add more scenarios for testing sizeMapping V2

* small docs changes

* feedback1 changes

* modify logic for bailing out

* add module description

* refactor isUsingNewSizeMapping function by making it a pure function

* add unit test case for isUsingNewSizeMapping function

* made adUnit checks more robusts and fully make adUnit.mediaTypes mandatory

* remove redundancy in checAdUnitSetupHook

* add banner units test cases for checkAdUnitSetupHook function

* add video and native mediaTypes units test for checkAdUnitSetupHook function

* rewrite some of the log messages

* redefine log messages to make it simple to the end user

* code optimization done so that getFilteredMediaTypes function gets called only once per adUnit per auction

* add code comments

* code refactorization and more unit test cases

* more unit tests for getBids function

* add sizeMapping usage example

* delete sizeMappingV2 directory

* add doctype declaration

* fix LGTM alert and revert changes to pbjs_api.spec.js

* fix LGTM alerts in sizeMappingV2_spec file

* add file extension for imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idettman idettman idettman approved these changes

@jaiminpanchal27 jaiminpanchal27 jaiminpanchal27 approved these changes

@robertrmartinez robertrmartinez Awaiting requested review from robertrmartinez

Assignees

@idettman idettman

@robertrmartinez robertrmartinez

Labels
docs LGTM needs 2nd review Core module updates require two approvals from the core team pinned won't be closed by stalebot
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Fawke @benjaminclot @idettman @jaiminpanchal27 @robertrmartinez