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 GDPR parameters in ad request #2522

Merged
merged 66 commits into from
Jun 18, 2018
Merged

Conversation

guillaume-sticky
Copy link
Contributor

@guillaume-sticky guillaume-sticky commented May 11, 2018

Type of change

  • Bugfix
  • [ x] 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

Add some parameters support for freewheel-ssp bidder adapter.
Especially GDPR parameters to be transmit to our ad server

Copy link
Collaborator

@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.

see comments. also, should add tests.

@@ -223,6 +223,20 @@ export const spec = {
componentId: getComponentId(currentBidRequest.params.format)
};

if (typeof currentBidRequest.params.gdpr !== 'undefined') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this wasn't done properly. buildRequests(bidRequests, bidderRequest) has a second bidderRequest argument where you'll find the gdpr params. please see the doc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, more specifically, I believe gdpr_consent could exist while gdpr is undefined. So checking gdpr !== 'undefined' is not a good way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx, I'll have a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes have been made, thx again.

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.

Saw some additional changes that should be reviewed.


var vastParams = currentBidRequest.params.vastUrlParams;
if (typeof vastParams === 'object') {
for (kye in vastParams) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems to be a small typo here; believe it should be key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thx

requestParams._fw_gdpr_consent = bidderRequest.gdprConsent.consentString;

if (typeof currentBidRequest.gdprConsent.gdprApplies === 'boolean') {
requestParams._fw_gdpr = currentBidRequest.gdprConsent.gdprApplies;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should update currentBidRequest to bidderRequest in these two lines of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@jsnellbaker jsnellbaker self-assigned this Jun 6, 2018
@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging 2a18f58 into 71503a7 - view on LGTM.com

new alerts:

  • 1 for Missing variable declaration

Comment posted by LGTM.com

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.

@guillaume-sticky Thanks for making the changes; LGTM now.


var vastParams = currentBidRequest.params.vastUrlParams;
if (typeof vastParams === 'object') {
for (key in vastParams) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update - I thought this was addressed in the commit, but it seems it wasn't. The LGTM check noted this key variable isn't properly declared (missing a let). Could you update this spot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jsnellbaker jsnellbaker merged commit 2c19032 into prebid:master Jun 18, 2018
@jsnellbaker
Copy link
Collaborator

@guillaume-sticky Please submit a docs PR in the docs repo and make an update in your bidders page to add the following variable.

gdpr_supported: true

This can go directly below the variable for your 1.0 compliance. This will allow your adapter to appear in a table that shows GDPR compliant adapters. Thanks.

@guillaume-sticky
Copy link
Contributor Author

will do. Thx!

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.

5 participants