-
Couldn't load subscription status.
- Fork 96
Fixes #38833 - Replace foremanModal usage with PF5 modals #782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the existing ForemanModal system with PatternFly 5 modals, simplifying modal state management by removing Redux logic in favor of local state. The changes modernize the modal implementation and improve code maintainability by eliminating complex Redux-based modal rendering patterns.
- Replaced ForemanModal components with PF5 Modal components throughout the application
- Migrated from Redux-based modal state management to local useState hooks
- Updated all tests from enzyme-based snapshot testing to React Testing Library
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| webpack/mocks/foremanReact/components/ForemanModal/ | Removed ForemanModal mock files no longer needed |
| webpack/ForemanTasks/Components/common/ClickConfirmation/ | Migrated to PF5 Modal with local state and added UnlockModal components |
| webpack/ForemanTasks/Components/TasksTable/ | Updated to use individual modal components with local state management |
| webpack/ForemanTasks/Components/TasksTable/Components/ConfirmModal/ | Complete rewrite replacing Redux-connected component with individual modal components |
| webpack/ForemanTasks/Components/TaskDetails/ | Updated Task and TaskButtons components to use new modal system |
| webpack/ForemanTasks/Components/TaskActions/ | Removed UnlockModals file, functionality moved to ClickConfirmation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
webpack/ForemanTasks/Components/TaskDetails/Components/TaskButtons.js
Outdated
Show resolved
Hide resolved
f8d48f4 to
988ba15
Compare
|
|
||
| describe('Modal Close Behavior', () => { | ||
| it('resets checkbox state when modal is closed via cancel', () => { | ||
| const setModalClosed = jest.fn(); | ||
|
|
||
| render( | ||
| <ClickConfirmation {...defaultProps} setModalClosed={setModalClosed} /> | ||
| ); | ||
|
|
||
| const checkbox = screen.getByRole('checkbox'); | ||
| const cancelButton = screen.getByRole('button', { name: 'Cancel' }); | ||
|
|
||
| // Check the checkbox | ||
| fireEvent.click(checkbox); | ||
| expect(checkbox).toBeChecked(); | ||
|
|
||
| // Close the modal | ||
| fireEvent.click(cancelButton); | ||
|
|
||
| // The checkbox should be reset (though we can't test this directly since modal closes) | ||
| expect(setModalClosed).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| describe('Modal Close Behavior', () => { | |
| it('resets checkbox state when modal is closed via cancel', () => { | |
| const setModalClosed = jest.fn(); | |
| render( | |
| <ClickConfirmation {...defaultProps} setModalClosed={setModalClosed} /> | |
| ); | |
| const checkbox = screen.getByRole('checkbox'); | |
| const cancelButton = screen.getByRole('button', { name: 'Cancel' }); | |
| // Check the checkbox | |
| fireEvent.click(checkbox); | |
| expect(checkbox).toBeChecked(); | |
| // Close the modal | |
| fireEvent.click(cancelButton); | |
| // The checkbox should be reset (though we can't test this directly since modal closes) | |
| expect(setModalClosed).toHaveBeenCalled(); | |
| }); |
If we can't test that the checkbox reset, we should delete this testcase. setModalClosed is already tested in calls setModalClosed when cancel button is clicked
webpack/ForemanTasks/Components/common/ClickConfirmation/ClickConfirmation.test.js
Outdated
Show resolved
Hide resolved
| id={id} | ||
| title={__('Unlock')} | ||
| body={__( | ||
| "This will unlock the resources that the task is running against. Please note that this might lead to inconsistent state and should be used with caution, after making sure that the task can't be resumed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "This will unlock the resources that the task is running against. Please note that this might lead to inconsistent state and should be used with caution, after making sure that the task can't be resumed." | |
| "This action will unlock the resources held by the task. Please note that this might lead to inconsistent database state and should be used with caution after making sure that the task can't be resumed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should change the wording in this PR as its purpose is to update the modals, and this wording is the same as the old modal. Also maybe we need docs advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Let's go without the string changes in this PR, then.
| title={__('Force Unlock')} | ||
| body={sprintf( | ||
| __( | ||
| `Resources for %s task(s) will be unlocked and will not prevent other tasks from being run. As the task(s) might be still running, it should be avoided to use this unless you are really sure the task(s) got stuck.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `Resources for %s task(s) will be unlocked and will not prevent other tasks from being run. As the task(s) might be still running, it should be avoided to use this unless you are really sure the task(s) got stuck.` | |
| `Resources held by %s task(s) will be unlocked and will not prevent other tasks from being run. As the task(s) might be still running, you should avoid using this action unless you are certain that the task(s) are stuck.` |
Replaces the tests to RTL, removed the redux logic for modal rendering as I think it makes refactors and code reading more complicated.