From 53aaae296d41588d43ac4c95f5df09c903e38889 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miha=20Ple=C5=A1ko?= Date: Fri, 9 Aug 2019 15:23:02 +0200 Subject: [PATCH] Protect user work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, when user updates PXE Server details and submits form, all related PxeMenus and PxImages are discarded so she needs to run "Recreate Relationships" to inventory them again. This approach works but unfortunately user has option to manually edit PxeImages when they are inventoried, and by discarding them we also discard her work. With this commit we therefore go out of our way and double check which PxeMenus (and subsequent PxeImages) are *really* affected by update and only discard those. This way, if user only updates PxeServer's name, nothing else will be recreated hence user's manual work will be preserved. Fixes https://github.com/ManageIQ/manageiq/issues/18940 Signed-off-by: Miha Pleško --- app/controllers/api/pxe_servers_controller.rb | 13 +++- spec/requests/pxe_servers_spec.rb | 74 ++++++++++++++++++- 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/pxe_servers_controller.rb b/app/controllers/api/pxe_servers_controller.rb index b064a9adae..c97070b7ea 100644 --- a/app/controllers/api/pxe_servers_controller.rb +++ b/app/controllers/api/pxe_servers_controller.rb @@ -48,9 +48,18 @@ def edit_resource(type, id, data) menus = data.delete('pxe_menus') authentication = data.delete('authentication') PxeServer.transaction do - if menus + # When server URI changes all menus have to be discarded, sorry. + # But when URI remains we can perserve menus, as long as they aren't changed too. + if data['uri'] != server.uri server.pxe_menus.destroy_all - data['pxe_menus'] = create_pxe_menus(menus) + data['pxe_menus'] = create_pxe_menus(menus || []) + elsif menus + desired_menus = menus.map { |el| el['file_name'] } + current_menus = server.pxe_menus.map(&:file_name) + to_keep, to_destroy = current_menus.partition { |menu| desired_menus.include?(menu) } + to_create = desired_menus.reject { |menu| current_menus.include?(menu) } + server.pxe_menus.where(:file_name => to_destroy).destroy_all + data['pxe_menus'] = server.pxe_menus.where(:file_name => to_keep) + create_pxe_menus(to_create.map { |f| { 'file_name' => f } }) end server.update!(data) server.update_authentication({:default => authentication.transform_keys(&:to_sym)}, {:save => true}) if authentication && server.requires_credentials? diff --git a/spec/requests/pxe_servers_spec.rb b/spec/requests/pxe_servers_spec.rb index e73bdf991f..f311dc64a2 100644 --- a/spec/requests/pxe_servers_spec.rb +++ b/spec/requests/pxe_servers_spec.rb @@ -194,7 +194,6 @@ describe 'update /api/pxe_servers/:id' do let(:url) { "/api/pxe_servers/#{pxe_server.id}" } - it 'update pxe server' do api_basic_authorize collection_action_identifier(:pxe_servers, :edit, :patch) patch(url, :params => {:name => 'updated name', :uri => 'updated://url', :pxe_menus => [{:file_name => 'updated menu'}]}) @@ -209,6 +208,79 @@ patch(url, :params => {:name => 'updated name', :uri => 'updated://url', :pxe_menus => [{:file_name => 'updated menu'}]}) expect(response).to have_http_status(:forbidden) end + + describe 'update pxe server menus' do + let!(:pxe_menu_1) { FactoryBot.create(:pxe_menu, :pxe_server => pxe_server, :file_name => 'existing1') } + let!(:pxe_image_1) { FactoryBot.create(:pxe_image, :pxe_server => pxe_server, :pxe_menu => pxe_menu_1) } + let!(:pxe_menu_2) { FactoryBot.create(:pxe_menu, :pxe_server => pxe_server, :file_name => 'existing2') } + let!(:pxe_image_2) { FactoryBot.create(:pxe_image, :pxe_server => pxe_server, :pxe_menu => pxe_menu_2) } + let(:params) { {:name => 'updated name', :uri => uri, :pxe_menus => menus.map { |m| {:file_name => m} }} } + let(:uri) { pxe_server.uri } + let(:menus) { %w[existing1 existing2] } + + before { api_basic_authorize collection_action_identifier(:pxe_servers, :edit, :patch) } + + context 'when nothing changed' do + it 'all menus remain' do + patch(url, :params => params) + expect(response).to have_http_status(:ok) + expect(PxeMenu.count).to eq(2) + expect(PxeImage.count).to eq(2) + expect(PxeMenu.find_by(:id => pxe_menu_1.id)).to eq(pxe_menu_1) + expect(PxeMenu.find_by(:id => pxe_menu_2.id)).to eq(pxe_menu_2) + end + end + + context 'when uri changed' do + let(:uri) { 'http://new.uri' } + it 'all menus are recreated' do + patch(url, :params => params) + expect(response).to have_http_status(:ok) + expect(PxeMenu.count).to eq(2) + expect(PxeImage.count).to eq(0) + expect(PxeMenu.find_by(:id => pxe_menu_1.id)).to be_nil + expect(PxeMenu.find_by(:id => pxe_menu_2.id)).to be_nil + expect(PxeMenu.find_by(:file_name => pxe_menu_1.file_name)).not_to be_nil + expect(PxeMenu.find_by(:file_name => pxe_menu_2.file_name)).not_to be_nil + end + end + + context 'when new menus are added' do + let(:menus) { %w[existing1 existing2 new] } + it 'existing menus remain' do + patch(url, :params => params) + expect(response).to have_http_status(:ok) + expect(PxeMenu.count).to eq(3) + expect(PxeImage.count).to eq(2) + expect(PxeMenu.find_by(:id => pxe_menu_1.id)).to eq(pxe_menu_1) + expect(PxeMenu.find_by(:id => pxe_menu_2.id)).to eq(pxe_menu_2) + expect(PxeMenu.find_by(:file_name => 'new')).not_to be_nil + end + end + + context 'when menus are deleted' do + let(:menus) { %w[existing1] } + it 'menus are destroyed' do + patch(url, :params => params) + expect(response).to have_http_status(:ok) + expect(PxeMenu.count).to eq(1) + expect(PxeImage.count).to eq(1) + expect(PxeMenu.find_by(:id => pxe_menu_1.id)).to eq(pxe_menu_1) + expect(PxeMenu.find_by(:id => pxe_menu_2.id)).to be_nil + end + end + + context 'when different menus are specified' do + let(:menus) { %w[new] } + it 'old menus are destroyed' do + patch(url, :params => params) + expect(response).to have_http_status(:ok) + expect(PxeMenu.count).to eq(1) + expect(PxeImage.count).to eq(0) + expect(PxeMenu.find_by(:file_name => 'new')).not_to be_nil + end + end + end end describe 'delete /api/pxe_servers/:id' do