Protect user work

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 ManageIQ/manageiq#18940

Signed-off-by: Miha Pleško <[email protected]>

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?

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