fix: flexible rollout strategy without context #146
Conversation
The flexible rollout strategy should evaluate default and random stickiness even if context is not provided.
Pull Request Test Coverage Report for Build 5463576496
💛 - Coveralls |
gardleopard
force-pushed
the
support_random_and_default_stickiness_if_context_is_not_set
branch
from
2b046a0
to
2341bb7
Compare
|
|
||
| stickiness = params.fetch('stickiness', 'default') | ||
| return false unless context.instance_of?(Unleash::Context) || ['random', 'default'].include?(stickiness) |
There was a problem hiding this comment.
This is a bit complex logic. Any chance to try to extract the check into a private method, and use the method name to document what the check is trying to accomplish?
|
|
||
| stickiness = params.fetch('stickiness', 'default') | ||
| return false unless context.instance_of?(Unleash::Context) || ['random', 'default'].include?(stickiness) |
There was a problem hiding this comment.
| return false unless context.instance_of?(Unleash::Context) || ['random', 'default'].include?(stickiness) | |
| return false if is_context_invalid?(context, stickiness) |
# Context is always valid when using random or default strategies.
def is_context_invalid?(context, stickiness)
!context.instance_of?(Unleash::Context) || ['random', 'default'].include?(stickiness)
endThere was a problem hiding this comment.
Feel free to rename the method, not sure it's the most appropriate. Just a suggestion.
There was a problem hiding this comment.
I did it slightly different in order to keep the argument order and be more explicit.
|
|
||
| stickiness = params.fetch('stickiness', 'default') | ||
| return false unless context_is_sufficient?(stickiness, context) |
There was a problem hiding this comment.
Any way we can simplify further by turning the unless to a if ?
Would a name like context_required?/context_needed?/context_valid?/context_invalid? be more descriptive?
There was a problem hiding this comment.
ok i can change it
| @@ -32,6 +33,12 @@ def is_enabled?(params = {}, context = nil) | |||
|
|
|||
| private | |||
|
|
|||
| def context_is_sufficient?(stickiness, context) | |||
| return true if ['random', 'default'].include?(stickiness) | |||
There was a problem hiding this comment.
Note that it will not evaluate the strategy randomly when the stickiness is set to random.
You sure that's the desired behaviour?
Maybe it's worth having a rspec test just for the'stickiness' => 'radom' case? (where a couple of different seeds are hard coded?)
There was a problem hiding this comment.
this will just let it continue to the resolve stickiness like it did before, this is just a safe guard. It returns to the is_enabled method, not to the caller.
There was a problem hiding this comment.
but what happens up the stack, in the is_enabled? method? it returns right away, no? or is the boolean logic a bit hard to read?
There was a problem hiding this comment.
it only returns false when its supposed to, if the context is sufficient it continues...
| } | ||
|
|
||
| expect(strategy.is_enabled?(params, unleash_context)).to be_falsey | ||
| expect(strategy.is_enabled?(params, nil)).to be_falsey |
There was a problem hiding this comment.
Can you check that this also passes? (if you deem that necessary)
| expect(strategy.is_enabled?(params, nil)).to be_falsey | |
| expect(strategy.is_enabled?(params, "invalid_context")).to be_falsey | |
| expect(strategy.is_enabled?(params, nil)).to be_falsey |
There was a problem hiding this comment.
nice catch, i fixed it now
gardleopard
deleted the
support_random_and_default_stickiness_if_context_is_not_set
branch
The flexible rollout strategy should evaluate default and random stickiness even if context is not provided.