Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Removing deleted Custom Buttons #370

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions db/migrate/20190509142148_update_custom_button_sets.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
class UpdateCustomButtonSets < ActiveRecord::Migration[5.0]
class MiqSet < ActiveRecord::Base
serialize :set_data
end

class CustomButton < ActiveRecord::Base; end

def up
say_with_time("Removing deleted buttons from Custom Button Sets") do
MiqSet.select(:id, :set_data).where(:set_type => 'CustomButtonSet').each do |cbs|
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(',')}')")
Copy link
Contributor

@himdel himdel May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpichler as you've corrected me not to use Ruby sort, is there a good reason not to use the Ruby sort?

Copy link
Contributor

@lpichler lpichler Jun 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not against ruby solution but I knew that we are using implementation in SQL

https://github.com/ManageIQ/manageiq/pull/18060/files#diff-72d768ae2e73b9e77080d4a7df59cf56R4

so you can copy with_array_order here or use ruby.

.ids
cbs.set_data[:button_order] = existing_buttons
cbs.save!
end
end
end
end
25 changes: 25 additions & 0 deletions spec/migrations/20190509142148_update_custom_button_sets_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
require_migration

describe UpdateCustomButtonSets do
let(:custom_button_set_stub) { migration_stub :MiqSet }
let(:custom_button_stub) { migration_stub :CustomButton }

migration_context :up do
it 'Removes non-existing buttons while keeping the buttons sorted' do
cb1 = custom_button_stub.create!(:id => 1)
cb2 = custom_button_stub.create!(:id => 2)
cb4 = custom_button_stub.create!(:id => 4)
cb5 = custom_button_stub.create!(:id => 5)
cb7 = custom_button_stub.create!(:id => 7)
cbs = custom_button_set_stub.create!(
:set_type => 'CustomButtonSet',
:set_data => { :button_order => [
cb1.id, cb7.id, cb2.id, cb4.id, 6, cb5.id, 3]})

migrate

cbs.reload
expect(cbs.set_data[:button_order]).to eql([cb1.id, cb7.id, cb2.id, cb4.id, cb5.id])
end
end
end