commit 693391babe939cc27f30889ea83d506102d36dcf
Author: Dominic Cleal <dcleal@redhat.com>
Date:   Mon May 12 14:26:05 2014 +0100

    fixes #5652 - run parse_args when calling new_vm for compute profiles

diff --git a/app/models/compute_resources/foreman/model/vmware.rb b/app/models/compute_resources/foreman/model/vmware.rb
index 745292c83..c467ae438 100644
--- a/app/models/compute_resources/foreman/model/vmware.rb
+++ b/app/models/compute_resources/foreman/model/vmware.rb
@@ -240,13 +240,6 @@ module Foreman::Model
     end
 
     def parse_args args
-      dc_networks = networks
-      args["interfaces_attributes"].each do |key, interface|
-        # Convert network id into name
-        net = dc_networks.find { |n| n.id == interface["network"] }
-        raise "Unknown Network ID: #{interface["network"]}" if net.nil?
-        interface["network"] = net.name
-      end
       args = args.symbolize_keys
 
       # convert rails nested_attributes into a plain, symbolized hash
@@ -263,12 +256,25 @@ module Foreman::Model
       args
     end
 
-    def create_vm controller_args = { }
-      args = parse_args controller_args.dup
+    # Change network IDs for names only at the point of creation, as IDs are
+    # used in the UI for select boxes etc.
+    def parse_networks args
+      args = args.deep_dup
+      dc_networks = networks
+      args["interfaces_attributes"].each do |key, interface|
+        # Convert network id into name
+        net = dc_networks.find { |n| [n.id, n.name].include?(interface["network"]) }
+        raise "Unknown Network ID: #{interface["network"]}" if net.nil?
+        interface["network"] = net.name
+      end if args["interfaces_attributes"]
+      args
+    end
 
+    def create_vm args = { }
       test_connection
       return unless errors.empty?
 
+      args = parse_networks(args)
       if args[:image_id].present?
         clone_vm(args)
       else
@@ -282,6 +288,7 @@ module Foreman::Model
     end
 
     def new_vm args
+      args = parse_args args
       opts = vm_instance_defaults.symbolize_keys.merge(args.symbolize_keys)
       client.servers.new opts
     end
@@ -301,6 +308,7 @@ module Foreman::Model
     # +searchIndex.FindChild("Resources")+ in RbVmomi that then returns nil
     # because it has no children.
     def clone_vm args
+      args = parse_args args
       path_replace = %r{/Datacenters/#{datacenter}/vm(/|)}
 
       interfaces = client.list_vm_interfaces(args[:image_id])
diff --git a/test/unit/compute_resources/vmware_test.rb b/test/unit/compute_resources/vmware_test.rb
index 9347cc639..98317a1fc 100644
--- a/test/unit/compute_resources/vmware_test.rb
+++ b/test/unit/compute_resources/vmware_test.rb
@@ -2,26 +2,23 @@ require 'test_helper'
 
 class VmwareTest < ActiveSupport::TestCase
   test "#create_vm calls new_vm when network provisioning" do
-    attrs_in = HashWithIndifferentAccess.new("cpus"=>"1", "scsi_controller_type"=>"ParaVirtualSCSIController", "interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"network-17", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"network-17", "_delete"=>""}}, "volumes_attributes"=>{"new_volumes"=>{"size_gb"=>"10", "_delete"=>""}, "0"=>{"size_gb"=>"1", "_delete"=>""}})
-    # All keys must be symbolized
-    attrs_out = {:cpus=>"1", :interfaces=>[{:type=>"VirtualVmxnet3", :network=>"Test network", :_delete=>""}], :volumes=>[{:size_gb=>"1", :_delete=>""}], :scsi_controller=>{:type=>"ParaVirtualSCSIController"}}
+    attrs_in = HashWithIndifferentAccess.new("cpus"=>"1", "interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"network-17", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"network-17", "_delete"=>""}}, "volumes_attributes"=>{"new_volumes"=>{"size_gb"=>"10", "_delete"=>""}, "0"=>{"size_gb"=>"1", "_delete"=>""}})
+    attrs_parsed = HashWithIndifferentAccess.new("cpus"=>"1", "interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"Test network", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"Test network", "_delete"=>""}}, "volumes_attributes"=>{"new_volumes"=>{"size_gb"=>"10", "_delete"=>""}, "0"=>{"size_gb"=>"1", "_delete"=>""}})
 
     mock_vm = mock('vm')
     mock_vm.expects(:save).returns(mock_vm)
-    mock_network = mock('network')
-    mock_network.stubs('id').returns('network-17')
-    mock_network.stubs('name').returns('Test network')
 
     cr = FactoryGirl.build(:vmware_cr)
-    cr.expects(:new_vm).with(attrs_out).returns(mock_vm)
+    cr.expects(:parse_networks).with(attrs_in).returns(attrs_parsed)
+    cr.expects(:new_vm).with(attrs_parsed).returns(mock_vm)
     cr.expects(:test_connection)
-    cr.expects(:networks).returns([mock_network])
     assert_equal mock_vm, cr.create_vm(attrs_in)
   end
 
   test "#new_vm merges defaults with user args and creates server" do
-    attrs_in = {:cpus=>"1", :interfaces=>[{:type=>"VirtualVmxnet3", :network=>"Test network", :_delete=>""}], :volumes=>[{:size_gb=>"1", :_delete=>""}]}
-    attrs_out = {:name => 'test', :cpus=>"1", :interfaces=>[{:type=>"VirtualVmxnet3", :network=>"Test network", :_delete=>""}], :volumes=>[{:size_gb=>"1", :_delete=>""}]}
+    attrs_in = HashWithIndifferentAccess.new("cpus"=>"1", "interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"network-17", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"network-17", "_delete"=>""}}, "volumes_attributes"=>{"new_volumes"=>{"size_gb"=>"10", "_delete"=>""}, "0"=>{"size_gb"=>"1", "_delete"=>""}})
+    attrs_parsed = {:cpus=>"1", :interfaces=>[{:type=>"VirtualVmxnet3", :network=>"network-17", :_delete=>""}], :volumes=>[{:size_gb=>"1", :_delete=>""}]}
+    attrs_out = {:name => 'test', :cpus=>"1", :interfaces=>[{:type=>"VirtualVmxnet3", :network=>"network-17", :_delete=>""}], :volumes=>[{:size_gb=>"1", :_delete=>""}]}
 
     mock_vm = mock('new server')
     mock_servers = mock('client.servers')
@@ -30,6 +27,7 @@ class VmwareTest < ActiveSupport::TestCase
     mock_client.expects(:servers).returns(mock_servers)
 
     cr = FactoryGirl.build(:vmware_cr)
+    cr.expects(:parse_args).with(attrs_in).returns(attrs_parsed)
     cr.expects(:vm_instance_defaults).returns(HashWithIndifferentAccess.new(:name => 'test', :cpus => '2', :interfaces => [mock('iface')], :volumes => [mock('vol')]))
     cr.expects(:client).returns(mock_client)
     assert_equal mock_vm, cr.new_vm(attrs_in)
@@ -37,18 +35,75 @@ class VmwareTest < ActiveSupport::TestCase
 
   test "#create_vm calls clone_vm when image provisioning" do
     attrs_in = HashWithIndifferentAccess.new("image_id"=>"2","cpus"=>"1", "interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"network-17", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"network-17", "_delete"=>""}}, "volumes_attributes"=>{"new_volumes"=>{"size_gb"=>"10", "_delete"=>""}, "0"=>{"size_gb"=>"1", "_delete"=>""}})
-    # All keys must be symbolized
-    attrs_out = {:image_id=>"2", :cpus=>"1", :interfaces=>[{:type=>"VirtualVmxnet3", :network=>"Test network", :_delete=>""}], :volumes=>[{:size_gb=>"1", :_delete=>""}]}
+    attrs_parsed = HashWithIndifferentAccess.new("image_id"=>"2","cpus"=>"1", "interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"Test network", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"Test network", "_delete"=>""}}, "volumes_attributes"=>{"new_volumes"=>{"size_gb"=>"10", "_delete"=>""}, "0"=>{"size_gb"=>"1", "_delete"=>""}})
 
     mock_vm = mock('vm')
-    mock_network = mock('network')
-    mock_network.stubs('id').returns('network-17')
-    mock_network.stubs('name').returns('Test network')
-
     cr = FactoryGirl.build(:vmware_cr)
-    cr.expects(:clone_vm).with(attrs_out).returns(mock_vm)
+    cr.expects(:parse_networks).with(attrs_in).returns(attrs_parsed)
+    cr.expects(:clone_vm).with(attrs_parsed).returns(mock_vm)
     cr.expects(:test_connection)
-    cr.expects(:networks).returns([mock_network])
     assert_equal mock_vm, cr.create_vm(attrs_in)
   end
+
+  describe "#parse_args" do
+    setup do
+      @cr = FactoryGirl.build(:vmware_cr)
+    end
+
+    test "converts empty hash" do
+      assert_equal({}, @cr.parse_args(HashWithIndifferentAccess.new))
+    end
+
+    test "converts form attrs to fog attrs" do
+      attrs_in = HashWithIndifferentAccess.new("cpus"=>"1", "interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"network-17", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"network-17", "_delete"=>""}}, "volumes_attributes"=>{"new_volumes"=>{"size_gb"=>"10", "_delete"=>""}, "0"=>{"size_gb"=>"1", "_delete"=>""}})
+      # All keys must be symbolized
+      attrs_out = {:cpus=>"1", :interfaces=>[{:type=>"VirtualVmxnet3", :network=>"network-17", :_delete=>""}], :volumes=>[{:size_gb=>"1", :_delete=>""}]}
+      assert_equal attrs_out, @cr.parse_args(attrs_in)
+    end
+
+    test "converts scsi_controller_type to hash" do
+      attrs_in = HashWithIndifferentAccess.new("cpus"=>"1", "scsi_controller_type"=>"ParaVirtualSCSIController", "interfaces_attributes"=>{}, "volumes_attributes"=>{})
+      attrs_out = {:cpus=>"1", :interfaces=>[], :volumes=>[], :scsi_controller=>{:type=>"ParaVirtualSCSIController"}}
+      assert_equal attrs_out, @cr.parse_args(attrs_in)
+    end
+
+    test "doesn't modify input hash" do
+      # else compute profiles won't save properly
+      attrs_in = HashWithIndifferentAccess.new("interfaces_attributes"=>{"0"=>{"network"=>"network-17"}})
+      @cr.parse_args(attrs_in)
+      assert_equal "network-17", attrs_in["interfaces_attributes"]["0"]["network"]
+    end
+  end
+
+  describe "#parse_networks" do
+    setup do
+      @mock_network = mock('network')
+      @mock_network.stubs('id').returns('network-17')
+      @mock_network.stubs('name').returns('Test network')
+      @cr = FactoryGirl.build(:vmware_cr)
+      @cr.stubs(:networks).returns([@mock_network])
+    end
+
+    test "converts empty hash" do
+      assert_equal({}, @cr.parse_networks(HashWithIndifferentAccess.new))
+    end
+
+    test "converts form network ID to network name" do
+      attrs_in = HashWithIndifferentAccess.new("interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"network-17", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"network-17", "_delete"=>""}})
+      attrs_out = HashWithIndifferentAccess.new("interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"Test network", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"Test network", "_delete"=>""}})
+      assert_equal attrs_out, @cr.parse_networks(attrs_in)
+    end
+
+    test "ignores existing network names" do
+      attrs = HashWithIndifferentAccess.new("interfaces_attributes"=>{"new_interfaces"=>{"type"=>"VirtualE1000", "network"=>"Test network", "_delete"=>""}, "0"=>{"type"=>"VirtualVmxnet3", "network"=>"Test network", "_delete"=>""}})
+      assert_equal attrs, @cr.parse_networks(attrs)
+    end
+
+    test "doesn't modify input hash" do
+      # else compute profiles won't save properly
+      attrs_in = HashWithIndifferentAccess.new("interfaces_attributes"=>{"0"=>{"network"=>"network-17"}})
+      @cr.parse_args(attrs_in)
+      assert_equal "network-17", attrs_in["interfaces_attributes"]["0"]["network"]
+    end
+  end
 end
