Removed validate_vm_control method and calls as it is already replaced by supports_control #11565
Conversation
|
@miq-bot add_label wip |
|
@miq-bot add_label refactoring |
gauravaradhye
changed the title
|
@miq-bot remove_label wip |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
There was a problem hiding this comment.
I like this as a intermediate version until those validate_ methods using this are also converted.
I guess you checked, that no UI or other part is relying on validate_vm_control ?
TBH I can see this supports_control feature go away completely as I think its only really used as a helper, but that is off topic.
So, please rebase then we can merge this
gauravaradhye
force-pushed
the
remove_validate_vm_control
branch
from
6f91514
to
277351e
Compare
|
@durandom I have rebased the commit but few test cases are failing and I couldn't find out why. Will need your help in finding out the reason after we have travis results. |
|
@gauravaradhye looks like the specs or some buttons? expect |
|
@durandom yeah but I haven't changed the return format for any method, so not able to figure out what's causing it. |
gauravaradhye
force-pushed
the
remove_validate_vm_control
branch
from
49210c1
to
2f43985
Compare
|
I got the tests working by changing the rspec, this is essentially because in earlier code, in some cases, :available was true with error message. This combination is not present in current code. Need to discuss it with @durandom |
| @@ -1720,7 +1720,7 @@ def setup_firefox_with_linux | |||
| end | |||
| context "when with snapshots" do | |||
| before { allow(@record).to receive_message_chain(:snapshots, :size).and_return(2) } | |||
| it_behaves_like 'default case' | |||
| it_behaves_like 'record with error message', 'remove_snapshot' | |||
There was a problem hiding this comment.
tbh, this spec file is a nightmare. Not only in length, but...
by changing this, you change what is expected as a return value of the subject, which is build_toolbar_disabled_button. And it should still be the default case, which is to return false. Yikes. To understand that it took me quite some time. So the problem lies in your refactoring of supports_control - See above
There was a problem hiding this comment.
don't get me wrong, I was bashing the former spec file, not your changes :)
| msg = validate_vm_control | ||
| return {:available => msg[0], :message => msg[1]} unless msg.nil? | ||
| unless supports_control? | ||
| return {:available => false, :message => unsupported_reason(:control)} |
There was a problem hiding this comment.
changing this to return {:available => true, :message => unsupported_reason(:control)} makes the test pass. But this is only part of the truth.
| def validate_vm_control | ||
| # Check the basic require to interact with a VM. | ||
| return [false, 'The VM is retired'] if self.retired? | ||
| return [false, 'The VM is a template'] if self.template? |
There was a problem hiding this comment.
As a reminder, I think false would indicate that the feature is generally unsupported by the class. Whereas true is: as a class I support it, but there are conditions that prevent me.
| return [true, "The VM is not connected to an active #{ui_lookup(:table => "ext_management_systems")}"] unless self.has_active_ems? | ||
| nil | ||
| end | ||
|
|
||
| included do | ||
| supports :control do |
There was a problem hiding this comment.
Maybe you have more luck moving this to Vm (and removing the validate_vm_control above).
It looks like control is generally unsupported by Templates
|
@gauravaradhye maybe I dont get exactly what the spec changes do. Whats the difference of 'default case' and 'record with error message' ? This PR #11711 does something similar |
|
@durandom In earlier code, the validate_vm_control returned true for few error messages and false for others. And the method build_toolbar_disable_button in toolbar_builder.rb file returns the error message only if is_available is false, else it returns false. With supports_control method, we are making is_available as false if the error message is present, hence the spec needs to be changed to expect the error message instead of the boolean value. That's why I have changed the spec. |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
|
ok, so, I was expecting a string to be a truthy return value for |
…d by supports_control method
gauravaradhye
force-pushed
the
remove_validate_vm_control
branch
from
2f43985
to
3818044
Compare
|
Checked commit gauravaradhye@3818044 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
|
@durandom Rebased. |
|
@blomquisg 👍 please merge |
|
@blomquisg ping! |
|
@miq-bot assign @blomquisg |
|
This switched to |
Removed validate_vm_control method and calls as it is already replaced by supports_control (cherry picked from commit 5cec786) https://bugzilla.redhat.com/show_bug.cgi?id=1400616
|
Euwe backport details: |
The method validate_vm_control was replaced by supports_control method long back, still few functions were calling the old method. Replaced all such methods to call the new method instead of old method and removed the old method to have consistency.