Fixes #13968 - Add error when no known proxy features found #3895

shlomizadok · 2016-09-28T09:51:33Z

No description provided.

shlomizadok · 2016-09-28T10:24:35Z

app/models/smart_proxy.rb

      end
    rescue => e
      errors.add(:base, _('Unable to communicate with the proxy: %s') % e)
      errors.add(:base, _('Please check the proxy is configured and running on the host.'))
    end
    features.any?
  end
+
+  def validate_features(features)
+    features.map{|f| Feature.name_map[f]} == nil


I feel like a Tuna fish for sending this code 😭

shlomizadok · 2016-09-28T10:30:17Z

Maybe commit message should be something like:
"Skip feature if not in Feature model"

domcleal · 2016-09-28T10:55:25Z

Maybe commit message should be something like: "Skip feature if not in Feature model"

I think it already did this. This now correctly logs an error when no known features are found. Perhaps "add error when no known proxy features found", if you wanted to change it.

Could you add a test for this bug and validation? It contains a skip when Rails.env is test, but I think that could probably be removed and ProxyAPI::Features be stubbed instead when necessary (e.g. in the factory).

domcleal · 2016-09-28T10:47:40Z

app/models/smart_proxy.rb

@@ -115,11 +115,12 @@ def associate_features

    begin
      reply = ProxyAPI::Features.new(:url => url).features
-      if reply.is_a?(Array) && reply.any?


I think the Array check here on reply is important - it's moved below to valid_features.is_a?(Array), but you know that's an array as you're constructing it with .map. Check the reply is an array before trying to use map on it.

You are right. I'll change that.

shlomizadok · 2016-09-28T11:01:23Z

Thanks for the review and comments @domcleal. Will add tests to validate this issue.

shlomizadok · 2016-09-28T12:12:02Z

@domcleal - Added tests, reworded commit message.
Let me know what you think.

domcleal · 2016-09-29T08:23:43Z

app/models/smart_proxy.rb

@@ -111,15 +111,15 @@ def sanitize_url
  end

  def associate_features
-    return true if Rails.env == 'test'


It looks like removing this is affecting a very large number of tests - sorry, the check might have to remain.

I do have one more suggestion if you'd like to continue with it - separate the feature association from the validation. So this method with the ProxyAPI::Features call becomes a before_validation and is skipped when Rails.env.test?, but the validation that the smart proxy has at least one feature is a normal validator which can be tested in isolation from the feature assignment.

I have been pulling my hair with how to fix the tests... Your idea is better. Will refactor

Refactored. Hope this is what you had in mind

domcleal · 2016-09-29T08:24:50Z

app/models/smart_proxy.rb

    begin
      reply = ProxyAPI::Features.new(:url => url).features
-      if reply.is_a?(Array) && reply.any?
-        self.features = Feature.where(:name => reply.map{|f| Feature.name_map[f]})
+      return errors.add(:base, _('Invalid features format')) unless reply.is_a?(Array)


Please expand this message a little, it'll be unclear to users - perhaps "An invalid response was received while requesting available features from this proxy" (similar to the message below)?

👍 Again your English is better than mine 😄
Will change.

shlomizadok · 2016-10-05T09:36:14Z

@domcleal - first approach didn't work well for me (I may have done something wrong).
I have moved to stubbing the Proxy API calls:
ProxyAPI::Features.any_instance.stubs(:features => Feature.name_map.keys)

Let me know if that's acceptable

shlomizadok · 2016-10-05T11:36:49Z

test/models/nic_test.rb

@@ -233,7 +233,7 @@ def teardown

    test '.proxy uses any BMC SmartProxy if none is found in subnet' do
      assert @subnet.proxies.select { |proxy| proxy.features.map(&:name).include?('BMC') }
-      assert_equal @interface.proxy.url, SmartProxy.with_features('BMC').first.url + '/bmc'
+      assert_includes SmartProxy.with_features('BMC').map { |sp| sp.url + '/bmc' }, @interface.proxy.url


We are not sure there is always only one proxy

shlomizadok · 2016-10-05T11:37:39Z

test/controllers/smart_proxies_controller_test.rb

@@ -18,6 +18,7 @@ def test_create_invalid
  end

  def test_create_valid
+    ProxyAPI::Features.any_instance.stubs(:features =>["dns", "dhcp"])


Not needed anymore. Will remove

shlomizadok · 2016-10-05T12:44:58Z

OK, I think that besides #3895 (comment) I'm good

domcleal · 2016-10-05T14:42:22Z

test/active_support_test_case_helper.rb

@@ -39,6 +40,10 @@ def skip_if_plugin_asked_to
    end
  end

+  def fetch_proxy_features
+    ProxyAPI::Features.any_instance.stubs(:features => Feature.name_map.keys)


This adds a new SQL query to every single test, as it's called regardless of whether the stubbing's needed or not. What's it used for? Can the stubbing be added to the tests that need it?

Yeah, it was a precaution for Katello tests. I will open a PR on the failing tests there (to use factories)

shlomizadok · 2016-10-06T05:02:31Z

Katello/katello#6377 should solve the failures in Katello

dLobatog · 2016-10-07T06:34:07Z

app/models/smart_proxy.rb

    begin
      reply = ProxyAPI::Features.new(:url => url).features
-      if reply.is_a?(Array) && reply.any?
-        self.features = Feature.where(:name => reply.map{|f| Feature.name_map[f]})
+      return errors.add(:base, _('An invalid response was received while requesting available features from this proxy')) unless reply.is_a?(Array)


How about printing the response too? That can be helpful while debugging

@dLobatog - Shall I print it in a logger message or just add it to the errors ?

Added a logger message.

dLobatog · 2016-10-07T06:40:40Z

app/models/smart_proxy.rb

      else
        self.features.clear
-        errors.add :base, _('No features found on this proxy, please make sure you enable at least one feature')
+        errors.add :base, _('No matching features found on this proxy, please make sure you enable at least one feature')


Not sure what 'matching' means in this context for a user that doesn't know about the bug. Also it tells the user to 'enable at least one feature' but control flow allows Foreman to get here when there was certainly one feature enabled (albeit not known by Foreman)

How about 'Features #{invalid_features} in this proxy are not recognized by Foreman. If these features come from a Smart Proxy plugin, make sure Foreman has the plugin installed too.' when there are invalid features, and leaving the old message "No features found..." for the case where there are no features found?

Changed. Let me know if this is what you had in mind.

dLobatog · 2016-10-07T06:42:41Z

@shlomizadok I left a few comments concerning the 'not features found' case vs 'only unknown features found' - thanks for fixing this

shlomizadok · 2016-10-08T20:17:58Z

[test katello]

domcleal · 2016-10-10T11:15:08Z

test/test_helper.rb

@@ -62,6 +62,10 @@ def set_basic_auth(user, password)
    @request.env['HTTP_ACCEPT'] = 'application/json'
  end

+  def fetch_proxy_features
+    ProxyAPI::Features.any_instance.stubs(:features => Feature.name_map.keys)


Wasn't this going to be removed now? (#3895 (comment))

The previous one was for unit (/models) tests. This one is for functional (/controllers) tests. I guess I can remove it too and add per each test.

@domcleal - removed ^, added per test

domcleal · 2016-10-13T09:22:00Z

Merged, thanks @shlomizadok.

theforeman-bot added Needs testing Not yet reviewed labels Sep 28, 2016

shlomizadok force-pushed the fix_13968 branch from 436b79a to aa6c947 Compare September 28, 2016 10:10

shlomizadok commented Sep 28, 2016

View reviewed changes

domcleal suggested changes Sep 28, 2016

View reviewed changes

domcleal added Waiting on contributor and removed Not yet reviewed labels Sep 28, 2016

shlomizadok force-pushed the fix_13968 branch from aa6c947 to f85d9bd Compare September 28, 2016 12:10

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Sep 28, 2016

shlomizadok changed the title ~~Fixes #13968 - Fail to save proxy if feature is missing~~ Fixes #13968 - Add error when no known proxy features found Sep 28, 2016

domcleal suggested changes Sep 29, 2016

View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Sep 29, 2016

shlomizadok force-pushed the fix_13968 branch from f85d9bd to 08e5899 Compare October 5, 2016 08:44

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 5, 2016

shlomizadok force-pushed the fix_13968 branch from 08e5899 to 6009021 Compare October 5, 2016 09:27

shlomizadok force-pushed the fix_13968 branch from 6009021 to f5dbaa8 Compare October 5, 2016 11:31

shlomizadok commented Oct 5, 2016

View reviewed changes

domcleal reviewed Oct 5, 2016

View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Oct 5, 2016

shlomizadok force-pushed the fix_13968 branch from f5dbaa8 to c794e52 Compare October 5, 2016 15:44

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 5, 2016

shlomizadok force-pushed the fix_13968 branch from c794e52 to c04a594 Compare October 5, 2016 18:13

domcleal added Waiting on contributor and removed Needs re-review labels Oct 6, 2016

dLobatog reviewed Oct 7, 2016

View reviewed changes

shlomizadok force-pushed the fix_13968 branch from c04a594 to d3789a5 Compare October 9, 2016 12:54

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 9, 2016

domcleal suggested changes Oct 10, 2016

View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Oct 10, 2016

Fixes #13968 - Add error when no known proxy features found

b587e18

shlomizadok force-pushed the fix_13968 branch from d3789a5 to b587e18 Compare October 10, 2016 15:32

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 10, 2016

domcleal approved these changes Oct 13, 2016

View reviewed changes

domcleal merged commit 6180287 into theforeman:develop Oct 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #13968 - Add error when no known proxy features found #3895

Fixes #13968 - Add error when no known proxy features found #3895

shlomizadok commented Sep 28, 2016

shlomizadok Sep 28, 2016

shlomizadok commented Sep 28, 2016

domcleal commented Sep 28, 2016

domcleal Sep 28, 2016

shlomizadok Sep 28, 2016

shlomizadok commented Sep 28, 2016

shlomizadok commented Sep 28, 2016

domcleal Sep 29, 2016

shlomizadok Sep 29, 2016

shlomizadok Oct 5, 2016

domcleal Sep 29, 2016

shlomizadok Sep 29, 2016

shlomizadok commented Oct 5, 2016

shlomizadok Oct 5, 2016

shlomizadok Oct 5, 2016

shlomizadok commented Oct 5, 2016

domcleal Oct 5, 2016

shlomizadok Oct 5, 2016

shlomizadok commented Oct 6, 2016

dLobatog Oct 7, 2016

shlomizadok Oct 7, 2016

shlomizadok Oct 9, 2016

dLobatog Oct 7, 2016

shlomizadok Oct 9, 2016

dLobatog commented Oct 7, 2016

shlomizadok commented Oct 8, 2016

domcleal Oct 10, 2016

shlomizadok Oct 10, 2016

shlomizadok Oct 10, 2016

domcleal commented Oct 13, 2016

Fixes #13968 - Add error when no known proxy features found #3895

Fixes #13968 - Add error when no known proxy features found #3895

Conversation

shlomizadok commented Sep 28, 2016

Choose a reason for hiding this comment

shlomizadok commented Sep 28, 2016

domcleal commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomizadok commented Sep 28, 2016

shlomizadok commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomizadok commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomizadok commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomizadok commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dLobatog commented Oct 7, 2016

shlomizadok commented Oct 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented Oct 13, 2016