Merge pull request #1159 from kbrock/edit_resource_vms

condense api_actions to api_resource for vms

Author: agrare

Diff for: app/controllers/api/vms_controller.rb

@@ -55,27 +55,23 @@ def delete_resource(type, id, _data = nil)
     end
 
     def set_owner_resource(type, id = nil, data = nil)
-      raise BadRequestError, "Must specify an id for setting the owner of a #{type} resource" unless id
+      api_resource(type, id, "Setting owner of") do |vm|
+        owner = data.blank? ? "" : data["owner"].strip
+        raise BadRequestError, "Must specify an owner" if owner.blank?
 
-      owner = data.blank? ? "" : data["owner"].strip
-      raise BadRequestError, "Must specify an owner" if owner.blank?
+        user = User.lookup_by_identity(owner)
+        raise BadRequestError, "Invalid user #{owner} specified" unless user
 
-      api_action(type, id) do |klass|
-        vm = resource_search(id, type, klass)
-        api_log_info("Setting owner of #{vm_ident(vm)}")
-
-        set_owner_vm(vm, owner)
+        vm.update!(:evm_owner => user, :miq_group => user.current_group)
+        {}
       end
     end
 
     def add_lifecycle_event_resource(type, id = nil, data = nil)
-      raise BadRequestError, "Must specify an id for adding a Lifecycle Event to a #{type} resource" unless id
-
-      api_action(type, id) do |klass|
-        vm = resource_search(id, type, klass)
-        api_log_info("Adding Lifecycle Event to #{vm_ident(vm)}")
-
-        add_lifecycle_event_vm(vm, lifecycle_event_from_data(data))
+      lifecycle_event = lifecycle_event_from_data(data)
+      api_resource(type, id, "Adding Life Cycle Event #{lifecycle_event['event']} to") do |vm|
+        LifecycleEvent.create_event(vm, lifecycle_event)
+        {}
       end
     end
 
@@ -88,11 +84,11 @@ def add_event_resource(type, id = nil, data = nil)
 
       data ||= {}
 
-      api_action(type, id) do |klass|
-        vm = resource_search(id, type, klass)
-        api_log_info("Adding Event to #{vm_ident(vm)}")
+      api_resource(type, id, "Adding Event to") do |vm|
+        event_timestamp = data["event_time"].blank? ? Time.now.utc : data["event_time"].to_s.to_time(:utc)
 
-        vm_event(vm, data["event_type"].to_s, data["event_message"].to_s, data["event_time"].to_s)
+        vm.add_ems_event(data["event_type"].to_s, data["event_message"].to_s, event_timestamp)
+        {}
       end
     end
 
@@ -107,18 +103,13 @@ def reboot_guest_resource(type, id = nil, _data = nil)
     central_admin :reboot_guest_resource, :reboot_guest
 
     def rename_resource(type, id, data = {})
-      raise BadRequestError, "Must specify an id for renaming a #{type} resource" unless id
-
       new_name = data.blank? ? "" : data["new_name"].strip
-      raise BadRequestError, "Must specify a new_name" if new_name.blank?
+      api_resource(type, id, "Renaming", :supports => :rename) do |vm|
+        raise BadRequestError, "Must specify a new_name" if new_name.blank?
 
-      api_action(type, id) do |klass|
-        vm = resource_search(id, type, klass)
-        api_log_info("Renaming #{vm_ident(vm)} to #{new_name}")
+        task_id = vm.rename_queue(User.current_user.userid, new_name)
 
-        result = validate_vm_for_action(vm, "rename")
-        result = rename_vm(vm, new_name) if result[:success]
-        result
+        action_result(true, "Renaming #{model_ident(vm, type)} to #{new_name}", :task_id => task_id)
       end
     end
     central_admin :rename_resource, :rename
@@ -154,20 +145,21 @@ def request_console_resource(type, id = nil, data = nil)
     end
 
     def set_miq_server_resource(type, id, data)
-      vm = resource_search(id, type)
-
-      miq_server = if data['miq_server'].empty?
-                     nil
-                   else
-                     miq_server_id = parse_id(data['miq_server'], :servers)
-                     raise 'Must specify a valid miq_server href or id' unless miq_server_id
-                     resource_search(miq_server_id, :servers)
-                   end
-
-      vm.miq_server = miq_server
-      action_result(true, "#{miq_server_message(miq_server)} for #{vm_ident(vm)}")
-    rescue => err
-      action_result(false, "Failed to set miq_server - #{err}")
+      if data['miq_server'].empty?
+        api_resource(type, id, "Removing miq_server from") do |vm|
+          vm.miq_server = nil
+          {}
+        end
+      else
+        api_resource(type, id, "Setting miq_server for") do |vm|
+          miq_server_id = parse_id(data['miq_server'], :servers)
+          raise BadRequestError, 'Must specify a valid miq_server href or id' unless miq_server_id
+
+          miq_server = resource_search(miq_server_id, :servers)
+          vm.miq_server = miq_server
+          {}
+        end
+      end
     end
 
     def request_retire_resource(type, id = nil, data = nil)
@@ -200,69 +192,11 @@ def options
 
     private
 
-    def miq_server_message(miq_server)
-      miq_server ? "Set miq_server id:#{miq_server.id}" : "Removed miq_server"
-    end
-
-    def vm_ident(vm)
-      "VM id:#{vm.id} name:'#{vm.name}'"
-    end
-
-    def validate_vm_for_action(vm, action)
-      action_result(vm.supports?(action), vm.unsupported_reason(action.to_sym))
-    end
-
-    def validate_vm_for_remote_console(vm, protocol = nil)
-      protocol ||= "mks"
-      vm.validate_remote_console_acquire_ticket(protocol)
-      action_result(true, "")
-    rescue MiqException::RemoteConsoleNotSupportedError => err
-      action_result(false, err.message)
-    end
-
-    def set_owner_vm(vm, owner)
-      desc = "#{vm_ident(vm)} setting owner to '#{owner}'"
-      user = User.lookup_by_identity(owner)
-      raise "Invalid user #{owner} specified" unless user
-      vm.evm_owner = user
-      vm.miq_group = user.current_group unless user.nil?
-      vm.save!
-      action_result(true, desc)
-    rescue => err
-      action_result(false, err.to_s)
-    end
-
-    def add_lifecycle_event_vm(vm, lifecycle_event)
-      desc = "#{vm_ident(vm)} adding lifecycle event=#{lifecycle_event['event']} message=#{lifecycle_event['message']}"
-      event = LifecycleEvent.create_event(vm, lifecycle_event)
-      action_result(event.present?, desc)
-    rescue => err
-      action_result(false, err.to_s)
-    end
-
     def lifecycle_event_from_data(data)
       data ||= {}
       data = data.slice("event", "status", "message", "created_by")
       data.keys.each { |k| data[k] = data[k].to_s }
       data
     end
-
-    def vm_event(vm, event_type, event_message, event_time)
-      desc = "Adding Event type=#{event_type} message=#{event_message}"
-      event_timestamp = event_time.blank? ? Time.now.utc : event_time.to_time(:utc)
-
-      vm.add_ems_event(event_type, event_message, event_timestamp)
-      action_result(true, desc)
-    rescue => err
-      action_result(false, err.to_s)
-    end
-
-    def rename_vm(vm, new_name)
-      desc = "#{vm_ident(vm)} renaming to #{new_name}"
-      task_id = vm.rename_queue(User.current_user.userid, new_name)
-      action_result(true, desc, :task_id => task_id)
-    rescue => err
-      action_result(false, err.to_s)
-    end
   end
 end

Diff for: spec/requests/vms_spec.rb

@@ -1046,15 +1046,15 @@ def query_match_regexp(*tables)
 
       post(vm_url, :params => gen_request(:set_owner, "owner" => "bad_user"))
 
-      expect_single_action_result(:success => false, :message => /.*/, :href => api_vm_url(nil, vm))
+      expect_bad_request(/Invalid user/)
     end
 
     it "set_owner to a vm" do
       api_basic_authorize action_identifier(:vms, :set_owner)
 
       post(vm_url, :params => gen_request(:set_owner, "owner" => @user.userid))
 
-      expect_single_action_result(:success => true, :message => "setting owner", :href => api_vm_url(nil, vm))
+      expect_single_action_result(:success => true, :message => /Setting owner/i, :href => api_vm_url(nil, vm))
       expect(vm.reload.evm_owner).to eq(@user)
     end
 
@@ -1063,7 +1063,7 @@ def query_match_regexp(*tables)
 
       post(api_vms_url, :params => gen_request(:set_owner, {"owner" => @user.userid}, vm1_url, vm2_url))
 
-      expect_multiple_action_result(2)
+      expect_multiple_action_result(2, :success => true, :message => /Setting owner/i)
       expect_result_resources_to_include_hrefs("results", [api_vm_url(nil, vm1), api_vm_url(nil, vm2)])
       expect(vm1.reload.evm_owner).to eq(@user)
       expect(vm2.reload.evm_owner).to eq(@user)
@@ -1218,7 +1218,7 @@ def query_match_regexp(*tables)
 
       post(vm_url, :params => gen_request(:add_lifecycle_event, events[0]))
 
-      expect_single_action_result(:success => true, :message => /adding lifecycle event/i, :href => api_vm_url(nil, vm))
+      expect_single_action_result(:success => true, :message => /adding life cycle event/i, :href => api_vm_url(nil, vm))
       expect(vm.lifecycle_events.size).to eq(1)
       expect(vm.lifecycle_events.first.event).to eq(events[0][:event])
     end
@@ -1229,7 +1229,7 @@ def query_match_regexp(*tables)
       post(api_vms_url, :params => gen_request(:add_lifecycle_event,
                                                events.collect { |e| {:href => vm_url}.merge(e) }))
 
-      expect_multiple_action_result(3)
+      expect_multiple_action_result(3, :success => true)
       expect(vm.lifecycle_events.size).to eq(events.size)
       expect(vm.lifecycle_events.collect(&:event)).to match_array(events.collect { |e| e[:event] })
     end
@@ -1307,22 +1307,8 @@ def query_match_regexp(*tables)
         )
       )
 
-      expected = {
-        "results" => a_collection_containing_exactly(
-          {
-            "message" => a_string_matching(/adding event .*etype1/i),
-            "success" => true,
-            "href"    => api_vm_url(nil, vm1)
-          },
-          {
-            "message" => a_string_matching(/adding event .*etype2/i),
-            "success" => true,
-            "href"    => api_vm_url(nil, vm2)
-          }
-        )
-      }
-      expect(response.parsed_body).to include(expected)
-      expect(response).to have_http_status(:ok)
+      expect_multiple_action_result(2, :success => true, :message => /Adding Event/i)
+      expect_result_resources_to_include_hrefs("results", [api_vm_url(nil, vm1), api_vm_url(nil, vm2)])
     end
   end
 
@@ -1940,12 +1926,7 @@ def query_match_regexp(*tables)
 
       post(api_vm_url(nil, vm), :params => { :action => 'set_miq_server', :miq_server => { :href => api_server_url(nil, server)} })
 
-      expected = {
-        'success' => true,
-        'message' => "Set miq_server id:#{server.id} for VM id:#{vm.id} name:'#{vm.name}'"
-      }
-      expect(response).to have_http_status(:ok)
-      expect(response.parsed_body).to eq(expected)
+      expect_single_action_result(:success => true, :message => /Setting miq_server for Vm id: #{vm.id} name: '#{vm.name}'/)
       expect(vm.reload.miq_server).to eq(server)
     end
 
@@ -1963,14 +1944,7 @@ def query_match_regexp(*tables)
         }
       )
 
-      expected = {
-        'results' => [
-          { 'success' => true, 'message' => "Set miq_server id:#{server.id} for VM id:#{vm.id} name:'#{vm.name}'" },
-          { 'success' => true, 'message' => "Set miq_server id:#{server2.id} for VM id:#{vm1.id} name:'#{vm1.name}'" }
-        ]
-      }
-      expect(response).to have_http_status(:ok)
-      expect(response.parsed_body).to eq(expected)
+      expect_multiple_action_result(2, :success => true, :message => /Setting miq_server for Vm/i)
       expect(vm.reload.miq_server).to eq(server)
       expect(vm1.reload.miq_server).to eq(server2)
     end
@@ -1979,14 +1953,10 @@ def query_match_regexp(*tables)
       api_basic_authorize action_identifier(:vms, :set_miq_server)
 
       post(api_vm_url(nil, vm), :params => { :action => 'set_miq_server', :miq_server => { :href => api_vm_url(nil, 1) } })
-
-      expected = { 'success' => false, 'message' => 'Failed to set miq_server - Must specify a valid miq_server href or id' }
-      expect(response.parsed_body).to eq(expected)
-      expect(response).to have_http_status(:bad_request)
+      expect_bad_request(/Must specify a valid miq_server href or id/)
 
       post(api_vm_url(nil, vm), :params => { :action => 'set_miq_server', :miq_server => { :id => nil } })
-      expect(response.parsed_body).to eq(expected)
-      expect(response).to have_http_status(:bad_request)
+      expect_bad_request(/Must specify a valid miq_server href or id/)
     end
 
     it "can unassign a server if an empty hash is passed" do
@@ -1995,9 +1965,7 @@ def query_match_regexp(*tables)
 
       post(api_vm_url(nil, vm), :params => { :action => 'set_miq_server', :miq_server => {} })
 
-      expected = {'success' => true, 'message' => "Removed miq_server for VM id:#{vm.id} name:'#{vm.name}'"}
-      expect(response.parsed_body).to eq(expected)
-      expect(vm.reload.miq_server).to be_nil
+      expect_single_action_result(:success => true, :message => "Removing miq_server from Vm id: #{vm.id} name: '#{vm.name}'")
     end
   end