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

Yieldbot refresh - nextPageview with specific refresh slots #1281

Merged
merged 2 commits into from Jul 10, 2017

Conversation

Projects
None yet
5 participants
@elljoh
Contributor

elljoh commented Jun 8, 2017

Type of change

  • Bugfix

Description of change

Fixes #1255

callBids(params) after the first i.e. "refresh" bid requests will only include bids for params.bids.

Other information

@grevory

Small change requested, then approved.

if (yieldbot._initialized !== true) {
yieldbot.pub(psn);
for (var slotName in slots) {

This comment has been minimized.

@grevory

grevory Jun 20, 2017

Contributor

There should be a conditional here to check slots.hasOwnProperty(slotName)

This comment has been minimized.

@elljoh

elljoh Jun 27, 2017

Contributor

@grevory added the check for own property and a test for the check.

The object being checked is created var slots = {}; so, I expect pretty unlikely to be exposed to any injected properties causing an issue with iteration of the enumerable properties... However, it's been known that overriding Object in the wild is not implausible.

Please comment here if you were thinking of any specific issues you had in mind.

Understood that it's generally accepted good form to be defensive on inherited properties.

If you're ok with this, I can squash the fork and merge the fork with upstream master. lmk.

Thanks!

@elljoh

This comment has been minimized.

Contributor

elljoh commented Jun 28, 2017

ok. rebasing to master and running tests for the #1177 move to modules, before pushing again.

@elljoh

This comment has been minimized.

Contributor

elljoh commented Jun 29, 2017

@grevory lmk if there is anything else I can do.

/elljoh

@elljoh

This comment has been minimized.

Contributor

elljoh commented Jul 10, 2017

@mkendall07 any chance you or @grevory can look at this?

Cheers, /elljoh

@mkendall07

This comment has been minimized.

Collaborator

mkendall07 commented Jul 10, 2017

LGTM.

@mkendall07 mkendall07 merged commit 852a933 into prebid:master Jul 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mkendall07 mkendall07 removed the in progress label Jul 10, 2017

jbAdyoulike added a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017

Yieldbot refresh - nextPageview with specific refresh slots (prebid#1281
)

* Yieldbot refresh - nextPageview with argument for specific refresh slots to bid on

* Yieldbot refresh - nextPageview with argument. slots own property

dluxemburg added a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018

Yieldbot refresh - nextPageview with specific refresh slots (prebid#1281
)

* Yieldbot refresh - nextPageview with argument for specific refresh slots to bid on

* Yieldbot refresh - nextPageview with argument. slots own property
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment