-
Notifications
You must be signed in to change notification settings - Fork 357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor get_rec_cls to get proper record class for various items #6224
Conversation
b97d331
to
80d4920
Compare
80d4920
to
aed95e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
0e966c1
to
a1384e1
Compare
29744c1
to
294230b
Compare
294230b
to
db165f1
Compare
@miq-bot add_label refactoring |
2ffdb7c
to
b3dff65
Compare
cedc94c
to
61cb708
Compare
@hstastna is this ready for review? |
@skateman Yes, it is. |
I'm good with the changes, but we need some more clickety-testing... |
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. |
So, from what I can tell, the refactoring part is great, matches the old state perfectly :) 👍 One thing I would consider changing: all those If the original was Now, the logic is if you get specifically a VM, do something random, that may still return EDIT: to be clear, I'm not objecting to As for the second part, I am confused about the new additions ( 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 :) |
For the first note, @skateman what do you think? I like Martin's suggestion that I should better use 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' 😏 . |
@hstastna I'm good with getting rid of the |
5ec04e3
to
6f73e89
Compare
Just a note that @himdel I've changed |
I didn't see anything break in Will merge when green, thanks :) |
Issue: ManageIQ#5924 Get proper record class for items displayed through Storage Relationships, also for items displayed through Cluster's Relationships, items in Services Workloads, VMs, Instances, Images displayed through various controllers.
6f73e89
to
cf2f25b
Compare
Some comments on commits hstastna/manageiq-ui-classic@5269ad8~...cf2f25b spec/controllers/application_controller/ci_processing_spec.rb
|
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 |
on upstream `get_rec_cls` was replaced by using `record_class` in ManageIQ#6224
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 methodrecord_class
which returnsVmOrTemplate
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 originalget_rec_cls
method.Note:
The following controllers need just the default
record_class
method:Note2:
You will probably not be able to test some actions, scenarios because of various issues, for example:
#6338, #6309, #6241, ...
TODO:
makerecord_class
methods privatecallsuper
in controllers instead ofVmOrTemplate
remove unnecessaryrecord_class
(same as default) from some controllersfix failing existing specscheck the remaining controllersadd new specs