-
Notifications
You must be signed in to change notification settings - Fork 720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GDPR-friendly /cookie_sync #512
Conversation
…robably still lots of bugs
… gdpr-fetch-vendorlists
endpoints/cookie_sync.go
Outdated
co.Status = http.StatusBadRequest | ||
co.Errors = append(co.Errors, errors.New("Failed to check request.bidders in request body. Was your JSON well-formed?")) | ||
http.Error(w, "Failed to check request.bidders in request body. Was your JSON well-formed?", http.StatusBadRequest) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a return here? If we are declaring an error condition in the response here, what use is further processing?
@@ -183,3 +183,10 @@ func assertBoolsEqual(t *testing.T, expected bool, actual bool) { | |||
t.Errorf("Expected %t, got %t", expected, actual) | |||
} | |||
} | |||
|
|||
func assertStringsEqual(t *testing.T, expected string, actual string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function being used? This is the only change in the file, and it is a pure addition, so assertStringsEqual()
couldn't have been there before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in vendorlist-fetching_test.go
. I was unsure whether I should put it in the file where it's used, or keep all the assert*Equal()
functions together.
I can move it you think it's better the other way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its fine here, just noticed this was standing alone in the diff.
usersync/usersyncers/syncer.go
Outdated
@@ -26,6 +26,16 @@ func NewSyncerMap(cfg *config.Configuration) map[openrtb_ext.BidderName]usersync | |||
} | |||
} | |||
|
|||
func ParseIdsGDPR(syncers map[openrtb_ext.BidderName]usersync.Usersyncer) map[openrtb_ext.BidderName]uint16 { | |||
gdprAwareSyncers := make(map[openrtb_ext.BidderName]uint16, len(syncers)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely happy with this name, as to me it implies the values should be syncers, not GDPR IDs. perhaps rename to gdprAwareSyncerIDs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, actually referred to the first variable in the function rather than the function name itself, but this is good too. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my first inline comment ... we should add a return or have a reason we don't return at that point.
* Added a basic gdpr module for consent management. No tests yet, and probably still lots of bugs * Updated gopkg.lock. * Moved the URL creation into a function. * Made the URL-maker function a method arg, so it could be mocked out during testing. * Started some test helper code. * Added lots of tests. * Updated the /cookie_sync endpoint to recognize GDPR. * Patched up the gdpr module to respect configurable timeouts. * Added two new tests for untested code. * Making some cookie_sync code a bit more readable. * Added a test for a missing case. * Updated the /cookie_sync docs * Code review comments.
* Added a basic gdpr module for consent management. No tests yet, and probably still lots of bugs * Updated gopkg.lock. * Moved the URL creation into a function. * Made the URL-maker function a method arg, so it could be mocked out during testing. * Started some test helper code. * Added lots of tests. * Updated the /cookie_sync endpoint to recognize GDPR. * Patched up the gdpr module to respect configurable timeouts. * Added two new tests for untested code. * Making some cookie_sync code a bit more readable. * Added a test for a missing case. * Updated the /cookie_sync docs * Code review comments.
* Added a basic gdpr module for consent management. No tests yet, and probably still lots of bugs * Updated gopkg.lock. * Moved the URL creation into a function. * Made the URL-maker function a method arg, so it could be mocked out during testing. * Started some test helper code. * Added lots of tests. * Updated the /cookie_sync endpoint to recognize GDPR. * Patched up the gdpr module to respect configurable timeouts. * Added two new tests for untested code. * Making some cookie_sync code a bit more readable. * Added a test for a missing case. * Updated the /cookie_sync docs * Code review comments.
This partially implements #504, updating the
/cookie_sync
endpoint. See the issue (or the docs in the PR) for the behavior.It does not yet forward the gdpr/consent info into the URLs so that it gets passed back into
/setuid
. I believe those changes will mostly be in theUsersyncer
interface, so I'll save them for the next changeset.