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

Yieldbot adunit bidder params slot name usage fix #1394

Merged
merged 2 commits into from
Aug 31, 2017
Merged

Yieldbot adunit bidder params slot name usage fix #1394

merged 2 commits into from
Aug 31, 2017

Conversation

elljoh
Copy link
Contributor

@elljoh elljoh commented Jul 19, 2017

Type of change

  • Bugfix
  • Example

Description of change

Resolves #1392

AdUnit configuration that uses the same slot name for the Yieldbot bidder params, i.e. multiple occurrences such as the below, results in erroneous bidmanager.addBidResponse(...) calls.

          {
              bidder: 'yieldbot',
              params: {
                  psn: '1234',
                  slot: 'test_slot'
              }
          }

See Issue #1392 for more details.

Other information

Resolves #1392

@jaiminpanchal27
Copy link
Collaborator

@elljoh You have conflicts. Please rebase with master

@elljoh elljoh force-pushed the yieldbot-multi-slot-sizes branch from e3068bf to e1112db Compare July 25, 2017 16:47
@elljoh
Copy link
Contributor Author

elljoh commented Jul 28, 2017

@jaiminpanchal27 heya, rebased to master.

@elljoh
Copy link
Contributor Author

elljoh commented Jul 31, 2017

@jaiminpanchal27 lmk if there is anything else needed here. thanks!

@jaiminpanchal27
Copy link
Collaborator

@elljoh Will check it today. Thanks

@jaiminpanchal27 jaiminpanchal27 self-assigned this Jul 31, 2017
jaiminpanchal27
jaiminpanchal27 requested changes Jul 31, 2017
View reviewed changes
Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 left a comment

Choose a reason for hiding this comment

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

@elljoh Left some comments.

test/spec/modules/yieldbotBidAdapter_spec.js Outdated
}

function removeBids(bidder) {
if (window && window.pbjs) {
window.pbjs._bidsRequested = window.pbjs._bidsRequested.filter(o => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should not touch pbjs private variables. This can break other tests.

test/spec/modules/yieldbotBidAdapter_spec.js
expect(sizes).to.deep.equal([]);
});

it('should return [] for function sizes', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplicate test..

test/spec/modules/yieldbotBidAdapter_spec.js Outdated
const adapter = new YieldbotAdapter();
adapter.callBids(bidderRequest);
mockYieldbotBidRequest();
removeBids('yieldbot');

let bidResponses = window.$$PREBID_GLOBAL$$._bidsReceived.filter(o => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should not access pbjs private vars. you can test this by stubbing bidmanager.addBidResponse

@jaiminpanchal27
Copy link
Collaborator

@elljoh Any update ?

@elljoh
Copy link
Contributor Author

elljoh commented Aug 10, 2017

@jaiminpanchal27 apologies, I'm out of the office until Monday 20170814. Will address your comments then.

Thank you.

@elljoh
Copy link
Contributor Author

elljoh commented Aug 29, 2017

@jaiminpanchal27 hi. anything remain for this pull request to merge?

@jaiminpanchal27
Copy link
Collaborator

@elljoh We require 2 LGTM's for PR to merge. I have approved this so someone should take it. Let me assign someone so that we get this in. Thanks

@dbemiller dbemiller merged commit f8d7dff into prebid:master Aug 31, 2017
ptomasroos pushed a commit to happypancake/Prebid.js that referenced this pull request Sep 1, 2017
@elljoh @ptomasroos
Yieldbot adunit bidder params slot name usage fix (prebid#1394)
9a07d0d 
* Yieldbot adunit bidder params slot name usage fix

* Yieldbot adunit bidder params slot name usage fix, PR feedback++
philipwatson pushed a commit to mbrtargeting/Prebid.js that referenced this pull request Sep 18, 2017
@elljoh @philipwatson
Yieldbot adunit bidder params slot name usage fix (prebid#1394)
5db373f 
* Yieldbot adunit bidder params slot name usage fix

* Yieldbot adunit bidder params slot name usage fix, PR feedback++
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Sep 18, 2017
Merge tag '0.28.0' of https://github.com/prebid/Prebid.js into prebuilt
da960cc 
* tag '0.28.0' of https://github.com/prebid/Prebid.js: (27 commits)
  Prebid 0.28.0 Release
  Revert "Upgrade sinon to 3.x (prebid#1491)" (prebid#1563)
  add () for correct order of operations in scaling increments for currency (prebid#1559)
  AppnexusAst adapter update: Added source and version to request payload (prebid#1555)
  remove unnecessary spread operator (prebid#1561)
  Adxcg adapter (prebid#1554)
  Upgrade sinon to 3.x (prebid#1491)
  Rename vastPayload to vastXml (prebid#1556)
  Single-size sizes array now can be taken, too (prebid#1535)
  Updated the istanbul-instrumenter-loader (prebid#1550)
  Add AerServ Adapter (prebid#1538)
  Fixed imports and made adform support aliasing (prebid#1518)
  Custom granularity fix (prebid#1546)
  Fix `documentation lint` issues (prebid#1544)
  Yieldbot adunit bidder params slot name usage fix (prebid#1394)
  Update serverbid adapter to use smartsync (prebid#1324)
  Add improvedigitalBidAdapter (prebid#1381)
  Fix prebid#1533 spring server typo (prebid#1542)
  userSync is off by default (prebid#1543)
  currency module (prebid#1374)
  ...
jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017
@elljoh @jbAdyoulike
Yieldbot adunit bidder params slot name usage fix (prebid#1394)
ae5f642 
* Yieldbot adunit bidder params slot name usage fix

* Yieldbot adunit bidder params slot name usage fix, PR feedback++
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Oct 12, 2017
@vzhukovsky
Merge pull request prebid#121 in AOLP_ADS_JS/prebid.js from release/1…
e797d03 
….28.0 to aolgithub-master

* commit '4d9ade3df767750743f8888ed9efd6c77f8d0050': (26 commits)
  Added changelog entry.
  Added new aol partners ids.
  Prebid 0.28.0 Release
  Revert "Upgrade sinon to 3.x (prebid#1491)" (prebid#1563)
  add () for correct order of operations in scaling increments for currency (prebid#1559)
  AppnexusAst adapter update: Added source and version to request payload (prebid#1555)
  remove unnecessary spread operator (prebid#1561)
  Adxcg adapter (prebid#1554)
  Upgrade sinon to 3.x (prebid#1491)
  Rename vastPayload to vastXml (prebid#1556)
  Single-size sizes array now can be taken, too (prebid#1535)
  Updated the istanbul-instrumenter-loader (prebid#1550)
  Add AerServ Adapter (prebid#1538)
  Fixed imports and made adform support aliasing (prebid#1518)
  Custom granularity fix (prebid#1546)
  Fix `documentation lint` issues (prebid#1544)
  Yieldbot adunit bidder params slot name usage fix (prebid#1394)
  Update serverbid adapter to use smartsync (prebid#1324)
  Add improvedigitalBidAdapter (prebid#1381)
  Fix prebid#1533 spring server typo (prebid#1542)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
@elljoh @dluxemburg
Yieldbot adunit bidder params slot name usage fix (prebid#1394)
fde158a 
* Yieldbot adunit bidder params slot name usage fix

* Yieldbot adunit bidder params slot name usage fix, PR feedback++
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaiminpanchal27 jaiminpanchal27 jaiminpanchal27 approved these changes

@dbemiller dbemiller Awaiting requested review from dbemiller

Assignees

@jaiminpanchal27 jaiminpanchal27

@dbemiller dbemiller

Labels
LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Yieldbot, adapter param configs using the same slot name
4 participants
@elljoh @jaiminpanchal27 @mkendall07 @dbemiller