AMP additionalConsent #2378

guscarreon · 2022-09-14T00:23:24Z

This pull request implements the second part of issue #1817. Particularly, the addtl_consent query field described in point number 5 of the issue's description:

If addtl_consent is supplied, copy its value to user.ext.ConsentedProvidersSettings.consented_providers

According to specs, is set independently of the consent_type. This pull request's core change happens inside the endpoints/openrtb2/amp_auction.go and is shown below:

436     setAmpExt(req.Site, "1")
437
438     setEffectiveAmpPubID(req, ampParams.Account)
439
440     if ampParams.Slot != "" {
441         req.Imp[0].TagID = ampParams.Slot
442     }
443
    +   if err := setConsentedProviders(req, ampParams); err != nil {
    +       return []error{err}
    +   }
    +
444     policyWriter, policyWriterErr := amp.ReadPolicy(ampParams, deps.cfg.GDPR.Enabled)
445     if policyWriterErr != nil {
446         return []error{policyWriterErr}
447     }
endpoints/openrtb2/amp_auction.go

All other changes are to support or test the addition shown above.

AlexBVolcy

Looks good, comments related to test coverage

AlexBVolcy · 2022-09-20T00:49:08Z

endpoints/openrtb2/amp_auction.go

+		} else {
+			return err
+		}
+		if err := reqWrap.RebuildRequest(); err != nil {
+			return err
+		}


Is there a way to add tests to trigger these errors?

Done. Added two additional test cases.

AlexBVolcy · 2022-09-20T00:50:10Z

openrtb_ext/request_wrapper.go

+	if hasCPSettings {
+		ue.consentedProvidersSettings = &ExtUserCPSettings{}
+		if err := json.Unmarshal(consentedProviderSettingsJson, ue.consentedProvidersSettings); err != nil {
+			return err


This block isn't covered by tests, can this be covered?

Done. Added a bunch of tests to cover this entire function and some other scenarios.

AlexBVolcy · 2022-09-20T00:50:52Z

openrtb_ext/request_wrapper.go

+				delete(ue.ext, "ConsentedProvidersSettings")
+			}
+		} else {
+			delete(ue.ext, "ConsentedProvidersSettings")


Can these delete cases be covered?

AlexBVolcy · 2022-09-27T02:12:04Z

endpoints/openrtb2/amp_auction_test.go

+		inAdditionalConsent string
+		inBidRequest        *openrtb2.BidRequest


Nitpick: Could these be renamed to givenAdditionalConsent and givenBidRequest?

AlexBVolcy · 2022-09-27T02:13:51Z

privacy/gdpr/consentwriter.go

+	}
+	reqWrap := &openrtb_ext.RequestWrapper{BidRequest: req}
+
+	// set GDPR value


I think you can delete this comment

AlexBVolcy · 2022-09-27T02:14:00Z

privacy/gdpr/consentwriter.go

-
-	if req.User == nil {
-		req.User = &openrtb2.User{}
+	// set Consent string


I think you can delete this comment

AlexBVolcy · 2022-09-27T02:29:18Z

endpoints/openrtb2/amp_auction.go

@@ -446,6 +446,10 @@ func (deps *endpointDeps) overrideWithParams(ampParams amp.Params, req *openrtb2
 		req.Imp[0].TagID = ampParams.Slot
 	}

+	if err := setConsentedProviders(req, ampParams); err != nil {
+		return []error{err}


This error return isn't covered by tests, is it possible to cover/trigger this?

Added TestOverrideWithParams to add coverage to various uncovered scenarios in the overrideWithParams function.

AlexBVolcy · 2022-09-27T02:30:39Z

openrtb_ext/request_wrapper.go

+	// this field is dirty
+	if ue.consentedProvidersDirty {
+		if ue.consentedProvidersSettings == nil {
+			ue.consentedProvidersSettings = &ExtUserCPSettings{}


This line is not covered by tests

Added test to cover line 385

VeronikaSolovei9 · 2022-09-30T06:46:23Z

openrtb_ext/user.go

@@ -10,6 +10,8 @@ type ExtUser struct {
 	// https://iabtechlab.com/wp-content/uploads/2018/02/OpenRTB_Advisory_GDPR_2018-02.pdf
 	Consent string `json:"consent,omitempty"`

+	ConsentedProvidersSettings *ExtUserCPSettings `json:"ConsentedProvidersSettings,omitempty"`


Is it a reason json mapping value starts from uppercase? Feels like it should start from lowercase.

It does look odd but I used the exact format described in issue #1817 and that's also how it was implemented in Prebid Server Java. Below an excerpt from a PBS-Java JSON test file:

70 "user": { 71 "buyeruid": "J5VLCWQP-26-CWFT", 72 "ext": { 73 "ConsentedProvidersSettings": { 74 "consented_providers": "someConsent" 75 } 76 } 77 }, ./src/test/resources/org/prebid/server/it/amp/test-rubicon-bid-request.json

json mapping is not case sensitive, it should work with ConsentedProvidersSettings and consentedproviderssettings. We can discuss it with the team.

VeronikaSolovei9 · 2022-09-30T06:47:54Z

openrtb_ext/request_wrapper.go

@@ -333,6 +337,13 @@ func (ue *UserExt) unmarshal(extJson json.RawMessage) error {
 		}
 	}

+	if consentedProviderSettingsJson, hasCPSettings := ue.ext["ConsentedProvidersSettings"]; hasCPSettings {


Please extract this value to constant - you use it in many places. Also should it start from lowercase?

I'm not sure what value to put into a constant. consentedProviderSettingsJson? Inside (ue *UserExt) unmarshal(extJson json.RawMessage) error it is only used once.

Oh I meant string constant "ConsentedProvidersSettings"

Has this string been extracted?

Has not, corrected

VeronikaSolovei9 · 2022-09-30T06:56:18Z

openrtb_ext/request_wrapper.go

+	eidsDirty                       bool
+	consentedProvidersSettings      *ExtUserCPSettings
+	consentedProvidersSettingsDirty bool
+	consentedProviders              string


Do you think it will be easier to handle consentedProvidersSettings *ExtUserCPSettings only? Keep track of consentedProviders looks redundant.
Look at `other fields - there is just one tracker for each object, no trackers for nested fields. What do you think?

Good catch, modified to get rid of this unecessary field in openrtb_ext/request_wrapper.go

Gus, I meant to keep track of object, not field.

consentedProvidersSettings *ExtUserCPSettings -> leave this consentedProvidersSettingsDirty bool consentedProviders string -> remove this consentedProvidersDirty bool

In case you add more fields to the object - it will have everything to keep track of it without significant changes

🤦‍♂️
I misread your comment.

AlexBVolcy

It's looks good. Just left one comment

AlexBVolcy · 2022-10-12T19:39:48Z

openrtb_ext/request_wrapper.go

@@ -333,6 +337,13 @@ func (ue *UserExt) unmarshal(extJson json.RawMessage) error {
 		}
 	}

+	if consentedProviderSettingsJson, hasCPSettings := ue.ext["ConsentedProvidersSettings"]; hasCPSettings {


Has this string been extracted?

…oviderSettingsOut() in amp_auction.go

VeronikaSolovei9

Commit messages are pure gold. LGTM

Gus Carreon added 4 commits September 13, 2022 20:09

AMP additionalConsent

65ecee6

go mod tidy

7ad490f

updated golang.org/x/net to v0.0.0-20220906165146-f3363e06e74c

fe7c2bb

Merged latest master

f561894

bsardo requested a review from AlexBVolcy September 14, 2022 17:58

bsardo assigned AlexBVolcy Sep 14, 2022

AlexBVolcy reviewed Sep 20, 2022

View reviewed changes

Alex's review

d605cde

bsardo requested a review from VeronikaSolovei9 September 22, 2022 17:31

bsardo assigned VeronikaSolovei9 Sep 22, 2022

Gus Carreon added 4 commits September 22, 2022 13:50

Removed unnecessary comment

8d7c49f

Merge branch 'master' into ALPC-716-2

4335893

refactored setConsentedProviders()

d1d2ddd

Minor tweaks

3a509d5

AlexBVolcy reviewed Sep 27, 2022

View reviewed changes

Gus Carreon added 2 commits September 27, 2022 11:44

Alex's second review part 1

8b7aee9

Alex's second review part 2

9e03574

VeronikaSolovei9 reviewed Sep 30, 2022

View reviewed changes

Gus Carreon and others added 9 commits October 3, 2022 16:29

Veronika's first revsew

ae1b147

Merge branch 'master' into ALPC-716-2

ba6f807

Veronika's second review

af47373

step out real quick

a22af4c

created to different structs

ca1b704

Implemented string parser

8e3c491

end-to-end test working

8f0f84b

merge latest master

6709ddf

consented_providers_settings array implemented

d631917

AlexBVolcy reviewed Oct 12, 2022

View reviewed changes

request_wrapper constants

e4febae

AlexBVolcy previously approved these changes Oct 13, 2022

View reviewed changes

resolved conflicts with latest master

81fc256

guscarreon dismissed AlexBVolcy’s stale review via 81fc256 October 13, 2022 21:11

Rollback to using GetConsentedProviderSettingsIn() and GetConsentedPr…

cb5aa3d

…oviderSettingsOut() in amp_auction.go

AlexBVolcy previously approved these changes Oct 21, 2022

View reviewed changes

rollback part 2

46e6675

guscarreon dismissed AlexBVolcy’s stale review via 46e6675 October 21, 2022 16:46

VeronikaSolovei9 approved these changes Oct 21, 2022

View reviewed changes

AlexBVolcy approved these changes Oct 24, 2022

View reviewed changes

bsardo merged commit 718628c into prebid:master Oct 24, 2022

guscarreon mentioned this pull request Nov 1, 2022

Declare request wrapper keys as constants #2443

Merged

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

AMP additionalConsent (prebid#2378)

15e3262

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AMP additionalConsent #2378

AMP additionalConsent #2378

guscarreon commented Sep 14, 2022

AlexBVolcy left a comment

AlexBVolcy Sep 20, 2022

guscarreon Sep 21, 2022

AlexBVolcy Sep 20, 2022

guscarreon Sep 21, 2022

AlexBVolcy Sep 20, 2022

guscarreon Sep 21, 2022

AlexBVolcy Sep 27, 2022

guscarreon Sep 27, 2022

AlexBVolcy Sep 27, 2022

guscarreon Sep 27, 2022

AlexBVolcy Sep 27, 2022

guscarreon Sep 27, 2022

AlexBVolcy Sep 27, 2022

guscarreon Sep 28, 2022

AlexBVolcy Sep 27, 2022

guscarreon Sep 28, 2022

VeronikaSolovei9 Sep 30, 2022

guscarreon Oct 3, 2022

VeronikaSolovei9 Oct 4, 2022

VeronikaSolovei9 Sep 30, 2022

guscarreon Oct 3, 2022

VeronikaSolovei9 Oct 4, 2022

AlexBVolcy Oct 12, 2022

guscarreon Oct 13, 2022

VeronikaSolovei9 Sep 30, 2022

guscarreon Oct 3, 2022

VeronikaSolovei9 Oct 4, 2022

guscarreon Oct 4, 2022

AlexBVolcy left a comment

AlexBVolcy Oct 12, 2022

VeronikaSolovei9 left a comment

AMP additionalConsent #2378

AMP additionalConsent #2378

Conversation

guscarreon commented Sep 14, 2022

AlexBVolcy 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment