Correct disabling method and refactored test for it's class

[romanblanco]

Purpose or Intent

The same kind of changes as in #10593 + we don't need to test #visible? method for the button that does not change it

Links

Related issue: #10593
https://bugzilla.redhat.com/show_bug.cgi?id=1376595
https://bugzilla.redhat.com/show_bug.cgi?id=1377686
https://bugzilla.redhat.com/show_bug.cgi?id=1330894

@durandom @PanSpagetka can you review?

[durandom]

I think the code here is also wrong, because it checks the message and not supports_feature?. That said the message could be nil as of now which will be a default string if #10980 is merged.

Also, if you want the test to be complete you should test both, the is_available_now_error_message case and the supports_feature? case. Or, be lazy like me :), and test only the latter one, because thats the future

[durandom]

cc @gauravaradhye

[martinpovolny]

@romanblanco : will you, please, address the concerns by @durandom ?

[durandom]

@romanblanco I think this https://bugzilla.redhat.com/show_bug.cgi?id=1377686 is because of the code.

[romanblanco]

@miq-bot add_label wip

[romanblanco]

maybe related to #10694

[romanblanco]

@miq-bot remove_label wip

[romanblanco]

@durandom @PanSpagetka can you review the changes? Issues described in BZ should be fixed.

[chessbyte]

why not make this return true/false only to make it a true predicate method. The way to do that is to change the line to the simpler:

return @error_message.present?

or the more idiomatic

@error_message.present?

This will return true/false.

[romanblanco]

Sure, that will be better, corrected. thanks 😊

[martinpovolny]

@jzigmund : can you, please, test this one today?

[durandom]

1. you should query the record if the feature is supported and if its not then query for the reason
2. I think the second / nested rescue is too broad.

def disabled?
  @error_message = if @record.feature_known?(@feature)
   @record.unsupported_reason(@feature) unless @record.supports?(@feature)
  else
   @record.is_available_now_error_message(@feature)
  end
  @error_message.present?
end

I dont know if @error_message needs to be assigned during the disabled? method, that seems to me like a side effect, but its probably beyond this pr :)

[romanblanco]

@durandom Wow, I didn't know about feature_known?, thanks! 👍

[jzigmund]

@martinpovolny @romanblanco the power buttons are enabled, works as expected, ACK

[miq-bot]

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

[miq-bot]

Checked commits romanblanco/manageiq@682f363~...bc2f2cb with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🏆

[chessbyte]

Euwe Backport conflict:

$ git cherry-pick -e -x -m 1  2892437 
error: could not apply 2892437... Merge pull request #11054 from romanblanco/refactor_generic_button_with_disable_test
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

$ git status
On branch euwe
Your branch is up-to-date with 'upstream/euwe'.
You are currently cherry-picking commit 2892437.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

    modified:   app/helpers/application_helper/button/generic_feature_button_with_disable.rb

Unmerged paths:
  (use "git add <file>..." to mark resolution)

    both modified:   spec/helpers/application_helper/buttons/generic_feature_button_with_disable_spec.rb

$ git diff

diff --cc spec/helpers/application_helper/buttons/generic_feature_button_with_disable_spec.rb
index 4e4d7c9,dd59678..0000000
--- a/spec/helpers/application_helper/buttons/generic_feature_button_with_disable_spec.rb
+++ b/spec/helpers/application_helper/buttons/generic_feature_button_with_disable_spec.rb
@@@ -1,46 -1,31 +1,48 @@@
  describe ApplicationHelper::Button::GenericFeatureButtonWithDisable do
++<<<<<<< HEAD
 +  [:start, :stop, :suspend, :reset, :reboot_guest,
 +   :collect_running_processes, :shutdown_guest].each do |feature|
 +    describe '#visible?' do
 +      context "when vm supports feature #{feature}" do
 +        before do
 +          @record = FactoryGirl.create(:vm_vmware)
 +          allow(@record).to receive(:is_available?).with(feature).and_return(true)
 +        end
 +
 +        it "will not be skipped for this vm" do
 +          view_context = setup_view_context_with_sandbox({})
 +          button = described_class.new(view_context, {}, {'record' => @record}, {:options => {:feature => feature}})
 +          expect(button.visible?).to be_truthy
 +        end
++=======
+   describe '#disabled?' do
+     context "when record has an error message" do
+       it "disables the button and returns the stop error message" do
+         record = double
+         message = "xx stop message"
+         allow(record).to receive(:feature_known?).with(:some_feature).and_return(true)
+         allow(record).to receive(:supports?).with(:some_feature).and_return(false)
+         allow(record).to receive(:unsupported_reason).with(:some_feature).and_return(message)
+         view_context = setup_view_context_with_sandbox({})
+         button = described_class.new(view_context, {}, {'record' => record}, {:options => {:feature => :some_feature}})
+         expect(button.disabled?).to be_truthy
+         button.calculate_properties
+         expect(button[:title]).to eq(message)
++>>>>>>> 2892437... Merge pull request #11054 from romanblanco/refactor_generic_button_with_disable_test
        end

-       context "when instance does not support feature #{feature}" do
-         before do
-           @record = FactoryGirl.create(:vm_vmware)
-           allow(@record).to receive(:is_available?).with(feature).and_return(false)
-         end
- 
-         it "will be skipped for this vm" do
-           view_context = setup_view_context_with_sandbox({})
-           button = described_class.new(view_context, {}, {'record' => @record}, {:options => {:feature => feature}})
-           expect(button.visible?).to be_falsey
-         end
-       end
-     end
-     describe '#disabled?' do
-       context "when record has an error message" do
-         before do
-           @record = FactoryGirl.create(:vm_vmware)
-           message = "xx stop message"
-           allow(@record).to receive(:is_available_now_error_message).with(feature).and_return(message)
-         end
- 
-         it "disables the button and returns the stop error message" do
-           view_context = setup_view_context_with_sandbox({})
-           button = described_class.new(view_context, {}, {'record' => @record}, {:options => {:feature => feature}})
-           expect(button.disabled?).to be_truthy
-         end
+       it "disables the button and returns the stop error message" do
+         # TODO: remove with deleting AvailabilityMixin module
+         record = double
+         message = "xx stop message"
+         allow(record).to receive(:feature_known?).with(:some_feature).and_return(false)
+         allow(record).to receive(:is_available?).with(:some_feature).and_return(false)
+         allow(record).to receive(:is_available_now_error_message).with(:some_feature).and_return(message)
+         view_context = setup_view_context_with_sandbox({})
+         button = described_class.new(view_context, {}, {'record' => record}, {:options => {:feature => :some_feature}})
+         expect(button.disabled?).to be_truthy
+         button.calculate_properties
+         expect(button[:title]).to eq(message)
        end
      end
    end

[durandom]

@chessbyte once #11075 is backported this one should merge without conflict

[chessbyte]

Euwe Backport details:

$ git log
commit 71397f3a798731c3302cbb39504d26830b866192
Author: Martin Povolny <[email protected]>
Date:   Mon Oct 3 15:54:49 2016 +0200

    Merge pull request #11054 from romanblanco/refactor_generic_button_with_disable_test

    Correct disabling method and refactored test for it's class
    (cherry picked from commit 28924370cb82529cc0148cc3b483f38dcf32cc26)