Remove legacy GDPR AMP config flag used to prevent buyer ID scrub on AMP requests #1565
Conversation
|
We'll wait to merge this until we build the final release this year. We had publisher complaints the first time around so this is deemed higher risk than normal, and we're not keen on doing this in Q4 2020. Also gives more time for hosts to discover the new GDPR account level config. |
config/config.go
Outdated
| @@ -1045,7 +1044,6 @@ func SetupViper(v *viper.Viper, filename string) { | |||
| v.SetDefault("gdpr.tcf2.special_purpose1.enabled", true) | |||
| v.SetDefault("gdpr.tcf2.purpose_one_treatement.enabled", true) | |||
| v.SetDefault("gdpr.tcf2.purpose_one_treatement.access_allowed", true) | |||
| v.SetDefault("gdpr.amp_exception", false) | |||
There was a problem hiding this comment.
What do you think of providing a warning to the host if this is enabled to guide them to understanding the change?
config/config.go
Outdated
| @@ -218,7 +218,7 @@ type GDPR struct { | |||
| NonStandardPublisherMap map[string]struct{} | |||
| TCF1 TCF1 `mapstructure:"tcf1"` | |||
| TCF2 TCF2 `mapstructure:"tcf2"` | |||
| AMPException bool `mapstructure:"amp_exception"` | |||
| AMPException bool `mapstructure:"amp_exception"` // Deprecated: Use account-level GDPR AMP settings | |||
There was a problem hiding this comment.
Nitpick: Can we move the comment on top of this line? Just to follow the format of the comment that follows in line 222
config/config.go
Outdated
| @@ -231,6 +231,10 @@ func (cfg *GDPR) validate(errs configErrors) configErrors { | |||
| if cfg.HostVendorID < 0 || cfg.HostVendorID > 0xffff { | |||
| errs = append(errs, fmt.Errorf("gdpr.host_vendor_id must be in the range [0, %d]. Got %d", 0xffff, cfg.HostVendorID)) | |||
| } | |||
|
|
|||
| if cfg.AMPException == true { | |||
| glog.Warning("GDPR AMP Exception is enabled but it is no longer supported. If you need to disable GDPR for AMP, you can do so on a per-account basis or disable it for all request types at the host level.") | |||
There was a problem hiding this comment.
Shouldn't we throw an error instead? If they set it to true and, for some reason, they miss this warning in their log, we might give them the false impression that the feature is still in place, isn't it? This PR already removed the AMPException() bool from the interface definition, so there's no way they are going to be able to use this value.
There was a problem hiding this comment.
We recently added a warning in the last release indicating that this config flag is deprecated and will be removed in a future version. This commit will now throw a fatal error as discussed.
|
Let's discuss with the Prebid.org Committee how we want to handle feature deprecation. |
config/config.go
Outdated
| @@ -214,7 +214,7 @@ func (cfg *GDPR) validate(errs []error) []error { | |||
| glog.Warning("gdpr.host_vendor_id was not specified. Host company GDPR checks will be skipped.") | |||
| } | |||
| if cfg.AMPException == true { | |||
| glog.Warning("gdpr.amp_exception is deprecated and will be removed in a future version. If you need to disable GDPR for AMP, you may do so per-account (gdpr.integration_enabled.amp) or at the host level for the default account (account_defaults.gdpr.integration_enabled.amp).") | |||
| errs = append(errs, fmt.Errorf("gdpr.amp_exception has been discontinued and must be set to false. If you need to disable GDPR for AMP, you may do so per-account (gdpr.integration_enabled.amp) or at the host level for the default account (account_defaults.gdpr.integration_enabled.amp)")) | |||
There was a problem hiding this comment.
I'd say the end result is we want hosts to remove the value in their config. I suggest a slight word change to something along the lines of:
"gdpr.amp_exception has been discontinued and must be removed from your config. If you need ..."
There was a problem hiding this comment.
We probably wanna remove AmpException() from the following places as well:
- https://github.com/prebid/prebid-server/blob/master/endpoints/cookie_sync_test.go#L261
- https://github.com/prebid/prebid-server/blob/master/endpoints/setuid_test.go#L444
- https://github.com/prebid/prebid-server/blob/master/endpoints/auction_test.go#L427
- https://github.com/prebid/prebid-server/blob/master/exchange/utils_test.go#L37
| @@ -264,7 +213,7 @@ func TestApply(t *testing.T) { | |||
| m.On("ScrubDevice", req.Device, test.expectedDeviceID, test.expectedDeviceIPv4, test.expectedDeviceIPv6, test.expectedDeviceGeo).Return(replacedDevice).Once() | |||
| m.On("ScrubUser", req.User, test.expectedUser, test.expectedUserGeo).Return(replacedUser).Once() | |||
|
|
|||
| test.enforcement.apply(req, test.ampGDPRException, m) | |||
| test.enforcement.apply(req, m) | |||
There was a problem hiding this comment.
Since we're not using ampGDPRException any longer, any reason to still keep it as part of the testCase struct?
There was a problem hiding this comment.
Nope, I'll remove. Good catch.
Related issue: #1312
The replacement strategy in #1564 should be merged in before this.