fix: support audience ids #161
Conversation
There was a problem hiding this comment.
LGTM, but I'm not quite sure I understand why we needed to change from returning the err, to always returning nil. Minor comment on lack of coverage that could result in a future regression albeit that it should be covered by the compat suite. Perhaps add a follow-up ticket for that fixture?
| @@ -34,23 +34,23 @@ func (m ExactMatcher) Match(user entities.UserContext) (bool, error) { | |||
| if stringValue, ok := m.Condition.Value.(string); ok { | |||
| attributeValue, err := user.GetStringAttribute(m.Condition.Name) | |||
| if err != nil { | |||
| return false, err | |||
| return false, nil | |||
There was a problem hiding this comment.
Should we log that we're swallowing these errors?
There was a problem hiding this comment.
the error here means that the user does not have that attribute key we are looking for. I can log it as debug but that might be a bit too noisy, wdyt?
There was a problem hiding this comment.
Created JIRA ticket to track this.
| var audienceConditionTree *entities.TreeNode | ||
| var err error | ||
| if rawExperiment.AudienceConditions == nil && len(rawExperiment.AudienceIds) > 0 { | ||
| audienceConditionTree, err = buildAudienceConditionTree(rawExperiment.AudienceIds) |
There was a problem hiding this comment.
Do we have coverage on this path?
I tried to explain this in the summary 😄. It boils down to the error being used to return |
| @@ -42,7 +42,7 @@ type Experiment struct { | |||
| TrafficAllocation []trafficAllocation `json:"trafficAllocation"` | |||
| AudienceIds []string `json:"audienceIds"` | |||
| ForcedVariations map[string]string `json:"forcedVariations"` | |||
| AudienceConditions interface{} `json:"audienceConditions"` | |||
There was a problem hiding this comment.
@mikeng13 this change is causing issues for datafiles with audience conditions defined as:
"audienceConditions": "20413101795", i have fixed it in this PR #165
Summary
experiment.audienceIdsis not parsed into audience condition tree in the absence ofexperiment.audienceConditionsnotcondition being short-circuited to false