refactor: DRY monitor action render wrapper#206
refactor: DRY monitor action render wrapper#206somethingwithproof wants to merge 2 commits intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the monitor plugin’s monitor.php action handling by introducing a shared wrapper that runs an action and then renders the monitor page, reducing repeated “do action + drawPage” branches.
Changes:
- Added
monitorRunActionAndRender()helper to centralize action-then-render flow. - Updated several
monitor.phpactions (ajax_mute_all,ajax_unmute_all,dbchange,remove,saveDb) to use the helper. - Added a standalone regression test to validate action-before-render ordering and ensure the wrapper is used in
monitor.php.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
monitor.php |
Routes multiple action branches through the new action+render wrapper helper. |
monitor_helpers.php |
Introduces monitorRunActionAndRender() shared helper. |
tests/test_action_render_wrapper.php |
Adds a CLI regression test covering wrapper call order and usage. |
You can also share your feedback on Copilot code review. Take the survey.
| | Copyright (C) 2004-2026 The Cacti Group | | ||
| | | | ||
| | This program is free software; you can redistribute it and/or | | ||
| | modify it under the terms of the GNU General Public License | | ||
| | as published by the Free Software Foundation; either version 2 | | ||
| | of the License, or (at your option) any later version. | |
There was a problem hiding this comment.
Good catch — updated in 7d38d68 to use the standard Cacti header block in both new files.
monitor_helpers.php
Outdated
| * @return void | ||
| */ | ||
| function monitorRunActionAndRender(callable $action): void { | ||
| call_user_func($action); |
There was a problem hiding this comment.
Updated in 7d38d68 to invoke the callable directly ($action()), which also improves readability.
monitor_helpers.php
Outdated
| if (!function_exists('monitorRunActionAndRender')) { | ||
| /** | ||
| * Execute one monitor action callback and then render the monitor page. | ||
| * | ||
| * @param callable $action Action callback with no arguments. | ||
| * | ||
| * @return void | ||
| */ | ||
| function monitorRunActionAndRender(callable $action): void { | ||
| call_user_func($action); | ||
| drawPage(); | ||
| } |
There was a problem hiding this comment.
Addressed in follow-up commit 7d38d68.
| <?php | ||
|
|
There was a problem hiding this comment.
Good catch — updated in 7d38d68 to use the standard Cacti header block in both new files.
tests/test_action_render_wrapper.php
Outdated
| $expected_calls = [ | ||
| "monitorRunActionAndRender('muteAllHosts');", | ||
| "monitorRunActionAndRender('unmuteAllHosts');", | ||
| "monitorRunActionAndRender('loadDashboardSettings');", | ||
| "monitorRunActionAndRender('removeDashboard');", | ||
| "monitorRunActionAndRender('saveSettings');" | ||
| ]; | ||
|
|
||
| foreach ($expected_calls as $expected_call) { | ||
| if (strpos($source, $expected_call) === false) { |
There was a problem hiding this comment.
Updated in 7d38d68 to use regex-based assertions with whitespace tolerance instead of exact string matching.
Summary
Implements issue #205 by consolidating repeated monitor action handling that performs an action and then renders the page.
Changes
monitorRunActionAndRender()inmonitor_helpers.phpmonitor.phpto route repeated action paths through helper:ajax_mute_allajax_unmute_alldbchangeremovesaveDbtests/test_action_render_wrapper.phpValidation
php -l monitor.phpphp -l monitor_helpers.phpphp -l tests/test_action_render_wrapper.phpphp tests/test_action_render_wrapper.phpIssue
Closes #205