Redesign Inventory Upload UI with improved UX #1126
Draft
+1,719
−2,479
Conversation
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
Eliminates shell script dependency by implementing direct HTTP upload using RestClient. This provides better error handling, follows Dynflow patterns more closely, and removes the need for external shell scripts. Created: - UploadReportDirectJob - Pure Ruby HTTP upload implementation - Comprehensive test suite with 14 tests covering error scenarios Modified: - QueueForUploadJob - Switch to new upload job - Controllers and tests - Reference new job class Deleted: - UploadReportJob - Old shell-based implementation - uploader.sh.erb - Bash upload script - Related tests for old implementation All 359 tests pass. This is part 1 of 2-part inventory upload redesign. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Use CloudRequest concern instead of direct RestClient calls - Include ::ForemanRhCloud::CloudRequest module - Replace RestClient::Request.execute with execute_cloud_request - Automatically handles proxy configuration, SSL verification, and error logging - Create FileUpload wrapper class to avoid monkey-patching File - Provides content_type attribute for RestClient multipart uploads - Delegates file methods (read, path, etc.) to underlying File object - Uses respond_to_missing? and method_missing for proper delegation - Add thread-safety documentation - Note that current implementation assumes single upload per organization - Matches existing pattern in GenerateReportJob - Full fix requires UI changes (tracked for PR #2) - Fix certificate check to use dig instead of chained fetch - Prevents KeyError when owner_details is empty hash - Safer handling of missing certificate data - Add comprehensive error handling tests - Test RestClient::InternalServerError handling - Test RestClient::Exceptions::Timeout handling - Verify progress output shows error messages - Add proxy configuration test - Verify execute_cloud_request is called (brings in proxy support) - Add file cleanup tests - Test file remains when upload aborted due to missing certificate - Test file remains when connection is disabled - Verify progress output and exit status for aborted uploads - Add FileUpload wrapper tests - Test delegation to underlying file object - Test content_type attribute for RestClient compatibility All 21 tests passing (0 failures, 0 errors)
Backend Changes: - Remove ProgressOutput from UploadReportDirectJob - Simplified plan() and try_execute() methods - Removed progress_output, instance_label, clear_task_output methods - Now a pure Dynflow action relying on standard task tracking - Create Tasks API controller - New endpoints: GET /api/rh_cloud/inventory_upload/tasks/current - New endpoints: GET /api/rh_cloud/inventory_upload/tasks/history - Queries ForemanTasks::Task with .for_action_types() and .with_duration - Update AccountsController to query ForemanTasks::Task - Replaced ProgressOutput.get() with direct task queries - Returns both old status strings (backward compat) and new task objects - New fields: generate_task, upload_task with full task details Frontend Changes: - Create TaskProgress component (Patternfly 4) - Uses Progress, Card, Badge from @patternfly/react-core - Shows task state, progress bar, timestamps, duration - Links to foreman-tasks detail page (/foreman_tasks/tasks/:id) - EmptyState for when no tasks exist - Create TaskHistory component - DataList showing recent tasks - Result icons (success/error/warning) - Relative timestamps and duration - Links to task details - Update Dashboard component - Replace ReportGenerate/ReportUpload with TaskProgress - Remove log polling (no longer needed) - Simplified lifecycle methods - Uses account.generate_task and account.upload_task Note: Redux actions and obsolete component removal pending. This commit is ready for manual testing.
- Convert Dashboard from Redux to native React state with useState - Remove Dashboard Redux actions, reducers, constants, and selectors - Remove obsolete components: * Terminal - replaced by TaskProgress * TabBody - no longer needed * ReportGenerate - no longer needed * ReportUpload - no longer needed * FullScreenModal - no longer needed with TaskProgress - Simplify NavContainer to remove FullScreenModal dependency - Update ForemanInventoryUploadReducers to not import dashboard reducers This simplifies the codebase and removes legacy terminal-based UI components. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Eliminates the shell-based report generation wrapper that called rake tasks, replacing it with direct Ruby invocations for better performance and simplicity. Backend changes: - Replace GenerateReportJob (shell wrapper) with HostInventoryReportJob in controllers - Delete GenerateReportJob and ShellProcess base class entirely - Update AccountsController to query HostInventoryReportJob tasks - Update API TasksController current/history endpoints - Remove obsolete ReportsController#last and UploadsController#last endpoints - Remove obsolete /reports/last and /uploads/last routes Frontend changes: - Add trigger buttons to TaskProgress component for "Generate and Upload" and "Generate Report" - Remove unused activeTab state from Dashboard (tabs don't need state) - Add organizationId and taskType props to TaskProgress - Delete obsolete Dashboard test files (DashboardActions.test.js, DashboardSelectors.test.js) - Fix linting issues in TaskHistory and other components Flow before: Controller → GenerateReportJob → shell rake → HostInventoryReportJob Flow after: Controller → HostInventoryReportJob (direct!) This completes the UI redesign by removing all terminal/log-based UI components and replacing them with modern Patternfly 4 task progress displays with direct Ruby-based report generation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Reviewer's GuideThis PR overhauls the Inventory Upload workflow by replacing the old tabbed Dashboard and Redux polling with new React TaskProgress and TaskHistory components (using hooks and PatternFly 4), consolidating report generation and upload into a single Dynflow HostInventoryReportJob, introducing a direct Ruby HTTP uploader (UploadReportDirectJob) with certificate handling, updating controllers and routes to serve task-centric JSON via new API endpoints, and cleaning up obsolete scripts and Redux code. Sequence diagram for unified report generation and upload workflowsequenceDiagram
actor User
participant UI as "React UI (TaskProgress)"
participant API as "Rails API Controller"
participant Job as "HostInventoryReportJob (Dynflow)"
participant Upload as "UploadReportDirectJob"
participant DB as "Database"
User->>UI: Clicks "Generate and upload report"
UI->>API: POST /foreman_inventory_upload/:org_id/reports
API->>Job: Start HostInventoryReportJob (with upload=true)
Job->>DB: Link task to Organization
Job->>Job: GenerateHostReport
Job->>Upload: UploadReportDirectJob (HTTP upload)
Upload->>DB: Store report file path
Job->>API: Return task status
API->>UI: Return task JSON
UI->>User: Show TaskProgress (real-time updates)
User->>UI: Clicks "Download report"
UI->>API: GET /foreman_inventory_upload/api/reports/:org_id
API->>User: Download report file
ER diagram for updated task-to-organization linkingerDiagram
ORGANIZATION ||--o{ TASK : "has"
TASK {
id string
label string
action string
state string
result string
progress number
started_at datetime
ended_at datetime
duration float
report_file_path string
}
ORGANIZATION {
id integer
label string
}
Class diagram for new and refactored Dynflow jobsclassDiagram
class HostInventoryReportJob {
+plan(base_folder, organization_id, hosts_filter, upload)
+organization_id
+report_file_path
+rescue_strategy_for_self
-organization
}
class UploadReportDirectJob {
+plan(filename, organization_id)
+try_execute
+upload_file(cer_path)
+move_to_done_folder
+certificate
+filename
+organization
+content_disconnected?
+logger
+rescue_strategy_for_self
}
class FileUpload {
+file
+content_type
+read()
+path
}
HostInventoryReportJob --> UploadReportDirectJob : uses
UploadReportDirectJob *-- FileUpload
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Simplifies the Dashboard by removing tab navigation and combining generate/upload into a single unified workflow. Improves TaskProgress component with optimistic updates, better progress indicators, and clearer status messages. Adds report file path display with download capability and fixes task-to-organization linking via action_subject. Changes: - Remove separate Upload tab (upload now part of generate task) - Update accordion status text (Generated/Uploaded vs Generating/Uploading) - Add report file path display and download button - Expand first organization accordion by default - Fix duration display to show actual task seconds - Improve UI text with sentence case and clearer messaging - Add optimistic UI updates for better responsiveness 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
jeremylenz
force-pushed
the
redesign/inventory-upload-ui
branch
from
d747765 to
04948bb
Compare
This change ensures that inventory uploads work correctly in all combinations of IoP mode and subscription connection settings: Backend changes: - Modified upload_report_direct_job.rb to allow uploads when in IoP mode, regardless of subscription_connection_enabled setting - Added test to verify uploads proceed in IoP mode even when setting is off Frontend changes: - Updated TaskProgress component to disable "Generate and upload report" button when IoP is OFF and subscription_connection_enabled is OFF - Added helpful tooltip explaining why upload is disabled and how to enable - Maintained TabHeader component compatibility (legacy, may be removed) Expected behavior: - IoP OFF + Setting ON: Upload to hosted (unchanged) - IoP ON + Setting ON: Upload to local HBI (unchanged) - IoP OFF + Setting OFF: Button disabled with tooltip (fixed) - IoP ON + Setting OFF: Upload to local HBI (fixed) Single-host reports continue to upload to local HBI in IoP mode as before. 🤖 Generated with Claude Code (https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…emoval The rh_cloud_inventory:report:generate_upload task was still calling the deleted GenerateReportJob. Updated to use HostInventoryReportJob directly, matching the pattern used in the controller and the generate task. Parameters: - base_folder: ForemanInventoryUpload.generated_reports_folder - organization_id: from organization - hosts_filter: empty string (all hosts) - upload: true (upload to cloud/HBI) 🤖 Generated with Claude Code (https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This commit enhances the inventory upload task progress UI with several improvements for better user experience: - Reduce polling interval from 5s to 2s for faster updates - Trigger immediate poll after starting a task for instant feedback - Always show Duration and Report saved to fields (even when blank) for consistent layout - Keep action buttons visible at all times, using disabled state instead of hiding them to reduce visual jarring - Fix flashing states during task transitions with optimistic UI that persists until new task data arrives - Convert AccountList from class component to modern function component with hooks - Pass fetchAccountsStatus callback through component tree to enable immediate polling Technical changes: - Added useEffect to clear lastTaskId when task data updates - Created areButtonsDisabled logic to prevent button state flashing - Added RelativeDateTime mock for tests - Updated snapshots to reflect new component structure 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Collaborator
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/foreman_inventory_upload/async/upload_report_direct_job.rb:105-110` </location>
<code_context>
+ def move_to_done_folder
+ FileUtils.mkdir_p(ForemanInventoryUpload.done_folder)
+ done_file = ForemanInventoryUpload.done_file_path(File.basename(filename))
+ FileUtils.mv(filename, done_file)
+ logger.debug("Moved #{filename} to #{done_file}")
+ end
</code_context>
<issue_to_address>
**suggestion (bug_risk):** FileUtils.mv may overwrite existing files without warning.
Consider checking if the destination file exists before moving, or set the :force option to control overwriting behavior.
```suggestion
def move_to_done_folder
FileUtils.mkdir_p(ForemanInventoryUpload.done_folder)
done_file = ForemanInventoryUpload.done_file_path(File.basename(filename))
if File.exist?(done_file)
logger.warn("Destination file #{done_file} already exists. Skipping move to avoid overwrite.")
else
FileUtils.mv(filename, done_file)
logger.debug("Moved #{filename} to #{done_file}")
end
end
```
</issue_to_address>
### Comment 2
<location> `webpack/ForemanInventoryUpload/Components/TaskHistory/TaskHistory.js:51-59` </location>
<code_context>
+ return null;
+ };
+
+ const formatDuration = seconds => {
+ if (!seconds) return __('N/A');
+ const mins = Math.floor(seconds / 60);
</code_context>
<issue_to_address>
**suggestion:** Duration formatting does not handle negative or non-numeric values.
Validate that 'seconds' is a non-negative number before formatting; otherwise, return 'N/A' or a suitable fallback.
```suggestion
const formatDuration = seconds => {
if (
typeof seconds !== 'number' ||
isNaN(seconds) ||
seconds < 0
) {
return __('N/A');
}
const mins = Math.floor(seconds / 60);
const secs = Math.floor(seconds % 60);
if (mins > 0) {
return `${mins}m ${secs}s`;
}
return `${secs}s`;
};
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new TaskProgress component is very large and handles multiple concerns (empty states, controls, progress display) — consider splitting it into smaller subcomponents (e.g. header, progress bar, actions) to improve readability and maintainability.
- AccountsController’s sub_action_status, task_status_string, and task_json methods embed substantial business logic in the controller — extracting that into a dedicated service or decorator would keep the controller focused on orchestration and make testing easier.
- Both the accounts#index and the new API endpoints join foreman_tasks and links for each organization/task — consider eager loading or caching task data to avoid potential N+1 queries when listing many organizations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new TaskProgress component is very large and handles multiple concerns (empty states, controls, progress display) — consider splitting it into smaller subcomponents (e.g. header, progress bar, actions) to improve readability and maintainability.
- AccountsController’s sub_action_status, task_status_string, and task_json methods embed substantial business logic in the controller — extracting that into a dedicated service or decorator would keep the controller focused on orchestration and make testing easier.
- Both the accounts#index and the new API endpoints join foreman_tasks and links for each organization/task — consider eager loading or caching task data to avoid potential N+1 queries when listing many organizations.
## Individual Comments
### Comment 1
<location> `webpack/ForemanInventoryUpload/Components/AccountList/AccountList.js:42` </location>
<code_context>
+ return <EmptyState />;
+ }
- const items = Object.keys(filteredAccount).map((label, index) => {
- const account = accounts[label];
- return <ListItem key={index} label={label} account={account} />;
</code_context>
<issue_to_address>
**suggestion:** Using array index as key in list rendering may cause issues if list order changes.
Consider using a unique identifier like account label or id as the React key instead of the array index to avoid rendering issues when the list order changes.
Suggested implementation:
```javascript
const items = Object.keys(filteredAccount).map((label) => {
const account = accounts[label];
return (
```
```javascript
return <ListItem key={label} label={label} account={account} />;
```
</issue_to_address>
### Comment 2
<location> `webpack/ForemanInventoryUpload/Components/TaskProgress/TaskProgress.js:93` </location>
<code_context>
+ ? __('Report generation started')
+ : __('Report generation and upload started');
+
+ window.tfm.toastNotifications.notify({
+ message,
+ type: 'info',
</code_context>
<issue_to_address>
**issue (bug_risk):** Assumes window.tfm.toastNotifications is always available; may cause runtime errors if not.
Consider adding a check to ensure window.tfm.toastNotifications exists before calling notify, to avoid potential runtime errors.
</issue_to_address>
### Comment 3
<location> `webpack/ForemanInventoryUpload/Components/TaskProgress/TaskProgress.js:190-198` </location>
<code_context>
+ return null;
+ };
+
+ const formatDuration = seconds => {
+ if (!seconds) return __('N/A');
+ const mins = Math.floor(seconds / 60);
</code_context>
<issue_to_address>
**suggestion:** Duration formatting does not handle null or undefined values robustly.
Explicitly check for null or undefined values instead of using a general falsy check, to avoid treating zero as invalid. Return 'N/A' only for null or undefined.
```suggestion
const formatDuration = seconds => {
if (seconds === null || seconds === undefined) return __('N/A');
const mins = Math.floor(seconds / 60);
const secs = Math.floor(seconds % 60);
if (mins > 0) {
return `${mins}m ${secs}s`;
}
return `${secs}s`;
};
```
</issue_to_address>
### Comment 4
<location> `lib/foreman_inventory_upload/async/upload_report_direct_job.rb:80` </location>
<code_context>
+ def upload_file(cer_path)
+ cert_content = File.read(cer_path)
+
+ File.open(filename, 'rb') do |file|
+ # Wrap file with FileUpload class for RestClient multipart handling
+ # RestClient requires objects with :read, :path, and :content_type methods
</code_context>
<issue_to_address>
**issue (bug_risk):** File is moved to done folder after upload; consider error handling for failed uploads.
Currently, files are moved to the done folder even if the upload fails, which could misclassify failed uploads. Please ensure files are only moved after a successful upload, or implement error handling to address this.
</issue_to_address>
### Comment 5
<location> `lib/foreman_inventory_upload/async/upload_report_direct_job.rb:124` </location>
<code_context>
+ }
+ end
+
+ def foreman_certificate
+ {
+ cert: File.read(Setting[:ssl_certificate]),
</code_context>
<issue_to_address>
**issue:** No error handling for missing SSL certificate or key files.
Add error handling to manage cases where the certificate or key files are missing or unreadable, so users receive a clear message or fallback behavior.
</issue_to_address>
### Comment 6
<location> `test/jobs/upload_report_direct_job_test.rb:96-35` </location>
<code_context>
+ assert_equal 'FOREMAN KEY', cert[:key]
+ end
+
+ test 'certificate method returns manifest cert in regular mode' do
+ ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(false)
+
+ action = create_and_plan_action(
+ ForemanInventoryUpload::Async::UploadReportDirectJob,
+ @filename,
+ @organization.id
+ )
+
+ cert = action.send(:certificate)
+ assert_equal 'FAKE CERTIFICATE', cert[:cert]
+ assert_equal 'FAKE KEY', cert[:key]
+ end
+
+ test 'certificate method returns foreman cert in IoP mode' do
</code_context>
<issue_to_address>
**suggestion (testing):** Test for certificate selection logic.
Add a test case for when no certificates are available to ensure the job handles this scenario correctly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Address individual code review comments: 1. **AccountList.js**: Use unique label as React key instead of array index to prevent rendering issues when list order changes 2. **TaskProgress.js & TaskHistory.js**: Improve formatDuration validation to explicitly check for null/undefined instead of falsy values, preventing edge cases with zero values 3. **upload_report_direct_job.rb**: Add error handling for missing SSL certificate/key files with clear error messages 4. **upload_report_direct_job_test.rb**: Add test cases for missing certificate scenarios and add File.readable stubs to existing IoP test Note: File move behavior (Comment 4) is already correct - files are only moved to done folder after successful upload since upload_file raises exceptions on failure. The move_to_done_folder call on line 73 only executes if upload_file (line 70) completes successfully. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Replace global isNaN with Number.isNaN, remove invalid spellcheck eslint-disable comments, and add missing ouiaId properties to PatternFly components for accessibility and testing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove obsolete test files for deleted GenerateReportJob and uploads controller :last action. Update tests to match new controller APIs and fix host ID ordering in playbook test. Changes: - Delete test/controllers/uploads_controller_test.rb (obsolete :last action) - Delete test/jobs/generate_report_job_test.rb (GenerateReportJob deleted) - Fix accounts_controller_test.rb to use new response structure - Fix cloud_request_controller_test.rb host ID sorting - Add route exceptions to rh_cloud_permissions_test.rb 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Convert UploadReportDirectJob and HostInventoryReportJob tests to use proper pattern - Use create_action + expects(:action_subject) + plan_action instead of create_and_plan_action - Add resource_locks method returning :link to both jobs to avoid exclusive lock in tests - Remove obsolete tests for removed features (instance_label, ProgressOutput integration, TaskOutput clearing) - Update remaining tests to remove references to ProgressOutput which was removed from UploadReportDirectJob This follows the Katello testing pattern for Dynflow actions and ensures action_subject is properly mocked without stubbing deep into Dynflow internals. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The :last action and GenerateReportJob were removed in the redesign, making this test obsolete. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…rectJob - Convert SingleHostReportJob tests to use proper Katello pattern - Add FileUtils.mkdir_p to ensure uploads folder exists before touching test files - SingleHostReportJob inherits resource_locks from HostInventoryReportJob All tests now pass: 353 tests, 1224 assertions, 0 failures, 0 errors, 0 skips 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add technical terms (auditd, hostid, krb5, rebootable, etc.) to eslint skipWords and wrap code comments containing hex UUIDs with targeted spellcheck disablers to resolve CI linting failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Update Dashboard.test.js: Remove outdated stopPolling test for class component lifecycle, replace with simple functional component test - Remove obsolete DashboardIntegration.test.js and snapshot: Integration test was incomplete and no longer relevant after Dashboard simplification - Update AccountList snapshot: Fix key prop values to use label instead of array index (Account1/2/3 vs 0/1/2) All three previously failing tests now pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Updates GenerateAllReportsJob to use HostInventoryReportJob instead of the removed GenerateReportJob class. Also updates the Actions history URL in the UI to search for the correct task class. This fixes the automatic daily report generation scheduled task and ensures the task history link shows current tasks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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?
This is built on top of #1124
This PR redesigns the Inventory Upload UI to provide a simpler, more intuitive user experience. The changes span both frontend React components and backend Rails controllers to deliver a unified workflow for inventory report generation and upload.
UI Improvements:
Backend Changes:
Code Quality:
Considerations taken when implementing this change?
What are the testing steps for this pull request?
Manual Testing:
API Testing:
🤖 Generated with Claude Code
Summary by Sourcery
Redesign the inventory upload workflow by consolidating report generation and upload into a unified Dynflow-based task, replacing the old Dashboard tabs with a modern TaskProgress UI, introducing TaskHistory and API endpoints for task monitoring, and swapping shell-based uploads for a reliable Ruby HTTP implementation.
New Features:
Bug Fixes:
Enhancements:
Tests:
Chores: