Merge pull request #22883 from kbrock/supports_array

Drop supports_feature? methods

Author: Fryguy

app/models/host.rb

@@ -1432,11 +1432,10 @@ def self.display_name(number = 1)
   end
 
   def verbose_supports?(feature, description = nil)
-    supports?(feature).tap do |value|
-      unless value
-        description ||= feature.to_s.humanize(:capitalize => false)
-        _log.warn("Cannot #{description} because <#{unsupported_reason(feature)}>")
-      end
+    if (reason = unsupported_reason(feature))
+      description ||= feature.to_s.humanize(:capitalize => false)
+      _log.warn("Cannot #{description} because <#{reason}>")
     end
+    !reason
   end
 end

app/models/mixins/supports_feature_mixin.rb

@@ -12,21 +12,17 @@
 #
 # To make a feature conditionally supported, pass a block to the +supports+ method.
 # The block is evaluated in the context of the instance.
-# If you call the private method +unsupported_reason_add+ with the feature
-# and a reason, then the feature will be unsupported and the reason will be
+# If a feature is not supported, return a string for the reason. A nil means it is supported
+# Alternatively, calling the private method +unsupported_reason_add+ with the feature
+# and a reason, marks the feature as unsupported, and the reason will be
 # accessible through
 #
 #   instance.unsupported_reason(:feature)
 #
-# The above allows you to call +supports_feature?+ or +supports?(feature) :methods
-# on the Class and Instance
-#
-#   Post.supports_publish?                       # => true
 #   Post.supports?(:publish)                     # => true
-#   Post.new.supports_publish?                   # => true
-#   Post.supports_fake?                          # => false
-#   Post.supports_archive?                       # => true
-#   Post.new(featured: true).supports_archive?   # => false
+#   Post.new.supports?(:publish)                 # => true
+#   Post.supports?(:archive)                     # => true
+#   Post.new(featured: true).supports?(:archive) # => false
 #
 # To get a reason why a feature is unsupported use the +unsupported_reason+ method
 #
@@ -51,18 +47,16 @@
 module SupportsFeatureMixin
   extend ActiveSupport::Concern
 
-  COMMON_FEATURES = %i[create delete destroy refresh_ems update].freeze
-
   # Whenever this mixin is included we define all features as unsupported by default.
   # This way we can query for every feature
   included do
     private_class_method :unsupported
     private_class_method :unsupported_reason_add
-    private_class_method :define_supports_feature_methods
+    class_attribute :supports_features, :instance_writer => false, :default => {}
   end
 
-  def self.reason_or_default(reason)
-    (reason.presence || _("Feature not available/supported"))
+  def self.default_supports_reason
+    _("Feature not available/supported")
   end
 
   # query instance for the reason why the feature is unsupported
@@ -74,22 +68,16 @@ def unsupported_reason(feature)
 
   # query the instance if the feature is supported or not
   def supports?(feature)
-    method_name = "supports_#{feature}?"
-    if respond_to?(method_name)
-      public_send(method_name)
-    else
-      unsupported_reason_add(feature)
-      false
-    end
+    self.class.check_supports(feature.to_sym, :instance => self)
   end
 
   private
 
   # used inside a +supports+ block to add a reason why the feature is not supported
   # just adding a reason will make the feature unsupported
-  def unsupported_reason_add(feature, reason = nil)
+  def unsupported_reason_add(feature, reason)
     feature = feature.to_sym
-    unsupported[feature] = SupportsFeatureMixin.reason_or_default(reason)
+    unsupported[feature] = reason
   end
 
   def unsupported
@@ -99,24 +87,43 @@ def unsupported
   class_methods do
     # This is the DSL used a class level to define what is supported
     def supports(feature, &block)
-      define_supports_feature_methods(feature, &block)
+      self.supports_features = supports_features.merge(feature.to_sym => block || true)
     end
 
     # supports_not does not take a block, because its never supported
     # and not conditionally supported
     def supports_not(feature, reason: nil)
-      define_supports_feature_methods(feature, :is_supported => false, :reason => reason)
+      self.supports_features = supports_features.merge(feature.to_sym => reason.presence || false)
     end
 
     # query the class if the feature is supported or not
     def supports?(feature)
-      method_name = "supports_#{feature}?"
-      if respond_to?(method_name)
-        public_send(method_name)
-      else
-        unsupported_reason_add(feature)
-        false
+      check_supports(feature.to_sym, :instance => self)
+    end
+
+    def check_supports(feature, instance:)
+      instance.send(:unsupported).delete(feature)
+
+      # undeclared features are not supported
+      value = supports_features[feature.to_sym]
+
+      if value.respond_to?(:call)
+        begin
+          # for class level supports, blocks are not evaluated and assumed to be true
+          result = instance.instance_eval(&value) unless instance.kind_of?(Class)
+          # if no errors yet but result was an error message
+          # then add the error
+          if !instance.send(:unsupported).key?(feature) && result.kind_of?(String)
+            instance.send(:unsupported_reason_add, feature, result)
+          end
+        rescue => e
+          _log.log_backtrace(e)
+          instance.send(:unsupported_reason_add, feature, "Internal Error: #{e.message}")
+        end
+      elsif value != true
+        instance.send(:unsupported_reason_add, feature, value || SupportsFeatureMixin.default_supports_reason)
       end
+      !instance.send(:unsupported).key?(feature)
     end
 
     # all subclasses that are considered for supporting features
@@ -166,46 +173,8 @@ def unsupported
     end
 
     # use this for making a class not support a feature
-    def unsupported_reason_add(feature, reason = nil)
-      feature = feature.to_sym
-      unsupported[feature] = SupportsFeatureMixin.reason_or_default(reason)
-    end
-
-    def define_supports_feature_methods(feature, is_supported: true, reason: nil, &block)
-      method_name = "supports_#{feature}?"
-      feature = feature.to_sym
-
-      # silence potential redefinition warnings
-      silence_warnings do
-        # defines the method on the instance
-        define_method(method_name) do
-          unsupported.delete(feature)
-          if block
-            begin
-              result = instance_eval(&block)
-              # if no errors yet but result was an error message
-              # then add the error
-              if !unsupported.key?(feature) && result.kind_of?(String)
-                unsupported_reason_add(feature, result)
-              end
-            rescue => e
-              _log.log_backtrace(e)
-              unsupported_reason_add(feature, "Internal Error: #{e.message}")
-            end
-          else
-            unsupported_reason_add(feature, reason) unless is_supported
-          end
-          !unsupported.key?(feature)
-        end
-
-        # defines the method on the class
-        define_singleton_method(method_name) do
-          unsupported.delete(feature)
-          # TODO: durandom - make reason evaluate in class context, to e.g. include the name of a subclass (.to_proc?)
-          unsupported_reason_add(feature, reason) unless is_supported
-          !unsupported.key?(feature)
-        end
-      end
+    def unsupported_reason_add(feature, reason)
+      unsupported[feature.to_sym] = reason
     end
   end
 end

app/models/vm/operations.rb

@@ -26,13 +26,15 @@ module Vm::Operations
     supports :launch_vmrc_console do
       begin
         validate_remote_console_vmrc_support
+        nil
       rescue => err
         _('VM VMRC Console error: %{error}') % {:error => err}
       end
     end
 
     supports :launch_native_console do
       validate_native_console_support
+      nil
     rescue StandardError => err
       _('VM NATIVE Console error: %{error}') % {:error => err}
     end

spec/models/mixins/supports_feature_mixin_spec.rb

@@ -33,22 +33,6 @@ def initialize(values = {})
 
   let(:test_inst) { test_class.new }
 
-  describe "#supports_feature?" do
-    it "defines supports on the instance" do
-      expect(test_inst.supports_std_accept?).to be_truthy
-      expect(test_inst.supports_module_accept?).to be_truthy
-      expect(test_inst.supports_std_denial?).to be_falsey
-    end
-  end
-
-  describe ".supports_feature?" do
-    it "defines supports on the class" do
-      expect(test_class.supports_std_accept?).to be_truthy
-      expect(test_class.supports_module_accept?).to be_truthy
-      expect(test_class.supports_std_denial?).to be_falsey
-    end
-  end
-
   describe ".supports?" do
     it "handles base supports" do
       expect(test_class.supports?(:std_accept)).to be_truthy
@@ -231,11 +215,37 @@ def initialize(values = {})
     end
 
     it "gives reason when implicit dynamic attrs" do
-      test_class.supports(:implicit_feature) {  "dynamically unsupported" unless attr1 }
+      test_class.supports(:implicit_feature) { "dynamically unsupported" unless attr1 }
       test_inst = test_class.new(:attr1 => false)
 
       expect(test_inst.unsupported_reason(:implicit_feature)).to eq("dynamically unsupported")
     end
+
+    it "gives reason when chained to a denail with a default reason" do
+      test_class.supports_not :denial_no_reason
+      test_class.supports(:denial_chained) { unsupported_reason(:denial_no_reason) }
+
+      expect(test_inst.unsupported_reason(:denial_chained)).to eq("Feature not available/supported")
+    end
+
+    it "gives reason when chained to a denail with a default reason (checking supported)" do
+      test_class.supports_not :denial_no_reason
+      test_class.supports(:denial_chained) { unsupported_reason(:denial_no_reason) unless supports?(:denial_no_reason) }
+
+      expect(test_inst.unsupported_reason(:denial_chained)).to eq("Feature not available/supported")
+    end
+
+    it "gives no reason when chained to an attribute with success" do
+      test_class.supports(:std_chained) { unsupported_reason(:std_accept) }
+
+      expect(test_inst.unsupported_reason(:std_chained)).to eq(nil)
+    end
+
+    it "gives no reason when chained to an attribute with success (checking supported)" do
+      test_class.supports(:std_chained) { unsupported_reason(:std_accept) unless supports?(:std_accept) }
+
+      expect(test_inst.unsupported_reason(:std_chained)).to eq(nil)
+    end
   end
 
   describe ".subclasses_supporting" do

spec/support/supports_helper.rb

@@ -25,7 +25,7 @@ def stub_supports_not(model, feature = :update, reason = nil)
 
         stub_supports(model, feature, :supported => false)
 
-        reason ||= SupportsFeatureMixin.reason_or_default(reason)
+        reason ||= SupportsFeatureMixin.default_supports_reason
         receive_reason = receive(:unsupported_reason).with(feature).and_return(reason)
         allow(model).to(receive_reason)
         allow_any_instance_of(model).to(receive_reason)