-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
validates User ID as a string #102
Conversation
lib/optimizely.rb
Outdated
@@ -135,8 +135,8 @@ def get_variation(experiment_key, user_id, attributes = nil) | |||
return nil | |||
end | |||
|
|||
if user_id.to_s.empty? | |||
@logger.log(Logger::ERROR, 'User ID cannot be empty.') | |||
if !user_id.is_a?(String) || user_id.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This same logic is duplicated in a few places. Can you create a validator function that we re-use?
@mikeng13 Please review changes. |
@@ -18,6 +18,7 @@ | |||
require_relative 'constants' | |||
require 'json' | |||
require 'json-schema' | |||
require 'optimizely/logger' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a relative require since this module is in the Optimizely
module?
@@ -306,6 +306,12 @@ module Constants | |||
'INTEGER' => 'integer', | |||
'STRING' => 'string' | |||
}.freeze | |||
|
|||
INPUT_VARIABLES = { | |||
'user_id' => 'User ID', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at VARIABLE_TYPES
the keys are all caps. Let's keep it consistent here.
build |
@mikeng13 All inputs validation is implemented in PR- #103 |
No description provided.