CCPA Video Bug #1333
CCPA Video Bug #1333
Conversation
| @@ -122,9 +122,8 @@ func TestAMPPageInfo(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestGDPRConsent(t *testing.T) { | |||
| consent := "BONV8oqONXwgmADACHENAO7pqzAAppY" | |||
| consent := "BOu5On0Ou5On0ADACHENAO7pqzAAppY" | |||
There was a problem hiding this comment.
Unrelated. Noticed the poor test setup in looking at the adjacent CCPA tests.
| ccpaPolicy.Value = "" | ||
| if err := ccpaPolicy.Write(req); err != nil { | ||
| errL = append(errL, fmt.Errorf("Unable to remove invalid CCPA consent from the request. (%v)", err)) | ||
| } |
There was a problem hiding this comment.
We now remove the invalid value from the request. This prevents invalid CCPA values from being passed downstream to bidders.
| expectedWarning := errortypes.InvalidPrivacyConsent{Message: "CCPA consent is invalid and will be ignored. (request.regs.ext.us_privacy must contain 4 characters)"} | ||
| assert.ElementsMatch(t, errL, []error{&expectedWarning}) | ||
|
|
||
| assert.Empty(t, req.Regs.Ext, "Invalid Consent Removed From Request") |
There was a problem hiding this comment.
New assertion to verify the invalid consent string is removed.
| @@ -1,68 +1,69 @@ | |||
| { | |||
There was a problem hiding this comment.
I normalized the format of the sample json files. I removed the no longer existing accountid field, moved the no longer valid gdpr section (it's uses the standard IAB locations now), and reformatted the user section to match IAB spec from an earlier customized user object. .. oh, and ran it through a json "prettifier".
| "gender": "F", | ||
| "keywords": "Hotels, Travelling" | ||
| }, | ||
| "device11": { |
There was a problem hiding this comment.
Was this a typo or an intention way to effectively remove the device info? I erred on the side of a typo and fixed it.
| errL = deps.validateRequest(bidReq) | ||
| if len(errL) > 0 { | ||
| if errortypes.ContainsFatalError(errL) { |
There was a problem hiding this comment.
This is the main fix. I added similar filters to AMP and Auction, but forgot about Video.
| headers := http.Header{} | ||
| headers.Add("User-Agent", "TestHeader") | ||
|
|
||
| deps := mockDeps(t, ex) | ||
| req, valErr, podErr := deps.parseVideoRequest(reqData, headers) | ||
| reqBody := string(getRequestPayload(t, reqData)) |
There was a problem hiding this comment.
This is part of normalizing the json file formats.
| @@ -32,6 +32,10 @@ func ReadPolicy(req *openrtb.BidRequest) (Policy, error) { | |||
| // Write mutates an OpenRTB bid request with the context of the CCPA policy. | |||
| func (p Policy) Write(req *openrtb.BidRequest) error { | |||
| if p.Value == "" { | |||
| return clearPolicy(req) | |||
There was a problem hiding this comment.
It used to do nothing, but now there's a use case to remove the value.
| } | ||
| } | ||
|
|
||
| func TestCCPAInvalidConsent(t *testing.T) { |
There was a problem hiding this comment.
Remove? The invalid CCPA string is covered by the 3rd case in above function I believe.
There was a problem hiding this comment.
Ooops! You're absolutely right. This is a useless empty test I forgot to remove when I turned the above one into a "table driven" format. Removed.
| @@ -232,7 +232,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re | |||
| } | |||
|
|
|||
| if acctIdErr := validateAccount(deps.cfg, labels.PubID); acctIdErr != nil { | |||
| errL = append(errL, acctIdErr) | |||
| errL := []error{err} | |||
There was a problem hiding this comment.
It is intentional that we want to drop any previous errors if we encounter this error?
There was a problem hiding this comment.
I added that bit just now. Yes, it's intentional. The previous expectation from the code is for errL to be empty here. If it had an error before my changes, that should've triggered an early exit. The video endpoint doesn't have a structured response for errors (yet? hopefully?) so there's currently no way to communicate the warning back to the caller. Best to drop it than for it to randomly show up along with another unrelated error such as an invalid account.
CCPA validation errors should result in warnings, not fatal errors. The video endpoint is missing the filter to ignore warnings. This PR also removes the invalid CCPA consent string from the request in further processing.