-
Notifications
You must be signed in to change notification settings - Fork 748
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
Activities: Back To Spec #3081
Activities: Back To Spec #3081
Conversation
endpoints/openrtb2/amp_auction.go
Outdated
writeError(errL, w, &labels) | ||
return | ||
} | ||
activityControll := privacy.NewActivityControl(&account.Privacy) |
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.
Nitpick Spelling Mistake: activityControll
--> activityControl
@@ -88,34 +57,17 @@ func activityRulesToEnforcementRules(rules []config.ActivityRule) ([]Rule, error | |||
result = ActivityDeny |
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.
I noticed that this line doesn't have any code coverage. It may be out of the scope of this PR's goals, so I'm fine leaving it as is, but wanted to point it out.
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.
Yeah, we only test it with allow=true.
Here is a quick fix.
These functions should take a parameter that will control the result:
func getTestActivityConfig(allow bool) config.Activity {
return config.Activity{
Default: ptrutil.ToPtr(true),
Rules: []config.ActivityRule{
{
Allow: allow,
Condition: config.ActivityCondition{
ComponentName: []string{"bidderA"},
ComponentType: []string{"bidder"},
},
},
},
}
}
func getTestActivityPlan(activityRes ActivityResult) ActivityPlan {
return ActivityPlan{
defaultResult: true,
rules: []Rule{
ConditionRule{
result: activityRes,
componentName: []string{"bidderA"},
componentType: []string{"bidder"},
},
},
}
}
Put "true" and "ActivityAllow" to everywhere where these parameters are needed.
And then modify existing test to receive false at least for one activity:
{
name: "specified_and_correct",
privacyConf: config.AccountPrivacy{
AllowActivities: &config.AllowActivities{
SyncUser: getTestActivityConfig(true),
FetchBids: getTestActivityConfig(true),
EnrichUserFPD: getTestActivityConfig(true),
ReportAnalytics: getTestActivityConfig(true),
TransmitUserFPD: getTestActivityConfig(true),
TransmitPreciseGeo: getTestActivityConfig(false),
TransmitUniqueRequestIds: getTestActivityConfig(true),
TransmitTids: getTestActivityConfig(true),
},
},
activityControl: ActivityControl{plans: map[Activity]ActivityPlan{
ActivitySyncUser: getTestActivityPlan(ActivityAllow),
ActivityFetchBids: getTestActivityPlan(ActivityAllow),
ActivityEnrichUserFPD: getTestActivityPlan(ActivityAllow),
ActivityReportAnalytics: getTestActivityPlan(ActivityAllow),
ActivityTransmitUserFPD: getTestActivityPlan(ActivityAllow),
ActivityTransmitPreciseGeo: getTestActivityPlan(ActivityDeny),
ActivityTransmitUniqueRequestIds: getTestActivityPlan(ActivityAllow),
ActivityTransmitTids: getTestActivityPlan(ActivityAllow),
}},
},
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.
Good catch @AlexBVolcy. Thanks for the implementation plan @VeronikaSolovei9. I'll add this to the PR tomorrow.
The componentName matching was built for an earlier draft where the format was "scope.name". This PR updates to just "name". This also means parsing an Activity Config can no longer return an error. The code is simplified as a result.