Fixes #39330 - Remove deprecated CV/LCE params from hosts, AKs, hostgroups#11753
Merged
jeremylenz merged 2 commits intoMay 28, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates from separate content_view + lifecycle_environment fields to ContentViewEnvironment (CVEnv): updates JS host edit UI, API params/docs, models (facets/activation keys), helpers, actions/services, views, plugin filters, validators/migrators, and tests to use CVEnv-backed params and associations. ChangesCVEnv migration and wiring
🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/assets/javascripts/katello/hosts/host_and_hostgroup_edit.js (1)
57-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t key inherit-option preservation off English text.
Line 57 only keeps the first option when its label contains
"Inherit"or is empty. In translated UIs that drops the inherit option after refresh, even though the option is still structurally the “blank/inherit” choice.Suggested fix
- if (previousInheritText && (previousInheritText.includes('Inherit') || previousInheritText === '')) { + if (inheritOption.length && (inheritOption.val() === '' || previousInheritDataId)) { var inheritOpt = $("<option />").text(previousInheritText).val(''); if (previousInheritDataId) inheritOpt.attr('data-id', previousInheritDataId); select.append(inheritOpt); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/assets/javascripts/katello/hosts/host_and_hostgroup_edit.js` around lines 57 - 60, The code is currently detecting the “inherit” option by checking previousInheritText.includes('Inherit') which breaks localized UIs; change the detection to rely on the option's value or presence of the data-id instead. Update the condition around previousInheritText/previousInheritDataId in the block that creates inheritOpt (variables: previousInheritText, previousInheritDataId, inheritOpt, select) to add the option when previousInheritText === '' or previousInheritValue === '' or when previousInheritDataId is present, instead of testing the displayed text; ensure you read the original option's value (not its label) to decide preservation and preserve any data-id attribute onto inheritOpt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/assets/javascripts/katello/hosts/host_and_hostgroup_edit.js`:
- Around line 101-126: KT.hosts.getSelectedContentViewEnvironment currently
returns null when the visible `#host_content_view_environment_id` is not present
(cv_lce_disabled? case), causing KT.hosts.toggle_installation_medium to skip
refreshing; update getSelectedContentViewEnvironment to fall back to the hidden
inputs rendered on host edit pages by checking for inputs named
"content_view_environment_ids[]" (e.g. select the first hidden input or
.first()) and returning its value (or its data-id if needed) when
`#host_content_view_environment_id` and `#hostgroup_content_view_environment_id` are
absent so toggle_installation_medium receives a valid
content_view_environment_id.
In `@app/helpers/katello/hosts_and_hostgroups_helper.rb`:
- Around line 74-82: The current else branch in the host_or_hostgroup handling
unconditionally returns
host_or_hostgroup.content_facet.content_view_environments.first which can be nil
and thus prevents the selected_host_group fallback from running; update the
logic in the host_or_hostgroup branch (symbols: host_or_hostgroup,
content_facet, content_view_environments) to only return the first environment
if it is present (e.g., assign first_env =
host_or_hostgroup.content_facet.content_view_environments.first and return
first_env only when first_env.present?), otherwise allow execution to continue
so the subsequent selected_host_group (symbol: selected_host_group)
content_facet.content_view_environment fallback can run.
In `@app/models/katello/host/content_facet.rb`:
- Around line 83-87: The current initialization pulls content_view_environments
with ContentViewEnvironment.where(id: cve_ids) which silently drops missing IDs
and loses the requested order; change this to load the records into a hash keyed
by id (using ContentViewEnvironment.where(id: cve_ids).index_by(&:id)) then map
the original cve_ids array to that hash to preserve order and detect missing
records, raising a clear error (e.g., ActiveRecord::RecordNotFound or
ArgumentError) if any requested id is absent before assigning
self.content_view_environments; adjust the code around
init_args.delete(:content_view_environment_ids), cve_ids, and the assignment to
content_view_environments accordingly.
In `@app/views/overrides/activation_keys/_host_environment_select.html.erb`:
- Around line 46-49: host_cve_ids may produce multiple hidden inputs with the
same generated DOM id (from hidden_field_tag
'host[content_facet_attributes][content_view_environment_ids][]'), causing
duplicate IDs; fix by iterating with an index (host_cve_ids.each_with_index) and
pass an explicit unique id option to hidden_field_tag (e.g.
"host_content_facet_attributes_content_view_environment_ids_#{index}") or remove
the id entirely by passing id: nil so each hidden input has no duplicate id
while keeping the same name attribute.
---
Outside diff comments:
In `@app/assets/javascripts/katello/hosts/host_and_hostgroup_edit.js`:
- Around line 57-60: The code is currently detecting the “inherit” option by
checking previousInheritText.includes('Inherit') which breaks localized UIs;
change the detection to rely on the option's value or presence of the data-id
instead. Update the condition around previousInheritText/previousInheritDataId
in the block that creates inheritOpt (variables: previousInheritText,
previousInheritDataId, inheritOpt, select) to add the option when
previousInheritText === '' or previousInheritValue === '' or when
previousInheritDataId is present, instead of testing the displayed text; ensure
you read the original option's value (not its label) to decide preservation and
preserve any data-id attribute onto inheritOpt.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f88a001e-bc3c-412f-b345-191cd0d72cb7
📒 Files selected for processing (34)
app/assets/javascripts/katello/hosts/host_and_hostgroup_edit.jsapp/controllers/katello/api/v2/activation_keys_controller.rbapp/controllers/katello/api/v2/host_contents_controller.rbapp/controllers/katello/concerns/api/v2/hostgroups_controller_extensions.rbapp/controllers/katello/concerns/api/v2/hosts_controller_extensions.rbapp/helpers/katello/hosts_and_hostgroups_helper.rbapp/helpers/katello/kickstart_repository_helper.rbapp/lib/actions/katello/activation_key/reassign.rbapp/lib/actions/katello/host/reassign.rbapp/lib/actions/katello/host/update_content_view.rbapp/models/katello/activation_key.rbapp/models/katello/concerns/content_facet_host_extensions.rbapp/models/katello/concerns/host_managed_extensions.rbapp/models/katello/concerns/hostgroup_extensions.rbapp/models/katello/content_view_environment.rbapp/models/katello/host/content_facet.rbapp/models/katello/hostgroup/content_facet.rbapp/views/katello/api/v2/activation_keys/base.json.rablapp/views/katello/api/v2/hostgroups_extensions/show.json.rablapp/views/overrides/activation_keys/_host_environment_select.html.erblib/katello/plugin.rbtest/controllers/api/v2/activation_keys_controller_test.rbtest/controllers/api/v2/hostgroups_controller_test.rbtest/controllers/api/v2/hosts_controller_test.rbtest/factories/host_factory.rbtest/helpers/hosts_and_hostgroups_helper_test.rbtest/helpers/url_helpers_test.rbtest/lib/concerns/renderer_extensions_test.rbtest/mailers/errata_mailer_test.rbtest/models/concerns/content_facet_host_extensions_test.rbtest/models/concerns/host_managed_extensions_test.rbtest/models/erratum_test.rbtest/models/host/content_facet_test.rbtest/support/host_support.rb
💤 Files with no reviewable changes (4)
- app/controllers/katello/concerns/api/v2/hostgroups_controller_extensions.rb
- app/views/katello/api/v2/activation_keys/base.json.rabl
- app/models/katello/activation_key.rb
- app/models/katello/concerns/host_managed_extensions.rb
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
3 times, most recently
from
8fe71fc to
4c9512a
Compare
…roups Remove the deprecated separate content_view_id and lifecycle_environment_id API parameters that were replaced by content_view_environment_ids (hosts/AKs) and content_view_environment_id (hostgroups) in 6.19. Since 7.0 allows breaking changes, this fully removes the backward-compatibility layer. Key changes: - Remove assign_single_environment from ContentFacet and ActivationKey (fixes incorrect first_or_create behavior in AK version) - Add ContentViewEnvironment.find_by_cv_and_lce! class method - Remove check_cve_attributes and related backward-compat in HostManagedExtensions - Simplify Hostgroup::ContentFacet by removing pending variable machinery - Remove setter delegates from HostgroupExtensions - Update ERB host form to use single CVEnv dropdown - Update JS to use content_view_environment_id instead of separate cascading dropdowns - Update parameter filters in plugin.rb - Remove deprecated params from API controllers and RABL responses Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
2 times, most recently
from
5c55c41 to
db62b7c
Compare
nofaralfasi
suggested changes
May 19, 2026
Contributor
nofaralfasi
left a comment
nofaralfasi
left a comment
There was a problem hiding this comment.
I noticed CI is failing HostgroupsControllerTest#test_create_with_ks_repo, and that test in test/controllers/foreman/hostgroups_controller_test.rb still posts deprecated content_view_id / lifecycle_environment_id params.
Since this PR removes those hostgroup params in favor of content_view_environment_id, should we update that test in this PR?
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
from
db62b7c to
489455d
Compare
Member
Author
|
@nofaralfasi Updated:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/helpers/katello/content_options_helper.rb`:
- Line 77: The optgroup label interpolates org.name into HTML without escaping,
allowing XSS; update the interpolation in
app/helpers/katello/content_options_helper.rb to HTML-escape org.name before
building the string (e.g., use h(org.name) or ERB::Util.html_escape(org.name)
when constructing the "%(<optgroup label=...>#{cvenv_options}</optgroup>)"
string) so the returned html_safe output contains a safe, escaped label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0736dbbd-3c7d-4a2c-8936-c0960220312d
📒 Files selected for processing (72)
app/assets/javascripts/katello/hosts/host_and_hostgroup_edit.jsapp/controllers/katello/api/rhsm/candlepin_proxies_controller.rbapp/controllers/katello/api/v2/activation_keys_controller.rbapp/controllers/katello/api/v2/content_view_versions_controller.rbapp/controllers/katello/api/v2/host_contents_controller.rbapp/controllers/katello/api/v2/hosts_bulk_actions_controller.rbapp/controllers/katello/concerns/api/v2/hostgroups_controller_extensions.rbapp/controllers/katello/concerns/api/v2/hosts_controller_extensions.rbapp/controllers/katello/concerns/authorization/api/v2/content_views_controller.rbapp/helpers/katello/content_options_helper.rbapp/helpers/katello/hosts_and_hostgroups_helper.rbapp/helpers/katello/kickstart_repository_helper.rbapp/lib/actions/katello/activation_key/reassign.rbapp/lib/actions/katello/content_view/add_to_environment.rbapp/lib/actions/katello/content_view/incremental_updates.rbapp/lib/actions/katello/content_view/remove.rbapp/lib/actions/katello/content_view/remove_from_environment.rbapp/lib/actions/katello/environment/destroy.rbapp/lib/actions/katello/host/reassign.rbapp/lib/actions/katello/host/update_content_view.rbapp/lib/actions/katello/organization/destroy.rbapp/lib/actions/katello/organization/environment_contents_refresh.rbapp/lib/katello/util/cve_hgcf_migrator.rbapp/lib/katello/util/cveak_migrator.rbapp/lib/katello/util/cvecf_migrator.rbapp/lib/katello/validators/generated_content_view_validator.rbapp/lib/katello/validators/hostgroup_kickstart_repository_validator.rbapp/models/katello/activation_key.rbapp/models/katello/authorization/repository.rbapp/models/katello/concerns/content_facet_host_extensions.rbapp/models/katello/concerns/host_managed_extensions.rbapp/models/katello/concerns/hostgroup_extensions.rbapp/models/katello/concerns/subscription_facet_host_extensions.rbapp/models/katello/content_view_environment.rbapp/models/katello/content_view_environment_activation_key.rbapp/models/katello/content_view_environment_content_facet.rbapp/models/katello/host/content_facet.rbapp/models/katello/host/info_provider.rbapp/models/katello/host/subscription_facet.rbapp/models/katello/hostgroup/content_facet.rbapp/models/katello/repository.rbapp/services/katello/content_view_manager.rbapp/services/katello/product_content_finder.rbapp/services/katello/registration_manager.rbapp/views/katello/api/v2/activation_keys/base.json.rablapp/views/katello/api/v2/hostgroups_extensions/show.json.rablapp/views/overrides/activation_keys/_host_environment_select.html.erbdeveloper_docs/quick_reference.mdlib/katello/plugin.rbtest/actions/katello/content_view_test.rbtest/actions/katello/environment_test.rbtest/controllers/api/v2/activation_keys_controller_test.rbtest/controllers/api/v2/content_views_controller_test.rbtest/controllers/api/v2/hostgroups_controller_test.rbtest/controllers/api/v2/hosts_controller_test.rbtest/controllers/foreman/hostgroups_controller_test.rbtest/factories/host_factory.rbtest/helpers/hosts_and_hostgroups_helper_test.rbtest/helpers/url_helpers_test.rbtest/lib/concerns/renderer_extensions_test.rbtest/lib/validators/hostgroup_kickstart_repository_validator_test.rbtest/mailers/errata_mailer_test.rbtest/models/activation_key_test.rbtest/models/concerns/content_facet_host_extensions_test.rbtest/models/concerns/host_managed_extensions_test.rbtest/models/concerns/hostgroup_extensions_test.rbtest/models/content_view_test.rbtest/models/erratum_test.rbtest/models/host/content_facet_test.rbtest/models/repository_test.rbtest/services/katello/registration_manager_test.rbtest/support/host_support.rb
💤 Files with no reviewable changes (3)
- app/controllers/katello/concerns/api/v2/hostgroups_controller_extensions.rb
- app/views/katello/api/v2/activation_keys/base.json.rabl
- app/models/katello/concerns/host_managed_extensions.rb
✅ Files skipped from review due to trivial changes (16)
- developer_docs/quick_reference.md
- app/lib/katello/validators/generated_content_view_validator.rb
- app/lib/actions/katello/content_view/incremental_updates.rb
- test/models/repository_test.rb
- app/lib/actions/katello/environment/destroy.rb
- app/lib/actions/katello/organization/environment_contents_refresh.rb
- app/controllers/katello/api/rhsm/candlepin_proxies_controller.rb
- app/lib/actions/katello/content_view/add_to_environment.rb
- app/models/katello/authorization/repository.rb
- app/controllers/katello/api/v2/content_view_versions_controller.rb
- app/controllers/katello/concerns/authorization/api/v2/content_views_controller.rb
- test/actions/katello/environment_test.rb
- app/lib/actions/katello/organization/destroy.rb
- app/lib/katello/util/cve_hgcf_migrator.rb
- test/models/content_view_test.rb
- app/models/katello/host/info_provider.rb
🚧 Files skipped from review as they are similar to previous changes (23)
- test/helpers/url_helpers_test.rb
- test/mailers/errata_mailer_test.rb
- test/factories/host_factory.rb
- app/lib/actions/katello/host/update_content_view.rb
- app/lib/actions/katello/activation_key/reassign.rb
- app/views/katello/api/v2/hostgroups_extensions/show.json.rabl
- app/models/katello/concerns/content_facet_host_extensions.rb
- app/views/overrides/activation_keys/_host_environment_select.html.erb
- app/lib/actions/katello/host/reassign.rb
- test/models/erratum_test.rb
- test/models/concerns/content_facet_host_extensions_test.rb
- test/helpers/hosts_and_hostgroups_helper_test.rb
- app/controllers/katello/concerns/api/v2/hosts_controller_extensions.rb
- app/models/katello/hostgroup/content_facet.rb
- app/controllers/katello/api/v2/host_contents_controller.rb
- app/helpers/katello/hosts_and_hostgroups_helper.rb
- test/models/host/content_facet_test.rb
- app/models/katello/host/content_facet.rb
- app/models/katello/content_view_environment.rb
- test/controllers/api/v2/hosts_controller_test.rb
- lib/katello/plugin.rb
- test/models/concerns/host_managed_extensions_test.rb
- test/controllers/api/v2/hostgroups_controller_test.rb
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
2 times, most recently
from
2df53a6 to
a05a66a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/models/katello/host/content_facet.rb (1)
89-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve requested CVEnv order and reject unknown IDs during initialization.
Line 92 uses
where(id: cvenv_ids), which can reorder results and silently ignore invalid IDs. That can produce incorrect CVEnv priority/order on initialization.Suggested fix
cvenv_ids = init_args.delete(:content_view_environment_ids) super(*args) if cvenv_ids.present? - self.content_view_environments = ContentViewEnvironment.where(id: cvenv_ids) + requested_ids = Array(cvenv_ids).map(&:to_s) + cvenvs_by_id = ContentViewEnvironment.where(id: requested_ids).index_by { |cvenv| cvenv.id.to_s } + missing_ids = requested_ids - cvenvs_by_id.keys + + raise ActiveRecord::RecordNotFound, "Couldn't find all Katello::ContentViewEnvironment records for IDs: #{missing_ids.join(', ')}" if missing_ids.any? + + self.content_view_environments = requested_ids.map { |id| cvenvs_by_id.fetch(id) } end self.cvenvs_changed = false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/katello/host/content_facet.rb` around lines 89 - 93, The current init uses ContentViewEnvironment.where(id: cvenv_ids) which can reorder results and silently ignore unknown IDs; change the init to coerce cvenv_ids to an array of IDs and load them with ContentViewEnvironment.find(ids) (or fetch and compare ids using index_by) so records are returned in the requested order and missing IDs cause an error (or are explicitly rejected) before assigning self.content_view_environments.
🧹 Nitpick comments (2)
test/controllers/api/v2/content_views_controller_test.rb (1)
656-659: ⚡ Quick winUse a version-independent CVEnv lookup in test setup.
Line 656 ties the lookup to
alternate_cv.versions.first, which is brittle if version ordering or promoted version changes; that can cascade into a nil dereference at Line 690.Suggested fix
- alternate_cvenv = Katello::ContentViewEnvironment.find_by( - :content_view_version_id => alternate_cv.versions.first.id, - :environment_id => alternate_env.id - ) + alternate_cvenv = Katello::ContentViewEnvironment.find_by!( + :content_view_id => alternate_cv.id, + :environment_id => alternate_env.id + )Also applies to: 690-690
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/api/v2/content_views_controller_test.rb` around lines 656 - 659, The test setup should not rely on a specific ContentViewVersion; replace the brittle lookup Katello::ContentViewEnvironment.find_by(content_view_version_id: alternate_cv.versions.first.id, environment_id: alternate_env.id) with a version-independent lookup that uses the content view id and environment id (e.g. find_by(content_view_id: alternate_cv.id, environment_id: alternate_env.id) or a where(...).first) so alternate_cvenv is resolved regardless of promoted/version ordering (also apply the same change where the lookup is repeated near line 690).test/controllers/api/v2/hosts_controller_test.rb (1)
61-62: ⚡ Quick winUse strict CVEnv lookups for clearer test failures.
These
where(...).firstlookups can produce nil and fail later asNoMethodError. Usingfind_by_cv_and_lce!makes failures immediate and explicit.Proposed refactor
- target_cvenv = ::Katello::ContentViewEnvironment.where(:content_view_id => `@cv3.id`, - :environment_id => `@dev.id`).first + target_cvenv = ::Katello::ContentViewEnvironment.find_by_cv_and_lce!(`@cv3.id`, `@dev.id`) ... - target_cvenvs = [::Katello::ContentViewEnvironment.where(:content_view_id => `@cv4.id`, - :environment_id => `@dev.id`).first, ::Katello::ContentViewEnvironment.where(:content_view_id => `@cv3.id`, - :environment_id => `@dev.id`).first] + target_cvenvs = [ + ::Katello::ContentViewEnvironment.find_by_cv_and_lce!(`@cv4.id`, `@dev.id`), + ::Katello::ContentViewEnvironment.find_by_cv_and_lce!(`@cv3.id`, `@dev.id`), + ] ... - target_cvenvs_ids = [::Katello::ContentViewEnvironment.where(:content_view_id => `@cv4.id`, - :environment_id => `@dev.id`).first, ::Katello::ContentViewEnvironment.where(:content_view_id => `@cv3.id`, - :environment_id => `@dev.id`).first].map(&:id) + target_cvenvs_ids = [ + ::Katello::ContentViewEnvironment.find_by_cv_and_lce!(`@cv4.id`, `@dev.id`).id, + ::Katello::ContentViewEnvironment.find_by_cv_and_lce!(`@cv3.id`, `@dev.id`).id, + ]Also applies to: 83-85, 107-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/api/v2/hosts_controller_test.rb` around lines 61 - 62, Replace the fragile "Katello::ContentViewEnvironment.where(...).first" lookup used to set target_cvenv with the strict lookup method "Katello::ContentViewEnvironment.find_by_cv_and_lce!" (passing the content view `@cv3` and environment `@dev`) so the test raises an explicit error on missing records; do the same for the other similar lookups in this test (the occurrences that use where(...).first around the later assertions) to ensure failures are immediate and clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/katello/concerns/content_facet_host_extensions.rb`:
- Line 77: The reject logic currently checks
attributes['content_view_environment_ids'].blank? which treats [""] as present;
update the reject_if condition used in content_facet_host_extensions.rb (the
reject block that inspects attributes['content_view_environment_ids']) to
normalize the param into an array and treat arrays of blank strings as empty—for
example by converting to Array(attributes['content_view_environment_ids']) and
checking that after rejecting blank entries it is empty—so that [""] is
considered blank and the reject_if behaves correctly.
In `@test/controllers/api/v2/activation_keys_controller_test.rb`:
- Around line 449-467: This test mutates Setting['allow_multiple_content_views']
and doesn't restore it; save the original value before setting it true, and
ensure you restore it at the end of the test (use an ensure/finally or test
teardown) so subsequent tests are not affected; update the test surrounding
Setting['allow_multiple_content_views'] (the block that sets it, uses multi_key
and calls put :update on the controller) to capture the original setting and
reset it after the assertions.
---
Duplicate comments:
In `@app/models/katello/host/content_facet.rb`:
- Around line 89-93: The current init uses ContentViewEnvironment.where(id:
cvenv_ids) which can reorder results and silently ignore unknown IDs; change the
init to coerce cvenv_ids to an array of IDs and load them with
ContentViewEnvironment.find(ids) (or fetch and compare ids using index_by) so
records are returned in the requested order and missing IDs cause an error (or
are explicitly rejected) before assigning self.content_view_environments.
---
Nitpick comments:
In `@test/controllers/api/v2/content_views_controller_test.rb`:
- Around line 656-659: The test setup should not rely on a specific
ContentViewVersion; replace the brittle lookup
Katello::ContentViewEnvironment.find_by(content_view_version_id:
alternate_cv.versions.first.id, environment_id: alternate_env.id) with a
version-independent lookup that uses the content view id and environment id
(e.g. find_by(content_view_id: alternate_cv.id, environment_id:
alternate_env.id) or a where(...).first) so alternate_cvenv is resolved
regardless of promoted/version ordering (also apply the same change where the
lookup is repeated near line 690).
In `@test/controllers/api/v2/hosts_controller_test.rb`:
- Around line 61-62: Replace the fragile
"Katello::ContentViewEnvironment.where(...).first" lookup used to set
target_cvenv with the strict lookup method
"Katello::ContentViewEnvironment.find_by_cv_and_lce!" (passing the content view
`@cv3` and environment `@dev`) so the test raises an explicit error on missing
records; do the same for the other similar lookups in this test (the occurrences
that use where(...).first around the later assertions) to ensure failures are
immediate and clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed0b6aea-1746-46ce-9a82-9582cd8cfd85
📒 Files selected for processing (68)
app/assets/javascripts/katello/hosts/host_and_hostgroup_edit.jsapp/controllers/katello/api/rhsm/candlepin_proxies_controller.rbapp/controllers/katello/api/v2/activation_keys_controller.rbapp/controllers/katello/api/v2/content_view_versions_controller.rbapp/controllers/katello/api/v2/hosts_bulk_actions_controller.rbapp/controllers/katello/concerns/api/v2/hosts_controller_extensions.rbapp/controllers/katello/concerns/authorization/api/v2/content_views_controller.rbapp/helpers/katello/content_options_helper.rbapp/helpers/katello/hosts_and_hostgroups_helper.rbapp/helpers/katello/kickstart_repository_helper.rbapp/lib/actions/katello/activation_key/reassign.rbapp/lib/actions/katello/content_view/add_to_environment.rbapp/lib/actions/katello/content_view/incremental_updates.rbapp/lib/actions/katello/content_view/remove.rbapp/lib/actions/katello/content_view/remove_from_environment.rbapp/lib/actions/katello/environment/destroy.rbapp/lib/actions/katello/host/reassign.rbapp/lib/actions/katello/host/update_content_view.rbapp/lib/actions/katello/organization/destroy.rbapp/lib/actions/katello/organization/environment_contents_refresh.rbapp/lib/katello/util/cve_hgcf_migrator.rbapp/lib/katello/util/cveak_migrator.rbapp/lib/katello/util/cvecf_migrator.rbapp/lib/katello/validators/generated_content_view_validator.rbapp/lib/katello/validators/hostgroup_kickstart_repository_validator.rbapp/models/katello/activation_key.rbapp/models/katello/authorization/repository.rbapp/models/katello/concerns/content_facet_host_extensions.rbapp/models/katello/concerns/hostgroup_extensions.rbapp/models/katello/concerns/subscription_facet_host_extensions.rbapp/models/katello/content_view_environment.rbapp/models/katello/content_view_environment_activation_key.rbapp/models/katello/content_view_environment_content_facet.rbapp/models/katello/host/content_facet.rbapp/models/katello/host/info_provider.rbapp/models/katello/host/subscription_facet.rbapp/models/katello/repository.rbapp/services/katello/content_view_manager.rbapp/services/katello/product_content_finder.rbapp/services/katello/registration_manager.rbapp/views/overrides/activation_keys/_host_environment_select.html.erbdeveloper_docs/quick_reference.mdtest/actions/katello/content_view_test.rbtest/actions/katello/environment_test.rbtest/controllers/api/v2/activation_keys_controller_test.rbtest/controllers/api/v2/api_controller_test.rbtest/controllers/api/v2/content_views_controller_test.rbtest/controllers/api/v2/hostgroups_controller_test.rbtest/controllers/api/v2/hosts_controller_test.rbtest/controllers/foreman/hostgroups_controller_test.rbtest/factories/host_factory.rbtest/helpers/hosts_and_hostgroups_helper_test.rbtest/helpers/url_helpers_test.rbtest/lib/concerns/renderer_extensions_test.rbtest/lib/validators/hostgroup_kickstart_repository_validator_test.rbtest/mailers/errata_mailer_test.rbtest/models/activation_key_test.rbtest/models/authorization/activation_key_authorization_test.rbtest/models/concerns/content_facet_host_extensions_test.rbtest/models/concerns/host_managed_extensions_test.rbtest/models/concerns/hostgroup_extensions_test.rbtest/models/content_view_test.rbtest/models/erratum_test.rbtest/models/host/content_facet_test.rbtest/models/hostgroup/content_facet_test.rbtest/models/repository_test.rbtest/services/katello/registration_manager_test.rbtest/support/host_support.rb
✅ Files skipped from review due to trivial changes (16)
- app/lib/actions/katello/environment/destroy.rb
- app/lib/actions/katello/content_view/incremental_updates.rb
- developer_docs/quick_reference.md
- app/controllers/katello/api/rhsm/candlepin_proxies_controller.rb
- app/lib/actions/katello/organization/environment_contents_refresh.rb
- app/controllers/katello/api/v2/hosts_bulk_actions_controller.rb
- app/controllers/katello/concerns/authorization/api/v2/content_views_controller.rb
- app/models/katello/host/info_provider.rb
- app/lib/actions/katello/content_view/remove_from_environment.rb
- app/lib/actions/katello/content_view/add_to_environment.rb
- test/models/repository_test.rb
- app/lib/katello/util/cveak_migrator.rb
- app/controllers/katello/api/v2/content_view_versions_controller.rb
- app/services/katello/product_content_finder.rb
- app/lib/actions/katello/organization/destroy.rb
- app/lib/katello/util/cve_hgcf_migrator.rb
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
from
a05a66a to
e57599e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/controllers/katello/api/v2/activation_keys_controller.rb (1)
223-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlank CVEnv payloads still raise before the clear path runs.
[]and""are truthy in Ruby, so Line 223 still callsfetch_content_view_environmentsfor blank inputs and Line 228 can raise viahandle_errorsbeforehandle_blank_cvenv_paramsgets a chance to clear the association. That turns a "remove all CVEnvs" update into a 4xx instead of persisting an empty set.Proposed fix
def find_content_view_environments `@content_view_environments` = [] - if params_likely_not_from_angularjs? && (params[:content_view_environments] || params[:content_view_environment_ids]) + handle_blank_cvenv_params + if params_likely_not_from_angularjs? && + (params[:content_view_environments].present? || params[:content_view_environment_ids].present?) `@content_view_environments` = ::Katello::ContentViewEnvironment.fetch_content_view_environments( labels: params[:content_view_environments], ids: params[:content_view_environment_ids], organization: `@organization` || `@activation_key`&.organization) if `@content_view_environments.blank`? handle_errors(labels: params[:content_view_environments], ids: params[:content_view_environment_ids]) end end - handle_blank_cvenv_params `@organization` ||= `@content_view_environments.first`&.organization end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/katello/api/v2/activation_keys_controller.rb` around lines 223 - 233, The current conditional calls ::Katello::ContentViewEnvironment.fetch_content_view_environments when params like params[:content_view_environments] or params[:content_view_environment_ids] are present but blank (e.g. "" or []), causing handle_errors to raise before handle_blank_cvenv_params can clear associations; update the condition in the controller so it only calls fetch_content_view_environments when the params are present and not blank (e.g. use .present? or !.blank? on params[:content_view_environments] and params[:content_view_environment_ids]) and ensure `@content_view_environments` is only set from fetch_content_view_environments in that non-blank case so handle_blank_cvenv_params can run for explicit empty payloads without triggering handle_errors.test/controllers/api/v2/hosts_controller_test.rb (1)
77-122:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
Setting[:allow_multiple_content_views]after each test.Both tests mutate a global setting without reset, which can leak state into subsequent tests.
Proposed fix
def test_host_contents_environments_param - Setting[:allow_multiple_content_views] = true + original_allow_multiple = Setting[:allow_multiple_content_views] + Setting[:allow_multiple_content_views] = true ::Host::Managed.any_instance.stubs(:update_candlepin_associations) host = FactoryBot.create(:host, :with_content, :with_subscription, :with_operatingsystem, :content_view => `@content_view`, :lifecycle_environment => `@environment`, :organization => `@environment.organization`) @@ host.content_facet.reload assert_equal 2, host.content_facet.content_view_environment_ids.count refute_equal orig_cvenvs, host.content_facet.content_view_environment_ids assert_equal_arrays target_cvenvs_ids, host.content_facet.content_view_environments.ids +ensure + Setting[:allow_multiple_content_views] = original_allow_multiple end def test_host_contents_cvenv_ids_param - Setting[:allow_multiple_content_views] = true + original_allow_multiple = Setting[:allow_multiple_content_views] + Setting[:allow_multiple_content_views] = true ::Host::Managed.any_instance.stubs(:update_candlepin_associations) host = FactoryBot.create(:host, :with_content, :with_subscription, :with_operatingsystem, :content_view => `@content_view`, :lifecycle_environment => `@environment`) @@ assert_equal 2, host.content_facet.content_view_environment_ids.count refute_equal orig_cvenvs, host.content_facet.content_view_environment_ids assert_equal_arrays target_cvenvs_ids, host.content_facet.content_view_environments.ids +ensure + Setting[:allow_multiple_content_views] = original_allow_multiple end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/api/v2/hosts_controller_test.rb` around lines 77 - 122, These tests (test_host_contents_cvenv_labels_param and test_host_contents_cvenv_ids_param) mutate the global Setting[:allow_multiple_content_views] and do not restore it; capture the original value before setting it to true and restore it after the test (use a teardown/ensure block or wrap the test with save/restore) so subsequent tests are not affected, referencing the Setting access and the two test methods to locate where to save and reset the setting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/helpers/katello/content_options_helper.rb`:
- Around line 95-101: The method filter_cvenvs_by_content_source currently
returns the full cvenvs when available_env_ids is empty, which repopulates
CVEnvs the content source can't serve; change it so that when content_source is
not pulp_primary and available_env_ids.empty? it returns an empty array ([])
instead of cvenvs. Update the early-return logic in
filter_cvenvs_by_content_source to use available_env_ids.empty? to return [] and
keep the later behavior that re-adds current_cvenv elsewhere intact.
In `@app/helpers/katello/kickstart_repository_helper.rb`:
- Around line 59-67: The code rebuilds new_host.content_facet but only restores
content_view_environments for the single-CVEnv case, dropping multi-CVEnv
associations and causing kickstart_repos(new_host) to run without CVEnv
restrictions; fix by copying the host's existing content_view_environments onto
the new facet when host.content_facet.present? (i.e., set
new_host.content_facet.content_view_environments =
host.content_facet.content_view_environments) and keep the existing
single_content_view_environment? branch that resolves CVEnv via
::Katello::ContentViewEnvironment.find_by_cv_and_lce! so both single- and
multi-CVEnv hosts preserve their CVEnv restrictions.
In `@test/controllers/api/v2/content_views_controller_test.rb`:
- Around line 655-659: Replace the order-dependent lookup that uses
alternate_cv.versions.first.id with the CV+LCE lookup helper so alternate_cvenv
is resolved deterministically; specifically, change the find_by call to use the
dedicated helper (e.g.
Katello::ContentViewEnvironment.find_by_content_view_and_lifecycle_environment(alternate_cv,
alternate_env) or the project's equivalent CV+LCE helper) instead of passing
:content_view_version_id => alternate_cv.versions.first.id and :environment_id
=> alternate_env.id.
---
Outside diff comments:
In `@app/controllers/katello/api/v2/activation_keys_controller.rb`:
- Around line 223-233: The current conditional calls
::Katello::ContentViewEnvironment.fetch_content_view_environments when params
like params[:content_view_environments] or params[:content_view_environment_ids]
are present but blank (e.g. "" or []), causing handle_errors to raise before
handle_blank_cvenv_params can clear associations; update the condition in the
controller so it only calls fetch_content_view_environments when the params are
present and not blank (e.g. use .present? or !.blank? on
params[:content_view_environments] and params[:content_view_environment_ids])
and ensure `@content_view_environments` is only set from
fetch_content_view_environments in that non-blank case so
handle_blank_cvenv_params can run for explicit empty payloads without triggering
handle_errors.
In `@test/controllers/api/v2/hosts_controller_test.rb`:
- Around line 77-122: These tests (test_host_contents_cvenv_labels_param and
test_host_contents_cvenv_ids_param) mutate the global
Setting[:allow_multiple_content_views] and do not restore it; capture the
original value before setting it to true and restore it after the test (use a
teardown/ensure block or wrap the test with save/restore) so subsequent tests
are not affected, referencing the Setting access and the two test methods to
locate where to save and reset the setting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52faa657-395b-43e7-8b46-23a4d00b9d6c
📒 Files selected for processing (68)
app/assets/javascripts/katello/hosts/host_and_hostgroup_edit.jsapp/controllers/katello/api/rhsm/candlepin_proxies_controller.rbapp/controllers/katello/api/v2/activation_keys_controller.rbapp/controllers/katello/api/v2/content_view_versions_controller.rbapp/controllers/katello/api/v2/hosts_bulk_actions_controller.rbapp/controllers/katello/concerns/api/v2/hosts_controller_extensions.rbapp/controllers/katello/concerns/authorization/api/v2/content_views_controller.rbapp/helpers/katello/content_options_helper.rbapp/helpers/katello/hosts_and_hostgroups_helper.rbapp/helpers/katello/kickstart_repository_helper.rbapp/lib/actions/katello/activation_key/reassign.rbapp/lib/actions/katello/content_view/add_to_environment.rbapp/lib/actions/katello/content_view/incremental_updates.rbapp/lib/actions/katello/content_view/remove.rbapp/lib/actions/katello/content_view/remove_from_environment.rbapp/lib/actions/katello/environment/destroy.rbapp/lib/actions/katello/host/reassign.rbapp/lib/actions/katello/host/update_content_view.rbapp/lib/actions/katello/organization/destroy.rbapp/lib/actions/katello/organization/environment_contents_refresh.rbapp/lib/katello/util/cve_hgcf_migrator.rbapp/lib/katello/util/cveak_migrator.rbapp/lib/katello/util/cvecf_migrator.rbapp/lib/katello/validators/generated_content_view_validator.rbapp/lib/katello/validators/hostgroup_kickstart_repository_validator.rbapp/models/katello/activation_key.rbapp/models/katello/authorization/repository.rbapp/models/katello/concerns/content_facet_host_extensions.rbapp/models/katello/concerns/hostgroup_extensions.rbapp/models/katello/concerns/subscription_facet_host_extensions.rbapp/models/katello/content_view_environment.rbapp/models/katello/content_view_environment_activation_key.rbapp/models/katello/content_view_environment_content_facet.rbapp/models/katello/host/content_facet.rbapp/models/katello/host/info_provider.rbapp/models/katello/host/subscription_facet.rbapp/models/katello/repository.rbapp/services/katello/content_view_manager.rbapp/services/katello/product_content_finder.rbapp/services/katello/registration_manager.rbapp/views/overrides/activation_keys/_host_environment_select.html.erbdeveloper_docs/quick_reference.mdtest/actions/katello/content_view_test.rbtest/actions/katello/environment_test.rbtest/controllers/api/v2/activation_keys_controller_test.rbtest/controllers/api/v2/api_controller_test.rbtest/controllers/api/v2/content_views_controller_test.rbtest/controllers/api/v2/hostgroups_controller_test.rbtest/controllers/api/v2/hosts_controller_test.rbtest/controllers/foreman/hostgroups_controller_test.rbtest/factories/host_factory.rbtest/helpers/hosts_and_hostgroups_helper_test.rbtest/helpers/url_helpers_test.rbtest/lib/concerns/renderer_extensions_test.rbtest/lib/validators/hostgroup_kickstart_repository_validator_test.rbtest/mailers/errata_mailer_test.rbtest/models/activation_key_test.rbtest/models/authorization/activation_key_authorization_test.rbtest/models/concerns/content_facet_host_extensions_test.rbtest/models/concerns/host_managed_extensions_test.rbtest/models/concerns/hostgroup_extensions_test.rbtest/models/content_view_test.rbtest/models/erratum_test.rbtest/models/host/content_facet_test.rbtest/models/hostgroup/content_facet_test.rbtest/models/repository_test.rbtest/services/katello/registration_manager_test.rbtest/support/host_support.rb
✅ Files skipped from review due to trivial changes (10)
- app/lib/katello/validators/generated_content_view_validator.rb
- app/lib/actions/katello/content_view/incremental_updates.rb
- app/models/katello/authorization/repository.rb
- app/lib/katello/util/cveak_migrator.rb
- app/controllers/katello/api/rhsm/candlepin_proxies_controller.rb
- app/models/katello/repository.rb
- app/models/katello/host/info_provider.rb
- app/lib/katello/util/cvecf_migrator.rb
- app/services/katello/content_view_manager.rb
- app/controllers/katello/api/v2/hosts_bulk_actions_controller.rb
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
from
e57599e to
7e76485
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/katello/api/v2/activation_keys_controller.rb (1)
223-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlank CVEnv params should be normalized before lookup/error handling.
Line 223 can run the lookup path for blank values, and Line 228 may trigger
handle_errorsbefore the blank-clearing branch (Lines 246-255). That risks rejecting valid “clear content view environments” updates.Suggested fix
def find_content_view_environments `@content_view_environments` = [] + handle_blank_cvenv_params - if params_likely_not_from_angularjs? && (params[:content_view_environments] || params[:content_view_environment_ids]) + if params_likely_not_from_angularjs? && + ((params.key?(:content_view_environments) && params[:content_view_environments].present?) || + (params.key?(:content_view_environment_ids) && params[:content_view_environment_ids].present?)) `@content_view_environments` = ::Katello::ContentViewEnvironment.fetch_content_view_environments( labels: params[:content_view_environments], ids: params[:content_view_environment_ids], organization: `@organization` || `@activation_key`&.organization) if `@content_view_environments.blank`? handle_errors(labels: params[:content_view_environments], ids: params[:content_view_environment_ids]) end end - handle_blank_cvenv_params `@organization` ||= `@content_view_environments.first`&.organization endAlso applies to: 246-255
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/katello/api/v2/activation_keys_controller.rb` around lines 223 - 234, The lookup/error path runs before blank CVEnv params are normalized, causing valid "clear content view environments" updates to be rejected; move or invoke handle_blank_cvenv_params (or otherwise normalize/strip blank entries from params[:content_view_environments] and params[:content_view_environment_ids]) before the conditional that calls params_likely_not_from_angularjs? and ::Katello::ContentViewEnvironment.fetch_content_view_environments so that `@content_view_environments` lookup and handle_errors operate on cleaned params; ensure `@organization` assignment using `@content_view_environments.first` remains correct after normalization.
♻️ Duplicate comments (1)
app/models/katello/host/content_facet.rb (1)
89-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve requested CVEnv ID order and fail fast on unknown IDs.
Line 92 uses
ContentViewEnvironment.where(id: cvenv_ids), which silently drops missing IDs and can reorder assignments. That can initialize hosts with an unexpected CVEnv set/priority.Proposed fix
if cvenv_ids.present? - self.content_view_environments = ContentViewEnvironment.where(id: cvenv_ids) + ordered_ids = Array(cvenv_ids).map(&:to_s) + cvenvs_by_id = ContentViewEnvironment.where(id: ordered_ids).index_by { |cvenv| cvenv.id.to_s } + missing_ids = ordered_ids - cvenvs_by_id.keys + raise ActiveRecord::RecordNotFound, "Couldn't find Katello::ContentViewEnvironment IDs: #{missing_ids.join(', ')}" if missing_ids.any? + + self.content_view_environments = ordered_ids.map { |id| cvenvs_by_id.fetch(id) } end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/katello/host/content_facet.rb` around lines 89 - 93, The current assignment uses ContentViewEnvironment.where(id: cvenv_ids) which can drop unknown IDs and not preserve the requested order; replace this with a lookup that preserves the input order and raises on missing IDs (e.g. use ContentViewEnvironment.find(cvenv_ids) or equivalent) and assign the result to self.content_view_environments; keep the existing presence check for cvenv_ids (the symbols to edit are cvenv_ids, content_view_environments, and ContentViewEnvironment.where).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/controllers/katello/api/v2/activation_keys_controller.rb`:
- Around line 223-234: The lookup/error path runs before blank CVEnv params are
normalized, causing valid "clear content view environments" updates to be
rejected; move or invoke handle_blank_cvenv_params (or otherwise normalize/strip
blank entries from params[:content_view_environments] and
params[:content_view_environment_ids]) before the conditional that calls
params_likely_not_from_angularjs? and
::Katello::ContentViewEnvironment.fetch_content_view_environments so that
`@content_view_environments` lookup and handle_errors operate on cleaned params;
ensure `@organization` assignment using `@content_view_environments.first` remains
correct after normalization.
---
Duplicate comments:
In `@app/models/katello/host/content_facet.rb`:
- Around line 89-93: The current assignment uses
ContentViewEnvironment.where(id: cvenv_ids) which can drop unknown IDs and not
preserve the requested order; replace this with a lookup that preserves the
input order and raises on missing IDs (e.g. use
ContentViewEnvironment.find(cvenv_ids) or equivalent) and assign the result to
self.content_view_environments; keep the existing presence check for cvenv_ids
(the symbols to edit are cvenv_ids, content_view_environments, and
ContentViewEnvironment.where).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcdc3b34-d900-4b59-9cbe-5a7da34de077
📒 Files selected for processing (68)
app/assets/javascripts/katello/hosts/host_and_hostgroup_edit.jsapp/controllers/katello/api/rhsm/candlepin_proxies_controller.rbapp/controllers/katello/api/v2/activation_keys_controller.rbapp/controllers/katello/api/v2/content_view_versions_controller.rbapp/controllers/katello/api/v2/hosts_bulk_actions_controller.rbapp/controllers/katello/concerns/api/v2/hosts_controller_extensions.rbapp/controllers/katello/concerns/authorization/api/v2/content_views_controller.rbapp/helpers/katello/content_options_helper.rbapp/helpers/katello/hosts_and_hostgroups_helper.rbapp/helpers/katello/kickstart_repository_helper.rbapp/lib/actions/katello/activation_key/reassign.rbapp/lib/actions/katello/content_view/add_to_environment.rbapp/lib/actions/katello/content_view/incremental_updates.rbapp/lib/actions/katello/content_view/remove.rbapp/lib/actions/katello/content_view/remove_from_environment.rbapp/lib/actions/katello/environment/destroy.rbapp/lib/actions/katello/host/reassign.rbapp/lib/actions/katello/host/update_content_view.rbapp/lib/actions/katello/organization/destroy.rbapp/lib/actions/katello/organization/environment_contents_refresh.rbapp/lib/katello/util/cve_hgcf_migrator.rbapp/lib/katello/util/cveak_migrator.rbapp/lib/katello/util/cvecf_migrator.rbapp/lib/katello/validators/generated_content_view_validator.rbapp/lib/katello/validators/hostgroup_kickstart_repository_validator.rbapp/models/katello/activation_key.rbapp/models/katello/authorization/repository.rbapp/models/katello/concerns/content_facet_host_extensions.rbapp/models/katello/concerns/hostgroup_extensions.rbapp/models/katello/concerns/subscription_facet_host_extensions.rbapp/models/katello/content_view_environment.rbapp/models/katello/content_view_environment_activation_key.rbapp/models/katello/content_view_environment_content_facet.rbapp/models/katello/host/content_facet.rbapp/models/katello/host/info_provider.rbapp/models/katello/host/subscription_facet.rbapp/models/katello/repository.rbapp/services/katello/content_view_manager.rbapp/services/katello/product_content_finder.rbapp/services/katello/registration_manager.rbapp/views/overrides/activation_keys/_host_environment_select.html.erbdeveloper_docs/quick_reference.mdtest/actions/katello/content_view_test.rbtest/actions/katello/environment_test.rbtest/controllers/api/v2/activation_keys_controller_test.rbtest/controllers/api/v2/api_controller_test.rbtest/controllers/api/v2/content_views_controller_test.rbtest/controllers/api/v2/hostgroups_controller_test.rbtest/controllers/api/v2/hosts_controller_test.rbtest/controllers/foreman/hostgroups_controller_test.rbtest/factories/host_factory.rbtest/helpers/hosts_and_hostgroups_helper_test.rbtest/helpers/url_helpers_test.rbtest/lib/concerns/renderer_extensions_test.rbtest/lib/validators/hostgroup_kickstart_repository_validator_test.rbtest/mailers/errata_mailer_test.rbtest/models/activation_key_test.rbtest/models/authorization/activation_key_authorization_test.rbtest/models/concerns/content_facet_host_extensions_test.rbtest/models/concerns/host_managed_extensions_test.rbtest/models/concerns/hostgroup_extensions_test.rbtest/models/content_view_test.rbtest/models/erratum_test.rbtest/models/host/content_facet_test.rbtest/models/hostgroup/content_facet_test.rbtest/models/repository_test.rbtest/services/katello/registration_manager_test.rbtest/support/host_support.rb
✅ Files skipped from review due to trivial changes (13)
- app/controllers/katello/concerns/authorization/api/v2/content_views_controller.rb
- app/lib/actions/katello/content_view/add_to_environment.rb
- app/controllers/katello/api/v2/content_view_versions_controller.rb
- app/lib/actions/katello/content_view/remove_from_environment.rb
- test/models/repository_test.rb
- app/lib/actions/katello/content_view/incremental_updates.rb
- app/lib/actions/katello/environment/destroy.rb
- app/models/katello/repository.rb
- app/lib/katello/validators/generated_content_view_validator.rb
- app/services/katello/content_view_manager.rb
- app/controllers/katello/api/v2/hosts_bulk_actions_controller.rb
- app/lib/katello/util/cvecf_migrator.rb
- app/controllers/katello/api/rhsm/candlepin_proxies_controller.rb
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
from
7e76485 to
f017428
Compare
Member
Author
|
ok, I think rabbit is happy (for now...) |
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
2 times, most recently
from
d8a10c5 to
dc16926
Compare
Member
Author
|
@lfu @nofaralfasi Inheritance should be working better now on the create host form. Hey, "John" 😄 |
Contributor
|
Is it intentional that the |
Member
Author
|
@nofaralfasi Yes, that was an intentional decision from a few years ago due to the complexities of content source / CVenv coherence and now also MultiCV. You can use the "Change content source" link to change the existing host's CVEnvs. |
This was referenced May 27, 2026
jturel
approved these changes
May 27, 2026
Member
jturel
left a comment
jturel
left a comment
There was a problem hiding this comment.
Not seeing any issues in my testing - let's ship it (pending ack from other reviewers)
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
from
8d51409 to
6c72df3
Compare
Member
Author
|
@lfu Added display_name on ContentViewEnvironment and replaced both inline formatting sites in the helper. (I kept host_and_hostgroup_edit as-is since that's a legacy JS file.) |
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
from
6c72df3 to
f0a78a4
Compare
Member
Author
|
@lfu actually, changed it so it just uses |
Contributor
Thanks, I tried that flow, but on my side the Change content source page does not show the host’s currently assigned CVEnvs. |
lfu
reviewed
May 28, 2026
Member
Author
|
@nofaralfasi We do have a special case (via url params) for arriving at Change Content Source from host edit. It shows this banner when the selected content source is the same as the existing: 
Also, the buttons change from "Run job invocation / Update hosts manually" to simply "Save," because there's no client-side step necessary if content source didn't actually change. However, it's true that you do still start with a blank form. Other than that, the way to confirm a host's current CVEnvs is just to view host details. I have confirmed that none of this is a change from previous behavior. |
jeremylenz
force-pushed
the
sat-38100-remove-deprecated-cv-lce-params
branch
from
f0a78a4 to
9b3d6c4
Compare
Member
Author
|
@lfu updated 👍 |
Update content_facet_ignore_update? to check content_view_environment_ids instead of the removed content_view_id/lifecycle_environment_id params. Without this, Host.new with content_facet_attributes would silently reject the nested attributes and not build the content facet. Update all test files to use content_view_environment_ids (hosts) and content_view_environment_id (hostgroups) instead of the removed params. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
@jeremylenz I’m still seeing “Run job invocation / Update hosts manually” in this flow, not the single “Save” button shown in your screenshot: |
Member
Author
|
Member
Author
|
Notice that the link includes url params |
Contributor
I’m reaching that page via the Change content source link on the host edit page, and the selected content source is the same as the current one. This is the exact URL I get: Do you know why |
Member
Author
|
It would be empty if your host somehow has a nil content source. Check in console, or in host details Details tab > Registration details card. |
Member
Author
|
This would be the case if you're using the Foreman base host, or an unregistered host. |
Contributor
|
Thanks @jeremylenz! |
nofaralfasi
approved these changes
May 28, 2026
Contributor
nofaralfasi
left a comment
nofaralfasi
left a comment
There was a problem hiding this comment.
I tested all scenarios from the PR test plan, and they all pass on my side:
- Host create with hostgroup CVEnv inheritance/pre-population: passed
- Edit existing host (CVEnv preserved): passed
- Edit multi-CVEnv host (all CVEnvs preserved): passed
- Create/edit hostgroup with CVEnv: passed
- AK create via API with
content_view_environment_ids: passed - AK update via API with
content_view_environment_ids: passed - Removed legacy params (
content_view_id,lifecycle_environment_id) are no longer accepted/effective on host/AK/hostgroup APIs: passed
Member
Author
|
Thanks all for the reviews! |
Merged
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
Related PR in foreman-ansible-modules: theforeman/foreman-ansible-modules#1977
Remove the deprecated separate
content_view_idandlifecycle_environment_idAPI parameters from hosts, activation keys, and hostgroups, in favor ofcontent_view_environment_ids(hosts/AKs) andcontent_view_environment_id(hostgroups).Also removes
assign_single_environmentfrom both ContentFacet and ActivationKey, replacing it withContentViewEnvironment.find_by_cv_and_lce!lookups. Removes thecheck_cve_attributes/attributes=/updateoverrides on HostManagedExtensions that handled the deprecated params. Updates the host ERB form to use a content view environment dropdown instead of separate LCE/CV cascading selects. Updatescontent_facet_ignore_update?to gate oncontent_view_environment_idsinstead of the removed params.Considerations taken when implementing this change?
The ERB host form,
ContentFacet.inherited_attributes, andContentFacet#initializeall used the deprecated params together. The hidden fields in the ERB form blocked||=ininherited_attributesfrom injecting hostgroup values. All three had to move tocontent_view_environment_idstogether to avoid breaking host create/edit inheritance.What are the testing steps for this pull request?
content_view_environment_ids:content_view_environment_ids:content_view_id,lifecycle_environment_id) are no longer accepted on host/AK/hostgroup create/update APIsSummary by CodeRabbit
New Features
Bug Fixes
Breaking Changes