Prevents negative values for Snapshot retention policies #51295
Conversation
to avoid negative values for retention policy
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
1 similar comment
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
alisonelizabeth
added
bug
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
|
@ffknob thanks so much for the contribution! 😄 I will try to take a look at this sometime next week. |
|
jenkins test this |
💚 Build Succeeded |
|
@elasticmachine merge upstream |
There was a problem hiding this comment.
@ffknob nice work! Thanks again for this contribution.
I left one suggested change around how to handling the validation. Let me know if you have any questions.
| @@ -93,13 +93,19 @@ export const PolicyStepRetention: React.FunctionComponent<StepProps> = ({ | |||
| <EuiFieldNumber | |||
| value={retention.expireAfterValue || ''} | |||
| onBlur={() => setTouched({ ...touched, expireAfterValue: true })} | |||
| onKeyDown={e => { | |||
There was a problem hiding this comment.
Rather than preventing the user from entering the - symbol, I think it might be better to add a new validation rule that prevents negative numbers. This way, a validation error message will appear as well, and the user will not be able to proceed to the next step until it is corrected.
For example, there is currently validation in place so that a user cannot set the min_count greater than max_count.
You should be able to modify this file: https://github.com/elastic/kibana/blob/cda7eba4c9dbb3739c726a194ceed155111f15fc/x-pack/legacy/plugins/snapshot_restore/public/app/services/validation/validate_policy.ts
There was a problem hiding this comment.
Hi @alisonelizabeth thanks for the tip. It surely looks better as a validation.
I did some testing and it looks like the image below.
I also tryied to add a Jest test, but I don't know eactly how to run it locally (do you? I mean, doyou know how I could have run specifically the x-pack/legacy/plugins/snapshot_restore/__jest__/client_integration/policy_add.test.ts test locally before pushing?)
Let me know what do you think!
There was a problem hiding this comment.
@ffknob this looks great! Thanks!
In regards to your question, the tests are currently being skipped, as they require react 16.9 for testing asynchronous code inside act() and we're not quite ready to upgrade across kibana. In order to run locally, you would need to install the newer version of react and remove the skip. I tested your changes locally and also ran the tests and confirmed they're all green 🎉
I will take care of merging the PR and backporting once CI is green 😄
|
jenkins test this |
💚 Build Succeeded |
Summary
Related to #51134
Sets the
minprop of theEuiFieldNumberto 0 so the user cannot use the arrow controls to get to a negative number. Also prevents the user from entering manually the-symbol.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers