Fixes #15476 - Improve validations for policy

app/controllers/policies_controller.rb

@@ -18,7 +18,7 @@ def index
   end
 
   def new
-    @policy = ::ForemanOpenscap::Policy.new
+    @policy = ::ForemanOpenscap::Policy.new(:wizard_initiated => true)
   end
 
   def show

app/models/foreman_openscap/policy.rb

@@ -4,7 +4,7 @@ class Policy < ActiveRecord::Base
     include Taxonomix
     attr_accessible :description, :name, :period, :scap_content_id, :scap_content_profile_id,
                     :weekday, :day_of_month, :cron_line, :location_ids, :organization_ids,
-                    :current_step, :hostgroup_ids
+                    :current_step, :hostgroup_ids, :wizard_initiated
     attr_writer :current_step
 
     belongs_to :scap_content
@@ -24,14 +24,10 @@ class Policy < ActiveRecord::Base
     validates :name, :presence => true, :uniqueness => true, :format => { :without => /\s/ }
     validate :ensure_needed_puppetclasses
     validates :period, :inclusion => {:in => %w(weekly monthly custom)},
-              :if                 => Proc.new { |policy| policy.new_record? ? policy.step_index > 3 : !policy.id.blank? }
-    validates :weekday, :inclusion => {:in => Date::DAYNAMES.map(&:downcase)},
-              :if                  => Proc.new { |policy| policy.period == 'weekly' && (policy.new_record? ? policy.step_index > 3 : !policy.id.blank?) }
-    validates :day_of_month, :numericality => {:greater_than => 0, :less_than => 32},
-              :if                          => Proc.new { |policy| policy.period == 'monthly'&& (policy.new_record? ? policy.step_index > 3 : !policy.id.blank?) }
-    validate :valid_cron_line
-    validate :ensure_period_specification_present
+              :if                 => Proc.new { |policy| policy.should_validate?(3) }
 
+    validate :valid_cron_line, :valid_weekday, :valid_day_of_month
+    validate :ensure_period_specification_present
 
     after_save :assign_policy_to_hostgroups
     # before_destroy - ensure that the policy has no hostgroups, or classes
@@ -171,8 +167,42 @@ def to_enc
       }.merge(period_enc)
     end
 
+    def should_validate?(step_i)
+      if new_record? && wizard_initiated?
+        step_index > step_i
+      elsif new_record? && !wizard_initiated?
+        true
+      else
+        !id.blank?
+      end
+    end
+
+    def wizard_initiated=(value)
+      @wizard_initiated = value ? true : false
+    end
+
+    def wizard_initiated?
+      @wizard_initiated
+    end
+
+    def update_attributes(hash)
+      case hash['period']
+      when 'monthly'
+        erase_period_attrs(['cron_line', 'weekday'], hash)
+      when 'weekly'
+        erase_period_attrs(['cron_line', 'day_of_month'], hash)
+      when 'custom'
+        erase_period_attrs(['weekday', 'day_of_month'], hash)
+      end
+      super hash
+    end
+
     private
 
+    def erase_period_attrs(attrs, hash)
+      attrs.each { |atrb| hash[atrb] = nil }
+    end
+
     def period_enc
       # get crontab expression as an array (minute hour day_of_month month day_of_week)
       cron_parts = case period
@@ -222,15 +252,30 @@ def ensure_needed_puppetclasses
     end
 
     def cron_line_split
-      cron_line.split(' ')
+      cron_line.to_s.split(' ')
     end
 
     def valid_cron_line
-      return true if period != 'custom' || step_index != 4
+      if period == 'custom' && should_validate?(4)
+        errors.add(:cron_line, _("does not consist of 5 parts separated by space")) unless cron_line_split.size == 5
+      else
+        errors.add(:cron_line, _("should be empty")) unless cron_line.blank?
+      end
+    end
 
-      unless cron_line_split.size == 5
-        errors[:base] << _("Cron line does not consist of 5 parts separated by space")
-        return false
+    def valid_weekday
+      if(period == 'weekly' && should_validate?(3))
+        errors.add(:weekday, _("is not a valid")) unless Date::DAYNAMES.map(&:downcase).include? weekday
+      else
+        errors.add(:weekday, _("should be empty")) unless weekday.blank?
+      end
+    end
+
+    def valid_day_of_month
+      if(period == 'monthly' && should_validate?(3))
+        errors.add(:day_of_month, _("must be between 1 and 31")) if !day_of_month || (day_of_month < 1 || day_of_month > 31)
+      else
+        errors.add(:day_of_month, _("should be empty")) unless day_of_month.blank?
       end
     end
 

app/views/policies/steps/_schedule_form.html.erb

@@ -6,6 +6,6 @@
                { :include_blank => _('Choose period') },
                { :onchange => 'period_selected(this)' }) %>
   <%= select_f(f, :weekday, days_of_week_hash, :first, :last, :include_blank => _('Choose weekday')) %>
-  <%= select_f(f, :day_of_month, (1..31).to_a, :to_i, :to_s, :help_inline => _('Number of a day in month, note that not all months have same count of days')) %>
+  <%= select_f(f, :day_of_month, (1..31).to_a, :to_i, :to_s, :help_inline => _('Number of a day in month, note that not all months have same count of days'), :include_blank => _('Choose day in month')) %>
   <%= text_f(f, :cron_line, :help_inline => _('You can specify custom cron line, e.g. "0  3  *  *  *", separate each of 5 values by space')) %>
 </div>

app/views/policies/steps/_step_form.html.erb

@@ -3,6 +3,7 @@
 <%= form_for @policy,
              :url => (@policy.id? ? policy_path(:id => @policy.id) : policies_path) do |f| %>
   <%= base_errors_for @policy %>
+  <%= f.hidden_field(:wizard_initiated, :value => @policy.wizard_initiated?) %>
   <%= f.hidden_field(:current_step, :value => @policy.next_step) unless @policy.current_step.blank? %>
   <% @policy.steps.each do | step | %>
     <%= render :partial => "policies/steps/#{step.downcase.tr(' ', '_')}_form", :locals => {:f => f, :step => step} %>

test/factories/policy_factory.rb

@@ -5,8 +5,8 @@
     weekday 'monday'
     scap_content
     scap_content_profile
-    day_of_month '1'
-    cron_line '* * * * 30'
+    day_of_month nil
+    cron_line nil
     hosts []
     assets []
   end

test/functional/api/v2/compliance/policies_controller_test.rb

@@ -23,7 +23,7 @@ class Api::V2::Compliance::PoliciesControllerTest < ActionController::TestCase
 
   test "should update a policy" do
     policy = FactoryGirl.create(:policy)
-    put :update, {:id => policy.id, :policy => {:period => 'monthly'}}
+    put :update, { :id => policy.id, :policy => { :period => 'monthly', :day_of_month => 15 }}
     updated_policy = ActiveSupport::JSON.decode(@response.body)
     assert(updated_policy['period'], 'monthly')
     assert_response :ok
@@ -37,7 +37,11 @@ class Api::V2::Compliance::PoliciesControllerTest < ActionController::TestCase
 
   test "should create a policy" do
     scap_content_profile = FactoryGirl.create(:scap_content_profile)
-    attributes = {:policy => {:name => 'my_policy', :scap_content_profile_id => scap_content_profile.id, :scap_content_id => scap_content_profile.scap_content_id}}
+    attributes = { :policy => { :name => 'my_policy',
+                                :scap_content_profile_id => scap_content_profile.id,
+                                :scap_content_id => scap_content_profile.scap_content_id,
+                                :period => 'weekly',
+                                :weekday => 'friday' }}
     post :create, attributes, set_session_user
     response = ActiveSupport::JSON.decode(@response.body)
     assert response['scap_content_profile_id'], scap_content_profile.to_param

test/unit/policy_test.rb

@@ -3,6 +3,8 @@
 class PolicyTest < ActiveSupport::TestCase
   setup do
     ForemanOpenscap::Policy.any_instance.stubs(:ensure_needed_puppetclasses).returns(true)
+    @scap_content = FactoryGirl.create(:scap_content)
+    @scap_profile = FactoryGirl.create(:scap_content_profile)
   end
 
   test "should assign hostgroups by their ids" do
@@ -17,4 +19,94 @@ class PolicyTest < ActiveSupport::TestCase
     assert_equal 2, policy.hostgroups.count
     assert policy.hostgroups.include?(hg2)
   end
+
+  test "should create policy with custom period" do
+    p = ForemanOpenscap::Policy.new(:name => "custom_policy",
+                                    :scap_content_id => @scap_content.id,
+                                    :scap_content_profile_id => @scap_profile.id,
+                                    :period => 'custom',
+                                    :cron_line => '6 * 15 12 0')
+    assert p.save
+  end
+
+  test "should not create policy with custom period" do
+    p = ForemanOpenscap::Policy.new(:name => "custom_policy",
+                                    :scap_content_id => @scap_content.id,
+                                    :scap_content_profile_id => @scap_profile.id,
+                                    :period => 'custom',
+                                    :cron_line => 'aaa')
+    refute p.save
+    assert p.errors[:cron_line].include?("does not consist of 5 parts separated by space")
+  end
+
+  test "should create policy with weekly period" do
+    p = ForemanOpenscap::Policy.new(:name => "custom_policy",
+                                    :scap_content_id => @scap_content.id,
+                                    :scap_content_profile_id => @scap_profile.id,
+                                    :period => 'weekly',
+                                    :weekday => 'monday')
+    assert p.save
+  end
+
+  test "should not create policy with weekly period" do
+    p = ForemanOpenscap::Policy.new(:name => "custom_policy",
+                                    :scap_content_id => @scap_content.id,
+                                    :scap_content_profile_id => @scap_profile.id,
+                                    :period => 'weekly',
+                                    :weekday => 'someday')
+    refute p.save
+    assert p.errors[:weekday].include?("is not a valid")
+  end
+
+  test "should create policy with monthly period" do
+    p = ForemanOpenscap::Policy.new(:name => "custom_policy",
+                                    :scap_content_id => @scap_content.id,
+                                    :scap_content_profile_id => @scap_profile.id,
+                                    :period => 'monthly',
+                                    :day_of_month => '1')
+    assert p.save
+  end
+
+  test "should not create policy with monthly period" do
+    p = ForemanOpenscap::Policy.new(:name => "custom_policy",
+                                    :scap_content_id => @scap_content.id,
+                                    :scap_content_profile_id => @scap_profile.id,
+                                    :period => 'monthly',
+                                    :day_of_month => '0')
+    refute p.save
+    assert p.errors[:day_of_month].include?("must be between 1 and 31")
+  end
+
+  test "should not create policy when attributes do not correspond to selected period in new record" do
+    p = ForemanOpenscap::Policy.new(:name => "custom_policy",
+                                    :scap_content_id => @scap_content.id,
+                                    :scap_content_profile_id => @scap_profile.id,
+                                    :period => 'monthly',
+                                    :weekday => 'tuesday',
+                                    :cron_line => "0 0 0 0 0")
+    policy = ForemanOpenscap::Policy.new(:name => "test policy",
+                                         :scap_content_id => @scap_content.id,
+                                         :scap_content_profile_id => @scap_profile.id,
+                                         :period => 'custom',
+                                         :weekday => 'tuesday',
+                                         :day_of_month => "15")
+    refute p.save
+    refute policy.save
+    assert p.errors[:weekday].include?("should be empty")
+    assert p.errors[:cron_line].include?("should be empty")
+    assert policy.errors[:weekday].include?("should be empty")
+    assert policy.errors[:day_of_month].include?("should be empty")
+  end
+
+  test "should update policy period" do
+    p = ForemanOpenscap::Policy.new(:name => "custom_policy",
+                                    :scap_content_id => @scap_content.id,
+                                    :scap_content_profile_id => @scap_profile.id,
+                                    :period => 'monthly',
+                                    :day_of_month => '5')
+    assert p.save
+    p.period = 'weekly'
+    p.weekday = 'monday'
+    assert p.save
+  end
 end