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

Consistent targeting set #2592

Merged

Conversation

ptomasroos
Copy link
Contributor

Type of change

  • Bugfix
  • Feature

Description of change

The idea is that users of SET_TARGETING who receives the argument targeting set, should be able to get the ad unit codes always involved even though a bid not might be available when rendering the ad

Other information

Previously I tried to introduce the adUnits into the arguments #1875
and since then the targetingSet has been starting to get passed. In order to make this without breaking any contracts my suggestion is that we always return a entry even though there is no bid available to the receiver of SET_TARGETING can iterate which ad unit codes are getting invoked.

src/targeting.js Outdated
targeting.push(emptyTargeting);
}
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be greatly simplified by replacing it with the following code after the flattenTargeting is called below

    adUnitCodes.forEach(code => {
      if (!targeting[code]) {
        targeting[code] = {};
      }
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Will give it a try

@ptomasroos
Copy link
Contributor Author

ptomasroos commented May 30, 2018

Updated, all tests run as expect @snapwich
Thanks for the feedback

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM

@snapwich snapwich added the needs 2nd review Core module updates require two approvals from the core team label May 31, 2018
@jaiminpanchal27 jaiminpanchal27 merged commit 6fbc404 into prebid:master Jun 5, 2018
Pupis pushed a commit to adform/Prebid.js that referenced this pull request Jun 7, 2018
* Populate adUnitCode's even though there's no bid available

* Add new targeting test for no bids, and upgrade old implicit getAdserverTargeting calls

* Updated to a more simplex generation method
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* Populate adUnitCode's even though there's no bid available

* Add new targeting test for no bids, and upgrade old implicit getAdserverTargeting calls

* Updated to a more simplex generation method
StefanWallin pushed a commit to mittmedia/Prebid.js that referenced this pull request Sep 28, 2018
* Populate adUnitCode's even though there's no bid available

* Add new targeting test for no bids, and upgrade old implicit getAdserverTargeting calls

* Updated to a more simplex generation method
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* Populate adUnitCode's even though there's no bid available

* Add new targeting test for no bids, and upgrade old implicit getAdserverTargeting calls

* Updated to a more simplex generation method
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