feat: Added new getFeatureVariableJson and getAllFeatureVariables Apis#251
feat: Added new getFeatureVariableJson and getAllFeatureVariables Apis#251thomaszurkan-optimizely merged 13 commits intomasterfrom
Conversation
| variable.delete('subType') | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Rather than putting the same manipulation code here. Can you directly access the two new variables that you have added and modify their type. Just a matter of preference, you may keep it this way.
There was a problem hiding this comment.
I have also noticed that, not sure why it has to be done in both places
There was a problem hiding this comment.
@oakbani @pawels-optimizely i changed it to only alter the ones that need to be altered.
pawels-optimizely
left a comment
pawels-optimizely
left a comment
There was a problem hiding this comment.
are there tests for get_feature_variable_json with notification or I missed it ?
get_all_feature_variables tests look good.
|
@pawels-optimizely added notification listener tests for get_feature_variable_json |
thomaszurkan-optimizely
left a comment
thomaszurkan-optimizely
left a comment
There was a problem hiding this comment.
Can we add a all variables source FEATURE_TEST? I think that would expose a scope problem on your source_info. I am just looking at the PR and haven't loaded into an IDE so correct me if I'm wrong on the source_info scope. But, I still think we should add the test.
|
|
||
| source_string = Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT'] | ||
| if decision && decision['source'] == Optimizely::DecisionService::DECISION_SOURCES['FEATURE_TEST'] | ||
| source_info = { |
There was a problem hiding this comment.
isn't this out of scope?
There was a problem hiding this comment.
No. It was decided to add a notification for all-feature-variables later. This is bein added in all the sdks now
| feature_flag['variables'].each do |variable| | ||
| if variable['type'] == 'string' && variable['subType'] == 'json' | ||
| variable['type'] = 'json' | ||
| variable.delete('subType') |
There was a problem hiding this comment.
I realize why. I just :(
There was a problem hiding this comment.
Yeah i agree. it looks ugly.
| 'feature', 'test_user', {'browser_type' => 'firefox'}, | ||
| feature_enabled: false, | ||
| feature_key: 'json_single_variable_feature', | ||
| source: 'rollout', |
There was a problem hiding this comment.
can you add a feature_test test. so, that you also show the proper values in source_info?
There was a problem hiding this comment.
added source info to one of the tests
optimizely-codereview-bot
requested review from
pawels-optimizely and
thomaszurkan-optimizely
thomaszurkan-optimizely
left a comment
thomaszurkan-optimizely
left a comment
There was a problem hiding this comment.
For the source_info, I was more asking why you didn't just do:
source_info = {}
and then set it if the it is a feature test and eliminate source_info || {}
LGTM
6 participants
Summary
Added new getFeatureVariableJson and getAllFeatureVariables Apis
Test plan
Added Unit tests