Refactor get_rec_cls to get proper record class for various items

[hstastna]

Issue: #5924

We want to get rid of going through so many cases in get_rec_cls which is not very efficient. This PR addresses the solution. I've created a new method record_class which returns VmOrTemplate by default. No need to define this method in each controller.
I've also added some more special cases in some controllers in record_class which were missing in the original get_rec_cls method.

Note:
The following controllers need just the default record_class method:

• auth_key_pair_cloud
• availability_zone
• cloud_tenant
• cloud_volume
• ems_cloud
• flavor
• host_aggregate
• vm_cloud
• vm_infra
• vm_or_template

Note2:
You will probably not be able to test some actions, scenarios because of various issues, for example:
#6338, #6309, #6241, ...

---

TODO:

• make record_class methods private
• call super in controllers instead of VmOrTemplate
• remove unnecessary record_class (same as default) from some controllers
• fix failing existing specs
• check the remaining controllers
• add new specs

[skateman]

I'd suggest you to define a record_class in CiProcessing that defaults to VmOrTemplate and just redefine it in each controller where it's needed. You can also call super to fall back to the default where is needed (see examples).

Also the old get_rec_cls can be completely replaced by this new record_class method.

Great job BTW 👍

[hstastna]

@miq-bot add_label refactoring

[skateman]

@hstastna is this ready for review?

[hstastna]

@skateman Yes, it is.

[skateman]

I'm good with the changes, but we need some more clickety-testing...

[hstastna]

Maybe @h-kataria @himdel @ZitaNemeckova @mzazrivec ? Could you please test this? Or you can ping anyone else who you think he/she could help :) Thanks in advance.

[himdel]

So, from what I can tell, the refactoring part is great, matches the old state perfectly :) 👍

One thing I would consider changing: all those super calls don't really make sense here, not if the original explicitly had VmOrTemplate - don't write super when you really mean VmOrTemplate, this is just making the code harder to reason about.

If the original was %w[all_vms vms].include?(params[:display]) ? VmOrTemplate : Storage, the idea behind the logic was that for vms, return VmOrTemplate. That's clear and easy to reason about.

Now, the logic is if you get specifically a VM, do something random, that may still return
VmOrTemplate. That doesn't sound right :). (All of those will break if the logic of the root implementation gets more complex than return VmOrTemplate.)

EDIT: to be clear, I'm not objecting to condition ? MyClass : super, but I am objecting to is_this_a_vm? ? super : MyClass.

---

As for the second part, I am confused about the new additions (EmsInfraController, HostController, ResourcePoolController)? Are those fixing specific bugs? Are those just in case?

From what I can tell, the functional difference is essentially this:

diff --git a/app/controllers/application_controller/ci_processing.rb b/app/controllers/application_controller/ci_processing.rb
index 21d8771718..bbbc9512eb 100644
--- a/app/controllers/application_controller/ci_processing.rb
+++ b/app/controllers/application_controller/ci_processing.rb
@@ -267,6 +267,28 @@ module ApplicationController::CiProcessing
       %w[all_vms vms].include?(params[:display]) || params[:pressed].starts_with?('miq_template') ? VmOrTemplate : EmsCluster
     when "storage"
       %w[all_vms vms].include?(params[:display]) ? VmOrTemplate : Storage
+    when "ems_infra"
+      case params[:pressed]
+      when /^ems_cluster/
+        EmsCluster
+      when /^orchestration_stack/
+        OrchestrationStack
+      when /^storage/
+        Storage
+      else
+        VmOrTemplate
+      end
+    when "host"
+      case params[:display] || @display
+      when 'storages'
+        Storage
+      when 'vms', 'miq_templates'
+        VmOrTemplate
+      else
+        Host
+      end
+    when "resource_pool"
+      %w[all_vms vms].include?(params[:display]) ? VmOrTemplate : ResourcePool
     else
       VmOrTemplate
     end

EDIT: yes, those new additions are fixes for things that didn't work in those places, will test :)

[hstastna]

For the first note, @skateman what do you think? I like Martin's suggestion that I should better use VmOrTemplate and not call super if I really mean VmOrTemplate.

---

For the second note, I was going through (hopefully) all the pages, controllers manually, clicking on the actions and checking how they work in real and which record class should be set. This is why you recognize some functional difference. And then, I've found that this PR helps to fix some existing issues, for example #6338 which makes sense regarding my new additions. So the new additions are intentional, not 'just in case' 😏 .

[skateman]

@hstastna I'm good with getting rid of the super.

[hstastna]

Just a note that allow_any_instance_of present in some specs was not added by me and improvements regarding this are not a subject of this PR.

@himdel I've changed super to VmOrTemplate in appropriate controllers, according to the suggestions, so I think this PR is ready (after it gets green).

[himdel]

I didn't see anything break in ems_infra, host or resource_pool 👍 (but, I don't know what exactly was broken there before)

Will merge when green, thanks :)

[himdel]

@hstastna can you rebase please? @fhlavac fixed that travis error in #5854 (merged now)

[miq-bot]

Some comments on commits hstastna/manageiq-ui-classic@5269ad8~...cf2f25b

spec/controllers/application_controller/ci_processing_spec.rb

• ⚠️ - 213 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
• ⚠️ - 273 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
• ⚠️ - 360 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
• ⚠️ - 369 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
• ⚠️ - 431 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
• ⚠️ - 510 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
• ⚠️ - 570 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

[miq-bot]

Checked commits hstastna/manageiq-ui-classic@5269ad8~...cf2f25b with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
25 files checked, 0 offenses detected
Everything looks fine. ⭐

[himdel]

Backported to ivanchuk via #7241

@miq-bot add_label ivanchuk/backported