Skip to content

Commit

Permalink
correct logic for default values on feature toggles so toggle value r…
Browse files Browse the repository at this point in the history
…espected when toggle exists and default is true (#93)

fix: patch logic for default value so that the SDK correctly checks toggle state if a false value is provided as a default
  • Loading branch information
shieldo committed Mar 29, 2022
1 parent 99c3f22 commit dcac422
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 57 deletions.
2 changes: 1 addition & 1 deletion lib/unleash/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def is_enabled?(feature, context = nil, default_value_param = false, &fallback_b

toggle = Unleash::FeatureToggle.new(toggle_as_hash)

toggle.is_enabled?(context, default_value)
toggle.is_enabled?(context)
end

# enabled? is a more ruby idiomatic method name than is_enabled?
Expand Down
12 changes: 6 additions & 6 deletions lib/unleash/feature_toggle.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ def to_s
"<FeatureToggle: name=#{name},enabled=#{enabled},strategies=#{strategies},variant_definitions=#{variant_definitions}>"
end

def is_enabled?(context, default_result)
result = am_enabled?(context, default_result)
def is_enabled?(context)
result = am_enabled?(context)

choice = result ? :yes : :no
Unleash.toggle_metrics.increment(name, choice) unless Unleash.configuration.disable_metrics
Expand All @@ -37,7 +37,7 @@ def get_variant(context, fallback_variant = Unleash::FeatureToggle.disabled_vari

context = ensure_valid_context(context)

return Unleash::FeatureToggle.disabled_variant unless self.enabled && am_enabled?(context, true)
return Unleash::FeatureToggle.disabled_variant unless self.enabled && am_enabled?(context)
return Unleash::FeatureToggle.disabled_variant if sum_variant_defs_weights <= 0

variant = variant_from_override_match(context) || variant_from_weights(context, resolve_stickiness)
Expand All @@ -57,18 +57,18 @@ def resolve_stickiness
end

# only check if it is enabled, do not do metrics
def am_enabled?(context, default_result)
def am_enabled?(context)
result =
if self.enabled
self.strategies.empty? ||
self.strategies.any? do |s|
strategy_enabled?(s, context) && strategy_constraint_matches?(s, context)
end
else
default_result
false
end

Unleash.logger.debug "Unleash::FeatureToggle (enabled:#{self.enabled} default_result:#{default_result} " \
Unleash.logger.debug "Unleash::FeatureToggle (enabled:#{self.enabled} " \
"and Strategies combined with contraints returned #{result})"

result
Expand Down
3 changes: 1 addition & 2 deletions spec/unleash/client_specification_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
# load client spec
SPECIFICATION_PATH = 'client-specification/specifications'.freeze

DEFAULT_RESULT = false
DEFAULT_VARIANT = Unleash::Variant.new(name: 'unknown', enabled: false).freeze

before do
Expand Down Expand Up @@ -41,7 +40,7 @@
toggle = Unleash::FeatureToggle.new(test_toggle)
context = Unleash::Context.new(test['context'])

toggle_result = toggle.is_enabled?(context, DEFAULT_RESULT)
toggle_result = toggle.is_enabled?(context)

expect(toggle_result).to eq(test['expectedResult'])
end
Expand Down
66 changes: 18 additions & 48 deletions spec/unleash/feature_toggle_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,9 @@
)
end

it 'should return true if enabled, and default is true' do
it 'should return true if enabled' do
context = Unleash::Context.new(user_id: 1)
expect(feature_toggle.is_enabled?(context, true)).to be_truthy
end

it 'should return true if enabled, and default is false' do
context = Unleash::Context.new(user_id: 1)
expect(feature_toggle.is_enabled?(context, false)).to be_truthy
expect(feature_toggle.is_enabled?(context)).to be_truthy
end
end

Expand All @@ -51,14 +46,9 @@
)
end

it 'should return false if disabled and default is false' do
it 'should return false if disabled' do
context = Unleash::Context.new(user_id: 1)
expect(feature_toggle.is_enabled?(context, false)).to be_falsey
end

it 'should return true if disabled and default is true' do
context = Unleash::Context.new(user_id: 1)
expect(feature_toggle.is_enabled?(context, true)).to be_truthy
expect(feature_toggle.is_enabled?(context)).to be_falsey
end
end

Expand All @@ -81,24 +71,14 @@
)
end

it 'should return true if enabled, user_id matched, and default is true' do
context = Unleash::Context.new(user_id: "12345")
expect(feature_toggle.is_enabled?(context, true)).to be_truthy
end

it 'should return true if enabled, user_id matched, and default is false' do
it 'should return true if enabled and user_id is matched' do
context = Unleash::Context.new(user_id: "12345")
expect(feature_toggle.is_enabled?(context, false)).to be_truthy
expect(feature_toggle.is_enabled?(context)).to be_truthy
end

it 'should return false if enabled, user_id unmatched, and default is true' do
it 'should return false if enabled and user_id is unmatched' do
context = Unleash::Context.new(user_id: "54321")
expect(feature_toggle.is_enabled?(context, true)).to be_falsey
end

it 'should return false if enabled, user_id unmatched, and default is false' do
context = Unleash::Context.new(user_id: "54321")
expect(feature_toggle.is_enabled?(context, false)).to be_falsey
expect(feature_toggle.is_enabled?(context)).to be_falsey
end
end

Expand All @@ -121,24 +101,14 @@
)
end

it 'should return false if disabled, user_id matched, and default is false' do
context = Unleash::Context.new(user_id: "12345")
expect(feature_toggle.is_enabled?(context, false)).to be_falsey
end

it 'should return false if disabled, user_id unmatched, and default is false' do
context = Unleash::Context.new(user_id: "54321")
expect(feature_toggle.is_enabled?(context, false)).to be_falsey
end

it 'should return true if disabled, user_id matched, and default is true' do
it 'should return false if disabled and user_id matched' do
context = Unleash::Context.new(user_id: "12345")
expect(feature_toggle.is_enabled?(context, true)).to be_truthy
expect(feature_toggle.is_enabled?(context)).to be_falsey
end

it 'should return true if disabled, user_id unmatched, and default is true' do
it 'should return false if disabled and user_id unmatched' do
context = Unleash::Context.new(user_id: "54321")
expect(feature_toggle.is_enabled?(context, true)).to be_truthy
expect(feature_toggle.is_enabled?(context)).to be_falsey
end
end

Expand Down Expand Up @@ -473,22 +443,22 @@

it 'should return true if it matches all constraints' do
context = Unleash::Context.new(user_id: "123", environment: "dev")
expect(feature_toggle.is_enabled?(context, true)).to be_truthy
expect(feature_toggle.is_enabled?(context)).to be_truthy
end

it 'should return false if it does not match all constraints (env)' do
context = Unleash::Context.new(user_id: "123", environment: "prod")
expect(feature_toggle.is_enabled?(context, true)).to be_falsey
expect(feature_toggle.is_enabled?(context)).to be_falsey
end

it 'should return false if it does not match all constraints (user_id)' do
context = Unleash::Context.new(user_id: "11", environment: "dev")
expect(feature_toggle.is_enabled?(context, true)).to be_falsey
expect(feature_toggle.is_enabled?(context)).to be_falsey
end

it 'should return false if it does not match any constraint (env and user_id)' do
context = Unleash::Context.new(user_id: "11", environment: "prod")
expect(feature_toggle.is_enabled?(context, true)).to be_falsey
expect(feature_toggle.is_enabled?(context)).to be_falsey
end
end

Expand Down Expand Up @@ -519,12 +489,12 @@

it 'should return true if it matches the constraint' do
context = Unleash::Context.new(user_id: "123", environment: "dev")
expect(feature_toggle.is_enabled?(context, true)).to be_truthy
expect(feature_toggle.is_enabled?(context)).to be_truthy
end

it 'should return false if it does not match the constraint' do
context = Unleash::Context.new(user_id: "123", environment: "prod")
expect(feature_toggle.is_enabled?(context, true)).to be_falsey
expect(feature_toggle.is_enabled?(context)).to be_falsey
end
end

Expand Down

0 comments on commit dcac422

Please sign in to comment.