TID activity #3012
TID activity #3012
Conversation
| var sourceCopy *openrtb2.Source | ||
| sourceCopy = ptrutil.Clone(bidRequest.Source) |
There was a problem hiding this comment.
i think it's possible to remove the var declaration on line 150, and just have this line instead sourceCopy := ptrutil.Clone(bidRequest.Source)
There was a problem hiding this comment.
This may be the case, I think Clone(), while being a generic, will get the pointer type from the type if the argument used. That it, whatever pointer argument you feed it, it will return the same pointer type.
func Clone[T any](v *T) *T
The provided type, a pointer to a T type, indicates that the return value should be another pointer to type T.
There was a problem hiding this comment.
Yup. I'd expect sourceCopy := ptrutil.Clone(bidRequest.Source) to work here.
There was a problem hiding this comment.
I tried it and it still needs some type conversion. Based on other usages of "Clone" in code, type is declared beforehand. I may miss something, please advice.
There was a problem hiding this comment.
Darn. Go's generic features are still pretty new. I was hoping this syntax would infer the generic type from the bidRequest.Source, but it this didn't work then it would seem the compiler is inferring from the variable's type to align the return type of Clone.
There was a problem hiding this comment.
There are other parts of code which generate transaction ids. To be sure this functionality works correctly, could you please add an exchange level end-to-end test? Similar to the ccpa-featureflag-off/on tests. We do this for other privacy concerns, such eidpermissions, gdpr, lmt, etc..
endpoints/openrtb2/video_auction.go
Outdated
| @@ -5,6 +5,7 @@ import ( | |||
| "encoding/json" | |||
| "errors" | |||
| "fmt" | |||
| "github.com/prebid/prebid-server/privacy" | |||
There was a problem hiding this comment.
FYI: Other editors group non-built in imports after the built-in ones. This is likely to be automatically overwritten in the future by other contributors. Does your IDE have that as an option?
There was a problem hiding this comment.
I think my IDE sorted it in alphabetical order. I'll fix this.
exchange/utils.go
Outdated
| passTIDActivityAllowed := auctionReq.Activities.Allow(privacy.ActivityTransmitTids, scopedName) | ||
| if !passTIDActivityAllowed { | ||
| privacyEnforcement.TID = true | ||
| } |
There was a problem hiding this comment.
The Transmit TID activity is new and doesn't have the baggage of TCF, CCPA, etc.. Could this be simplified as:
privacyEnforcement.TID = !auctionReq.Activities.Allow(privacy.ActivityTransmitTids, scopedName)
exchange/utils_test.go
Outdated
| @@ -4288,6 +4289,7 @@ func TestCleanOpenRTBRequestsActivitiesFetchBids(t *testing.T) { | |||
| expectedUserYOB: 1982, | |||
| expectedUserLat: 123.456, | |||
| expectedDeviceDIDMD5: "some device ID hash", | |||
| expectedSourceTID: "61018dc9-fa61-4c41-b7dc-f90b9ae80e87", | |||
There was a problem hiding this comment.
Nitpick: Is there a need to use a guid as a test string? Could we instead use something simpler, such as "1" or "anyTID"? Specific values which are not immediately obvious as fake data may be misinterpreted as conveying a special meaning.
|
Addressed all comments, added E2E tests. |
No description provided.