Skip to content
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

Privacy Request Metrics #1378

Closed
wants to merge 4 commits into from
Closed

Conversation

SyntaxNode
Copy link
Contributor

Following up on #1360 to extend to CCPA, COPPA, and LMT for requests.

hhhjort
hhhjort previously approved these changes Jul 6, 2020
Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

parsedConsent, err := vendorconsent.ParseString(consent)
if err == nil {
cleanMetrics.gdprTcfVersion = int(parsedConsent.Version())
version := int(parsedConsent.Version())
privacyLabels.GDPRTCFVersion = pbsmetrics.TCFVersionValue(version)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this call refer to the TCFVersionToValue function found in pbsmetrics/metrics.go instead?

280 func TCFVersionToValue(version int) TCFVersionValue {
281     switch {
282     case version == 1:
283         return TCFVersionV1
284     case version == 2:
285         return TCFVersionV2
286     }
287     return TCFVersionErr
288 }
pbsmetrics/metrics.go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the fact that this part of the code appears to be covered by test cases, I could not find a test inside the exchange directory asserting the contents of the third return value of cleanOpenRTBRequests, privacyLabels pbsmetrics.PrivacyLabels. Should we include a test to assert this logic?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're exactly right. Bug fixed. Tests added.

@SyntaxNode SyntaxNode closed this Jul 15, 2020
@SyntaxNode SyntaxNode reopened this Jul 15, 2020
@SyntaxNode
Copy link
Contributor Author

This PR isn't behaving correctly. Something is out of sync. I'm going to close and recreate.

@SyntaxNode SyntaxNode closed this Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants