Add datafile isValid check in feature accessor methods #109
Conversation
lib/optimizely.rb
Outdated
| @@ -343,6 +343,12 @@ def get_feature_variable_string(feature_flag_key, variable_key, user_id, attribu | |||
| # Returns the string variable value. | |||
| # Returns nil if the feature flag or variable are not found. | |||
|
|
|||
| unless @is_valid | |||
| logger = SimpleLogger.new | |||
There was a problem hiding this comment.
I see we do this an awful lot. Can we instead attach this to the Optimizely instance's @logger if the instance is not valid and reuse that? We already instantiate a new SimpleLogger on lines 66, 75, and 82. We can attach that to the instance variable @logger and re-use throughout the code instead of instantiating every time we need to use it.
|
build |
…lid-datafile-for-feature-accessor
| invalid_project = Optimizely::Project.new('invalid', nil, spy_logger) | ||
| expect(invalid_project.get_feature_variable_boolean('boolean_single_variable_feature', 'boolean_variable', user_id, user_attributes)) | ||
| .to eq(nil) | ||
| expect(logger).to have_received(:log).once.with(Logger::ERROR, 'Provided datafile is in an invalid format.') |
There was a problem hiding this comment.
Is this right? How come we use similar messages twice?
There was a problem hiding this comment.
The first one comes from the initialize method. And the second one is logged in the relevant API method. Though duplicate, I think we should keep this at both places for clarity.
If a client is initialized, it should log the datafile error. When an API method is called, it should log exactly why the method is aborting.
Let me know if we should revise the error log in the API method. And shorten it to only 'Aborting "
There was a problem hiding this comment.
Yeah that makes sense. I agree with your assessment.
| invalid_project = Optimizely::Project.new('invalid', nil, spy_logger) | ||
| expect(invalid_project.get_feature_variable_double('double_single_variable_feature', 'double_variable', user_id, user_attributes)) | ||
| .to eq(nil) | ||
| expect(logger).to have_received(:log).once.with(Logger::ERROR, 'Provided datafile is in an invalid format.') |
There was a problem hiding this comment.
Shouldn't we only be logging the second message?
| expect(invalid_project.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', user_id, user_attributes)) | ||
| .to eq(nil) | ||
| expect(logger).to have_received(:log).once.with(Logger::ERROR, 'Provided datafile is in an invalid format.') | ||
| expect(logger).to have_received(:log).once.with(Logger::ERROR, 'Provided datafile is in an invalid format. Aborting get_feature_variable_integer.') |
There was a problem hiding this comment.
Same as above, seems redundant
Fixes #107