Fixes #38460 - Disable CCV auto-publish on child CV minor version publish.

[qcjames53]

What are the changes introduced in this pull request?

When publishing a minor version of a content view (in the testing steps achieved through an incremental update applying errata), parent composite content views no longer respect the --propagate-all-composites flag. This avoids a duplicate publish where a content view publishes a minor version, updates the parent composite content view, then propagates itself to the composite again.

Considerations taken when implementing this change?

The unit tests were made with a ton of AI help. Please double-check everything is reasonable.

What are the testing steps for this pull request?

1. Create and sync a product with missing errata (consider https://jlsherrill.fedorapeople.org/fake-repos/needed-errata/)
2. Add this product to a content view and publish it (CV Version 1.0).
3. Note the content view version id and an id of an applicable errata (obtain both from URL or other method).
4. Add the new content view to a composite content view. Set the CV to 'Always update to latest version'. Set the CCV to auto-publish.
5. Manually publish the composite content view (CCV Version 1.0).
6. Run an incremental update on the CV with the --propagate-all-composites flag set:

hammer content-view version incremental-update --content-view-version-id <cvv-id> --errata-ids <erratum-id> --propagate-all-composites true

7. If this bug is fixed, there should be a new content view version (1.1) and a new composite content view version (1.1). There should be no composite content view version 2.0.
8. Ensure auto-publish is not broken by manually publishing a new major version of the content view (2.0) and make sure the composite content view also publishes a new version (2.0) on it's own.
9. Give the unit tests a deep review.

Summary by Sourcery

Prevent duplicate auto-publishing of composite content views when performing incremental updates on child content view versions with propagate enabled.

Bug Fixes:

• Ensure composite content views already updated via propagate are excluded from subsequent auto-publish triggers during incremental updates.

Tests:

• Add unit tests for incremental update auto-publish behavior covering propagated composites, non-propagated composites, backward compatibility, composite views, and non-latest versions.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• When building propagated_composite_cv_ids in ContentView::IncrementalUpdates#plan, consider calling .uniq (and possibly .compact) on the mapped IDs to avoid passing duplicates or nils through to the auto-publish exclusion logic.
• The new tests rely on stubbing ContentViewVersion.find(nil) and other internals of the action; using a concrete old_version ID instead of nil would make the tests less coupled to the implementation and more resilient to future refactors.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- When building `propagated_composite_cv_ids` in `ContentView::IncrementalUpdates#plan`, consider calling `.uniq` (and possibly `.compact`) on the mapped IDs to avoid passing duplicates or nils through to the auto-publish exclusion logic.
- The new tests rely on stubbing `ContentViewVersion.find(nil)` and other internals of the action; using a concrete `old_version` ID instead of `nil` would make the tests less coupled to the implementation and more resilient to future refactors.

## Individual Comments

### Comment 1
<location> `test/actions/katello/content_view_version/incremental_update_test.rb:20` </location>
<code_context>
+      let(:component_cv) { katello_content_views(:library_dev_view) }
+      let(:component_version) { component_cv.versions.first }
+      let(:composite_cv) { katello_content_views(:composite_view) }
+      let(:mock_output) { {} }
+
+      before do
</code_context>

<issue_to_address>
**suggestion (testing):** Consider asserting `auto_publish_content_view_version_id` in addition to `auto_publish_content_view_ids`

Right now the tests only verify the filtering of `auto_publish_content_view_ids`, but not the behavior of `auto_publish_content_view_version_id`, which is also updated in the implementation.

In addition to the existing assertions, please:
- In a positive case (e.g., `includes composites in auto-publish list when not propagated`), assert `mock_output[:auto_publish_content_view_version_id] == component_version.id`.
- In negative cases (composite CV, non-latest version), assert that `mock_output[:auto_publish_content_view_version_id]` is `nil` or absent, matching the expectations for `auto_publish_content_view_ids`.

This will ensure the tests fully cover the auto-publish output contract.

Suggested implementation:

```ruby
    it 'includes composites in auto-publish list when not propagated' do
      # existing setup / execution
      perform_action action, input

      assert_equal [composite_cv.id], mock_output[:auto_publish_content_view_ids]
      assert_equal component_version.id, mock_output[:auto_publish_content_view_version_id]
    end

```

```ruby
    it 'does not include composite cv in auto-publish list when composite cv is the target' do
      # existing setup / execution
      perform_action action, input

      assert_empty mock_output[:auto_publish_content_view_ids]
      assert_nil mock_output[:auto_publish_content_view_version_id]
    end

```

```ruby
    it 'does not include composite in auto-publish list when component version is not latest' do
      # existing setup / execution
      perform_action action, input

      assert_empty mock_output[:auto_publish_content_view_ids]
      assert_nil mock_output[:auto_publish_content_view_version_id]
    end

```

I assumed the test names and assertion shapes based on your description. To wire this into your actual file:

1. Locate the example that currently asserts `mock_output[:auto_publish_content_view_ids]` and whose description matches “includes composites in auto-publish list when not propagated”; add `assert_equal component_version.id, mock_output[:auto_publish_content_view_version_id]` right after the existing assertion.
2. For each “negative” example where you assert an empty/absent `auto_publish_content_view_ids` (e.g. composite CV as target, non-latest version cases), add `assert_nil mock_output[:auto_publish_content_view_version_id]` (or the equivalent expectation style you are using) next to the existing `mock_output[:auto_publish_content_view_ids]` assertion.
3. If your test suite uses `test "..." do` instead of `it '...' do`, or uses `refute`/`wont_be_nil` instead of `assert_nil`, adapt the snippets accordingly while keeping the semantics: positive case → equals `component_version.id`, negative cases → `nil`.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[qcjames53]

@sourcery-ai review

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• In IncrementalUpdateTest, the stub ContentViewVersion.stubs(:find).with(nil) coupled with not setting old_version in the input makes the test rely on an implicit nil argument; consider explicitly setting old_version (or avoiding the find(nil) path) so the tests are less brittle against future changes in the action inputs.
• The test case handles partial propagation with multiple composites fakes a second composite ID via composite_cv.id + 100; it would be more robust to create a real second composite content view (with a corresponding version) so the test reflects realistic data and avoids relying on ID arithmetic.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `IncrementalUpdateTest`, the stub `ContentViewVersion.stubs(:find).with(nil)` coupled with not setting `old_version` in the input makes the test rely on an implicit `nil` argument; consider explicitly setting `old_version` (or avoiding the `find(nil)` path) so the tests are less brittle against future changes in the action inputs.
- The test case `handles partial propagation with multiple composites` fakes a second composite ID via `composite_cv.id + 100`; it would be more robust to create a real second composite content view (with a corresponding version) so the test reflects realistic data and avoids relying on ID arithmetic.

## Individual Comments

### Comment 1
<location> `test/actions/katello/content_view_version/incremental_update_test.rb:9-10` </location>
<code_context>
+    include Support::Actions::Fixtures
+    include FactoryBot::Syntax::Methods
+
+    before :all do
+      User.current = users(:admin)
+    end
+
</code_context>

<issue_to_address>
**suggestion (testing):** Prefer per-test setup to avoid leaking User.current across test cases

Using `before :all` here shares `User.current` across examples and can leak state as tests are added or cleanup changes. Please use per-example setup (e.g., `before` or `setup`) so each test runs with its own explicitly set `User.current` and doesn’t depend on cross-test state.
</issue_to_address>

### Comment 2
<location> `test/actions/katello/content_view_version/incremental_update_test.rb:41-43` </location>
<code_context>
+        # Stub repositories to avoid the content comparison logic in run
+        component_version.stubs(:repositories).returns([])
+
+        # Stub find for old_version (will be nil in most tests, stub to return a version with empty repos)
+        old_version_stub = stub('old_version', repositories: [])
+        ::Katello::ContentViewVersion.stubs(:find).with(nil).returns(old_version_stub)
+      end
+
</code_context>

<issue_to_address>
**nitpick:** Remove or narrow the stub for ContentViewVersion.find(nil) if it’s no longer needed

This stub is described as for `old_version`, but `run` only calls `ContentViewVersion.find` with `input[:new_content_view_version_id]`, and each example stubs that explicitly. The `find(nil)` stub may never be used. If it isn’t, please remove it; if it is, add a brief comment or assertion indicating which code path requires `find(nil)` so it’s clear to future readers.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[pavanshekar]

Thanks @qcjames53!

LGTM! Verified fix works correctly and all tests pass 👍

bash-5.1$ hammer content-view version incremental-update --content-view-version-id 108 --errata-ids 43357 --propagate-all-composites true
[......................................................................................................................................] [100%]
Content View: ChildCV version 1.1
Added Content:

Content View: ParentCCV version 1.1
Added Content:

bash-5.1$ hammer content-view version list --content-view "ParentCCV" --organization "Default Organization"
----|---------------|---------|-------------|-----------------------
ID  | NAME          | VERSION | DESCRIPTION | LIFECYCLE ENVIRONMENTS
----|---------------|---------|-------------|-----------------------
111 | ParentCCV 1.1 | 1.1     |             | Library               
109 | ParentCCV 1.0 | 1.0     |             |                       
----|---------------|---------|-------------|-----------------------

[pavanshekar]

The CI test failure in Actions::Katello::ContentView::IncrementalUpdatesTest#test_0001_plans is because the existing test needs to expect the new parameter.

Please update the test expectations in test/actions/katello/content_view_test.rb:

Line 953-954 - Add :propagated_composite_cv_ids => []:

      assert_action_planned_with(action, ::Actions::Katello::ContentViewVersion::IncrementalUpdate, content_view.version(library), [library],
                              :content => {:errata_ids => ["FOO"]}, :resolve_dependencies => true, :description => "BadDescription", :propagated_composite_cv_ids => [])

Line 968-969 - Add :propagated_composite_cv_ids => [composite_version.content_view_id]:

      assert_action_planned_with(action, ::Actions::Katello::ContentViewVersion::IncrementalUpdate, component, [],
                                :content => {:errata_ids => ["FOO"]}, :resolve_dependencies => true, :description => "BadDescription", :propagated_composite_cv_ids => [composite_version.content_view_id])

[qcjames53]

I've fixed the failing tests. Thanks.

[jturel]

I think this change may require an adjustment to BATS: https://ci.theforeman.org/job/katello-nightly-rpm-pipeline/2685/tapResults/

Caught it since I've been keeping an eye on the results after redoing auto publish

[qcjames53]

Thanks for the heads up, Jonathon. Zach just took care of this BATS issue in this PR: theforeman/forklift#1932