TransmitUserFPD and TransmitPreciseGeo activity integration #2906
Conversation
VeronikaSolovei9
force-pushed
the
activity_ufpd_geo
branch
from
4225f2f to
e4e9d80
Compare
exchange/utils.go
Outdated
| privacyEnforcement.CCPA = ccpaEnforcer.ShouldEnforce(bidderRequest.BidderName.String()) | ||
| // potentially block passing IDs based on COPPA | ||
| if req.BidRequest.Regs != nil && req.BidRequest.Regs.COPPA == 1 { | ||
| privacyEnforcement.COPPA = true | ||
| } |
There was a problem hiding this comment.
COPPA is already set at the start of the loop. Why do we need to include it again here?
There was a problem hiding this comment.
Right, removed
exchange/utils.go
Outdated
| privacyEnforcement.COPPA = true | ||
| } | ||
| // potentially block passing geo based on LMT | ||
| privacyEnforcement.LMT = lmtEnforcer.ShouldEnforce(unknownBidder) |
There was a problem hiding this comment.
COPPA and LMT are already set at the start of the loop. Why do we need to include it again here?
| } | ||
| if test.expectedScrubDeviceExecuted { | ||
| m.On("ScrubDevice", req.Device, ScrubStrategyDeviceIDAll, ScrubStrategyIPV4Lowest8, ScrubStrategyIPV6Lowest32, ScrubStrategyGeoFull).Return(replacedDevice).Once() | ||
| } |
There was a problem hiding this comment.
Nice use of the mock library.
privacy/scrubber.go
Outdated
| //transmitEids covers user.eids and user.ext.eids | ||
| if bidRequest.User != nil { | ||
| bidRequest.User.EIDs = nil | ||
| bidRequest.User.Ext = scrubExtIDs(bidRequest.User.Ext, "eids") |
There was a problem hiding this comment.
We could unmarshal user.ext at the start of this method if either UFPD or Eids are true, modify as necessary throughout the function, and at the end marshal it back once more if either UFPD or Eids are true. That would bring us down to at most 1 unmarshal/marshal pair for each ext. Thoughts?
There was a problem hiding this comment.
Done. Please see this change in this commit bf4b27b
privacy/scrubber.go
Outdated
| } | ||
| } | ||
|
|
||
| if userExtModified { | ||
| userExt, _ := json.Marshal(userExtParsed) | ||
| bidRequest.User.Ext = userExt |
There was a problem hiding this comment.
Are we sure that User is not a shared object at this point?
There was a problem hiding this comment.
The bidRequest is absolutely at least a shallow copy at this point. I'm good with adding a shallow clone here to be certain if it's not otherwise proven. We'll replace it in the near future with the RequestWrapper and the cost of cloning the User object doesn't seem too concerning.
There was a problem hiding this comment.
Added clones.
Will it be ok if I put a follow up PR with this change only? |
Yup. Let's go with the stricter 32 bits for now. |
This PR contains existing GDPR related code refactoring due to activity integration.
I deviate from existing scrubber strategy implementation. New scrubber function
ScrubRequestreceives enforcer data, not a strategy. There are couple of reasons for this:bidRequest.User.EIDs = nil,bidRequest.User.Data = niland in User.Ext remove "data" instead of "eids".With this implementation all existing unit tests pass, local testing of activities looks good.