-
Notifications
You must be signed in to change notification settings - Fork 731
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
Refactor Bid Splitter Privacy Functions #3645
Conversation
gdpr/impl.go
Outdated
pc, err := parseConsent(p.consent) | ||
if err != nil { | ||
return p.defaultPermissions(), err | ||
return DenyAll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error was always only interpreted this way, so codifying that behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an err always is treated as DenyAll, then what about line 80 below where an error becomes p.defaultPermissions()
?
@@ -20,8 +20,8 @@ type Permissions interface { | |||
|
|||
// Determines whether or not to send PI information to a bidder, or mask it out. | |||
// | |||
// If the consent string was nonsensical, the returned error will be an ErrorMalformedConsent. | |||
AuctionActivitiesAllowed(ctx context.Context, bidderCoreName openrtb_ext.BidderName, bidder openrtb_ext.BidderName) (permissions AuctionPermissions, err error) | |||
// If the consent string was nonsensical, the no permissions are granted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, should be: the_n_ no permissions are granted
fetchBidsActivityAllowed := activities.Allow(privacy.ActivityFetchBids, scope, privacy.NewRequestFromBidRequest(*r)) | ||
if !fetchBidsActivityAllowed { | ||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking you could just have the function call in the if statement like
if !activities.Allow(privacy.ActivityFetchBids, scope, privacy.NewRequestFromBidRequest(*r)) {
return true
}
As opposed to using the fetchBidsActivityAllowed
variable. Or is that out of the scope of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.