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

Teads Bid Adapter: support floc and uid2 user IDs #7116

Merged
merged 3 commits into from
Aug 2, 2021

Conversation

ruizb
Copy link
Contributor

@ruizb ruizb commented Jun 28, 2021

Type of change

  • Feature

Description of change

Support FLoC ID (flocId) and Unified ID v2 (uid2Id) user IDs in the Teads adapter.

Additionally, read the _tfpvi first-party cookie when available on the website's domain.

Documentation has been updated on the prebid.github.io repository: prebid/prebid.github.io#3072.

Contact email of the adapter’s maintainer : innov-ssp@teads.tv.

@ruizb ruizb marked this pull request as ready for review June 28, 2021 16:25
@ChrisHuie ChrisHuie changed the title Teads adapter: support some user IDs Teads adapter: support floc and uid2 user IDs Jun 30, 2021
@ChrisHuie ChrisHuie changed the title Teads adapter: support floc and uid2 user IDs Teads Bid Adapter: support floc and uid2 user IDs Jun 30, 2021
const BIDDER_CODE = 'teads';
const GVL_ID = 132;
const ENDPOINT_URL = 'https://a.teads.tv/hb/bid-request';
const ENDPOINT_URL = '//SSP_PORT_8080_TCP_ADDR:SSP_PORT_8080_TCP_PORT/hb/bid-request';
Copy link
Collaborator

Choose a reason for hiding this comment

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

What exactly is going on here?

This looks like some macro to be replaced but I do not see it happening anywhere in your adapter?

SSP_PORT_8080_TCP_ADDR:SSP_PORT_8080_TCP_PORT

Copy link
Contributor

@nailyk nailyk Jul 16, 2021

Choose a reason for hiding this comment

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

yes it's an error on our side, we will revert that change! (edit: fixed in 2nd commit)

* @returns `{} | {firstPartyCookieTeadsId: string}`
*/
function getFirstPartyTeadsIdParameter() {
const storage = getStorageManager(GVL_ID, BIDDER_CODE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think it would be better to just init your storage manager top level of module

You are calling this function every single time anyways.

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

couple questions

@Viewtiful
Copy link
Contributor

Hello @robertrmartinez , we have done the changes following your questions.
Could you please have another look and review again the PR when you have the time to do so?
Thank you !

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Adapter looks good but would like the storage code to be stubbed in the tests.

We are trying to be more diligent about not letting people modify the window during tests, setting local storage and cookies is not reliable, especially when you have many adapters doing so.

Please see the examples I just gave and update the spec file.

Thanks, let me know if you have any questions

});

it('should add firstPartyCookieTeadsId param to payload if first-party cookie is available', function () {
storage.setCookie('_tfpvi', 'my-teads-id');
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not want to actually be setting and reading from storage / cookies inside of tests.

We should be mocking the stubbing / mocking the storage manager, which has its own tests to verify it works.

Here is an example:

You'll need to export your storage var so that your tests can mock it:
https://github.com/prebid/Prebid.js/blob/master/modules/rubiconAnalyticsAdapter.js#L11

Then in your beforeEach you should setup the stub
https://github.com/prebid/Prebid.js/blob/master/test/spec/modules/rubiconAnalyticsAdapter_spec.js#L596-L600

Don't forget to restore it back in the afterEach
https://github.com/prebid/Prebid.js/blob/master/test/spec/modules/rubiconAnalyticsAdapter_spec.js#L629-L631

Then in your tests you can set them up to return what you want and make sure your module works as expected:
https://github.com/prebid/Prebid.js/blob/master/test/spec/modules/rubiconAnalyticsAdapter_spec.js#L1297

// Following the introduction of tests involving reading/writing cookies,
// this allows for running this spec as a single file with:
// `gulp test --file "test/spec/modules/teadsBidAdapter_spec.js"`.
window.$$PREBID_GLOBAL$$.processQueue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the stubbing of storage, you can get rid of this I believe.

This type of code in test files can cause downstream tests to be very flakey.

We do not want to modify the window object and its contents if we do not have to in tests.

@ruizb
Copy link
Contributor Author

ruizb commented Aug 2, 2021

Thanks @robertrmartinez, it's so much better/stable to use a stub rather than the global environment! 👍

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Awesome stuff

Thanks for being so flexible!

@robertrmartinez robertrmartinez merged commit 665ffba into prebid:master Aug 2, 2021
prebidtappx pushed a commit to prebidtappx/Prebid.js that referenced this pull request Nov 15, 2021
* Teads adapter: support some user IDs

* review changes

* Stub access to storage functions in tests of Teads adapter

Co-authored-by: Kylian Deau <kylian.deau@teads.tv>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants