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.

Sign up for GitHub

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

Jump to bottom

minor consentManagement fix #5050

Merged
merged 1 commit into from
Apr 13, 2020
Merged

minor consentManagement fix #5050

merged 1 commit into from
Apr 13, 2020

Conversation

harpere
Copy link
Collaborator

@harpere harpere commented Mar 30, 2020

Type of change

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

in case tcfData.tcString is not defined.

harpere
harpere commented Mar 30, 2020
View reviewed changes
modules/consentManagement.js
@@ -99,7 +99,7 @@ function lookupIabConsent(cmpSuccess, cmpError, hookConfig) {
if (success) {
if (tcfData.eventStatus === 'tcloaded' || tcfData.eventStatus === 'useractioncomplete') {
cmpSuccess(tcfData, hookConfig);
} else if (tcfData.eventStatus === 'cmpuishown' && tcfData.tcString.length > 0 && tcfData.purposeOneTreatment === true) {
} else if (tcfData.eventStatus === 'cmpuishown' && tcfData.tcString && tcfData.purposeOneTreatment === true) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in case tcfData.tcString is not defined

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this change.

The original spec asked for this use-case that the tcString had to exist. Further if there was a scenario where the tcString was undefined and we tried to process it, the module should reject anyways (see here).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jsnellbaker it may be true that the spec says it should always be a string, but that doesn't mean we shouldn't defend against bad data. @robertrmartinez saw a console error during testing, so it must have reached this code. But ultimately there's no negative impact because the function just exits anyway. So not a big deal now. But if the code changes at some point, it might be a problem.

@harpere harpere added the needs 2nd review Core module updates require two approvals from the core team label Mar 30, 2020
jsnellbaker
jsnellbaker requested changes Mar 31, 2020
View reviewed changes
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.

Hi Eric,

See comment below.

modules/consentManagement.js
@@ -99,7 +99,7 @@ function lookupIabConsent(cmpSuccess, cmpError, hookConfig) {
if (success) {
if (tcfData.eventStatus === 'tcloaded' || tcfData.eventStatus === 'useractioncomplete') {
cmpSuccess(tcfData, hookConfig);
} else if (tcfData.eventStatus === 'cmpuishown' && tcfData.tcString.length > 0 && tcfData.purposeOneTreatment === true) {
} else if (tcfData.eventStatus === 'cmpuishown' && tcfData.tcString && tcfData.purposeOneTreatment === true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this change.

The original spec asked for this use-case that the tcString had to exist. Further if there was a scenario where the tcString was undefined and we tried to process it, the module should reject anyways (see here).

jsnellbaker
jsnellbaker approved these changes Apr 13, 2020
View reviewed changes
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.

This is fine to implement in lieu of the feedback.

@jsnellbaker jsnellbaker merged commit 7438f5a into master Apr 13, 2020
redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Apr 16, 2020
@redaguermas
Merge branch 'master' of https://github.com/prebid/Prebid.js
51525d7 
* 'master' of https://github.com/prebid/Prebid.js: (102 commits)
  Marsmedia - Add vastXml and fix id response (prebid#5067)
  PubMatic adapter to support image sync (prebid#5104)
  minor consentManagement fix (prebid#5050)
  fix circle ci failing tests (prebid#5113)
  Add Relaido Adapter (prebid#5101)
  Add new bid adapter for ConnectAd (prebid#4806)
  change payload (prebid#5105)
  Utils updates (prebid#5092)
  Read OpenRTB app objects if set in config + bug fix for when ad units are reloaded (prebid#5086)
  Criteo : added first party data mapping to bidder request (prebid#4954)
  updateAdGenerationManual (prebid#5032)
  New bid adapter: Wipes (prebid#5051)
  Prebid manager analytics utm tags (prebid#4998)
  CRITEO RTUS Integration with Yieldmo Prebid (prebid#5075)
  isSafariBrowser update  (prebid#5077)
  Support min &max duration for onevideo (prebid#5079)
  increment pre version
  Prebid 3.15.0 release
  prebid#5011 Fix to set Secure attribute on cookie when SameSite=none (prebid#5064)
  Prebid adapter for windtalker (prebid#5040)
  ...
iggyfisk pushed a commit to happypancake/Prebid.js that referenced this pull request Jun 22, 2020
@harpere @iggyfisk
minor consentManagement fix (prebid#5050)
a81f7bb 
Co-authored-by: Eric Harper <eharper@rubiconproject.com>
@patmmccann patmmccann mentioned this pull request Aug 4, 2020
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsnellbaker jsnellbaker jsnellbaker approved these changes

@robertrmartinez robertrmartinez robertrmartinez approved these changes

Assignees

@jsnellbaker jsnellbaker

Labels
needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@harpere @robertrmartinez @jsnellbaker