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

Add support for the 2 gdpr related fields on bids #2549

Closed
wants to merge 1 commit into from

Conversation

erolosty
Copy link

@erolosty erolosty commented May 16, 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

Adding gdpr support to the quantcast bidder adapter

  • test parameters for validating bids
{
    bidder: 'quantcast',
    params: {
        publisherId: 'test-publisher', // REQUIRED - Publisher ID provided by Quantcast
        battr: [1, 2] // OPTIONAL - Array of blocked creative attributes as per OpenRTB Spec List 5.3
    }
}

Be sure to test the integration with your adserver using the Hello World sample page.

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

Other information

let gdprSignal;
let gdprConsentString;

if (bid.gdprConsent !== undefined) {
Copy link

Choose a reason for hiding this comment

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

How about just check if (bid.gdprConsent), to avoid "Cannot read property of null"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally to @aodinok suggestion, the gdprConsent object doesn't exist in the bidRequests but rather in the bidderRequest object. This additional variable needs to be added to the buildRequests() function call.

This would naturally affect the remaining GDPR related code below.

@@ -94,6 +105,8 @@ export const spec = {
referrer,
domain
},
gdprSignal: gdprSignal,
Copy link

@aodinok aodinok May 16, 2018

Choose a reason for hiding this comment

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

This line can be just: gdprSignal,

@erolosty
Copy link
Author

@jsnellbaker any chance you could review the code?

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hello @erolosty I'm sorry about the small delay here. I spotted an issue that needs to be addressed otherwise the consent information won't be passed through in your request, please see the in-line comment below.

let gdprSignal;
let gdprConsentString;

if (bid.gdprConsent !== undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally to @aodinok suggestion, the gdprConsent object doesn't exist in the bidRequests but rather in the bidderRequest object. This additional variable needs to be added to the buildRequests() function call.

This would naturally affect the remaining GDPR related code below.

@jsnellbaker
Copy link
Collaborator

Hello @erolosty Did you have the chance to review the feedback? Please let me know if you have any questions.

@stale
Copy link

stale bot commented Jun 13, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 13, 2018
@jsnellbaker
Copy link
Collaborator

Hi @erolosty wanted to touch-base here again on this PR. Can you please take a look over the feedback and let me know if you have any questions? Thanks.

@stale stale bot removed the stale label Jun 13, 2018
@soarez soarez mentioned this pull request Jun 15, 2018
10 tasks
@jsnellbaker
Copy link
Collaborator

Closing per request made in #2733

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.

3 participants