Skip to content
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

(Host|Service) Detail view: Introduce parents and children tab #1098

Open
wants to merge 8 commits into
base: dependencies
Choose a base branch
from

Conversation

sukhwinder33445
Copy link
Contributor

No description provided.

@sukhwinder33445 sukhwinder33445 self-assigned this Nov 25, 2024
@sukhwinder33445 sukhwinder33445 changed the base branch from main to dependencies November 25, 2024 14:19
@sukhwinder33445 sukhwinder33445 force-pushed the introduce-parents-children-tab branch from 59dc8c9 to 74bb945 Compare December 5, 2024 10:16
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Dec 5, 2024
@nilmerg nilmerg linked an issue Dec 9, 2024 that may be closed by this pull request
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the issue:

The tabs are only visible if the object is linked in the database table dependency_node.

The tabs are always visible right now.

@sukhwinder33445 sukhwinder33445 force-pushed the introduce-parents-children-tab branch 2 times, most recently from c37ece0 to 9ab8072 Compare December 16, 2024 09:21
@raviks789
Copy link
Contributor

Except for the error in the search bar while trying to apply filter in Parents/Children tabs, everything else seems to work as listed out in the issue.

@@ -55,13 +66,18 @@ public function init()
}

$this->host = $host;
$this->loadTabsForObject($host);
$this->loadTabsForObject($this->host);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, I have reverted the change.

Comment on lines 270 to 273
$this->sendMultipartUpdate();
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->sendMultipartUpdate();
return;
$this->sendMultipartUpdate();
return;

Comment on lines 308 to 312
'name' => t('Name'),
'severity desc, last_state_change desc' => t('Severity'),
'state' => t('Current State'),
'last_state_change desc' => t('Last State Change')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use $this->translate() method instead as you have done in parentsAction for translation. Also, you seemed to have use the function t() in some places and $this->translate() in some places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

protected function createTabs(): Tabs
{
$hasDependencyNode = DependencyNode::on($this->getDb())
->columns('1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->columns('1')
->columns([new ipl\Sql\Expression('1')])

protected function createTabs(): Tabs
{
$hasDependecyNode = DependencyNode::on($this->getDb())
->columns('1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->columns('1')
->columns([[new ipl\Sql\Expression('1')]])

@@ -63,7 +72,12 @@ public function init()
}

$this->service = $service;
$this->loadTabsForObject($service);
$this->loadTabsForObject($this->service);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this change is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reverted it.

Comment on lines 175 to 179
'name' => t('Name'),
'severity desc, last_state_change desc' => t('Severity'),
'state' => t('Current State'),
'last_state_change desc' => t('Last State Change')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in my comment in HostController::childrenAction, you could also use $this->translate() method instead as you have done in parentsAction for translation.

Filter::equal('service_id', $this->service->id),
Filter::equal('host_id', $this->service->host_id)
))
->first() !== null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, you could either ignore this strict comparison, or you could use ->execute()->hasResult() that would return boolean.

if ($hasDependencyNode) {
$tabs->add('parents', [
'label' => t('Parents'),
'url' => Url::fromPath('icingadb/host/parents', ['name' => $this->host->name])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned earlier, you could create and use Link::hostParents method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, Link class should not be used here.

Comment on lines +5 to +15
.title > * {
margin: 0 .28125em; // 0 calculated   width

&:first-child {
margin-left: 0;
}

&:last-child {
margin-right: 0;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule is already applied in ipl-web item-list.less. Is there a reason why you are doing this here again?

@sukhwinder33445 sukhwinder33445 force-pushed the introduce-parents-children-tab branch from 9ab8072 to fb557c7 Compare December 18, 2024 12:00
- Otherwise the searchbar uses it as base filter and apply it on the query
- Set the outer tab as active.

Previously, the inner tab was activated in the setTitleTab method, but the outer tab does not know about the state of inner tabs.
So whenever sendMultipartUpdate() -> getActiveTab() was called, the retured value was always null.
@sukhwinder33445 sukhwinder33445 force-pushed the introduce-parents-children-tab branch from fb557c7 to 03a160d Compare December 18, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List views for parents and children
3 participants