Conversation
8453195 to
3c998e0
Compare
Add a new form that supports adding and removing dispalyed columns in tabular viewmode.
Add TabularViewModeSwitcher as a subclass of ViewModeSwitcher and remove tabular view as option of the ViewModeSwitcher.
Adjust HostsController and ServicesController to use the new ColumnChooser form and the TabularViewModeSwitcher. The Controller class is adjusted to work with the TabularViewModeSwitcher The HostGroupsController and ServiceGroupsController are adjusted, to ensure they use with the GridViewModeSwitcher.
Add a button to the StateItemTable to open the ColumnChooser form.
Exclude regular columns by using fixed columns and expand Objectsuggestions to exclude a given set of customvars.
3c998e0 to
2786e1d
Compare
library/Icingadb/Web/Controller.php
Outdated
| } else { | ||
| $viewModeSwitcher = new ViewModeSwitcher(); | ||
| } | ||
| $viewModeSwitcher = $this->getViewModeSwitcherInstance(); |
There was a problem hiding this comment.
Not liking the idea of having a controller to override a method to customize the switcher. Introduce a parameter instead. The advantage is, it's immediately visible at the call site which switcher is used, without having to trace up the class hierarchy. It also means one controller could use different switchers for different actions if ever needed.
If you worry about type-safety, you can still facilitate static analysis using phpstan by using the type class-string<ViewModeSwitcher> in the phpdoc string.
library/Icingadb/Web/Controller.php
Outdated
| if (! $columnsDef) { | ||
| if ($viewMode === 'tabular') { | ||
| $this->httpBadRequest('Missing parameter "columns"'); | ||
| $columnsDef = $this->getDefaultColumns(); |
There was a problem hiding this comment.
The same as for getViewModeSwitcherInstance applies here. Use a parameter instead.
Also, name, state.output is only a viable default in two cases, cases which override it at the moment anyway. So I think this can be mandatory parameter. (With no default)
| Url::fromPath("icingadb/{$this->getControllerPath()}/columnControl")->setParam( | ||
| 'columns', | ||
| implode(',', array_keys($this->columns)) | ||
| ) |
There was a problem hiding this comment.
getControllerPath introduces an unnecessary hard-coded dependency of e.g. HostItemTable <-> HostsController. An easy change avoids this: Add a new setter setColumnChooserUrl(Url $url): static
If the setter is not called, no column chooser opener appears.
| $headerRow->addHtml($headerCell); | ||
| } | ||
|
|
||
| if ($headerCell !== null) { |
There was a problem hiding this comment.
$headerCell cannot be null, unless you initialize it before the loop, which I'd recommend anyway. (Albeit that this can't actually happen, but then why did you check for null to begin with?)
OR
Check if $this->columns isn't empty. May be even simpler.
| (new ColumnChooser(Url::fromPath('icingadb/services/complete'), Service::on($this->getDb())->getResolver())) | ||
| ->setAction((string) Url::fromRequest()) | ||
| ->on(ColumnChooser::ON_SENT, function (ColumnChooser $form) { | ||
| if ($form->hasBeenSubmitted()) { | ||
| $url = Url::fromPath('icingadb/services'); | ||
| $url->setParam('columns', $form->getValue('columns', '')); | ||
| $this->redirectNow($url); | ||
| } else { | ||
| foreach ($form->getPartUpdates() as $update) { | ||
| if (! is_array($update)) { | ||
| $update = [$update]; | ||
| } | ||
|
|
||
| $this->addPart(...$update); | ||
| } | ||
| } | ||
| })->handleRequest($this->getServerRequest()) |
There was a problem hiding this comment.
This should be the job of Controller::createColumnControl. There's already a related comment at the end of it.
| ->setReadOnly() | ||
| ->setOrdered() | ||
| ->setSuggestionUrl($this->suggestionUrl) | ||
| ->setValue($this->getColumns($this->getRequest())) |
There was a problem hiding this comment.
Huh. A widget like this should not have to access the request during assemble. Once createColumnControl initializes this widget, it knows the columns anyway so getColumns($req) should be gone then.
| if ($label !== null) { | ||
| $term->setLabel($label); | ||
| } | ||
| } catch (\Exception) { |
There was a problem hiding this comment.
Catch InvalidRelationException here
| protected $customVarSources; | ||
|
|
||
| /** @var ?array<string, string> */ | ||
| /** @var ?array<string, mixed> */ |
| protected ?array $fixedColumns = null; | ||
|
|
||
| /** @var array Names of excluded custom variables as {model}.vars.{flatname} */ | ||
| protected array $excludedCustomVars = []; |
There was a problem hiding this comment.
Please tell me how I'd notice that this is required and for what. I can't imagine this right now and why custom variables are special in that regard. The TermInput already excludes already entered terms in the suggestion list.
There was a problem hiding this comment.
As discussed I tried using SearchSuggestions instead, but I can't say I'm satisfied with the result. So I'd be happy for any advice on how to improve them.
…lass` Use the parameter to customize the type of ViewModeSwitcher used
Add a parameter for default columns to be used if no columns are set in the URL
Use `SearchSuggestions` in `HostsController` and `ServicesController`
Use the setter to set the url for the ColumnChooser
It was previously hard coded...
Ensure `GridViewModeSwitcher::class` is passed
Add a new form to choose the displayed columns in hosts and services view.
resolves: #82
fixes #850
requires: Icinga/ipl-web#277 for the
TabularViewModeSwitchericonrequires: Icinga/ipl-web#357 to display error messages for invalid columns