refactor: DRY flowview filters layout wrapper#246
refactor: DRY flowview filters layout wrapper#246somethingwithproof wants to merge 2 commits intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a shared helper to wrap FlowView filter page rendering with the standard Cacti header/footer (optionally respecting embed mode), updates flowview_filters.php to use it, and adds a small regression test script.
Changes:
- Introduce
flowview_filters_render_with_layout()helper to centralize header/footer wrapping logic. - Refactor
flowview_filters.phpeditand default actions to use the shared helper. - Add
tests/test_layout_wrapper.phpto verify wrapper behavior (and thatflowview_filters.phpuses the helper).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ui_helpers.php | Adds a helper function to wrap a renderer callback with header/footer, optionally respecting embed. |
| flowview_filters.php | Uses the shared helper for edit and default rendering paths and includes the helper file. |
| tests/test_layout_wrapper.php | Adds a basic regression test for wrapper behavior and checks that flowview_filters.php invokes the helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| | of the License, or (at your option) any later version. | | ||
| +-------------------------------------------------------------------------+ |
There was a problem hiding this comment.
Addressed in 9f37b1d: expanded ui_helpers.php to the standard full Cacti header block used by this plugin.
tests/test_layout_wrapper.php
Outdated
| if (strpos($source, "flowview_filters_render_with_layout('edit_filter', true);") === false) { | ||
| fwrite(STDERR, 'Expected edit action to use shared layout helper.' . PHP_EOL); | ||
| exit(1); | ||
| } | ||
|
|
||
| if (strpos($source, "flowview_filters_render_with_layout('show_filters');") === false) { |
There was a problem hiding this comment.
Addressed in 9f37b1d: converted source checks in the regression test to regex-based assertions with formatting tolerance.
Summary
This PR implements issue #245 by removing repeated page layout wrapper logic from
flowview_filters.php.Changes
ui_helpers.phpwith shared helper:flowview_filters_render_with_layout($renderer, $respect_embed = false)flowview_filters.phpaction routing:editnow callsflowview_filters_render_with_layout('edit_filter', true)flowview_filters_render_with_layout('show_filters')Validation
php -l flowview_filters.phpphp -l ui_helpers.phpphp -l tests/test_layout_wrapper.phpphp tests/test_layout_wrapper.phpIssue
Closes #245