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

Update Rubicon Adapter with SRA-GET support (HB-2145) #13

Closed
wants to merge 65 commits into from

Conversation

idettman
Copy link

@idettman idettman commented Feb 13, 2018

IMPORTANT! Do not merge, for code review only

Type of change

  • Feature

Description of change

Adds "Single Request Architecture" (SRA) support to the Rubicon Adapter.

// By default, SRA mode is turned off

// Enable SRA with the following config change
pbjs.setConfig({
  rubicon: {
    singleRequest: true
  }
});
  • test parameters for validating bids
const exampleBids = [
{
  code: 'div-1',
  sizes: [[728, 90], [970, 90]],
  bids: [
    {
      bidder: 'rubicon',
      params: {
        accountId: "1234",
        siteId: "1234",
        zoneId: "1001"
    },
    {
      bidder: 'rubicon',
      params: {
        accountId: "1234",
        siteId: "1234",
        zoneId: "1002",
    },
    {
      bidder: 'rubicon',
      params: {
        accountId: "1234",
        siteId: "9876", // Note: <-- this siteId doesn't match the other two bids, so a second request is sent for bids with that 'siteId' value
        zoneId: "1003"
    }]
}];

Other information

HB-2145

Copy link

@harpere harpere left a comment

Choose a reason for hiding this comment

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

reviewed the code changes, still need to review the tests.

? memo + curr + '=' + encodeURIComponent(data[index + 1]) + '&' : memo,
''
).slice(0, -1); // remove trailing &
page_url = bidRequest.params.secure ? page_url.replace(/^http:/i, 'https:') : page_url;
Copy link

Choose a reason for hiding this comment

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

may want to put all of this page_url logic in a function since it's used here and for video slots.

// bids are not grouped if single request mode is not enabled
requests = videoRequests.concat(bidRequests.filter(bidRequest => bidRequest.mediaType !== 'video').map(bidRequest => {
const bidParams = spec.createSlotParams(bidRequest);
const combinedSlotParams = spec.combineSlotUrlParams([bidParams]);
Copy link

Choose a reason for hiding this comment

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

why are you calling combineSlotUrlParams() when this isn't single-request?

method: 'GET',
url: FASTLANE_ENDPOINT,
data: Object.keys(combinedSlotParams).reduce((paramString, key) => {
return (typeof combinedSlotParams[key] === 'string' || combinedSlotParams[key] !== '') ? `${paramString}${key}=${encodeURIComponent(combinedSlotParams[key])}&` : paramString;
Copy link

Choose a reason for hiding this comment

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

what if the parameter is a number, like floor for example?

Copy link

Choose a reason for hiding this comment

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

also, wouldn't this be an && rather than an ||?

method: 'GET',
url: FASTLANE_ENDPOINT,
data: Object.keys(combinedSlotParams).reduce((paramString, key) => {
return (typeof combinedSlotParams[key] === 'string' || combinedSlotParams[key] !== '') ? `${paramString}${key}=${encodeURIComponent(combinedSlotParams[key])}&` : paramString;
Copy link

Choose a reason for hiding this comment

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

same comments as earlier

'dt.id': digiTrustId.id,
'dt.keyv': digiTrustId.keyv,
'dt.pref': 0
};
Copy link

Choose a reason for hiding this comment

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

did this need to be changed? was it not working previously?

Copy link
Author

@idettman idettman Feb 16, 2018

Choose a reason for hiding this comment

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

It was working previously, but the get param object properties were being stored in an array (opposed to plain object) to honor ordering of parameters, but since order does not matter, I'm skipping the step of using an array. I converted this to return an object for simpler key value assignment to the param object

@idettman idettman changed the base branch from rubicon_adapter_sra_support to master February 20, 2018 17:52
@idettman idettman changed the title SRA-GET support for Rubicon bid adapter (HB-2145) Update Rubicon Adapter with SRA-GET support - release/1.5 (HB-2145) Feb 20, 2018
@idettman idettman changed the title Update Rubicon Adapter with SRA-GET support - release/1.5 (HB-2145) Update Rubicon Adapter with SRA-GET support - release/0.34.5 (HB-2145) Feb 20, 2018
@@ -422,6 +449,93 @@ describe('the rubicon adapter', () => {
expect(window.DigiTrust.getUser.calledOnce).to.equal(true);
});
});

describe('singleRequest config', () => {
Copy link

Choose a reason for hiding this comment

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

tests confirming requests are combined as expected are good, but should have some tests verifying the params are combined into the grouped request as expected, and that the bidRequest param is an array of all the grouped requests.

{p2: 'test'}
])).to.deep.equal({p1: 'foo;foo;', p2: 'test;bar;test', p3: 'baz;;'});
});
});
});

describe('interpretResponse', () => {
Copy link

Choose a reason for hiding this comment

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

should be some new tests for SRA responses.

Copy link

@harpere harpere left a comment

Choose a reason for hiding this comment

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

new tests LGTM. thanks.

@idettman idettman changed the title Update Rubicon Adapter with SRA-GET support - release/0.34.5 (HB-2145) Update Rubicon Adapter with SRA-GET support (HB-2145) Mar 6, 2018
@snapwich snapwich closed this Jun 8, 2018
@idettman idettman deleted the rubicon-adapter-sra-support branch March 18, 2019 18:34
robertrmartinez pushed a commit that referenced this pull request May 22, 2023
…ync (prebid#9700)

* collect EIDs for bid request

* add ad slot positioning to payload

* RPO-2012: Update local storage name-spacing for c_uid (#8)

* Updates c_uid namespacing to be more specific for concert

* fixes unit tests

* remove console.log

* RPO-2012: Add check for shared id (#9)

* Adds check for sharedId

* Updates cookie name

* remove trailing comma

* [RPO-3152] Enable Support for GPP Consent (#12)

* Adds gpp consent integration to concert bid adapter

* Update tests to check for gpp consent string param

* removes user sync endpoint and tests

* updates comment

* cleans up consentAllowsPpid function

* comment fix

* rename variables for clarity

* fixes conditional logic for consent allows function (#13)

---------

Co-authored-by: antoin <antoin.campbell@voxmedia.com>
Co-authored-by: Antoin <antoinfive@gmail.com>
robertrmartinez pushed a commit that referenced this pull request May 22, 2023
)

* collect EIDs for bid request

* add ad slot positioning to payload

* RPO-2012: Update local storage name-spacing for c_uid (#8)

* Updates c_uid namespacing to be more specific for concert

* fixes unit tests

* remove console.log

* RPO-2012: Add check for shared id (#9)

* Adds check for sharedId

* Updates cookie name

* remove trailing comma

* [RPO-3152] Enable Support for GPP Consent (#12)

* Adds gpp consent integration to concert bid adapter

* Update tests to check for gpp consent string param

* removes user sync endpoint and tests

* updates comment

* cleans up consentAllowsPpid function

* comment fix

* rename variables for clarity

* fixes conditional logic for consent allows function (#13)

* [RPO-3262] Update getUid function to check for pubcid and sharedid (#14)

* Update getUid function to check for pubcid and sharedid

* updates adapter version

---------

Co-authored-by: antoin <antoin.campbell@voxmedia.com>
Co-authored-by: Antoin <antoinfive@gmail.com>
musikele pushed a commit that referenced this pull request Aug 28, 2023
…ebid#10356)

* collect EIDs for bid request

* add ad slot positioning to payload

* RPO-2012: Update local storage name-spacing for c_uid (#8)

* Updates c_uid namespacing to be more specific for concert

* fixes unit tests

* remove console.log

* RPO-2012: Add check for shared id (#9)

* Adds check for sharedId

* Updates cookie name

* remove trailing comma

* [RPO-3152] Enable Support for GPP Consent (#12)

* Adds gpp consent integration to concert bid adapter

* Update tests to check for gpp consent string param

* removes user sync endpoint and tests

* updates comment

* cleans up consentAllowsPpid function

* comment fix

* rename variables for clarity

* fixes conditional logic for consent allows function (#13)

* [RPO-3262] Update getUid function to check for pubcid and sharedid (#14)

* Update getUid function to check for pubcid and sharedid

* updates adapter version

* [RPO-3405] Add browserLanguage to request meta object

---------

Co-authored-by: antoin <antoin.campbell@voxmedia.com>
Co-authored-by: Antoin <antoinfive@gmail.com>
Co-authored-by: Brett Bloxom <38990705+BrettBlox@users.noreply.github.com>
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 this pull request may close these issues.

3 participants