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

Size Mapping V2 supports 'Identical Ad Units' #5062

Merged
merged 4 commits into from
Apr 7, 2020

Conversation

Fawke
Copy link
Contributor

@Fawke Fawke commented Apr 1, 2020

Type of change

  • Feature

Description of change

This PR introduces support for 'Identical Ad Unit' declaration in Size Mapping V2. An Identical Ad Unit is an ad unit with the same code as one of the existing ad units but different mediaTypes object.

Example of Identical Ad Units:

[
  {
    code: 'ad-code-1',
    mediaTypes: {
      banner: {
        sizeConfig: [
          { minViewPort: [0, 0], sizes: [] },
          { minViewPort: [1000, 0], sizes: [[1000, 300], [1000, 90], [970, 250], [970, 90], [728, 90]] },
        ],
      },
    },
    bids: [{
      bidder: 'bidderA',
      params: {
        placementId: 123
      }
    }]
  }, {
    code: 'ad-code-1',
    mediaTypes: {
      banner: {
        sizeConfig: [
          { minViewPort: [0, 0], sizes: [] },
          { minViewPort: [320, 0], sizes: [[320, 100], [320, 50], [300, 50]] },
          { minViewPort: [600, 0], sizes: [] },
        ],
      },
    },
    bids: [{
      bidder: 'bidderA',
      params: {
        placementId: 456
      }
    }]
  },
]

The example above has two identical ad units with the same code ad-code-1 but with different values for mediaTypes object.

The PR also improves the log messages generated by Size Mapping V2, tries to make it more intuitive and easy to understand.

Resolves #5044

@Fawke
Copy link
Contributor Author

Fawke commented Apr 2, 2020

This PR is ready for review. Only thing that I'd have liked to do was remove some ambiguity from the log messages.

For example:

Screenshot 2020-04-02 at 4 52 41 PM

If we have two Identical ad units (sharing the same code), from the log messages it is kind of hard to determine which ad unit got disabled, or which ad unit got an error, since ad unit code is no longer a unique identifier.

In the screenshot above, we have two identical ad units sharing the same code ad-code-1, in which after sizeMapping filtration, the second instance of ad-code-1 gets disabled. But looking at the log messages, there's currently no way to tell was it the first instance or the second instance.

Once this PR is merged, I'll create a separate github issue to keep track of this behaviour and provide a fix in a separate PR.

@jsnellbaker jsnellbaker added needs 2nd review Core module updates require two approvals from the core team and removed needs review labels Apr 6, 2020
Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Nice, looks pretty good!

I am always a big fan of improving our logging.

@jsnellbaker jsnellbaker merged commit 9de6609 into master Apr 7, 2020
rjvelicaria pushed a commit to openx/Prebid.js that referenced this pull request Apr 9, 2020
* add support for identical ad units and improve log messages

* slight change in log message presentation

* fix typo

* document return value type
iggyfisk pushed a commit to happypancake/Prebid.js that referenced this pull request Jun 22, 2020
* add support for identical ad units and improve log messages

* slight change in log message presentation

* fix typo

* document return value type
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.

SizeMappingV2 bug with multiple ad units having the same code
3 participants