Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Automate Domains and Providers clickable from the list displayed through Tenant's Relationships #6144

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/controllers/automation_manager_controller.rb
Expand Up @@ -93,6 +93,18 @@ def x_show
generic_x_show
end

# Display provider through Tenant's textual summary
def show
@explorer = true
@lastaction = 'explorer'

build_accordions_and_trees

self.x_node = "at-#{params[:id]}"
@record = ExtManagementSystem.find_by(:id => params[:id])
generic_x_show
end

def tree_record
@record = case x_active_tree
when :automation_manager_providers_tree then automation_manager_providers_tree_rec
Expand Down
12 changes: 12 additions & 0 deletions app/controllers/miq_ae_class_controller.rb
Expand Up @@ -83,6 +83,18 @@ def explorer
render :layout => "application"
end

# Display any Automate Domain through Tenant's textual summary
def show
@sb[:action] = nil
@explorer = true
build_accordions_and_trees

self.x_node = "aen-#{params[:id]}"
get_node_info(x_node)

render :layout => 'application'
end

def set_right_cell_text(id, rec = nil)
nodes = id.split('-')
case nodes[0]
Expand Down
6 changes: 2 additions & 4 deletions app/controllers/ops_controller.rb
Expand Up @@ -45,8 +45,7 @@ def display_providers
:breadcrumb_title => _('Providers'),
:association => :nested_providers,
:parent => @record,
:no_checkboxes => true,
:clickable => false
:no_checkboxes => true
)
end

Expand All @@ -56,8 +55,7 @@ def display_ae_namespaces
:breadcrumb_title => _('Automate Domains'),
:association => :nested_ae_namespaces,
:parent => @record,
:no_checkboxes => true,
:clickable => false
:no_checkboxes => true
)
end

Expand Down
13 changes: 13 additions & 0 deletions app/controllers/provider_foreman_controller.rb
Expand Up @@ -101,6 +101,19 @@ def load_or_clear_adv_search
end
end

# Display provider through Tenant's textual summary
def show
@explorer = true
@lastaction = "explorer"

build_accordions_and_trees

params[:action] = 'tree-select'
provider_node(params[:id], ExtManagementSystem.find_by(:id => params[:id]).type)

render :layout => "application"
end

def x_show
tree_record unless unassigned_configuration_profile?(params[:id])

Expand Down
26 changes: 24 additions & 2 deletions app/controllers/restful_redirect_controller.rb
Expand Up @@ -5,7 +5,24 @@ def index
case params[:model]
when 'ExtManagementSystem'
record = ExtManagementSystem.find_by(:id => params[:id])
record ? redirect_to(polymorphic_path(record)) : handle_missing_record
if record
if %w[ManageIQ::Providers::ConfigurationManager].include?(record.type) || record.type.starts_with?('ManageIQ::Providers::Foreman')
redirect_to(:controller => 'provider_foreman', :action => 'show', :id => params[:id])
elsif %w[ManageIQ::Providers::AnsibleTower::AutomationManager].include?(record.type)
redirect_to(:controller => 'automation_manager', :action => 'show', :id => params[:id])
elsif %w[ManageIQ::Providers::EmbeddedAnsible::AutomationManager].include?(record.type)
redirect_to(:controller => 'ansible_playbook', :action => 'show_list')
else
begin
redirect_to(polymorphic_path(record))
rescue NoMethodError
flash_to_session(_("Cannot redirect to \"%{record}\" provider.") % {:record => record.name}, :error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the word redirect here is a bit unfortunate. I understand that we're in redirect controller and what we're doing here is rendering redirects, but the user reading the above flash message isn't expecting any redirects, the user is just clicking in the ui.

May I suggest a string like: Cannot display ... provider or something like that?

Copy link
Contributor Author

@hstastna hstastna Oct 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Cannot redirect..." flash message/string was suggested by @martinpovolny.
And in my opinion, "cannot redirect..." can be more accurate. We just have problems with redirecting to the proper place from list of Providers from Tenant's summary, not with displaying provider's info in general. Displaying still can be possible for example via different place/controller. I just cannot guarantee proper redirecting for all kinds of providers here, so I better implemented displaying this flash message. I am little bit worried that we would create a little misunderstanding by using "cannot display..". It would feel like we are not able to display the provider at all. @martinpovolny @mzazrivec What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we aren't able to display the provider in this case, are we? My point was that redirect is really a matter of internal implementation here, not necessarilly something a user would understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, the flash message is ok. And yes, it could be confusing for some users because as you said, it's about internal implementation. I have no problem with changing the string in a flash message. But .. I think that this all is just about that we are trying to see into users' heads and assume what's understandable for them. Anyway, @martinpovolny what do you think about Milan's assumption?

redirect_to(:controller => 'ops', :action => 'explorer')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to redirect user back to the tenant detail, but can stay in the screen displaying providers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion with Martin P we decided to redirect user back to the Tenant textual summary.

end
end
else
handle_missing_record
end
when 'ServiceTemplateTransformationPlanRequest'
req = ServiceTemplateTransformationPlanRequest.select(:source_id).find(params[:id])
req ? redirect_to(:controller => 'migration', :action => 'index', :anchor => "plan/#{req.source_id}") : handle_missing_record
Expand All @@ -15,7 +32,12 @@ def index
record = VmOrTemplate.select(:id, :type).find(params[:id])
record ? handle_vm_redirect(record) : handle_missing_record
else
handle_missing_record
if params[:model].starts_with?('ExtManagementSystem')
params[:model], params[:id] = params[:model].split('/')
index
else
handle_missing_record
end
end
end

Expand Down
15 changes: 13 additions & 2 deletions app/helpers/application_helper.rb
Expand Up @@ -324,8 +324,19 @@ def view_to_url(view, parent = nil)
return url_for_only_path(:action => action) + "/" # In explorer, don't jump to other controllers
end
else
controller = "vm_cloud" if controller == "template_cloud"
controller = "vm_infra" if controller == "template_infra"
controller = case controller
when 'template_cloud'
'vm_cloud'
when 'template_infra'
'vm_infra'
when 'miq_ae_domain'
'miq_ae_class'
else
controller
end

return url_for_only_path(:controller => 'restful_redirect', :model => 'ExtManagementSystem') if controller == 'ext_management_system'

return url_for_only_path(:controller => controller, :action => action, :id => nil) + "/"
end
else
Expand Down
5 changes: 5 additions & 0 deletions app/views/miq_ae_class/show.html.haml
@@ -0,0 +1,5 @@
#main_div
= render :partial => "all_tabs"

-# To include MyCodeMirror JS and CSS files
= render :partial => "/layouts/my_code_mirror", :locals => {:text_area_id => "miq_none", :mode => "ruby"}
4 changes: 4 additions & 0 deletions app/views/provider_foreman/show.html.haml
@@ -0,0 +1,4 @@
= render(:partial => 'shared/provider_paused', :locals => {:record => @record})

#main_div
= render(:partial => 'layouts/x_gtl')
1 change: 1 addition & 0 deletions config/routes.rb
Expand Up @@ -2081,6 +2081,7 @@
explorer
method_form_fields
namespace
show
),
:post => %w(
add_update_method
Expand Down
45 changes: 27 additions & 18 deletions spec/controllers/automation_manager_controller_spec.rb
Expand Up @@ -95,9 +95,7 @@
end

context "asserts correct privileges" do
before do
login_as user_with_feature %w(automation_manager_provider_tag)
end
before { login_as user_with_feature %w[automation_manager_provider_tag] }

it "should raise an error for feature that user has no access to" do
expect { controller.send(:assert_privileges, "automation_manager_add_provider") }
Expand Down Expand Up @@ -126,10 +124,8 @@
expect(assigns(:flash_array).first[:message]).to include("has already been taken")
end

context "#edit" do
before do
stub_user(:features => :all)
end
describe "#edit" do
before { stub_user(:features => :all) }

it "renders the edit page when the manager id is supplied" do
post :edit, :params => { :id => @automation_manager1.id }
Expand Down Expand Up @@ -170,7 +166,7 @@
end
end

context "#refresh" do
describe "#refresh" do
before do
stub_user(:features => :all)
allow(controller).to receive(:x_node).and_return("root")
Expand All @@ -197,10 +193,8 @@
end
end

context "#delete" do
before do
stub_user(:features => :all)
end
describe "#delete" do
before { stub_user(:features => :all) }

it "deletes the provider when the manager id is supplied" do
allow(controller).to receive(:replace_right_cell)
Expand Down Expand Up @@ -239,6 +233,7 @@
allow(controller).to receive(:current_page).and_return(1)
controller.send(:build_accordions_and_trees)
end

it "renders right cell text for root node" do
controller.send(:get_node_info, "root")
right_cell_text = controller.instance_variable_get(:@right_cell_text)
Expand All @@ -248,9 +243,7 @@
context 'searching text' do
let(:search) { "some_text" }

before do
controller.instance_variable_set(:@search_text, search)
end
before { controller.instance_variable_set(:@search_text, search) }

it 'updates right cell text according to search text' do
controller.send(:get_node_info, "root")
Expand Down Expand Up @@ -510,6 +503,7 @@
login_as user_with_feature(%w(automation_manager_providers automation_manager_cs_filter_accord automation_manager_configuration_scripts_accord))
controller.instance_variable_set(:@right_cell_text, nil)
end

render_views

it 'can render details for a job template' do
Expand Down Expand Up @@ -567,7 +561,7 @@
end
end

context "#build_credentials" do
describe "#build_credentials" do
it "uses params[:default_password] for validation if one exists" do
controller.params = {:default_userid => "userid",
:default_password => "password2"}
Expand All @@ -589,6 +583,7 @@
@user = user_with_feature %w(automation_manager_providers automation_manager_configured_system)
login_as @user
end

it "builds foreman tree with no nodes after rbac filtering" do
user_filters = {'belongs' => [], 'managed' => [tags]}
allow(@user).to receive(:get_filters).and_return(user_filters)
Expand All @@ -598,7 +593,7 @@
end
end

context "#configscript_service_dialog" do
describe "#configscript_service_dialog" do
before do
stub_user(:features => :all)
@cs = FactoryBot.create(:ansible_configuration_script)
Expand Down Expand Up @@ -631,8 +626,9 @@
end
end

context "#tags_edit" do
describe "#tags_edit" do
let!(:user) { stub_user(:features => :all) }

before do
EvmSpecHelper.create_guid_miq_server_zone
allow(@ans_configured_system).to receive(:tagged_with).with(:cat => user.userid).and_return("my tags")
Expand Down Expand Up @@ -674,6 +670,19 @@
end
end

describe '#show' do
before { controller.params = {:id => @automation_manager1.id} }

it 'displays Automation Manager provider through Tenants textual summary' do
expect(controller).to receive(:build_accordions_and_trees)
expect(controller).to receive(:generic_x_show)
controller.send(:show)
expect(controller.instance_variable_get(:@explorer)).to be(true)
expect(controller.instance_variable_get(:@record)).to eq(@automation_manager1)
expect(controller.x_node).to eq("at-#{controller.params[:id]}")
end
end

def find_treenode_for_provider(provider, tree)
key = ems_key_for_provider(provider)
tree[0][:nodes]&.find { |c| c['key'] == key }
Expand Down