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

Targeting filtering API is broken and mismatched #1217

Closed
snapwich opened this issue May 22, 2017 · 1 comment · Fixed by #1220
Closed

Targeting filtering API is broken and mismatched #1217

snapwich opened this issue May 22, 2017 · 1 comment · Fixed by #1220
Assignees

Comments

@snapwich
Copy link
Collaborator

Type of issue

Bug

Description

Sometimes the targeting is filtering on a single ad unit code, sometimes on an array of codes, and the implementation seems to switch between those two expectations in a broken fashion. I tried to fix this myself but I'm not sure if API changes would be required to fix these mismatches.

Here targeting.getAllTargeting is called with a single adUnit.
Here targeting.getAllTargeting is called with an array of adUnit codes (according to the documentation).
Here target.getAllTargeting expects undefined or a single adUnit code. So either this code needs to be updated to allow an array, or the documentation needs to be updated to only allow a single adUnit code.

Here getWinningBidTargeting is passed the new array of adUnitCodes, but getWinningBidTargeting expects no arguments (it's possible getWinningBidTargeting could be updated to pass through the adUnitCodes it is given to targeting.getWinningBids call, but targeting.getWinningBids expects a single adUnit for filtering, not the array it would be given)

Here targeting.resetPresetTargeting is operating on either pbjs._adUnitCodes or the remapped result of targeting.getAllTargeting (which I think is erroneously placed in an array) which are fundamentally different structures (an array of adUnit code strings vs an array of targeting objects [I think]).

Solution

It seems to me in order to solve this, we need to refactor all the targeting code around either expecting an array of adUnitCodes or a single adUnitCode and then update the documentation accordingly? I'm not quite sure which to favor, probably the array of adUnitCodes? (which would require the larger refactor)

Also, a lot of this code is very confusing to read as well; such as using .map or .find to loop through data structures and returning nothing or throwing away the results while mutating, which makes it really hard to follow what kind of data structures we're actually working with in the end. We might benefit from rewriting this code in a clearer fashion.

Other information

#1158 brought the issue to light (failed some of our internal testing) and I think doubled down on some of the bugs that I think might have already existed. I have suggested internally that Rubicon not update to 0.24 because I think this pull-request specifically broke some of our implementations.

@bretg
Copy link
Collaborator

bretg commented May 22, 2017

FWIW - we found this while testing 0.24. The case we caught it with was around refreshes -- when there's a refresh, the adid from the first auction is reported instead of the new auction. That old adid has timed out by then, so a blank is served instead.

From Rich's findings above, sounds like there may be other ways for this issue to surface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants