Api V1 percentage_of_actors gate endpoint #179
Conversation
AlexWheeler
force-pushed
the
api-v1-percentage-of-actors-gate
branch
from
f18c2f9 to
9431bd0
Compare
| end | ||
|
|
||
| def percentage | ||
| @percentage ||= params.fetch('percentage') { json_error_response(:percentage_invalid) }.to_i |
There was a problem hiding this comment.
here we use fetch because its possible the client misspelled or did not send the percentage parameter and we'd end up with nil.to_i and we'll end up with 0 instead of responding with error so they can correct the request.
| end | ||
|
|
||
| def percentage | ||
| @percentage ||= params.fetch('percentage') { json_error_response(:percentage_invalid) }.to_i |
There was a problem hiding this comment.
perhaps we want to pass the percentage into a regex to make sure it is an integer?
since
"percentage": "alex" would also end up with 0
def percentage
if number_between_0_and_100?(params['percentage'])
@percentage ||= params['percentage'].to_i
else
json_error_response(:percentage_invalid)
end
end
def number_between_0_and_100?(val)
!!/^(100|[1-9][0-9]?)$/.match(val)
end
52581a1 to
58e70e4
Compare
| end | ||
|
|
||
| def number_less_than_or_equal_100?(val) | ||
| !!/^(100|[1-9][0-9]?)$/.match(val) |
There was a problem hiding this comment.
^ and $ only match beginning of line. What do you think about using \A and \Z to match beginning and end of string?
Alternatively, did you consider using the stricter integer conversion ruby has (Integer(val))? I've never used a regex for this, so I am just curious what the difference would be between Integer(val) with a range check and a rescue for ArgumentError vs the regex.
irb(main):002:0> Integer("1")
=> 1
irb(main):004:0> Integer("asdf")
ArgumentError: invalid value for Integer(): "asdf"
from (irb):4:in `Integer'
from (irb):4
from /Users/jnunemaker/.rbenv/versions/2.3.1/bin/irb:11:in `<main>'
Then, once converted to an integer, I check the if the value is in a valid range. What do you think
There was a problem hiding this comment.
ah I do like this approach better, this way we can always just use percentage instead of params['percentage'] then percentage elsewhere and it splits up the logic nicely
| describe 'invalid percentage parameter' do | ||
| before do | ||
| flipper[:my_feature].disable | ||
| post '/api/v1/features/my_feature/percentage_of_actors', { percentage: '300' } |
There was a problem hiding this comment.
This is a good out of range test. Does it feel worthwhile to add an invalid value as well, like "asdf"?
There was a problem hiding this comment.
totally, adding one now thanks
| end | ||
|
|
||
| def number_less_than_or_equal_100?(val) | ||
| !!/^(100|[1-9][0-9]?)$/.match(val) |
There was a problem hiding this comment.
Technically you can enable for 0 percent using flipper and the flipper-ui. Do you feel we should support the same semantics for the API or just point people to disable?
There was a problem hiding this comment.
correct, I think it makes sense to support same semantics especially if users are used to that
* POST / DELETE request to enable / disable * send parameter 'percentage' to request percentage to enable * adds specs
AlexWheeler
force-pushed
the
api-v1-percentage-of-actors-gate
branch
from
58e70e4 to
2dadf41
Compare
| def percentage | ||
| @percentage ||= begin | ||
| Integer(params['percentage']) | ||
| rescue ArgumentError, TypeError |
There was a problem hiding this comment.
I like this much better great suggestion. Here we rescue TypeError as well for nil cases Integer(nil)
| it '400s with correct error response when percentage parameter is invalid' do | ||
| expect(last_response.status).to eq(400) | ||
| expect(json_response).to eq({ 'code' => 3, 'message' => 'Percentage must be a positive number less than or equal to 100.', 'more_info' => '' }) | ||
| end |
There was a problem hiding this comment.
added test for invalid value that is not an integer or nil
| @@ -23,6 +23,7 @@ def as_json | |||
| ERRORS = { | |||
| feature_not_found: Error.new(1, "Feature not found.", "", 404), | |||
| group_not_registered: Error.new(2, "Group not registered.", "", 404), | |||
| percentage_invalid: Error.new(3, "Percentage must be a positive number less than or equal to 100.", "", 400), | |||
There was a problem hiding this comment.
Missed this in my review. Should this be 422? 400 is bad request and that isn't accurate as if it gets here it was a well formed request, it just wasn't valid. I think 422 is what rails typically uses for this type of error. Thoughts?
|
|
||
| def ensure_valid_enable_params | ||
| json_error_response(:feature_not_found) unless feature_names.include?(feature_name) | ||
| json_error_response(:percentage_invalid) unless percentage >= 0 && percentage <= 100 |
There was a problem hiding this comment.
I typically try to avoid unless with more than one conditional. I would probably have written this if percentage < 0 || percentage > 100 as it just feels a bit easier to read. I can tweak it later. No biggie.
|
🎉 |
:{ thing } _ { error_description }
i.e.
:{feature} _ {not_found}
:{group} _ {not_registered}
:{percentage } _ {invalid}