PBS Bid Adapter: allow pbs stored impression configuration

[muuki88]

Type of change

• Feature

Description of change

The prebid server java implementation allows [stored impressions|https://github.com/prebid/prebid-server-java/blob/master/docs/developers/stored-requests.md] to be configured. This change allows publishers to
enable stored impressions per ad unit.

On your prebid server you have a stored impression with id content_pos1

{
  "id": "content_pos1",
  "banner": {
    "format": [
      { "w": 300, "h": 250  },
      { "w": 300, "h": 600 }
    ]
  },
  "ext": {
    "prebid": {
      "bidder": {
        "appnexus": {
          "placement_id": 10433394
        }
      }
    }
  }
}

Then you could configure your ad unit like this

const adUnit = {
    adUnit: {
      code: 'content_pos1',
      pbs: {
         // 'true' will use the adUnit.code . You could also supply a string to set a different id 
         storedImp: true
      },
      mediaTypes: {
        banner: { sizes: [ [300, 250], [300,600] ] }
      },
      bids: [   ]
    }
}

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

• A link to a PR on the docs repo at https://github.com/prebid/prebid.github.io/

Open questions

• will this work with no bids configured?
• which sizes will be used - server side or the ones sent by the client?
• is there any test for the prebidServerAdapter?
• will floor prices work?

Relates to #6361

[bretg]

No changes are needed to the PBS bid adapter or overall syntax-- with the new ortb2 method of First Party Data, this is already possible:

AdUnit-specific SRID:

pbjs.addAdUnits({
    code: "test-div",
    mediaTypes: {
        banner: {
            sizes: [[300,250]]
        }
    },
    ortb2Imp: {
        ext: {
            prebid: {
	        storedrequest: {
                    id: "123"
            }
        }
    },
    ...
});

Request-level SRID (price-granularity, etc)

pbjs.setConfig({
    s2sConfig: {
        ...
        extPrebid: {
            storedrequest: {
                id: "456"
            }
        }
    }
});

The existing pbsBidAdapter should already merge these into the right place in the ortb going to PBS.

However, I haven't tested this, and PBJS may not like there being no bids element on an adunit. Would be great if you had time to try that and note what breaks.

[muuki88]

Thanks @bretg for the tip. I'll try to make it work.

However from the server adapter it looks like the path needs to be called ortb2Imp.ext.data not ortb2Imp.ext.prebid

Prebid.js/modules/prebidServerBidAdapter/index.js

Line 618 in 108a3a6

const ortb2 = {...utils.deepAccess(adUnit, 'ortb2Imp.ext.data')};

[bretg]

from the server adapter it looks like the path needs to be called ortb2Imp.ext.data

Good catch. The intention was that pbsBidAdapter would merge ortb2Imp at the ext level.

[muuki88]

Should I fix that in this PR? Because if the ortb2 property works, there's no need for the code change I made in this PR 😂

[bretg]

I'd say yes - that would be great.

[muuki88]

I can confirm that with the changes in this pull request and the code example from @bretg the stored impression feature works. I'll still need to check with an empty bids array.

Regarding my changes. Will this break anything? I moved everything to the prebid scope, which is easy to implement, but can cause unwanted effects on someone already relying on ext.data. Options I see

• Add additional ext.prebid behaviour and keep the ext.data logic as is
• make the ext.{???} property configurable

@ChrisHuie As much as I would like to add tests for this, I'm not sure if I'm up to the task ATM 😬 Adapting or extending a test is one thing, but without types and no adapter to copy from this is quite a task I'm not sure I have the time for right now.

[bretg]

We can't get rid of ext.data. Need to look at the whole ext tree:

const ortb2 = {...utils.deepAccess(adUnit, 'ortb2Imp.ext')};

I imagine this might have consquences on the code below.

[muuki88]

I managed to extend the ext object with very little code changes and keeping the old logic mostly the same

[muuki88]

I found the test 😂 However it seems that they rely on some side effect happning in another test as

gulp test --file "test/spec/modules/prebidServerBidAdapter_spec.js" --watch

SUMMARY: ✔ 69 tests completed ✖ 17 tests failed

FAILED TESTS: S2S Adapter request function ✖ adds appnexus aliases to request HeadlessChrome 89.0.4389 (Linux 0.0.0) AssertionError: expected { Object (auctiontimestamp, targeting) } to have deep property 'aliases'

  + expected - actual

  
at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:1053:45 <- test/spec/modules/prebidServerBidAdapter_spec.js:8832:91)


  ✖ adds dynamic aliases to request
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  TypeError: pbjs.aliasBidder is not a function
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:1078:25 <- test/spec/modules/prebidServerBidAdapter_spec.js:8857:12)

  ✖ skips dynamic aliases to request when skipPbsAliasing enabled
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  TypeError: pbjs.aliasBidder is not a function
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:1140:25 <- test/spec/modules/prebidServerBidAdapter_spec.js:8916:12)

  ✖ converts appnexus params to expected format for PBS
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  AssertionError: expected undefined to exist

  + expected - actual

  
at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:1173:7 <- test/spec/modules/prebidServerBidAdapter_spec.js:8948:107)


  ✖ when config 'currency.adServerCurrency' value is a string: ORTB has property 'cur' value set to a single item array
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  TypeError: Cannot read property 'requestBody' of undefined
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:1385:63 <- test/spec/modules/prebidServerBidAdapter_spec.js:9183:120)

  gdpr tests
    ✖ "after each" hook for "adds gdpr consent information to ortb2 request depending on presence of module"
      HeadlessChrome 89.0.4389 (Linux 0.0.0)
    TypeError: Cannot read property 'removeAll' of undefined
        at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:536:39 <- test/spec/modules/prebidServerBidAdapter_spec.js:8328:26)

  us_privacy (ccpa) consent data
    ✖ "after each" hook for "is added to ortb2 request when in bidRequest"
      HeadlessChrome 89.0.4389 (Linux 0.0.0)
    TypeError: Cannot read property 'removeAll' of undefined
        at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:668:39 <- test/spec/modules/prebidServerBidAdapter_spec.js:8455:26)

  gdpr and us_privacy (ccpa) consent data
    ✖ "after each" hook for "is added to ortb2 request when in bidRequest"
      HeadlessChrome 89.0.4389 (Linux 0.0.0)
    TypeError: Cannot read property 'removeAll' of undefined
        at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:713:39 <- test/spec/modules/prebidServerBidAdapter_spec.js:8493:26)

  price floors module
    ✖ should correctly pass adServerCurrency when set to getFloor not default
      HeadlessChrome 89.0.4389 (Linux 0.0.0)
    TypeError: Cannot read property 'requestBody' of undefined
        at runTest (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:836:69 <- test/spec/modules/prebidServerBidAdapter_spec.js:8619:126)
        at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:918:9 <- test/spec/modules/prebidServerBidAdapter_spec.js:8708:9)

response handler
  ✖ handles OpenRTB video responses
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  AssertError: expected spy to be called once but was called 0 times
      at Object.fail (node_modules/sinon/pkg/sinon.js:142:21)
      at failAssertion (node_modules/sinon/pkg/sinon.js:101:16)
      at Object.assert.<computed> [as calledOnce] (node_modules/sinon/pkg/sinon.js:126:13)
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:2033:20 <- test/spec/modules/prebidServerBidAdapter_spec.js:9826:20)

  ✖ handles response cache from ext.prebid.cache.vastXml
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  AssertError: expected spy to be called once but was called 0 times
      at Object.fail (node_modules/sinon/pkg/sinon.js:142:21)
      at failAssertion (node_modules/sinon/pkg/sinon.js:101:16)
      at Object.assert.<computed> [as calledOnce] (node_modules/sinon/pkg/sinon.js:126:13)
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:2064:20 <- test/spec/modules/prebidServerBidAdapter_spec.js:9856:20)

  ✖ add adserverTargeting object to bids when ext.prebid.targeting is defined
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  AssertError: expected spy to be called once but was called 0 times
      at Object.fail (node_modules/sinon/pkg/sinon.js:142:21)
      at failAssertion (node_modules/sinon/pkg/sinon.js:101:16)
      at Object.assert.<computed> [as calledOnce] (node_modules/sinon/pkg/sinon.js:126:13)
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:2093:20 <- test/spec/modules/prebidServerBidAdapter_spec.js:9882:20)

  ✖ handles response cache from ext.prebid.targeting
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  AssertError: expected spy to be called once but was called 0 times
      at Object.fail (node_modules/sinon/pkg/sinon.js:142:21)
      at failAssertion (node_modules/sinon/pkg/sinon.js:101:16)
      at Object.assert.<computed> [as calledOnce] (node_modules/sinon/pkg/sinon.js:126:13)
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:2123:20 <- test/spec/modules/prebidServerBidAdapter_spec.js:9910:20)

  ✖ handles response cache from ext.prebid.targeting with wurl
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  AssertError: expected spy to be called once but was called 0 times
      at Object.fail (node_modules/sinon/pkg/sinon.js:142:21)
      at failAssertion (node_modules/sinon/pkg/sinon.js:101:16)
      at Object.assert.<computed> [as calledOnce] (node_modules/sinon/pkg/sinon.js:126:13)
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:2153:20 <- test/spec/modules/prebidServerBidAdapter_spec.js:9939:20)

  ✖ handles response cache from ext.prebid.targeting with wurl and removes invalid targeting
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  AssertError: expected spy to be called once but was called 0 times
      at Object.fail (node_modules/sinon/pkg/sinon.js:142:21)
      at failAssertion (node_modules/sinon/pkg/sinon.js:101:16)
      at Object.assert.<computed> [as calledOnce] (node_modules/sinon/pkg/sinon.js:126:13)
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:2183:20 <- test/spec/modules/prebidServerBidAdapter_spec.js:9968:20)

  ✖ add request property pbsBidId with ext.prebid.bidid value
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  AssertError: expected spy to be called once but was called 0 times
      at Object.fail (node_modules/sinon/pkg/sinon.js:142:21)
      at failAssertion (node_modules/sinon/pkg/sinon.js:101:16)
      at Object.assert.<computed> [as calledOnce] (node_modules/sinon/pkg/sinon.js:126:13)
      at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:2206:20 <- test/spec/modules/prebidServerBidAdapter_spec.js:9989:20)

s2sConfig
  ✖ should set correct bidder names to bidders property when using an alias for that bidder
    HeadlessChrome 89.0.4389 (Linux 0.0.0)
  AssertionError: expected [ 'appnexus', 'rubicon-alias' ] to deeply equal [ 'appnexus', 'rubicon' ]

  + expected - actual

   [
     "appnexus"
  -  "rubicon-alias"
  +  "rubicon"
   ]
  
at Context.<anonymous> (webpack:///test/spec/modules/prebidServerBidAdapter_spec.js:2589:42 <- test/spec/modules/prebidServerBidAdapter_spec.js:10323:88)

I'll update the PR with the changes requested by @bretg and @ChrisHuie

[muuki88]

This feels wrong, but I guess there's a reason why empty string pbadslot entries are being filtered.

[bretg]

It's being deleted if it's not a string. I don't recall pushing for that validation, but it's ok with me.

[ChrisHuie]

@bretg @muuki88 -> would it make sense here to try and convert what is passed to a string first and if that isn't possible then move to delete it? Or is it always going to be a string if it's passed and the only other data types you could get would be more complex ones like arrays, objects, and etc...

[muuki88]

I guess it should always be a string and the implementation wanted to make sure nothing else will be sent to the server so it doesn't crash.

Not sure if this kind of validation should happen here.

[bretg]

I think this is ok as-is.

[muuki88]

The second tests looks like my change could be responsible

1) should build utm data from local storage
     Prebid Manager Analytics Adapter build utm tag data
     Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

2) should allow creation of a fpd.context.pbAdSlot property on adUnits from inside the event handler
     Unit: Prebid Module onEvent beforeRequestBids
     Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

[bretg]

fpd.context.pbAdSlot

For the record, that path is now obsolete. Should be looking at adunit.ortb2Imp.ext.data.pbadslot.

[muuki88]

Is it possible to restart the circle ci tests and see if they were flaky?

[bretg]

@mmoschovas - please check to see if this PR overlaps with the work you're doing for #6528