From a1bbc3679539bbcd3a2391b48654e7d6e642b0d1 Mon Sep 17 00:00:00 2001 From: raviks789 Date: Thu, 26 Sep 2024 10:58:36 +0200 Subject: [PATCH] fix1: resolve review comments --- library/Icingadb/Common/IcingaRedis.php | 12 ++++- library/Icingadb/Common/StateBadges.php | 18 ++++---- .../RedundancyGroupParentStateSummary.php | 43 +++++++++--------- .../Icingadb/Widget/Detail/ObjectDetail.php | 18 +++----- ...ProblemList.php => DependencyNodeList.php} | 5 ++- .../ItemList/RedundancyGroupListItem.php | 44 +++++++++++-------- .../Icingadb/Widget/ObjectsStateBadges.php | 33 ++++++++++++++ ...upStatistics.php => ObjectsStatistics.php} | 22 +++++----- .../RedundancyGroupParentStateBadges.php | 33 -------------- public/css/common.less | 2 +- .../css/list/redundancy-group-list-item.less | 25 +++++++++++ public/css/list/root-problem-list.less | 31 ------------- .../redundancy-group-parent-state-badges.less | 2 +- 13 files changed, 147 insertions(+), 141 deletions(-) rename library/Icingadb/Widget/ItemList/{RootProblemList.php => DependencyNodeList.php} (84%) create mode 100644 library/Icingadb/Widget/ObjectsStateBadges.php rename library/Icingadb/Widget/{RedundancyGroupStatistics.php => ObjectsStatistics.php} (63%) delete mode 100644 library/Icingadb/Widget/RedundancyGroupParentStateBadges.php create mode 100644 public/css/list/redundancy-group-list-item.less delete mode 100644 public/css/list/root-problem-list.less diff --git a/library/Icingadb/Common/IcingaRedis.php b/library/Icingadb/Common/IcingaRedis.php index a22a0f03b..b86b5817f 100644 --- a/library/Icingadb/Common/IcingaRedis.php +++ b/library/Icingadb/Common/IcingaRedis.php @@ -8,6 +8,7 @@ use Generator; use Icinga\Application\Config; use Icinga\Application\Logger; +use ipl\Sql\Expression; use Predis\Client as Redis; class IcingaRedis @@ -163,7 +164,16 @@ protected static function fetchState(string $key, array $ids, array $columns): G foreach ($results as $i => $json) { if ($json !== null) { $data = json_decode($json, true); - $keyMap = array_fill_keys($columns, null); + $keyMap = []; + + foreach ($columns as $alias => $column) { + if ($column instanceof Expression) { + $column = $alias; + } + + $keyMap[] = $column; + } + unset($keyMap['is_overdue']); // Is calculated by Icinga DB, not Icinga 2, hence it's never in redis // TODO: Remove once https://github.com/Icinga/icinga2/issues/9427 is fixed diff --git a/library/Icingadb/Common/StateBadges.php b/library/Icingadb/Common/StateBadges.php index 16ae6eb85..bd8bcc6e1 100644 --- a/library/Icingadb/Common/StateBadges.php +++ b/library/Icingadb/Common/StateBadges.php @@ -4,11 +4,11 @@ namespace Icinga\Module\Icingadb\Common; +use InvalidArgumentException; use ipl\Html\BaseHtmlElement; use ipl\Html\Html; use ipl\Stdlib\BaseFilter; use ipl\Stdlib\Filter; -use ipl\Web\Filter\QueryString; use ipl\Web\Url; use ipl\Web\Widget\Link; use ipl\Web\Widget\StateBadge; @@ -76,10 +76,12 @@ protected function getBaseUrl(): ?Url * @param string $state * * @return int + * + * @throws InvalidArgumentException if the given state is not valid */ protected function getStateInt(string $state): int { - return 0; + throw new InvalidArgumentException('%s is not a valid state', $state); } /** @@ -138,18 +140,17 @@ public function createLink($content, Filter\Rule $filter = null): Link * Create a state bade * * @param string $state - * @param bool $createLink Create link for the badge if true * * @return ?BaseHtmlElement */ - protected function createBadge(string $state, bool $createLink = true) + protected function createBadge(string $state): ?BaseHtmlElement { $key = $this->prefix . "_{$state}"; if (isset($this->item->$key) && $this->item->$key) { $stateBadge = new StateBadge($this->item->$key, $state); - if ($createLink) { + if ($this->url !== null) { $this->createLink( $stateBadge, Filter::equal($this->type . '.state.soft_state', $this->getStateInt($state)) @@ -166,11 +167,10 @@ protected function createBadge(string $state, bool $createLink = true) * Create a state group * * @param string $state - * @param bool $createLink Create link for the badge if true * * @return ?BaseHtmlElement */ - protected function createGroup(string $state, bool $createLink = true) + protected function createGroup(string $state): ?BaseHtmlElement { $content = []; $handledKey = $this->prefix . "_{$state}_handled"; @@ -179,7 +179,7 @@ protected function createGroup(string $state, bool $createLink = true) if (isset($this->item->$unhandledKey) && $this->item->$unhandledKey) { $unhandledStateBadge = new StateBadge($this->item->$unhandledKey, $state); - if ($createLink) { + if ($this->url !== null) { $unhandledStateBadge = $this->createLink( $unhandledStateBadge, Filter::all( @@ -196,7 +196,7 @@ protected function createGroup(string $state, bool $createLink = true) if (isset($this->item->$handledKey) && $this->item->$handledKey) { $handledStateBadge = new StateBadge($this->item->$handledKey, $state, true); - if ($createLink) { + if ($this->url !== null) { $handledStateBadge = $this->createLink( $handledStateBadge, Filter::all( diff --git a/library/Icingadb/Model/RedundancyGroupParentStateSummary.php b/library/Icingadb/Model/RedundancyGroupParentStateSummary.php index e9945a3b2..743f1d285 100644 --- a/library/Icingadb/Model/RedundancyGroupParentStateSummary.php +++ b/library/Icingadb/Model/RedundancyGroupParentStateSummary.php @@ -1,6 +1,6 @@ new Expression( + 'objects_problem_handled' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_host_state.soft_state = 1' . ' AND (redundancy_group_from_to_host_state.is_handled = \'y\'' . ' OR redundancy_group_from_to_host_state.is_reachable = \'n\') THEN 1 ELSE 0 END' @@ -33,7 +32,7 @@ public function getSummaryColumns() . ' AND (redundancy_group_from_to_service_state.is_handled = \'y\'' . ' OR redundancy_group_from_to_service_state.is_reachable = \'n\') THEN 1 ELSE 0 END)' ), - 'parents_problem_unhandled' => new Expression( + 'objects_problem_unhandled' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_host_state.soft_state = 1' . ' AND redundancy_group_from_to_host_state.is_handled = \'n\'' . ' AND redundancy_group_from_to_host_state.is_reachable = \'y\' THEN 1 ELSE 0 END' @@ -41,40 +40,40 @@ public function getSummaryColumns() . ' AND redundancy_group_from_to_service_state.is_handled = \'n\'' . ' AND redundancy_group_from_to_service_state.is_reachable = \'y\' THEN 1 ELSE 0 END)' ), - 'parents_pending' => new Expression( + 'objects_pending' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_host_state.soft_state = 99 THEN 1 ELSE 0 END' . '+ CASE WHEN redundancy_group_from_to_service_state.soft_state = 99 THEN 1 ELSE 0 END)' ), - 'parents_problems_unacknowledged' => new Expression( + 'objects_problems_unacknowledged' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_host_state.is_problem = \'y\'' . ' AND redundancy_group_from_to_host_state.is_acknowledged = \'n\' THEN 1 ELSE 0 END' . '+ CASE WHEN redundancy_group_from_to_service_state.is_problem = \'y\'' . ' AND redundancy_group_from_to_service_state.is_acknowledged = \'n\' THEN 1 ELSE 0 END)' ), - 'parents_total' => new Expression( + 'objects_total' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_host.id IS NOT NULL THEN 1 ELSE 0 END)' . '+ SUM(CASE WHEN redundancy_group_from_to_service.id IS NOT NULL THEN 1 ELSE 0 END)' ), - 'parents_ok' => new Expression( + 'objects_ok' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_host_state.soft_state = 0 THEN 1 ELSE 0 END' . '+ CASE WHEN redundancy_group_from_to_service_state.soft_state = 0 THEN 1 ELSE 0 END)' ), - 'parents_unknown_handled' => new Expression( + 'objects_unknown_handled' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_service_state.soft_state = 3' . ' AND (redundancy_group_from_to_service_state.is_handled = \'y\'' . ' OR redundancy_group_from_to_service_state.is_reachable = \'n\') THEN 1 ELSE 0 END)' ), - 'parents_unknown_unhandled' => new Expression( + 'objects_unknown_unhandled' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_service_state.soft_state = 3' . ' AND redundancy_group_from_to_service_state.is_handled = \'n\'' . ' AND redundancy_group_from_to_service_state.is_reachable = \'y\' THEN 1 ELSE 0 END)' ), - 'parents_warning_handled' => new Expression( + 'objects_warning_handled' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_service_state.soft_state = 1' . ' AND (redundancy_group_from_to_service_state.is_handled = \'y\'' . ' OR redundancy_group_from_to_service_state.is_reachable = \'n\') THEN 1 ELSE 0 END)' ), - 'parents_warning_unhandled' => new Expression( + 'objects_warning_unhandled' => new Expression( 'SUM(CASE WHEN redundancy_group_from_to_service_state.soft_state = 1' . ' AND redundancy_group_from_to_service_state.is_handled = \'n\'' . ' AND redundancy_group_from_to_service_state.is_reachable = \'y\' THEN 1 ELSE 0 END)' diff --git a/library/Icingadb/Widget/Detail/ObjectDetail.php b/library/Icingadb/Widget/Detail/ObjectDetail.php index 39ead2ddf..0c95cd903 100644 --- a/library/Icingadb/Widget/Detail/ObjectDetail.php +++ b/library/Icingadb/Widget/Detail/ObjectDetail.php @@ -23,7 +23,7 @@ use Icinga\Module\Icingadb\Model\CustomvarFlat; use Icinga\Module\Icingadb\Model\UnreachableParent; use Icinga\Module\Icingadb\Web\Navigation\Action; -use Icinga\Module\Icingadb\Widget\ItemList\RootProblemList; +use Icinga\Module\Icingadb\Widget\ItemList\DependencyNodeList; use Icinga\Module\Icingadb\Widget\MarkdownText; use Icinga\Module\Icingadb\Common\ServiceLinks; use Icinga\Module\Icingadb\Forms\Command\Object\ToggleObjectFeaturesForm; @@ -443,7 +443,7 @@ protected function createPluginOutput(): array 'div', [ 'id' => 'check-output-' . $this->object->checkcommand_name, - 'class' => 'collapsible', + 'class' => ['collapsible', 'check-command-output'], 'data-visible-height' => 100 ], $pluginOutput @@ -606,6 +606,10 @@ protected function fetchCustomVars() protected function createRootProblems(): ?array { + if ($this->object->state->is_reachable) { + return null; + } + $rootProblems = UnreachableParent::on($this->getDb(), $this->object) ->with([ 'redundancy_group', @@ -624,14 +628,6 @@ protected function createRootProblems(): ?array $this->applyRestrictions($rootProblems); - if ($rootProblems->count() > 0) { - $rootProblemList = (new RootProblemList( - $rootProblems->execute() - )); - - return [HtmlElement::create('h2', null, t('Root Problems')), $rootProblemList]; - } - - return null; + return [HtmlElement::create('h2', null, t('Root Problems')), new DependencyNodeList($rootProblems)]; } } diff --git a/library/Icingadb/Widget/ItemList/RootProblemList.php b/library/Icingadb/Widget/ItemList/DependencyNodeList.php similarity index 84% rename from library/Icingadb/Widget/ItemList/RootProblemList.php rename to library/Icingadb/Widget/ItemList/DependencyNodeList.php index 2011c8156..4de7b8a29 100644 --- a/library/Icingadb/Widget/ItemList/RootProblemList.php +++ b/library/Icingadb/Widget/ItemList/DependencyNodeList.php @@ -4,9 +4,10 @@ namespace Icinga\Module\Icingadb\Widget\ItemList; +use Icinga\Module\Icingadb\Model\DependencyNode; use Icinga\Module\Icingadb\Model\UnreachableParent; -class RootProblemList extends StateList +class DependencyNodeList extends StateList { protected $defaultAttributes = ['class' => ['root-problem-list']]; @@ -22,7 +23,7 @@ protected function getItemClass(): string protected function createListItem(object $data) { - /** @var UnreachableParent $data */ + /** @var UnreachableParent|DependencyNode $data */ if ($data->redundancy_group_id !== null) { return new RedundancyGroupListItem($data->redundancy_group, $this); } elseif ($data->service_id !== null) { diff --git a/library/Icingadb/Widget/ItemList/RedundancyGroupListItem.php b/library/Icingadb/Widget/ItemList/RedundancyGroupListItem.php index 956bcd85c..9d177893d 100644 --- a/library/Icingadb/Widget/ItemList/RedundancyGroupListItem.php +++ b/library/Icingadb/Widget/ItemList/RedundancyGroupListItem.php @@ -11,7 +11,7 @@ use Icinga\Module\Icingadb\Model\RedundancyGroupParentStateSummary; use Icinga\Module\Icingadb\Util\PluginOutput; use Icinga\Module\Icingadb\Widget\PluginOutputContainer; -use Icinga\Module\Icingadb\Widget\RedundancyGroupStatistics; +use Icinga\Module\Icingadb\Widget\ObjectsStatistics; use ipl\Html\BaseHtmlElement; use ipl\Html\Html; use ipl\Sql\Expression; @@ -33,6 +33,8 @@ class RedundancyGroupListItem extends StateListItem use Auth; use Database; + protected $baseAttributes = ['class' => ['list-item', 'redundancy-group-list-item']]; + protected function getStateBallSize(): string { return StateBall::SIZE_LARGE; @@ -78,27 +80,27 @@ protected function assembleCaption(BaseHtmlElement $caption): void $members = RedundancyGroup::on($this->getDb()) ->columns([ 'id' => 'id', - 'parent_output' => new Expression( + 'objects_output' => new Expression( 'CASE WHEN redundancy_group_from_to_host_state.output IS NULL' . ' THEN redundancy_group_from_to_service_state.output' . ' ELSE redundancy_group_from_to_host_state.output END' ), - 'parent_long_output' => new Expression( + 'objects_long_output' => new Expression( 'CASE WHEN redundancy_group_from_to_host_state.long_output IS NULL' . ' THEN redundancy_group_from_to_service_state.long_output' . ' ELSE redundancy_group_from_to_host_state.long_output END' ), - 'parent_checkcommand_name' => new Expression( + 'objects_checkcommand_name' => new Expression( 'CASE WHEN redundancy_group_from_to_host.checkcommand_name IS NULL' . ' THEN redundancy_group_from_to_service.checkcommand_name' . ' ELSE redundancy_group_from_to_host.checkcommand_name END' ), - 'parent_last_state_change' => new Expression( + 'objects_last_state_change' => new Expression( 'CASE WHEN redundancy_group_from_to_host_state.last_state_change IS NULL' . ' THEN redundancy_group_from_to_service_state.last_state_change' . ' ELSE redundancy_group_from_to_host_state.last_state_change END' ), - 'parent_severity' => new Expression( + 'objects_severity' => new Expression( 'CASE WHEN redundancy_group_from_to_host_state.severity IS NULL' . ' THEN redundancy_group_from_to_service_state.severity' . ' ELSE redundancy_group_from_to_host_state.severity END' @@ -107,8 +109,8 @@ protected function assembleCaption(BaseHtmlElement $caption): void ->with($relations) ->filter($filter) ->orderBy([ - 'parent_severity', - 'parent_last_state_change', + 'objects_severity', + 'objects_last_state_change', ], 'DESC'); $this->applyRestrictions($members); @@ -118,28 +120,32 @@ protected function assembleCaption(BaseHtmlElement $caption): void if($data) { $caption->addHtml(new PluginOutputContainer( - (new PluginOutput($data->parent_output . "\n" .$data->parent_long_output)) - ->setCommandName($data->parent_checkcommand_name) + (new PluginOutput($data->objects_output . "\n" .$data->objects_long_output)) + ->setCommandName($data->objects_checkcommand_name) )); } - $caption->addHtml(new RedundancyGroupStatistics($summary->first())); + $caption->addHtml(new ObjectsStatistics($summary->first())); } protected function assembleTitle(BaseHtmlElement $title): void { + $subject = $this->createSubject(); if ($this->state->failed) { - $verb = 'has'; + $stateTextElement = Html::sprintf( + t('%s has %s', ' has '), + $subject, + Html::tag('span', ['class' => 'state-text'], 'FAILED') + ); } else { - $verb = 'is'; + $stateTextElement = Html::sprintf( + t('%s is %s', ' is '), + $subject, + Html::tag('span', ['class' => 'state-text'], 'OK') + ); } - $title->addHtml(Html::sprintf( - t('%s %s %s', ' has/is '), - $this->createSubject(), - $verb, - Html::tag('span', ['class' => 'state-text'], $this->state->failed ? 'Failed' : 'OK') - )); + $title->addHtml($stateTextElement); } protected function assemble(): void diff --git a/library/Icingadb/Widget/ObjectsStateBadges.php b/library/Icingadb/Widget/ObjectsStateBadges.php new file mode 100644 index 000000000..28ead02f4 --- /dev/null +++ b/library/Icingadb/Widget/ObjectsStateBadges.php @@ -0,0 +1,33 @@ +addAttributes(['class' => 'objects-state-badges']); + + $this->add(array_filter([ + $this->createGroup('problem'), + $this->createGroup('warning'), + $this->createGroup('unknown'), + $this->createBadge('ok'), + $this->createBadge('pending') + ])); + } +} diff --git a/library/Icingadb/Widget/RedundancyGroupStatistics.php b/library/Icingadb/Widget/ObjectsStatistics.php similarity index 63% rename from library/Icingadb/Widget/RedundancyGroupStatistics.php rename to library/Icingadb/Widget/ObjectsStatistics.php index 2e75c0a08..1c0f76ba4 100644 --- a/library/Icingadb/Widget/RedundancyGroupStatistics.php +++ b/library/Icingadb/Widget/ObjectsStatistics.php @@ -11,7 +11,7 @@ use ipl\Html\ValidHtml; use ipl\Html\HtmlString; -class RedundancyGroupStatistics extends ObjectStatistics +class ObjectsStatistics extends ObjectStatistics { protected $summary; @@ -23,26 +23,26 @@ public function __construct($summary) protected function createDonut(): ValidHtml { $donut = (new Donut()) - ->addSlice($this->summary->parents_ok, ['class' => 'slice-state-ok']) - ->addSlice($this->summary->parents_warning_handled, ['class' => 'slice-state-warning-handled']) - ->addSlice($this->summary->parents_warning_unhandled, ['class' => 'slice-state-warning']) - ->addSlice($this->summary->parents_problem_handled, ['class' => 'slice-state-critical-handled']) - ->addSlice($this->summary->parents_problem_unhandled, ['class' => 'slice-state-critical']) - ->addSlice($this->summary->parents_unknown_handled, ['class' => 'slice-state-unknown-handled']) - ->addSlice($this->summary->parents_unknown_unhandled, ['class' => 'slice-state-unknown']) - ->addSlice($this->summary->parents_pending, ['class' => 'slice-state-pending']); + ->addSlice($this->summary->objects_ok, ['class' => 'slice-state-ok']) + ->addSlice($this->summary->objects_warning_handled, ['class' => 'slice-state-warning-handled']) + ->addSlice($this->summary->objects_warning_unhandled, ['class' => 'slice-state-warning']) + ->addSlice($this->summary->objects_problem_handled, ['class' => 'slice-state-critical-handled']) + ->addSlice($this->summary->objects_problem_unhandled, ['class' => 'slice-state-critical']) + ->addSlice($this->summary->objects_unknown_handled, ['class' => 'slice-state-unknown-handled']) + ->addSlice($this->summary->objects_unknown_unhandled, ['class' => 'slice-state-unknown']) + ->addSlice($this->summary->objects_pending, ['class' => 'slice-state-pending']); return HtmlString::create($donut->render()); } protected function createTotal(): ValidHtml { - return Text::create($this->shortenAmount($this->summary->parents_total)); + return Text::create($this->shortenAmount($this->summary->objects_total)); } protected function createBadges(): ValidHtml { - $badges = new RedundancyGroupParentStateBadges($this->summary); + $badges = new ObjectsStateBadges($this->summary); if ($this->hasBaseFilter()) { $badges->setBaseFilter($this->getBaseFilter()); } diff --git a/library/Icingadb/Widget/RedundancyGroupParentStateBadges.php b/library/Icingadb/Widget/RedundancyGroupParentStateBadges.php deleted file mode 100644 index e5970ee82..000000000 --- a/library/Icingadb/Widget/RedundancyGroupParentStateBadges.php +++ /dev/null @@ -1,33 +0,0 @@ -addAttributes(['class' => 'redundancy-group-parent-state-badges']); - - $this->add(array_filter([ - $this->createGroup('problem', false), - $this->createGroup('warning', false), - $this->createGroup('unknown', false), - $this->createBadge('ok', false), - $this->createBadge('pending', false) - ])); - } -} diff --git a/public/css/common.less b/public/css/common.less index 333df94b8..202d8c13a 100644 --- a/public/css/common.less +++ b/public/css/common.less @@ -197,7 +197,7 @@ div.show-more { margin-left: 1em / 1.333em; // 1em / h2 font size } -.object-detail .plugin-output { +.object-detail .check-command-output .plugin-output { .rounded-corners(.25em); background-color: @gray-lighter; padding: .5em; diff --git a/public/css/list/redundancy-group-list-item.less b/public/css/list/redundancy-group-list-item.less new file mode 100644 index 000000000..5ae916ff5 --- /dev/null +++ b/public/css/list/redundancy-group-list-item.less @@ -0,0 +1,25 @@ +.redundancy-group-list-item.list-item { + .caption { + display: flex; + justify-content: space-between; + + .plugin-output { + vertical-align: middle; + .text-ellipsis(); + } + + .object-statistics { + ul { + display: flex; + } + + .state-badges { + font-size: 0.75em; + } + } + } + + .state-problem { + background-color: @state-critical; + } +} \ No newline at end of file diff --git a/public/css/list/root-problem-list.less b/public/css/list/root-problem-list.less deleted file mode 100644 index 5b1e07e53..000000000 --- a/public/css/list/root-problem-list.less +++ /dev/null @@ -1,31 +0,0 @@ -.root-problem-list.item-list { - .list-item { - .caption { - .plugin-output { - background: none; - } - - display: flex; - justify-content: space-between; - - .plugin-output { - vertical-align: middle; - .text-ellipsis(); - } - - .object-statistics { - ul { - display: flex; - } - - .state-badges { - font-size: 0.75em; - } - } - } - - .state-problem { - background-color: @state-critical; - } - } -} diff --git a/public/css/widget/redundancy-group-parent-state-badges.less b/public/css/widget/redundancy-group-parent-state-badges.less index 31e3d5c1c..ad369c66a 100644 --- a/public/css/widget/redundancy-group-parent-state-badges.less +++ b/public/css/widget/redundancy-group-parent-state-badges.less @@ -1,3 +1,3 @@ -.redundancy-group-parent-state-badges { +.objects-state-badges { .state-badges(); }