Fix ensure unfinishable Upload job is marked done

[m-bucher]

Fixes #1227

What are the changes introduced in this pull request?

Add code to mark aborted Jobs as done (broke with #1127).

Considerations taken when implementing this change?

What are the testing steps for this pull request?

Created unit-tests (might need some refinement).

Have a system not registered to Insights (no consumer certs) and start Upload-Job.
Expected Result: Upload aborted, but Task finishes.

[sourcery-ai]

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

• In try_execute, instead of sprinkling done! before each early return, consider centralizing completion in an ensure or helper so future branches can’t easily forget to mark the job as done.
• In the mark as done when upload aborted due to missing certificate test, the comment mentions verifying the file is not moved but the assertions only check action.done?; either assert on file existence or update the comment to match the actual behavior.
• Both new tests repeat the same pattern for creating and cleaning up temporary upload files; extracting a small helper (e.g. create_test_upload_file(name)) would reduce duplication and make the intent clearer.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `try_execute`, instead of sprinkling `done!` before each early return, consider centralizing completion in an `ensure` or helper so future branches can’t easily forget to mark the job as done.
- In the `mark as done when upload aborted due to missing certificate` test, the comment mentions verifying the file is not moved but the assertions only check `action.done?`; either assert on file existence or update the comment to match the actual behavior.
- Both new tests repeat the same pattern for creating and cleaning up temporary upload files; extracting a small helper (e.g. `create_test_upload_file(name)`) would reduce duplication and make the intent clearer.

## Individual Comments

### Comment 1
<location path="test/jobs/upload_report_direct_job_test.rb" line_range="405-414" />
<code_context>
     end
   end
+
+  test 'mark as done when upload aborted due to missing certificate' do
+    # Remove certificate from organization
+    Organization.any_instance.stubs(:owner_details).returns({})
+
+    # Create a real test file to verify it's not moved
+    FileUtils.mkdir_p(@uploads_folder)
+    test_file = File.join(@uploads_folder, 'test_file_for_cleanup.tar.xz')
+    FileUtils.mkdir_p(@uploads_folder)
+    FileUtils.touch(test_file)
+
+    begin
+      action = create_action(ForemanInventoryUpload::Async::UploadReportDirectJob)
+      action.expects(:action_subject).with(@organization)
+      plan_action(action, test_file, @organization.id)
+
+      # Execute the action
+      action.send(:try_execute)
+
+      # Verify file still exists (not moved or deleted)
+      assert action.done?
+    ensure
</code_context>
<issue_to_address>
**issue (testing):** Test name and comment suggest verifying file is not moved, but the test only checks `action.done?` and never asserts anything about the file path.

This test currently only checks `action.done?`, despite the comment stating it verifies the file is not moved. To align the test with its intent and prevent regressions, please also assert that:
- the file still exists at the original path (e.g. `assert File.exist?(test_file)`), and
- the upload directory contents remain unchanged.
If the implementation guarantees additional behavior (such as no temp/target files being created), consider asserting that as well.
</issue_to_address>

### Comment 2
<location path="test/jobs/upload_report_direct_job_test.rb" line_range="430-439" />
<code_context>
+    end
+  end
+
+  test 'file cleanup when upload aborted due to disconnected mode' do
+    Setting.stubs(:[]).with(:subscription_connection_enabled).returns(false)
+
+    # Create a real test file
+    FileUtils.mkdir_p(@uploads_folder)
+    test_file = File.join(@uploads_folder, 'test_file_disconnected.tar.xz')
+    FileUtils.mkdir_p(@uploads_folder)
+    FileUtils.touch(test_file)
+
+    begin
+      action = create_action(ForemanInventoryUpload::Async::UploadReportDirectJob)
+      action.expects(:action_subject).with(@organization)
+      plan_action(action, test_file, @organization.id)
+
+      # Execute the action
+      action.send(:try_execute)
+
+      # Verify file still exists
+      assert File.exist?(test_file), "File should remain when connection is disabled"
+      assert action.done?
+    ensure
</code_context>
<issue_to_address>
**issue:** Test name suggests file cleanup, but the assertions expect the file to remain; this inconsistency makes the test misleading.

The test name implies cleanup, but the assertions and message expect the file to remain (`File.exist?(test_file)`, "File should remain when connection is disabled"). Please update the test name, comments, and expectations to clearly reflect the intended behavior in disconnected mode (cleanup vs retention) so the job’s purpose isn’t misread.
</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.}

[m-bucher]

@jeremylenz ready-for-review

[sourcery-ai]

Hey - I've left some high level feedback:

• The try_execute method now has duplicated early-return branches that both log and call done!; consider extracting a small helper (e.g., abort_upload(reason_message)) to centralize the logging and completion behavior so future abort paths stay consistent and easier to maintain.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `try_execute` method now has duplicated early-return branches that both log and call `done!`; consider extracting a small helper (e.g., `abort_upload(reason_message)`) to centralize the logging and completion behavior so future abort paths stay consistent and easier to maintain.

---

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.}

[jeremylenz]

Tested and works as expected.

In the log I see

[I|bac|25a4bddc] Skipping organization 'org2', no candlepin certificate defined.

and the task completes with success. Tested with both IoP and non-IoP setups, each with 2 orgs (1 manifest and 1 non-manifest org).

Thanks @m-bucher!