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

Include gdpr TripleLift #2663

Merged
merged 19 commits into from
Jul 10, 2018
Merged

Conversation

brittanyzellman
Copy link
Contributor

@brittanyzellman brittanyzellman commented May 31, 2018

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Upgraded to 1.12 to include SRA capability and GDPR compliance module.

@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging 45efe0e into 476cad8 - view on lgtm.com

new alerts:

  • 1 for Useless conditional

Comment posted by lgtm.com

@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging 2ebff0c into 476cad8 - view on lgtm.com

new alerts:

  • 1 for Useless conditional

Comment posted by lgtm.com

@brittanyzellman
Copy link
Contributor Author

@mkendall07 I would like to reach out various publishers with a timeline of when they can expect the adapter to be deployed. Pending all tests/approval, when can I expect a merge and deploy?
Thanks!

@@ -0,0 +1,186 @@
<html>
Copy link
Member

Choose a reason for hiding this comment

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

please remove this file. We already have a gdpr hello world example.

@mkendall07 mkendall07 changed the title Include gdpr tl Include gdpr TripleLift May 31, 2018
@YOzaz
Copy link

YOzaz commented Jun 1, 2018

We're a publisher, and waiting for TripleLift to be merged... hope it'll be today?

@ehoch
Copy link
Contributor

ehoch commented Jun 2, 2018

@brittanyzellman This is causing

``Error processing command : Cannot read property 'gdprApplies' of undefined TypeError: Cannot read property 'gdprApplies' of undefined```

If the consent module is included, but not configured (like we have in the case of our non-EU traffic). Can you add a check for existence of bidderRequest.gdprConsent ?

@brittanyzellman
Copy link
Contributor Author

@mkendall07 checking back in here for any timeline info you can give us? We have publishers asking and aren't quite sure what to tell them to expect in terms of deployment.

@ptim
Copy link
Contributor

ptim commented Jun 12, 2018

@brittanyzellman not sure, but merge may be waiting on removal of integrationExamples/gpt/custom.html, per:

please remove this file. We already have a gdpr hello world example.

#2663 (comment)

@brittanyzellman
Copy link
Contributor Author

All irrelevant files are deleted and adapter passes all checks. Still waiting for a timeline as to when publishers can expect this to be merged and deployed? @mkendall07

@ptim
Copy link
Contributor

ptim commented Jun 15, 2018

I'm a pub using triplelift - can I request some documentation for configuring native ad units under Prebid v1?

It appears that it's necessary to send bid requests for our native ad slots as mediaType.banner with a [1,1] size rather than using the native ad configuration... I only discovered this via experimentation - can't see it documented anywhere (there's no readme for this adapter, and no entry in the prebid docs repo).

TIA

@brittanyzellman
Copy link
Contributor Author

@ptim please reach out to your point of contact at Triplelift. They will have all relevant documentation on how to integrate. I'm assuming that you are already integrated with us through Prebid?

@YOzaz
Copy link

YOzaz commented Jun 18, 2018

@ptim Triplelift does not support "real" pbjs native bidding, but rather is a wrapper for [1,1] auto-expanding ad. Same is for Sharethrough, for example.

@brittanyzellman
Copy link
Contributor Author

@mkendall07 reaching out again for a timeline on getting this merged and deployed... if there is someone else I should add as a reviewer and call out here I'm happy to do so. Please let me know..

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

@brittanyzellman
You need to include a test spec file and a bidder markdown file. Please see http://prebid.org/dev-docs/bidder-adaptor.html#adding-unit-tests for example.

@brittanyzellman
Copy link
Contributor Author

@mkendall07 any word on when we can expect this to be merged/deployed?

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

Just a few small updates needed. Overall LGTM. Thanks!
Also your at 79.31% unit test coverage which is slightly short but I'll let it slide :)

floor: 1.009
}
},{
bidder: 'appnexus',
Copy link
Member

Choose a reason for hiding this comment

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

any reason you included an AppNexus bid in your test?


const BIDDER_CODE = 'triplelift';
const STR_ENDPOINT = document.location.protocol + '//tlx.3lift.com/header/auction?';
var applies = true;
Copy link
Member

Choose a reason for hiding this comment

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

prefer let over var

Copy link
Member

Choose a reason for hiding this comment

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

a better name here would be gdprApplies


code: BIDDER_CODE,
supportedMediaTypes: [BANNER],
aliases: ['triplelift'],
Copy link
Member

Choose a reason for hiding this comment

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

not necessary to alias your own code.

@mkendall07
Copy link
Member

@brittanyzellman
See comments above.

@brittanyzellman
Copy link
Contributor Author

brittanyzellman commented Jul 10, 2018

@mkendall07 all changes are made, I had AN in the sample for testing just so to see whether other bidders were able to load and bid but I've removed it from the md file.
Thanks!

@mkendall07
Copy link
Member

@brittanyzellman
Thanks for the changes. One last thing, please open a PR to add

prebid_1_0_supported : true
gdpr_supported: true

To the header of this file:
https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/triplelift.md

@mkendall07
Copy link
Member

@brittanyzellman
also please see: #2833

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants