commit 4d4b84f6b722874504f508fa92802f6ddee37c11
Author: Ohad Levy <ohadlevy@gmail.com>
Date:   Mon Jun 20 14:07:32 2011 +0300

    fixes #35 - Allow hostgroup nesting for puppet classes and parameters

diff --git a/.gitmodules b/.gitmodules
index 1b4c30a4a..232646955 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -28,3 +28,6 @@
 [submodule "vendor/plugins/scoped_search"]
 	path = vendor/plugins/scoped_search
 	url = git://github.com/wvanbergen/scoped_search.git
+[submodule "vendor/plugins/ancestry"]
+	path = vendor/plugins/ancestry
+	url = git://github.com/stefankroes/ancestry.git
diff --git a/app/controllers/hostgroups_controller.rb b/app/controllers/hostgroups_controller.rb
index 94c676d71..1132bcc7a 100644
--- a/app/controllers/hostgroups_controller.rb
+++ b/app/controllers/hostgroups_controller.rb
@@ -8,7 +8,6 @@ class HostgroupsController < ApplicationController
   def index
     begin
       values = Hostgroup.search_for(params[:search],:order => params[:order])
-      flash.clear
     rescue => e
       error e.to_s
       values = Hostgroup.search_for ""
@@ -26,6 +25,11 @@ class HostgroupsController < ApplicationController
     @hostgroup = Hostgroup.new
   end
 
+  def nest
+    @hostgroup = Hostgroup.new(:parent_id => params[:id])
+    render :action => :new
+  end
+
   # Clone the hostgroup
   def clone
     new = @hostgroup.clone
diff --git a/app/helpers/hostgroups_helper.rb b/app/helpers/hostgroups_helper.rb
index eb36f1cff..d6b9de573 100644
--- a/app/helpers/hostgroups_helper.rb
+++ b/app/helpers/hostgroups_helper.rb
@@ -1,4 +1,14 @@
 module HostgroupsHelper
   include CommonParametersHelper
   include HostsAndHostgroupsHelper
+
+  def warning_message group
+    msg = ["Are you sure?"]
+    if group.has_children?
+      msg << "This group has nested groups!\n"
+      msg << "Deleting this group will unlink its nested groups and any associated puppet classes and / or parameters"
+    end
+    msg.join("\n")
+  end
+
 end
diff --git a/app/helpers/hosts_and_hostgroups_helper.rb b/app/helpers/hosts_and_hostgroups_helper.rb
index 543669e3f..2a7d93daa 100644
--- a/app/helpers/hosts_and_hostgroups_helper.rb
+++ b/app/helpers/hosts_and_hostgroups_helper.rb
@@ -38,6 +38,17 @@ module HostsAndHostgroupsHelper
     end
   end
 
+  def hostgroup_name group
+    return if group.blank?
+    content_tag(:span, group.to_s.gsub(group.name, ""), :class => "grey") +
+      link_to_if_authorized(h(group.name), hash_for_edit_hostgroup_path(:id => group))
+  end
+
+  def accessible_hostgroups
+    hg = (User.current.hostgroups.any? and !User.current.admin?) ? User.current.hostgroups : Hostgroup.all
+    hg.sort
+  end
+
   def image_file_entry item
     # If the host has an explicit image_path then use that
     # Else use the default based upon the host's medium and operatingsystem
diff --git a/app/helpers/hosts_helper.rb b/app/helpers/hosts_helper.rb
index 868964e98..4b0beaa51 100644
--- a/app/helpers/hosts_helper.rb
+++ b/app/helpers/hosts_helper.rb
@@ -83,10 +83,6 @@ module HostsHelper
     (User.current.domains.any? and !User.current.admin?) ? User.current.domains : Domain.all
   end
 
-  def accessible_hostgroups
-    (User.current.hostgroups.any? and !User.current.admin?) ? User.current.hostgroups : Hostgroup.all
-  end
-
   def update_details_from_hostgroup
     return nil unless @host.new_record?
     remote_function(:url => { :action => "process_hostgroup" },
diff --git a/app/models/host.rb b/app/models/host.rb
index 6e54a15f8..a3fa6c3e1 100644
--- a/app/models/host.rb
+++ b/app/models/host.rb
@@ -254,7 +254,7 @@ class Host < Puppet::Rails::Host
   end
 
   def all_puppetclasses
-    return hostgroup.nil? ? puppetclasses : (hostgroup.puppetclasses + puppetclasses).uniq
+    return hostgroup.nil? ? puppetclasses : (hostgroup.classes + puppetclasses).uniq
   end
 
   # provide information about each node, mainly used for puppet external nodes
@@ -288,7 +288,7 @@ class Host < Puppet::Rails::Host
     # read OS parameters
     operatingsystem.os_parameters.each {|p| parameters.update Hash[p.name => p.value] } unless operatingsystem.nil?
     # read group parameters only if a host belongs to a group
-    hostgroup.group_parameters.each {|p| parameters.update Hash[p.name => p.value] } unless hostgroup.nil?
+    parameters.update hostgroup.parameters unless hostgroup.nil?
     # and now read host parameters, override if required
     host_parameters.each {|p| parameters.update Hash[p.name => p.value] }
     return parameters
diff --git a/app/models/hostgroup.rb b/app/models/hostgroup.rb
index 134f0af96..5deeda3ae 100644
--- a/app/models/hostgroup.rb
+++ b/app/models/hostgroup.rb
@@ -1,9 +1,10 @@
 class Hostgroup < ActiveRecord::Base
+  has_ancestry :orphan_strategy => :rootify
   include Authorization
   include HostCommon
   has_and_belongs_to_many :puppetclasses
   has_and_belongs_to_many :users, :join_table => "user_hostgroups"
-  validates_uniqueness_of :name
+  validates_uniqueness_of :name, :scope => :ancestry
   validates_format_of :name, :with => /\A(\S+\s?)+\Z/, :message => "can't be blank or contain trailing white spaces."
   has_many :group_parameters, :dependent => :destroy, :foreign_key => :reference_id
   accepts_nested_attributes_for :group_parameters, :reject_if => lambda { |a| a[:value].blank? }, :allow_destroy => true
@@ -43,11 +44,17 @@ class Hostgroup < ActiveRecord::Base
   #TODO: add a method that returns the valid os for a hostgroup
 
   def all_puppetclasses
-    puppetclasses
+    classes
+  end
+
+  alias_method :to_label, :to_s
+
+  def to_label
+    "/" + ancestors.map{|a| a.name + "/"}.join + name
   end
 
   def as_json(options={})
-    super({:only => [:name, :id, :value], :include => [:puppetclasses, :group_parameters, :environment]}.merge(options))
+    super({:only => [:name, :id], :methods => [:classes, :parameters], :include => [:puppetclasses, :group_parameters, :environment]}.merge(options))
   end
 
   def hostgroup
@@ -58,6 +65,27 @@ class Hostgroup < ActiveRecord::Base
     ptable.layout
   end
 
+  def classes
+    klasses = []
+    ids = ancestor_ids
+    ids << id unless new_record? or self.frozen?
+    Hostgroup.sort_by_ancestry(Hostgroup.find(ids, :include => :puppetclasses)).each do |hg|
+      klasses << hg.puppetclasses
+    end
+    klasses.flatten
+  end
+
+  # returns self and parent parameters as a hash
+  def parameters
+    hash = {}
+    ids = ancestor_ids
+    ids << id unless new_record? or self.frozen?
+    Hostgroup.sort_by_ancestry(Hostgroup.find(ids, :include => :group_parameters)).each do |hg|
+      hg.group_parameters.each {|p| hash[p.name] = p.value }
+    end
+    hash
+  end
+
   def params
     parameters = {}
     # read common parameters
@@ -65,7 +93,7 @@ class Hostgroup < ActiveRecord::Base
     # read OS parameters
     operatingsystem.os_parameters.each {|p| parameters.update Hash[p.name => p.value] } unless operatingsystem.nil?
     # read group parameters only if a host belongs to a group
-    hostgroup.group_parameters.each {|p| parameters.update Hash[p.name => p.value] } unless hostgroup.nil?
+    parameters.update self.parameters unless hostgroup.nil?
     parameters
   end
 
diff --git a/app/views/hostgroups/_form.html.erb b/app/views/hostgroups/_form.html.erb
index 95a885fd6..2e25d6895 100644
--- a/app/views/hostgroups/_form.html.erb
+++ b/app/views/hostgroups/_form.html.erb
@@ -1,10 +1,16 @@
 <% form_for @hostgroup do |f| %>
   <%= f.error_messages %>
   <p>
+    <%= f.label :parent_id, "Parent", :class => "span-3" %>
+    <%= f.collection_select(:parent_id, accessible_hostgroups, :id, :to_label, { :include_blank => true },
+                            { :class => "span-3" }) %>
+
     <%= f.label :name, nil, :class => "span-3" %>
-    <%= f.text_field :name, :class => "last" %>
+    <%= f.text_field :name, :class => "span-3" %>
   </p>
 
+  <br>
+
   <% field_set_tag 'Host Defaults' do -%>
 
     <% field_set_tag 'Primary Settings' do %>
diff --git a/app/views/hostgroups/index.html.erb b/app/views/hostgroups/index.html.erb
index cd242ab80..2c704047b 100644
--- a/app/views/hostgroups/index.html.erb
+++ b/app/views/hostgroups/index.html.erb
@@ -7,12 +7,13 @@
     <th>Name</th>
     <th></th>
   </tr>
-  <% for hostgroup in @hostgroups %>
+  <% Hostgroup.sort_by_ancestry(@hostgroups).each do |hostgroup| %>
     <tr class="<%= cycle("even", "odd") -%>">
-      <td><%=link_to_if_authorized h(hostgroup.name), hash_for_edit_hostgroup_path(:id => hostgroup) %></td>
+      <td> <%= hostgroup_name hostgroup %> </td>
       <td class="ra">
+        <%= display_link_if_authorized 'Nest',    hash_for_nest_hostgroup_path(:id => hostgroup) %>
         <%= display_link_if_authorized 'Clone',   hash_for_clone_hostgroup_path(:id => hostgroup) %>
-        <%= display_link_if_authorized "Destroy", hash_for_hostgroup_path(:id => hostgroup, :auth_action => :destroy), :confirm => 'Are you sure?', :method => :delete %>
+        <%= display_link_if_authorized "Destroy", hash_for_hostgroup_path(:id => hostgroup, :auth_action => :destroy), :confirm => warning_message(hostgroup), :method => :delete %>
       </td>
     </tr>
   <% end %>
diff --git a/app/views/hosts/_form.html.erb b/app/views/hosts/_form.html.erb
index 33c626f01..f7302e566 100644
--- a/app/views/hosts/_form.html.erb
+++ b/app/views/hosts/_form.html.erb
@@ -6,12 +6,12 @@
       <%= f.label :name %>
       <%= f.text_field :name, :value => @host.shortname, :title => @host.to_s, :size => 12 %>
     </div>
-    <div class="span-6">
+    <div class="span-3">
       <%= f.label :hostgroup_id, "Host group" %>
       <%= image_tag "spinner.gif", :id => "indicator1", :style => "display:none;" %>
-      <%= f.collection_select(:hostgroup_id, accessible_hostgroups, :id, :name, { :include_blank => true },
-                              { :onchange => update_details_from_hostgroup }) %>
     </div>
+    <%= f.collection_select(:hostgroup_id, accessible_hostgroups, :id, :to_label, { :include_blank => true },
+                            { :onchange => update_details_from_hostgroup, :class => "span-3" }) %>
     <div class="span-6">
       <%= f.label :environment_id %>
       <%= f.collection_select :environment_id, Environment.all, :id, :to_label, { :include_blank => true } %>
diff --git a/app/views/hosts/_list.html.erb b/app/views/hosts/_list.html.erb
index 36e73e1f3..08ae378a4 100644
--- a/app/views/hosts/_list.html.erb
+++ b/app/views/hosts/_list.html.erb
@@ -20,7 +20,7 @@
       <td><%= icon(host.os, :size => "18x18", :title => host.os.to_s) + " " + host.os.to_s if host.os %></td>
       <td><%= h host.try(:environment) %></td>
       <td><%= h host.try(:model) %></td>
-      <td><%= h host.try(:hostgroup) %></td>
+      <td><%= hostgroup_name host.hostgroup %></td>
       <td><%= last_report_column(host) %></td>
       <td>
         <%= display_link_if_authorized 'Edit', hash_for_edit_host_path(:id => host) %>
diff --git a/app/views/puppetclasses/_class_selection.html.erb b/app/views/puppetclasses/_class_selection.html.erb
index 79903989a..ad9211335 100644
--- a/app/views/puppetclasses/_class_selection.html.erb
+++ b/app/views/puppetclasses/_class_selection.html.erb
@@ -12,7 +12,7 @@
           :collection => obj.puppetclasses ,:as => :klass,
           :locals => { :type => obj.class.to_s.downcase } %>
       </div>
-      <% if (klasses = obj.hostgroup.try(:puppetclasses)).is_a?(Array) and obj.is_a?(Host) -%>
+      <% if (klasses = obj.hostgroup.try(:classes)).is_a?(Array) and obj.is_a?(Host) -%>
         <% for klass in klasses -%>
           <li title="included already from host group"><%= h klass.name %></li>
         <% end -%>
diff --git a/config/routes.rb b/config/routes.rb
index d9e318fdb..30897c6cb 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -37,7 +37,7 @@ ActionController::Routing::Routes.draw do |map|
   map.resources :puppetclasses, :member => { :assign => :post }, :collection => {:import_environments => :get, :auto_complete_search => :get} do |pc|
     pc.resources :hosts, :requirements => {:id => /[^\/]+/}
   end
-  map.resources :hostgroups, :member => { :clone => :get }, :collection => { :auto_complete_search => :get }
+  map.resources :hostgroups, :member => { :nest => :get, :clone => :get }, :collection => { :auto_complete_search => :get }
   map.resources :common_parameters
   map.resources :environments, :collection => {:import_environments => :get, :obsolete_and_new => :post}
   map.resources :fact_values, :only => [:create, :index], :collection => { :auto_complete_search => :get }
diff --git a/db/migrate/20110619130336_add_ancestry_to_hostgroup.rb b/db/migrate/20110619130336_add_ancestry_to_hostgroup.rb
new file mode 100644
index 000000000..c3d31256e
--- /dev/null
+++ b/db/migrate/20110619130336_add_ancestry_to_hostgroup.rb
@@ -0,0 +1,11 @@
+class AddAncestryToHostgroup < ActiveRecord::Migration
+  def self.up
+    add_column :hostgroups, :ancestry, :string
+    add_index :hostgroups, :ancestry
+  end
+
+  def self.down
+    remove_index :hostgroups, :ancestry
+    remove_column :hostgroups, :ancestry
+  end
+end
diff --git a/public/stylesheets/style.css b/public/stylesheets/style.css
index 6ed6159f1..3eccde19f 100644
--- a/public/stylesheets/style.css
+++ b/public/stylesheets/style.css
@@ -627,3 +627,7 @@ a.button:active span {
     height: 25px;
 }
 .menu-dropdown {text-transform: capitalize;}
+
+.grey {
+  color:#aaa;
+}
diff --git a/test/functional/hostgroups_controller_test.rb b/test/functional/hostgroups_controller_test.rb
index 1e2522485..d82e4870d 100644
--- a/test/functional/hostgroups_controller_test.rb
+++ b/test/functional/hostgroups_controller_test.rb
@@ -19,6 +19,12 @@ class HostgroupsControllerTest < ActionController::TestCase
     assert_template 'new'
   end
 
+  def test_nest
+    get :nest, {:id => Hostgroup.first.id}, set_session_user
+    assert_template 'new'
+  end
+
+
   def test_create_invalid
     Hostgroup.any_instance.stubs(:valid?).returns(false)
     post :create, {}, set_session_user
@@ -71,15 +77,15 @@ class HostgroupsControllerTest < ActionController::TestCase
   end
 
   def test_destroy
-    hostgroup = Hostgroup.first
-    delete :destroy, {:id => hostgroup}, set_session_user
+    hostgroup = hostgroups(:unusual)
+    delete :destroy, {:id => hostgroup.id}, set_session_user
     assert_redirected_to hostgroups_url
     assert !Hostgroup.exists?(hostgroup.id)
   end
 
   def test_destroy_json
-    hostgroup = Hostgroup.first
-    delete :destroy, {:format => "json", :id => hostgroup}, set_session_user
+    hostgroup = hostgroups(:common)
+    delete :destroy, {:format => "json", :id => hostgroup.id}, set_session_user
     template = ActiveSupport::JSON.decode(@response.body)
     assert_response :ok
     assert !Hostgroup.exists?(hostgroup.id)
diff --git a/test/unit/hostgroup_test.rb b/test/unit/hostgroup_test.rb
index 47e407d33..ffd873818 100644
--- a/test/unit/hostgroup_test.rb
+++ b/test/unit/hostgroup_test.rb
@@ -87,4 +87,57 @@ class HostgroupTest < ActiveSupport::TestCase
     assert record.valid?
   end
 
+  test "should be able to nest a group parameters" do
+    # creates a 3 level hirecy, each one with his own parameters
+    # and overrides.
+    pid = Time.now.to_i
+    assert (top = Hostgroup.create(:name => "topA", :group_parameters_attributes => {
+      pid += 1=>{"name"=>"topA", "value"=>"1", :nested => ""},
+      pid += 1=>{"name"=>"topB", "value"=>"1", :nested => ""},
+      pid += 1=>{"name"=>"topC", "value"=>"1", :nested => ""},
+    }))
+    assert (second = Hostgroup.create(:name => "SecondA", :parent_id => top.id, :group_parameters_attributes => {
+      pid += 1 =>{"name"=>"topA", "value"=>"2", :nested => ""},
+      pid += 1 =>{"name"=>"secondA", "value"=>"2", :nested => ""}}))
+
+    assert second.parameters.include? "topA"
+    assert_equal "2", second.parameters["topA"]
+    assert second.parameters.include? "topB"
+    assert_equal "1", second.parameters["topB"]
+    assert second.parameters.include? "topC"
+    assert_equal "1", second.parameters["topC"]
+    assert second.parameters.include? "secondA"
+    assert_equal "2", second.parameters["secondA"]
+
+    assert (third = Hostgroup.create(:name => "ThirdA", :parent_id => second.id, :group_parameters_attributes => {
+      pid += 1 =>{"name"=>"topB", "value"=>"3", :nested => ""},
+      pid += 1 =>{"name"=>"topA", "value"=>"3", :nested => ""}}))
+
+    assert third.parameters.include? "topA"
+    assert_equal "3", third.parameters["topA"]
+    assert third.parameters.include? "topB"
+    assert_equal "3", third.parameters["topB"]
+    assert third.parameters.include? "topC"
+    assert_equal "1", third.parameters["topC"]
+    assert third.parameters.include? "secondA"
+    assert_equal "2", third.parameters["secondA"]
+  end
+
+  test "should inheirt parent classes" do
+   assert (top = Hostgroup.create(:name => "topA", "puppetclass_ids"=>[Puppetclass.first.id]))
+   assert (second = Hostgroup.create(:name => "secondB", :parent_id => top.id, "puppetclass_ids"=>[Puppetclass.last.id]))
+
+   assert_equal [Puppetclass.first, Puppetclass.last].sort, second.classes.sort
+  end
+
+  test "should remove relationships if deleting a parent hostgroup" do
+   assert (top = Hostgroup.create(:name => "topA"))
+   assert (second = Hostgroup.create(:name => "secondB", :parent_id => top.id))
+
+   assert top.has_children?
+   assert !second.is_root?
+   assert top.destroy
+   assert Hostgroup.find(second.id).is_root?
+  end
+
 end
diff --git a/vendor/plugins/ancestry b/vendor/plugins/ancestry
new file mode 160000
index 000000000..770f2e36e
--- /dev/null
+++ b/vendor/plugins/ancestry
@@ -0,0 +1 @@
+Subproject commit 770f2e36eb8b9865ee0ea1f81f5ca21068bebe3e
