feat(api): Feature variable APIs now return default variable value when featureEnabled property is false. #276
Conversation
…en featureEnabled property is false.
| genericUserId, | ||
| Collections.singletonMap(ATTRIBUTE_HOUSE_KEY, AUDIENCE_SLYTHERIN_VALUE)), | ||
| Math.PI, 2); | ||
|
|
There was a problem hiding this comment.
nit. Remove this extraneous line.
| genericUserId, | ||
| Collections.singletonMap(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE)), | ||
| (long) expectedValue); | ||
|
|
There was a problem hiding this comment.
nit. Remove this extraneous line.
| @@ -649,7 +649,7 @@ String getFeatureVariableValueForType(@Nonnull String featureKey, | |||
| String variableValue = variable.getDefaultValue(); | |||
| Map<String, ?> copiedAttributes = copyAttributes(attributes); | |||
| FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes); | |||
| if (featureDecision.variation != null) { | |||
| if (featureDecision.variation != null && featureDecision.variation.getFeatureEnabled()) { | |||
There was a problem hiding this comment.
Similar feedback as given in Ruby SDK. Here the log message is incorrect for the new scenario, as the user is bucketed into the variation, but because feature is disabled we return default value. Please fix log message corresponding to this scenario.
…and returning default value
| } else { | ||
| variableValue = variable.getDefaultValue(); | ||
| logger.info("Feature \"{}\" for variation \"{}\" was not enabled. " + | ||
| "The default value \"{}\" for \"{}\" is being returned.", |
There was a problem hiding this comment.
IMO mentioning variable key and value is too much information in log message. Just saying The default value is being returned. is enough here.
| * returns the variable value of the variation the user is bucketed into | ||
| * if the variation is not null and the variable has a usage within the variation. | ||
| * returns the default variable value | ||
| * if the variation is not null featureEnabled is false. |
There was a problem hiding this comment.
nit. The docstring looks ill formatted. Please fix this.
Also, missing and. ...variation is not null and featureEnabled is false.
| testUserId, | ||
| Collections.singletonMap(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE)), | ||
| expectedValue); | ||
|
|
There was a problem hiding this comment.
nit. extraneous line.
| assertEquals(expectedValue, value); | ||
| } | ||
|
|
||
| /** | ||
| * Verify that the {@link Optimizely#getFeatureVariableString(String, String, String, Map)} | ||
| * notification listener of getFeatureVariableString is called when feature is in experiment and feature is true |
There was a problem hiding this comment.
What does notification listener have to do with this change? Please fix docstring.
|
|
||
| /** | ||
| * Verify that the {@link Optimizely#getFeatureVariableString(String, String, String, Map)} | ||
| * notification listener of getFeatureVariableString is called when feature is in experiment and feature enabled is false |
Pull Request Test Coverage Report for Build 940
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 940
💛 - Coveralls |
Summary
Test plan
Added unit tests.