feat: Added a new getAllFeatureVariables Api #470
Conversation
| @@ -957,6 +957,127 @@ Optimizely.prototype.getFeatureVariableJson = function(featureKey, variableKey, | |||
| } | |||
| }; | |||
|
|
|||
| /** | |||
| * Returns values for all the json variables attached to the given feature | |||
There was a problem hiding this comment.
This returns all variable values, not just values for JSON variables, right?
There was a problem hiding this comment.
You are right. It was a copy/paste mistake.
| var variableValue = variable.defaultValue; | ||
| if (decision.variation !== null) { | ||
| var value = projectConfig.getVariableValueForVariation(configObj, variable, decision.variation, logger); | ||
| if (value !== null) { |
There was a problem hiding this comment.
Seems like you are following _getFeatureVariableForType, can we refactor & call that from here instead of duplicating this logic?
Aside, reading this now I find it a little hard to follow with the nested ifs. Can we flatten this out while preserving the same behavior? decision.variation !== null, value !== null, featureEnabled determine the log message. decision.variation !== null and featureEnabled determine the variable value.
There was a problem hiding this comment.
Thanks for the suggestion. since i dont understand all the behaviour in detail, i followed whatever was already there. The tests already written are extremely thorough. I will go ahead and simplify the stuff and rely on the tests to ensure correctness
There was a problem hiding this comment.
@mjc1283 I refactored this.
- Moved common code in a separate function
- Separated logs logic and variable assignment.
- Simplified checks for logs to make them more readable and added comments
There was a problem hiding this comment.
content of the tests is good, I agree with @mjc1283 comments
| @@ -89,6 +89,17 @@ declare module '@optimizely/optimizely-sdk' { | |||
| userId: string, | |||
| attributes?: UserAttributes | |||
| ): string | null; | |||
| getFeatureVariableJson( | |||
There was a problem hiding this comment.
👍 Good catch updating this! Let's use unknown for the return value of these, instead of any.
| if (decision.variation !== null) { | ||
| featureEnabled = decision.variation.featureEnabled; | ||
| var value = projectConfig.getVariableValueForVariation(configObj, variable, decision.variation, this.logger); | ||
| Optimizely.prototype._getFeatureVariableValueFromVariation = function(featureKey, featureEnabled, variation, variable, userId) { |
There was a problem hiding this comment.
Can you add a doc comment for _getFeatureVariableValueFromVariation?
Summary
Implemented getAllFeatureVariables API
Test plan
Added new unit tests.