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

Update consentManagementUsp.js to not store and reuse null consent #5452

Merged
merged 8 commits into from
Jul 21, 2020
Merged

Update consentManagementUsp.js to not store and reuse null consent #5452

merged 8 commits into from
Jul 21, 2020

Conversation

patmmccann
Copy link
Collaborator

Fixes bug described at #5440 to comply with intent of #4425

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

No longer checks to see if consentData are stored; should recheck _uspapi

@patmmccann patmmccann marked this pull request as ready for review July 2, 2020 23:42
@patmmccann
Copy link
Collaborator Author

of note, build failed because of bad merge d8e5796

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.

If this is something we wish to implement, then the changes here should work.

As a note - if we are just taking this aspect of the feature out, then we could just take out the entire unit test instead of modifying it. It's not really serving a unique purpose now and would be doing the same job as the other basic workflow unit test.

Assigning to @harpere to 2nd review on this change.

@jsnellbaker jsnellbaker requested a review from harpere July 7, 2020 18:08
@jsnellbaker jsnellbaker added the needs 2nd review Core module updates require two approvals from the core team label Jul 7, 2020
@jsnellbaker jsnellbaker removed their assignment Jul 7, 2020
@jsnellbaker
Copy link
Collaborator

On an additional note - I retested the browserstack tests locally and they passed fine. I'm not sure why circleci tests are failing. It seems like it's trying to use a previous commit where a flatMap was added for the amxBidAdapter, but the config/stats for the job appear to be pointing to the most recent commit (where the flatMap was removed). I think we can just ignore it unless someone has another idea.

@patmmccann
Copy link
Collaborator Author

patmmccann commented Jul 9, 2020 via email

@bretg bretg requested review from mkendall07 and removed request for harpere July 16, 2020 01:17
@bretg bretg assigned mkendall07 and unassigned harpere Jul 16, 2020
@bretg
Copy link
Collaborator

bretg commented Jul 16, 2020

@harpere is out right now -- assigning over to @mkendall07

@mkendall07
Copy link
Member

mkendall07 commented Jul 16, 2020

This change looks good if it's the behavior we want. The only slight caveat here is this part (from #4425)

the USP (CCPA) api function __uspapi() always responds synchronously, whether or not privacy data is available, while the GDPR CMP may respond asynchronously, especially if waiting for a user response. Unfortunately this doesn't necessarily mean it's always a synchronous transaction because if there are nested cross-domain iframes we may need to do an async postMessage().

Note the bolded part, it's possible this puts an async delay into every auction request. As long as we are ok living with that it's a LGTM.

@mkendall07
Copy link
Member

@bretg @patmmccann for weigh in ^^

@patmmccann
Copy link
Collaborator Author

patmmccann commented Jul 16, 2020 via email

@mkendall07
Copy link
Member

makes sense. It looks like circleci is continuously failing. Is this branch up to date with master?

@patmmccann
Copy link
Collaborator Author

there was a bad merge to master of the amx adapter that was later fixed

@patmmccann
Copy link
Collaborator Author

@mkendall07 it is up to date with master now

@bretg
Copy link
Collaborator

bretg commented Jul 21, 2020

Re-ran the build and it passed. Since this is a bug, I'll assume that we're not going to need documentation. Will merge and add it to the rel notes.

@bretg bretg merged commit 3a8f5cd into prebid:master Jul 21, 2020
@patmmccann patmmccann deleted the patch-20 branch August 4, 2020 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants