Merge pull request #40 from stbenjam/bugs2

Fix discovery and an inheritance issue

app/controllers/foreman_salt/minions_controller.rb

@@ -69,7 +69,7 @@ def load_ajax_vars
       @minion = Host::Base.authorized(:view_hosts, Host).find_by_id(params[:host_id])
       if @minion
         unless @minion.is_a?(Host::Managed)
-          @minion      = @host.becomes(Host::Managed)
+          @minion      = @minion.becomes(Host::Managed)
           @minion.type = 'Host::Managed'
         end
         @minion.attributes = params[:host]

app/models/foreman_salt/concerns/host_managed_extensions.rb

@@ -33,10 +33,9 @@ def params_with_salt_proxy
       end
 
       def salt_modules_for_enc
-        return [] unless self.salt_environment
-
-        hostgroup_modules = self.hostgroup ? self.hostgroup.all_salt_modules : []
-        self.salt_environment.salt_modules.where(:id => self.salt_modules + hostgroup_modules).pluck(:name)
+        return [] unless salt_environment
+        modules = salt_modules + (hostgroup ? hostgroup.all_salt_modules : [])
+        ForemanSalt::SaltModule.in_environment(salt_environment).where(:id => modules).pluck(:name).uniq
       end
 
       def salt_master

app/models/foreman_salt/concerns/hostgroup_extensions.rb

@@ -16,23 +16,15 @@ module HostgroupExtensions
       end
 
       def all_salt_modules
-        if ancestry.present?
-          (self.salt_modules + self.inherited_salt_modules).uniq
-        else
-          self.salt_modules
-        end
+        ForemanSalt::SaltModule.in_environment(salt_environment).where(:id => salt_module_ids + inherited_salt_module_ids)
       end
 
       def inherited_salt_modules
         ForemanSalt::SaltModule.where(:id => inherited_salt_module_ids)
       end
 
       def inherited_salt_module_ids
-        if ancestry.present?
-          self.class.sort_by_ancestry(ancestors.reject { |ancestor| ancestor.salt_module_ids.empty? }).map(&:salt_module_ids).inject(&:+).uniq
-        else
-          []
-        end
+        ancestors.map(&:salt_module_ids).flatten.uniq
       end
 
       def salt_proxy

test/factories/foreman_salt_factories.rb

@@ -4,24 +4,25 @@
   end
 
   factory :salt_environment, :class => 'ForemanSalt::SaltEnvironment' do
-    sequence(:name) { |n| "module#{n}" }
+    sequence(:name) { |n| "environment#{n}" }
   end
 end
 
 FactoryGirl.modify do
   factory :host do
     trait :with_salt_proxy do
-      salt_proxy { FactoryGirl.create :smart_proxy, :with_salt_feature }
+      salt_proxy { FactoryGirl.build :smart_proxy, :with_salt_feature }
     end
   end
 
   factory :hostgroup do
     trait :with_salt_proxy do
-      salt_proxy { FactoryGirl.create :smart_proxy, :with_salt_feature }
+      salt_proxy { FactoryGirl.build :smart_proxy, :with_salt_feature }
     end
 
     trait :with_salt_modules do
-      salt_modules { FactoryGirl.create_list :salt_module, 10 }
+      salt_environment { FactoryGirl.build :salt_environment }
+      salt_modules { FactoryGirl.create_list :salt_module, 10, :salt_environments => [self.salt_environment] }
     end
   end
 

test/unit/hostgroup_extensions_test.rb

@@ -25,16 +25,32 @@ class HostgroupExtensionsTest < ActiveSupport::TestCase
     end
 
     test 'child and parent salt modules are combined' do
+      environment = FactoryGirl.create :salt_environment
+      parent = FactoryGirl.create :hostgroup, :with_salt_modules, :salt_environment => environment
+      child = FactoryGirl.create :hostgroup, :with_salt_modules, :salt_environment => environment, :parent => parent
+
+      total = parent.salt_modules.count + child.salt_modules.count
+      assert_equal total, child.all_salt_modules.count
+    end
+
+    test 'child doesnt get modules from outside its environment' do
       parent = FactoryGirl.create :hostgroup, :with_salt_modules
       child = FactoryGirl.create :hostgroup, :with_salt_modules, :parent => parent
-      assert_equal 10, (child.salt_modules - parent.salt_modules).length
+      assert_equal child.salt_modules.count, child.all_salt_modules.count
     end
 
-    test 'second child inherits from parent' do
+    test 'inheritance when only parent has modules' do
       parent = FactoryGirl.create :hostgroup, :with_salt_modules
       child_one = FactoryGirl.create :hostgroup, :parent => parent
       child_two = FactoryGirl.create :hostgroup, :parent => child_one
-      assert_equal [], parent.all_salt_modules - child_two.all_salt_modules
+      assert_blank parent.all_salt_modules - child_two.all_salt_modules
+    end
+
+    test 'inheritance when no parents have modules' do
+      parent = FactoryGirl.create :hostgroup
+      child_one = FactoryGirl.create :hostgroup, :parent => parent
+      child_two = FactoryGirl.create :hostgroup, :with_salt_modules, :parent => child_one
+      assert child_two.all_salt_modules.any?
     end
   end
 end