Fix Refresh Relationships and Power States button for VMs, Instances, Images

[hstastna]

Related BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1600678
Related UI PR: ManageIQ/manageiq-ui-classic#4301

---

There is a problem with Refresh Relationships and Power States button in all of the pages under Compute > Clouds and also Compute > Infra while trying to refresh relationships and power states of any VM, Instance or Image. It's because validate_refresh_ems method is called every time we try to refresh relationships and power states, but this method does not exist. Similar problem was recently with destroy action, this is the PR which fixed that: #17780

Steps to reproduce: (one of possible scenarios)

1. Go to Compute > Clouds/Infra > Providers and add some provider if there is no one
2. In provider's details screen, in Relationships table, click on Images/Instances/VMs
3. Choose some items (check the checkboxes) or display details page of some item
4. Click on Configuration > Refresh Relationships and Power States
   => refresh is not initiated, error:

----] I, [2018-08-15T19:01:10.256508 #1514:2b1901556f28]  INFO -- : Started POST "/ems_cloud/button/38?pressed=miq_template_refresh" for ::1 at 2018-08-15 19:01:10 +0200
[----] I, [2018-08-15T19:01:10.305767 #1514:2b1901556f28]  INFO -- : Processing by EmsCloudController#button as JS
[----] I, [2018-08-15T19:01:10.305926 #1514:2b1901556f28]  INFO -- :   Parameters: {"miq_grid_checks"=>"2099", "check_2099"=>"2099", "pressed"=>"miq_template_refresh", "id"=>"38"}
[----] W, [2018-08-15T19:01:10.317314 #1514:2b1901556f28]  WARN -- : DEPRECATION WARNING: Please pass the privilege you want to check for when refreshing
 (called from process_vm_buttons at /home/hstastna/manageiq/manageiq-ui-classic/app/controllers/application_controller/ci_processing.rb:999)
[----] F, [2018-08-15T19:01:10.483397 #1514:2b1901556f28] FATAL -- : Error caught: [NoMethodError] undefined method `validate_refresh_ems' for #<ManageIQ::Providers::Amazon::CloudManager::Template:0x007fa98b94a368>
Did you mean?  validates_presence_of
/home/hstastna/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/activemodel-5.0.7/lib/active_model/attribute_methods.rb:433:in `method_missing'
/home/hstastna/manageiq/manageiq/app/models/mixins/availability_mixin.rb:24:in `is_available?'
/home/hstastna/manageiq/manageiq-ui-classic/app/helpers/application_helper.rb:219:in `block in records_support_feature?'
/home/hstastna/manageiq/manageiq-ui-classic/app/helpers/application_helper.rb:215:in `each'
/home/hstastna/manageiq/manageiq-ui-classic/app/helpers/application_helper.rb:215:in `find'
/home/hstastna/manageiq/manageiq-ui-classic/app/helpers/application_helper.rb:215:in `records_support_feature?'

Before:

After:

[hstastna]

@miq-bot add_label bug

[hstastna]

@agrare @Ladas What do you think about this? The problem and the fix is very similar to #17780

[hstastna]

I also haven't found any BZ for exactly this problem. Do you think I should create the new one or can the problem be part of related BZ I have mentioned in the description of this PR (https://bugzilla.redhat.com/show_bug.cgi?id=1600678) ? It's about some not working toolbar buttons, generally. Thank you for any advice! :)

[Ladas]

looks good

@agrare the reason we see this issue is that now every button must have supports or the old validate.

[Ladas]

@miq-bot assign @agrare

[agrare]

@hstastna @Ladas IDK about this, it doesn't seem like the common supports_feature usecase where some types support X and some do not. Every VM/Template/Host/EMS support refresh, it is implemented in the base class.

[Ladas]

@agrare right, then probably more places will be failing with missing validate/supports method :-)

[agrare]

@Ladas yeah exactly what I'm worried about. For example will we get the same error for refresh_ems on an ExtManagementSystem? How about Host?

[agrare]

@martinpovolny WDYT? These buttons aren't something that make sense for supports_feature_mixin IMO.

[Ladas]

@agrare I think for consistency, every button acts the same now. So I think the UI team is now hunting the actions not having support.

[martinpovolny]

@martinpovolny WDYT? These buttons aren't something that make sense for supports_feature_mixin IMO.

Actually a good deal of buttons test for a particular feature. In many cases button "A" tests for feature "A". But generally buttons "B, C, D" can test for a feature "E". See here: https://github.com/ManageIQ/guides/blob/master/ui/toolbars.md

This makes sense to me: a particular function requires existing support on the backend.

From the UI perspective we would love to do this in a unified way. However in reality we have support? and validates and in some places we have ad-hoc code. I know that Marcel was working on unifying that but I am not sure that someone has taken over that effort.

As @Ladas wrote:

@agrare I think for consistency, every button acts the same now. So I think the UI team is now hunting the actions not having support.

Really we have BZs that are caused by inconsistencies in the behavior of button actions and are trying to unify and generalize the behavior. We believe that unification of the behavion not only leaves us with less code but gives us less buggy and easier to maintain codebase ;-)

[martinpovolny]

@agrare : To sum up: Is there any other feature that the UI can test for to see if refresh is supported? If so then we don't need this PR. Else we do need it. We need to know if refresh is supported.

[agrare]

To sum up: Is there any other feature that the UI can test for to see if refresh is supported?

I'm still confused, are these buttons generic for all types of inventory or specific for VMs?
If they are specific for VMs then you don't need to see if refresh is supported, refresh is supported always for all VM instances so no need to check. There are some buttons which aren't supported/unsupported based on the type of object right? Remove from VMDB comes to mind right?

If it is generic for all types then I don't know why we don't need to also add this to ExtManagementSystem, Host, anything that has #refresh_ems defined.

Basically I'm confused why this is only being added to Vms.

[Ladas]

@agrare so the change is that @romanblanco refactored the button code, so that every action must have supports (or the old validate_something..), so we can do this checks in a consistent way. E.g. there was a 'supports' code for refresh, but it was as a hacky pieces of code scattered somewhere in controllers (and I think it always returned true, it was just hidden in some crazy conditions).

That being said, I think we should have specs or something to check all buttons are working (e.g. all of them have supports/validate implemented), since it looks like we are discovering that manually now, @romanblanco @martinpovolny ?

[romanblanco]

That being said, I think we should have specs or something to check all buttons are working (e.g. all of them have supports/validate implemented), since it looks like we are discovering that manually now, @romanblanco @martinpovolny ?

@Ladas the problem with this is we don't have a consistent list of all the supported features for each entities we test in the toolbar buttons.

I can get a list of features and a list of toolbar buttons doing the test, but I don't know how they should respond.

[Ladas]

@romanblanco not sure I understand. There should be a way to write a spec for every button, that will catch bugs like in this PR?

[Ladas]

@romanblanco ah, I see what you're saying. I think it's ok if you'll test that everything is unsupported by default (except things like refresh that are supported by all providers). So we'll at least test that it won't explode.

[agrare]

@hstastna is this still an problem? I wasn't able to reproduce...I added a provider and was able to refresh relationships & power states for a provider, vm, and a host without issue.

[hstastna]

@agrare Yes, it is still a problem. Did you follow my steps to reproduce in the description of the PR? The important detail here is that for VMs under Compute > Infra > VMs it works but in any other screen displaying VMs (nested list of VMs), it does not work. So it depends where you are, how did you display the list of VMs.

[agrare]

@hstastna okay I still don't see this, but if we're seeing this on vms we'll eventually see it for the rest of the types so can you add supports :refresh_ems to the rest of the models that have it?

A quick grep shows: provider, physical_chassis, physical_rack, physical_server, physical_server, physical_switch, physical_storage, host, vm_or_template, ext_management_system, and orchestration_stack.

[hstastna]

@agrare Thank you, Adam. I will take a look and will add the line to the rest of models. But probably we will not need to add it to all of them you have mentioned. For example, when I try to refresh relationships of some provider, it works well. This is because not every action goes thru generic_button_operation method (in the UI code). But if yes, then this line is crucial: https://github.com/hstastna/manageiq-ui-classic/blob/master/app/controllers/application_controller/ci_processing.rb#L665 The problem will be mainly with refresh of items displayed in a nested list.
Originally, I was trying to fix the problem (and some another problem with some other action, before this problem with refresh occurred) by making changes in testable_action method to return false (yes, it fixed the problem), but I was told not to do it because it could be very problematic in the future. It turned out that adding supports :refresh_ems to appropriate model is the best solution.

[hstastna]

@agrare The only place I've found where refresh does not work with this PR, is refresh of Hosts displayed thru Compute > Clouds > Host Aggregates > some host aggregate details page. But no error.. so there must be different problem. I will take a look more and will try to fix it.

[hstastna]

@agrare I think that this PR can be merged. The problem with refresh of Hosts displayed thru Compute > Clouds > Host Aggregates > some host aggregate details page is not a problem with refresh but any action in that page (tagging etc.). I would better fix this in a separate PR.

[kbrock]

I think this needs a rebase with current master - that should fix the travis failure

[agrare]

@hstastna you just need to include SupportsFeatureMixin to any models that don't have it yet

[miq-bot]

Checked commits hstastna/manageiq@566fb1e~...493d20f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
11 files checked, 0 offenses detected
Everything looks fine. 🏆