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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36608 - Fix customizing discovered hosts / use safe navigation for content facet in host_managed_extensions #10658

Merged
merged 3 commits into from
Aug 2, 2023

Conversation

jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Jul 20, 2023

What are the changes introduced in this pull request?

  • Fix some safe navigation errors around customizing discovered hosts.
  • Hide generated content views from content view drop-down on edit host(group) page.
  • Hide change content source link when using discovered host edit page.
  • Allow you to edit content view and lifecycle environment after inheriting from hostgroup

Considerations taken when implementing this change?

I wasn't able to reproduce the original issue in https://bugzilla.redhat.com/show_bug.cgi?id=2069324 (non-inheritance of CV and LCE). But I did find these other issues.

What are the testing steps for this pull request?

  1. Create a normal HG (not nested) with all information.

  2. Discover the host:

Check out foreman_discovery repo and add to your .local.rb
bundle install and run migrations

Create a fake discovered host:

cd foreman_discovery/extra
./discover-host
# or if necessary, ./discover-host --url https://<your-server-fqdn>
  1. Go to Hosts >> Discovered Hosts >> Click on Provision >> Select Host group >> Customize Host

Before: You'll get a silly error
image

After:

  • You shouldn't get an error
  • Lifecycle environment and content view should be correctly inherited from the hostgroup

@theforeman-bot
Copy link

Issues: #36608

@jeremylenz jeremylenz force-pushed the 36608-discovered-host-errors branch from 39c1da2 to 50d4b98 Compare July 20, 2023 17:23
@jeremylenz jeremylenz force-pushed the 36608-discovered-host-errors branch from 50d4b98 to e6def05 Compare July 20, 2023 17:34
@jeremylenz jeremylenz changed the title 36608 discovered host errors Fixes #36608 - Fix customizing discovered hosts / use safe navigation for content facet in host_managed_extensions Jul 20, 2023
@jeremylenz jeremylenz marked this pull request as ready for review July 20, 2023 17:35
@jeremylenz jeremylenz force-pushed the 36608-discovered-host-errors branch from e6def05 to 58d7d0b Compare July 20, 2023 20:17
@parthaa
Copy link
Contributor

parthaa commented Aug 1, 2023

This PR works well. No ugly undefined stuff. But the behavior is weird if you have a host group that just has the environment but no CV. The suggested behavior should be more like create host vs edit host. The patch below tries to simulate that (and also fixes things like generated cvs showing up in the cv drop down etc.).

Suggested Patch:

diff --git a/app/helpers/katello/hosts_and_hostgroups_helper.rb b/app/helpers/katello/hosts_and_hostgroups_helper.rb
index d86319241d..9ecddc6bad 100644
--- a/app/helpers/katello/hosts_and_hostgroups_helper.rb
+++ b/app/helpers/katello/hosts_and_hostgroups_helper.rb
@@ -4,6 +4,18 @@ module Katello
       "kt_activation_keys"
     end
 
+    def edit_action?
+      params[:action] == 'edit'
+    end
+
+    def cv_lce_disabled?
+      edit_action? && !using_discovered_hosts_page?
+    end
+
+    def using_discovered_hosts_page?
+      controller.controller_name == "discovered_hosts"
+    end
+
     def using_hostgroups_page?
       controller.controller_name == "hostgroups"
     end
@@ -164,7 +176,7 @@ module Katello
 
       views = []
       if lifecycle_environment
-        views = Katello::ContentView.in_environment(lifecycle_environment).readable.order(:name)
+        views = Katello::ContentView.in_environment(lifecycle_environment).ignore_generated.readable.order(:name)
         views |= [content_view] if content_view.present? && content_view.in_environment?(lifecycle_environment)
       elsif content_view
         views = [content_view]
diff --git a/app/models/katello/concerns/host_managed_extensions.rb b/app/models/katello/concerns/host_managed_extensions.rb
index 953ec23212..d5506c52bd 100644
--- a/app/models/katello/concerns/host_managed_extensions.rb
+++ b/app/models/katello/concerns/host_managed_extensions.rb
@@ -14,8 +14,9 @@ module Katello
               cve = content_facet&.assign_single_environment(content_view_id: cv_id, lifecycle_environment_id: lce_id)
               Rails.logger.warn "Couldn't assign content view environment; host has no content facet" if cve.blank?
             end
+
             if (cv_id.present? && lce_id.blank?) || (cv_id.blank? && lce_id.present?)
-              fail "content_view_id and lifecycle_environment_id must be provided together"
+              errors.add(:base, _("Content View and Lifecycle Environment must be provided together"))
             end
           end
         end
diff --git a/app/views/overrides/activation_keys/_host_environment_select.html.erb b/app/views/overrides/activation_keys/_host_environment_select.html.erb
index e93b123609..a5da5a8e03 100644
--- a/app/views/overrides/activation_keys/_host_environment_select.html.erb
+++ b/app/views/overrides/activation_keys/_host_environment_select.html.erb
@@ -5,9 +5,8 @@
     option.kt-cv  { margin-left: 1em; }
 </style>
 <% spinner_path = asset_path('spinner.gif') %>
-<% edit_action = params[:action] == 'edit' %>
 
-<% if edit_action && !using_hostgroups_page? %>
+<% if edit_action? && !using_hostgroups_page? && !using_discovered_hosts_page? %>
   <div style="margin-left: 270px">
     <%= link_to _("Change content source"), "/change_host_content_source?host_id=#{@host.id}" %>
   </div>
@@ -22,7 +21,7 @@
     select_tag cs_select_id, content_source_options(@hostgroup, :include_blank => blank_or_inherit_with_id(f, :content_source)), :data => {"spinner_path" => spinner_path},
                :class => 'form-control',  :name => cs_select_name
   else
-    select_tag cs_select_id, content_source_options(@host, :selected_host_group => @hostgroup || @host.hostgroup, :include_blank => blank_or_inherit_with_id(f, :content_source)), :data => {"spinner_path" => spinner_path}, :class => 'form-control',  :name => cs_select_name, :disabled => edit_action
+    select_tag cs_select_id, content_source_options(@host, :selected_host_group => @hostgroup || @host.hostgroup, :include_blank => blank_or_inherit_with_id(f, :content_source)), :data => {"spinner_path" => spinner_path}, :class => 'form-control',  :name => cs_select_name, :disabled => cv_lce_disabled?
   end
 end %>
 
@@ -35,7 +34,7 @@ end %>
     select_tag env_select_id, lifecycle_environment_options(@hostgroup, :include_blank => blank_or_inherit_with_id(f, :lifecycle_environment)),
                :class => 'form-control',  :name => env_select_name
   else
-    select_tag env_select_id, lifecycle_environment_options(@host, :selected_host_group => @hostgroup || @host.hostgroup, :include_blank => blank_or_inherit_with_id(f, :lifecycle_environment)), :class => 'form-control',  :name => env_select_name, :disabled => edit_action
+    select_tag env_select_id, lifecycle_environment_options(@host, :selected_host_group => @hostgroup || @host.hostgroup, :include_blank => blank_or_inherit_with_id(f, :lifecycle_environment)), :class => 'form-control',  :name => env_select_name, :disabled =>  cv_lce_disabled?
   end
 end %>
 
@@ -47,6 +46,6 @@ end %>
     select_tag cv_select_id,  content_views_for_host(@hostgroup, :include_blank => blank_or_inherit_with_id(f, :content_view)), :data => {"spinner_path" => spinner_path},
                :class => 'form-control',  :name => cv_select_name
   else
-    select_tag cv_select_id,  content_views_for_host(@host, :selected_host_group => @hostgroup || @host.hostgroup, :include_blank => blank_or_inherit_with_id(f, :content_view)), :data => {"spinner_path" => spinner_path}, :class => 'form-control',  :name => cv_select_name, :disabled => edit_action
+    select_tag cv_select_id,  content_views_for_host(@host, :selected_host_group => @hostgroup || @host.hostgroup, :include_blank => blank_or_inherit_with_id(f, :content_view)), :data => {"spinner_path" => spinner_path}, :class => 'form-control',  :name => cv_select_name, :disabled =>  cv_lce_disabled?
   end
 end %>

 - also hide generated CVs

Co-authored-by: Partha Aji <paji@redhat.com>
@jeremylenz
Copy link
Member Author

@parthaa Updated with your commit, I just changed some capitalizations :) but it seems to work great! let's see how the test run goes

Copy link
Contributor

@parthaa parthaa left a comment

Choose a reason for hiding this comment

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

APJ

@jeremylenz
Copy link
Member Author

[test katello]

@jeremylenz jeremylenz merged commit 5f1e74d into Katello:master Aug 2, 2023
4 of 5 checks passed
wbclark pushed a commit to wbclark/katello that referenced this pull request Sep 7, 2023
… for content facet in host_managed_extensions (Katello#10658)

(cherry picked from commit 5f1e74d)
wbclark pushed a commit that referenced this pull request Sep 27, 2023
… for content facet in host_managed_extensions (#10658)

(cherry picked from commit 5f1e74d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants