EEA countries account-level override #4118

przemkaczmarek · 2024-12-20T13:17:04Z

Implements #3678

SyntaxNode

@bsardo Configs that can be overridden at the account level are stored in the host level default account object. However, in this case the EEA Countries list is part of a different config area. Curious on your thoughts. Should the current location be deprecated in favor of account level?

SyntaxNode · 2024-12-20T16:14:02Z

exchange/exchange.go

@@ -316,8 +316,17 @@ func (e *exchange) HoldAuction(ctx context.Context, r *AuctionRequest, debugLog

 	recordImpMetrics(r.BidRequestWrapper, e.me)

+	// Retrieve host and account-level EEA countries config
+	eeaCountries := SelectEEACountries(e.privacyConfig.GDPR.EEACountries, r.Account.EEACountries)


Nitpick: The comment is not necessary. The inputs are clearly host and account level, but if you'd like to make that association clearer consider a revised method name.

SyntaxNode · 2024-12-20T16:17:32Z

exchange/exchange.go

+	eeaCountries := SelectEEACountries(e.privacyConfig.GDPR.EEACountries, r.Account.EEACountries)
+
+	// Create a map for efficient lookup
+	eeaCountriesMap := make(map[string]struct{})


The performance benefit of a map only comes into play when the map is used more than once. As it is now, this map is only referenced once in parseGDPRDefaultValue. This forces the processing of every element with the overhead of map creation. It would be more efficient to use a linear search in this instance.

Eventually, this map can live in the account config object when that caching layer is improved. Doesn't make sense there right now though.

SyntaxNode · 2024-12-20T16:18:14Z

config/account.go

@@ -381,3 +382,12 @@ func (ip *IPv4) Validate(errs []error) []error {
 	}
 	return errs
 }
+
+func (a *Account) ValidateEEACountries(errs []error) []error {


This validation method is not called.

SyntaxNode · 2024-12-20T16:22:23Z

config/account.go

@@ -42,6 +42,7 @@ type Account struct {
 	DefaultBidLimit         int                                         `mapstructure:"default_bid_limit" json:"default_bid_limit"`
 	BidAdjustments          *openrtb_ext.ExtRequestPrebidBidAdjustments `mapstructure:"bidadjustments" json:"bidadjustments"`
 	Privacy                 AccountPrivacy                              `mapstructure:"privacy" json:"privacy"`
+	EEACountries            []string                                    `mapstructure:"eea_countries" json:"eea_countries"`


Perhaps it would be better to include this within the AccountGDPR object since it's only used for GDPR enforcement and to match the host configuration of .privacy.gdpr. eea_countries.

I agree. Let's move it down a level as suggested.

bsardo · 2025-01-09T14:56:19Z

@bsardo Configs that can be overridden at the account level are stored in the host level default account object. However, in this case the EEA Countries list is part of a different config area. Curious on your thoughts. Should the current location be deprecated in favor of account level?

I agree we should deprecate it but my thought is that this should be done as part of a major release and should involve moving all GDPR fields (e.g. the purpose configs) that can be overridden at the account level to the account defaults object. In that case, I suggest we proceed with the approach Peter has taken here for now and perhaps consider this change for 4.0 or 5.0.

bsardo · 2025-01-30T00:15:08Z

exchange/exchange.go

+	var accountEEACountries []string
+	if r.Account.GDPR.EEACountries != nil {
+		accountEEACountries = r.Account.GDPR.EEACountries
+	}


I don't think you need this. You can just pass r.Account.GDPR.EEACountries into SelectEEACountries as the second parameter. If it's passed in as nil that should be ok.

bsardo · 2025-01-30T00:18:18Z

exchange/exchange.go

+		accountEEACountries = r.Account.GDPR.EEACountries
+	}
+
+	// Retrieve host and account-level EEA countries config


This comment should be tweaked a bit since it doesn't retrieve both host and account level configs but rather either host or account.

bsardo · 2025-01-30T00:21:21Z

exchange/exchange.go

@@ -584,7 +592,7 @@ func (e *exchange) parseGDPRDefaultValue(r *openrtb_ext.RequestWrapper) gdpr.Sig
 	if geo != nil {
 		// If we have a country set, and it is on the list, we assume GDPR applies if not set on the request.
 		// Otherwise we assume it does not apply as long as it appears "valid" (is 3 characters long).
-		if _, found := e.privacyConfig.GDPR.EEACountriesMap[strings.ToUpper(geo.Country)]; found {
+		if _, found := e.privacyConfig.GDPR.EEACountriesMap[strings.ToUpper(geo.Country)]; found || isEEACountry(geo.Country, eeaCountries) {


eeaCountries contains either the account config or the host config. We need only check this selected config: if _, found := isEEACountry(geo.Country, eeaCountries) {

bsardo · 2025-01-30T01:02:55Z

exchange/gdpr.go

+	// If account-level configuration is provided, it takes precedence.
+	if len(accountEEACountries) > 0 {
+		return accountEEACountries
+	}
+
+	// Otherwise, return the host-level configuration.


IMO you can remove these comments; the code is self-explanatory.

bsardo · 2025-01-30T01:04:21Z

exchange/gdpr.go

+
+// SelectEEACountries selects the EEA countries based on host and account configurations.
+// Account-level configuration takes precedence over the host-level configuration.
+func SelectEEACountries(hostEEACountries []string, accountEEACountries []string) []string {


This function should be private (first letter should be lower case) as it is only used in the exchange package.

bsardo · 2025-01-30T01:17:28Z

exchange/gdpr.go

+// Account-level configuration takes precedence over the host-level configuration.
+func SelectEEACountries(hostEEACountries []string, accountEEACountries []string) []string {
+	// If account-level configuration is provided, it takes precedence.
+	if len(accountEEACountries) > 0 {


Rather than checking if there are any elements, I think we should check if accountEEACountries is defined, in which case a not nil check would make more sense here.

bsardo · 2025-01-30T01:31:55Z

exchange/exchange_test.go

+			if result != tt.expected {
+				t.Errorf("isEEACountry(%s, %v) = %v; want %v", tt.country, tt.eeaList, result, tt.expected)
+			}


Use assert.Equal instead.

bsardo · 2025-01-30T01:36:01Z

exchange/exchange_test.go

+		{"Country in EEA", "FRA", eeaCountries, true},
+		{"Country in EEA lowercase", "fra", eeaCountries, true},
+		{"Country not in EEA", "USA", eeaCountries, false},
+		{"Empty country string", "", eeaCountries, false},
+		{"EEA list is empty", "FRA", []string{}, false},
+		{"EEA list is nil", "FRA", nil, false},


Please explicitly specify each field name along with the value and put each field/value pair on its own line. The name value should also be short with underscores used instead of spaces. t.Run will automatically substitute underscores for spaces so we like to avoid spaces in our test case names so if a test fails we can do a quick search for the test name failure in our code base without having to do a space to underscore substitution.
For example:

{ name: "found_uppercase", country: "FRA", eeaList: eeaCountries, expected: true, },

bsardo · 2025-01-30T01:39:24Z

exchange/gdpr.go

+
+// SelectEEACountries selects the EEA countries based on host and account configurations.
+// Account-level configuration takes precedence over the host-level configuration.
+func SelectEEACountries(hostEEACountries []string, accountEEACountries []string) []string {


Let's add a simple unit test in exchange/gdpr_test.go for this function.

bsardo · 2025-01-30T02:00:32Z

exchange/exchange.go

We should add an exchange package JSON integration test so that we can verify HoldAuction selects the correct EEA countries list. We can discuss this offline.

bsardo

Looks good, just a few test nitpicks.

bsardo · 2025-02-11T04:39:14Z

exchange/gdpr_test.go

+		expected            []string
+	}{
+		{
+			description:         "Account EEA countries provided, return account's",


Nitpick: Use underscores instead of spaces when separating words in test name so t.Run doesn't perform space for underscore substitutions making it easier to find failing tests in the code base via search.

bsardo · 2025-02-11T04:40:09Z

exchange/gdpr_test.go

+		expected            []string
+	}{
+		{
+			description:         "Account EEA countries provided, return account's",


Nitpick: I would also shorten your names removing the expected return info. That can easily be deciphered. Simply describing the inputs is recommended.

bsardo · 2025-02-11T04:45:59Z

exchange/exchange_test.go

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := isEEACountry(tt.country, tt.eeaList)
+			assert.Equal(t, tt.expected, result, "isEEACountry(%s, %v)", tt.country, tt.eeaList)


This can be simplified to assert.Equal(t, tt.expected, result). If this test fails, it will provide context on which test case fails making these last parameters unnecessary.

task 3678

36dba89

przemkaczmarek self-assigned this Dec 20, 2024

przemkaczmarek requested a review from bsardo December 20, 2024 13:33

SyntaxNode reviewed Dec 20, 2024

View reviewed changes

przemkaczmarek added 5 commits January 10, 2025 10:53

fix comments

5e04225

fix comments

28ffec7

fix comments

a0fe990

fix comments

acacb3e

fix comments

16f8efd

bsardo changed the title ~~Account-level override for the list of EEA countries #3678~~ EEA countries account-level override Jan 30, 2025

bsardo reviewed Jan 30, 2025

View reviewed changes

bsardo assigned bsardo and unassigned przemkaczmarek Jan 30, 2025

przemkaczmarek added 2 commits January 30, 2025 13:53

fix comments

8329b3e

fix integration tests

8e94c39

bsardo reviewed Feb 11, 2025

View reviewed changes

przemkaczmarek added 2 commits February 11, 2025 13:35

fix comments NitPICKS

a4a15ee

fix bugs

5b0ba54

bsardo approved these changes Feb 11, 2025

View reviewed changes

bsardo assigned VeronikaSolovei9 Feb 13, 2025

VeronikaSolovei9 approved these changes Feb 14, 2025

View reviewed changes

bsardo merged commit b335bc5 into prebid:master Feb 14, 2025
4 checks passed

cross-repo-issue-creation bot mentioned this pull request Feb 14, 2025

Port PR from PBS-Go: EEA countries account-level override prebid/prebid-server-java#3761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EEA countries account-level override #4118

EEA countries account-level override #4118

przemkaczmarek commented Dec 20, 2024 •

edited by bsardo

Loading

SyntaxNode left a comment

SyntaxNode Dec 20, 2024

SyntaxNode Dec 20, 2024

SyntaxNode Dec 20, 2024

SyntaxNode Dec 20, 2024

bsardo Jan 9, 2025

bsardo commented Jan 9, 2025 •

edited

Loading

bsardo Jan 30, 2025

bsardo Jan 30, 2025

bsardo Jan 30, 2025

bsardo Jan 30, 2025

bsardo Jan 30, 2025

bsardo Jan 30, 2025

bsardo Jan 30, 2025

bsardo Jan 30, 2025

bsardo Jan 30, 2025

bsardo Jan 30, 2025

bsardo left a comment

bsardo Feb 11, 2025

bsardo Feb 11, 2025

bsardo Feb 11, 2025

EEA countries account-level override #4118

EEA countries account-level override #4118

Conversation

przemkaczmarek commented Dec 20, 2024 • edited by bsardo Loading

SyntaxNode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo commented Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przemkaczmarek commented Dec 20, 2024 •

edited by bsardo

Loading

bsardo commented Jan 9, 2025 •

edited

Loading