[WIP] Removing deleted Custom Buttons #370
Conversation
romanblanco
force-pushed
the
custom_button_set
branch
2 times, most recently
from
4ed36af to
9610d43
Compare
| next if cbs.set_data[:button_order].blank? | ||
| existing_buttons = CustomButton | ||
| .where(:id => cbs.set_data[:button_order]) | ||
| .order("position(id::text in '#{cbs.set_data[:button_order].join(',')}')") |
There was a problem hiding this comment.
Ugh, that looks like an SQL injection.
We can believe button_order only contains ints, but unless we check it, this is dangerous.
Why not sort it in ruby?
There was a problem hiding this comment.
@lpichler as you've corrected me not to use Ruby sort, is there a good reason not to use the Ruby sort?
There was a problem hiding this comment.
I am not against ruby solution but I knew that we are using implementation in SQL
so you can copy with_array_order here or use ruby.
| @@ -0,0 +1,21 @@ | |||
| class UpdateCustomButtonSets < ActiveRecord::Migration[5.2] | |||
There was a problem hiding this comment.
This needs to be [5.0] as manageiq doesn't support 5.2 yet.
We're going to move this repo back to 5.0 to avoid this until then.
There was a problem hiding this comment.
@carbonin corrected. Thank you 👍
romanblanco
force-pushed
the
custom_button_set
branch
from
9610d43 to
f5a99d5
Compare
after manageiq/pull/18368 the purpose of `set_data[:button_order]` has been changed, and an error is raised after going through an array, where the button does not exist anymore
|
Checked commit romanblanco@f5a99d5 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 db/migrate/20190509142148_update_custom_button_sets.rb
spec/migrations/20190509142148_update_custom_button_sets_spec.rb
|
|
It may be too late for a migration as this change has made it to hammer. To fix https://bugzilla.redhat.com/show_bug.cgi?id=1770300, I think we'll need to change the validation to skip missing buttons instead of throwing. So, I think we can close this one.. |
|
@himdel ManageIQ/manageiq#19573 is merged, should we close this? |
|
Indeed, there will be one more core PR to make sure service UI and the automate buttons tree is right too, but we can close this one. @miq-bot close_issue |
|
@himdel Only @romanblanco or a committer can close this pull request. |
After ManageIQ/manageiq#18368 the purpose of
set_data[:button_order]has been changed, and an error is raised after going through an array,
where the button does not exist anymore
TODO:
before merging, this needs to
cc/ @lpichler @himdel