Get rid of the condition modifier.

app/models/condition.rb

@@ -2,9 +2,8 @@ class Condition < ApplicationRecord
   include UuidMixin
   before_validation :default_name_to_guid, :on => :create
 
-  validates_presence_of     :name, :description, :guid, :modifier, :expression, :towhat
-  validates_uniqueness_of   :name, :description, :guid
-  validates_inclusion_of    :modifier, :in => %w( allow deny )
+  validates :name, :description, :guid, :expression, :towhat, :presence => true
+  validates :name, :description, :guid, :uniqueness => true
 
   acts_as_miq_taggable
   acts_as_miq_set_member
@@ -257,6 +256,10 @@ def export_to_array
   end
 
   def self.import_from_hash(condition, options = {})
+    # To delete condition modifier in policy from versions 5.8 and older
+    condition["expression"].exp = {"not" => condition["expression"].exp} if condition["modifier"] == 'deny'
+    condition.delete("modifier")
+
     status = {:class => name, :description => condition["description"]}
     c = Condition.find_by(:guid => condition["guid"])
     msg_pfx = "Importing Condition: guid=[#{condition["guid"]}] description=[#{condition["description"]}]"

app/models/miq_action.rb

@@ -106,15 +106,14 @@ def self.invoke_actions(apply_policies_to, inputs, succeeded, failed)
     results = {}
 
     begin
-      failed.each do |p|
+      succeeded.each do |p|
         actions = case p
-                  when MiqPolicy then p.actions_for_event(inputs[:event], :failure).uniq
+                  when MiqPolicy then p.actions_for_event(inputs[:event], :success).uniq
                   else            p.actions_for_event
                   end
 
         actions.each do |a|
-          # merge in the synchronous flag and possibly the sequence if not already sorted by this
-          inputs = inputs.merge(:policy => p, :result => false, :sequence => a.sequence, :synchronous => a.synchronous)
+          inputs = inputs.merge(:policy => p, :result => true, :sequence => a.sequence, :synchronous => a.synchronous)
           _log.debug("action: [#{a.name}], seq: [#{a.sequence}], sync: [#{a.synchronous}], inputs to action: seq: [#{inputs[:sequence]}], sync: [#{inputs[:synchronous]}]")
 
           if a.name == "prevent"
@@ -124,15 +123,17 @@ def self.invoke_actions(apply_policies_to, inputs, succeeded, failed)
 
           name = a.action_type == "default" ? a.name.to_sym : a.action_type.to_sym
           results[name] ||= []
-          results[name] << {:policy_id => p.kind_of?(MiqPolicy) ? p.id : nil, :policy_status => :failure, :result => a.invoke(apply_policies_to, inputs)}
+          results[name] << {:policy_id => p.kind_of?(MiqPolicy) ? p.id : nil, :policy_status => :success, :result => a.invoke(apply_policies_to, inputs)}
         end
       end
 
-      succeeded.each do |p|
-        next unless p.kind_of?(MiqPolicy) # built-in policies are OpenStructs whose actions will be invoked only on failure
-        actions = p.actions_for_event(inputs[:event], :success).uniq
+      failed.each do |p|
+        next unless p.kind_of?(MiqPolicy) # built-in policies are OpenStructs whose actions will be invoked only on success
+        actions = p.actions_for_event(inputs[:event], :failure).uniq
+
         actions.each do |a|
-          inputs = inputs.merge(:policy => p, :result => true, :sequence => a.sequence, :synchronous => a.synchronous)
+          # merge in the synchronous flag and possibly the sequence if not already sorted by this
+          inputs = inputs.merge(:policy => p, :result => false, :sequence => a.sequence, :synchronous => a.synchronous)
           _log.debug("action: [#{a.name}], seq: [#{a.sequence}], sync: [#{a.synchronous}], inputs to action: seq: [#{inputs[:sequence]}], sync: [#{inputs[:synchronous]}]")
 
           if a.name == "prevent"
@@ -142,7 +143,7 @@ def self.invoke_actions(apply_policies_to, inputs, succeeded, failed)
 
           name = a.action_type == "default" ? a.name.to_sym : a.action_type.to_sym
           results[name] ||= []
-          results[name] << {:policy_id => p.kind_of?(MiqPolicy) ? p.id : nil, :policy_status => :success, :result => a.invoke(apply_policies_to, inputs)}
+          results[name] << {:policy_id => p.kind_of?(MiqPolicy) ? p.id : nil, :policy_status => :failure, :result => a.invoke(apply_policies_to, inputs)}
         end
       end
 

app/models/miq_policy.rb

@@ -63,7 +63,6 @@ def self.built_in_policies
             :name        => policy.attributes["name"],
             :description => policy.attributes["description"],
             :expression  => MiqExpression.new(policy.condition),
-            :modifier    => policy.modifier,
             :towhat      => "Vm"
           )]
         else
@@ -332,13 +331,9 @@ def applies_to?(rec, inputs = {})
   end
 
   def self.eval_condition(c, rec, inputs = {})
-    possible_results = c['modifier'] == 'allow' ? %w(allow deny) : %w(deny allow)
-    begin
-      index = Condition.evaluate(c, rec, inputs) ? 0 : 1
-      possible_results[index]
-    rescue => err
-      logger.log_backtrace(err)
-    end
+    Condition.evaluate(c, rec, inputs) ? 'allow' : 'deny'
+  rescue => err
+    logger.log_backtrace(err)
   end
   private_class_method :eval_condition
 

db/fixtures/miq_policy_sets.yml

@@ -72,18 +72,18 @@
       Condition:
       - name: if container image has high severity openscap rule results
         description: Has high severity OpenSCAP rule results
-        modifier: deny
         expression: !ruby/object:MiqExpression
           exp:
-            FIND:
-              search:
-                "=":
-                  field: ContainerImage.openscap_rule_results-result
-                  value: fail
-              checkany:
-                "=":
-                  field: ContainerImage.openscap_rule_results-severity
-                  value: High
+            not:
+              FIND:
+                search:
+                  "=":
+                    field: ContainerImage.openscap_rule_results-result
+                    value: fail
+                checkany:
+                  "=":
+                    field: ContainerImage.openscap_rule_results-severity
+                    value: High
           context_type:
         towhat: ContainerImage
         file_mtime:
@@ -123,12 +123,12 @@
       Condition:
       - name: "Do not scan image-inspector's image"
         description: "Don't scan image-inspector's image"
-        modifier: deny
         expression: !ruby/object:MiqExpression
           exp:
-            ENDS WITH:
-              field: ContainerImage-name
-              value: "/image-inspector"
+            not:
+              ENDS WITH:
+                field: ContainerImage-name
+                value: "/image-inspector"
           context_type:
         towhat: ContainerImage
         file_mtime:

product/policy/built_in_policies.yml

@@ -13,7 +13,6 @@
     - "=":
         field: Vm-power_state
         value: 'on'
-  :modifier: deny
   :mode: control
   :action: vm_stop
 - :name: Prevent Retired VM from Starting
@@ -26,7 +25,6 @@
     "=":
       field: Vm-retired
       value: true
-  :modifier: deny
   :mode: control
   :action: prevent
 - :name: Prevent Retired Instance from Starting
@@ -39,7 +37,6 @@
     "=":
       field: Vm-retired
       value: true
-  :modifier: deny
   :mode: control
   :action: vm_suspend
 - :name: Stop Retired VM
@@ -52,6 +49,5 @@
     "=":
       field: Vm-retired
       value: true
-  :modifier: deny
   :mode: control
   :action: vm_stop

spec/factories/condition.rb

@@ -2,7 +2,6 @@
   factory :condition do
     sequence(:name)        { |num| "condition_#{seq_padded_for_sorting(num)}" }
     sequence(:description) { |num| "Condition #{seq_padded_for_sorting(num)}" }
-    modifier               "allow"
     towhat                 "Vm"
     expression             { MiqExpression.new(">=" => {"field" => "Vm-num_cpu", "value" => "2"}) }
   end

spec/models/condition_spec.rb

@@ -151,4 +151,17 @@
       expect(Condition.subst_matches?(expr, vm1)).not_to be_truthy
     end
   end
+
+  describe ".import_from_hash" do
+    it "removes condition modifier" do
+      cond_hash = {
+        "description" => "test condition",
+        "expression"  => MiqExpression.new(">" => {"field" => "Vm-cpu_num", "value" => 2}),
+        "modifier"    => 'deny',
+        "towhat"      => "Vm"
+      }
+      condition, _s = Condition.import_from_hash(cond_hash)
+      expect(condition.expression.exp).to eq("not" => {">" => {"field" => "Vm-cpu_num", "value" => 2}})
+    end
+  end
 end

spec/models/miq_policy_spec.rb

@@ -149,7 +149,7 @@
     let(:policies) do
       [
         FactoryGirl.create(:miq_policy, :conditions => [conds[0]], :active => true, :mode => 'control').tap do |p|
-          p.replace_actions_for_event(events[0], [[actions[0], {:qualifier => :success}]])
+          p.replace_actions_for_event(events[0], [[actions[0], {:qualifier => :success}], [actions[1], {:qualifier => :failure}]])
         end,
         FactoryGirl.create(:miq_policy, :conditions => [conds[1]], :active => false).tap do |p|
           p.replace_actions_for_event(events[1], [[actions[1], {:qualifier => :success}]])
@@ -230,6 +230,20 @@
         described_class.enforce_policy(target, events[0].name)
       end
     end
+
+    describe ".eval_condition" do
+      it "returns 'allow' when condition is met" do
+        vm = FactoryGirl.create(:vm_vmware, :hardware => FactoryGirl.create(:hardware, :cpu_sockets => 2))
+        result = described_class.send(:eval_condition, conds[0], vm)
+
+        expect(result).to eq('allow')
+      end
+
+      it "returns 'deny' when condition is not met" do
+        result = described_class.send(:eval_condition, conds[0], target)
+        expect(result).to eq('deny')
+      end
+    end
   end
 
   describe ".built_in_policies" do
@@ -258,7 +272,7 @@
     it 'prevents retired instance from starting' do
       MiqQueue.destroy_all
       @vm.update_attributes(:retired => true)
-      expect(subject[:result]).to be false
+      expect(subject[:result]).to be true
       expect(subject[:actions].size).to eq(1)
       expect(subject[:details].first["name"]).to eq("(Built-in) Prevent Retired Instance from Starting")
       q = MiqQueue.first
@@ -268,7 +282,7 @@
     end
 
     it 'allows active vm to start' do
-      expect(subject[:result]).to be true
+      expect(subject[:result]).to be false
       expect(subject[:actions].size).to eq(0)
       expect(subject[:details].first["name"]).to eq("(Built-in) Prevent Retired Instance from Starting")
     end