Fix form field suggestions (#2840)

Other available fields should be suggested when no check command has been chosen for service template. fixes #2826, #2815
Icinga · Jan 16, 2024 · 0493f93 · 0493f93
2 parents 6851778 + 80d566f
commit 0493f93
Show file tree

Hide file tree

Showing 20 changed files with 203 additions and 34 deletions.
diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml
@@ -150,6 +150,7 @@ jobs:
 
       - name: Setup Libraries
         run: |
+          composer require --working-dir=_icingaweb2 -n --no-progress mockery/mockery
           mkdir _libraries
           git clone --depth 1 -b snapshot/nightly https://github.com/Icinga/icinga-php-library.git _libraries/ipl
           git clone --depth 1 -b snapshot/nightly https://github.com/Icinga/icinga-php-thirdparty.git _libraries/vendor

diff --git a/application/forms/IcingaObjectFieldForm.php b/application/forms/IcingaObjectFieldForm.php
@@ -53,13 +53,8 @@ public function setup()
             $command = null;
         }
 
-        if ($command) {
-            $suggestions = $this->fieldSuggestion = new FormFieldSuggestion($command, $this->db->enumDatafields());
-            $fields = $suggestions->getCommandFields();
-        } else {
-            $suggestions = null;
-            $fields = [];
-        }
+        $suggestions = $this->fieldSuggestion = new FormFieldSuggestion($command, $this->db->enumDatafields());
+        $fields = $suggestions->getCommandFields();
 
         $this->addElement('select', 'datafield_id', [
             'label'        => 'Field',
@@ -97,7 +92,7 @@ public function setup()
                     . ' user puts the focus on this field'
                 ),
                 'ignore'      => true,
-                'value'       => $suggestions ? $suggestions->getDescription($id) : null,
+                'value'       => $command ? $suggestions->getDescription($id) : null,
                 'rows'        => '3',
             ]);
         }

diff --git a/library/Director/Field/FormFieldSuggestion.php b/library/Director/Field/FormFieldSuggestion.php
@@ -20,7 +20,7 @@ class FormFieldSuggestion
     protected $descriptions = [];
     protected $booleans = [];
 
-    /** @var IcingaCommand */
+    /** @var ?IcingaCommand */
     protected $command;
 
     /** @var array */
@@ -29,7 +29,7 @@ class FormFieldSuggestion
     protected $fields = null;
 
     public function __construct(
-        IcingaCommand $command,
+        ?IcingaCommand $command,
         array $existingFields
     ) {
         $this->command = $command;
@@ -54,19 +54,23 @@ protected function prepareFields(): array
                 $this->blacklistedVars['$' . $m[1] . '$'] = $id;
             }
         }
-        foreach ($this->command->arguments() as $arg) {
-            if ($arg->argument_format === 'string') {
-                foreach (self::extractMacroNamesFromString($arg->argument_value) as $val) {
-                    $this->addSuggestion($val, $arg->description, $this->argumentVars);
+
+        if ($this->command) {
+            foreach ($this->command->arguments() as $arg) {
+                if ($arg->argument_format === 'string') {
+                    foreach (self::extractMacroNamesFromString($arg->argument_value) as $val) {
+                        $this->addSuggestion($val, $arg->description, $this->argumentVars);
+                    }
                 }
-            }
 
-            if (($arg->set_if_format === 'string' || $arg->set_if_format === null)
-                && $val = self::getMacroIfStringIsSingleMacro($arg->set_if)
-            ) {
-                $this->addSuggestion($val, $arg->description, $this->booleans);
+                if (($arg->set_if_format === 'string' || $arg->set_if_format === null)
+                    && $val = self::getMacroIfStringIsSingleMacro($arg->set_if)
+                ) {
+                    $this->addSuggestion($val, $arg->description, $this->booleans);
+                }
             }
         }
+
         asort($this->suggestedFields, SORT_NATURAL | SORT_FLAG_CASE);
         ksort($this->argumentVars);
         ksort($this->booleans);

diff --git a/library/Director/Test/BaseTestCase.php b/library/Director/Test/BaseTestCase.php
@@ -10,20 +10,15 @@
 use Icinga\Module\Director\Db\Migrations;
 use Icinga\Module\Director\Objects\IcingaObject;
 use Icinga\Module\Director\Objects\IcingaZone;
-use PHPUnit\Framework\TestCase;
+use Icinga\Test\BaseTestCase as IcingaBaseTestCase;
 
-abstract class BaseTestCase extends TestCase
+abstract class BaseTestCase extends IcingaBaseTestCase
 {
     private static $app;
 
     /** @var Db */
     private static $db;
 
-    public function setUp(): void
-    {
-        $this->app();
-    }
-
     protected function skipForMissingDb()
     {
         if ($this->hasDb()) {
@@ -81,11 +76,14 @@ protected static function getDb()
             self::$db = new Db($dbConfig);
             $migrations = new Migrations(self::$db);
             $migrations->applyPendingMigrations();
-            IcingaZone::create([
+            $zone = IcingaZone::create([
                 'object_name' => 'director-global',
                 'object_type' => 'external_object',
                 'is_global'   => 'y'
-            ])->store(self::$db);
+            ]);
+            if (! IcingaZone::exists($zone->getId(), self::$db)) {
+                $zone->store(self::$db);
+            }
         }
 
         return self::$db;

diff --git a/library/Director/Test/IcingaObjectTestCase.php b/library/Director/Test/IcingaObjectTestCase.php
@@ -88,5 +88,7 @@ public function tearDown(): void
                 $this->subject->delete();
             }
         }
+
+        parent::tearDown();
     }
 }
diff --git a/library/Director/Test/SyncTest.php b/library/Director/Test/SyncTest.php
@@ -31,6 +31,7 @@ abstract class SyncTest extends BaseTestCase
 
     public function setUp(): void
     {
+        parent::setUp();
         $this->source = ImportSource::create(array(
             'source_name'    => 'testimport',
             'provider_class' => 'Icinga\\Module\\Director\\Test\\ImportSourceDummy',
@@ -75,6 +76,7 @@ public function tearDown(): void
         // make sure cache is clean for other tests
         PrefetchCache::forget();
         DbObject::clearAllPrefetchCaches();
+        parent::tearDown();
     }
 
     /**

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -510,11 +510,6 @@ parameters:
 			count: 3
 			path: library/Director/ProvidedHook/IcingaDbCubeLinks.php
 
-		-
-			message: "#^Call to an undefined method Icinga\\\\Module\\\\Director\\\\Test\\\\TestSuiteUnit\\:\\:newTempfile\\(\\)\\.$#"
-			count: 1
-			path: library/Director/Test/TestSuiteUnit.php
-
 		-
 			message: "#^Access to an undefined property Zend_Controller_Action_HelperBroker\\:\\:\\$viewRenderer\\.$#"
 			count: 1

diff --git a/phpstan.neon b/phpstan.neon
@@ -25,6 +25,8 @@ parameters:
 
     excludePaths:
         - library/Director/CoreBeta
+        - test
+        - library/Director/Test
 
     universalObjectCratesClasses:
         - Icinga\Module\Director\Data\Db\DbObject

diff --git a/test/config/config.ini b/test/config/config.ini
diff --git a/test/php/library/Director/Form/IcingaObjectFieldFormTest.php b/test/php/library/Director/Form/IcingaObjectFieldFormTest.php
@@ -0,0 +1,151 @@
+<?php
+
+namespace Tests\Icinga\Module\Director\Field;
+
+use Icinga\Module\Director\Forms\IcingaObjectFieldForm;
+use Icinga\Module\Director\Objects\DirectorDatafield;
+use Icinga\Module\Director\Objects\IcingaHost;
+use Icinga\Module\Director\Test\BaseTestCase;
+use Icinga\Module\Director\Objects\IcingaCommand;
+
+class IcingaObjectFieldFormTest extends BaseTestCase
+{
+    /** @var string */
+    protected const COMMAND_NAME = 'icingaTestCommand';
+
+    /** @var string */
+    protected const DATAFIELD_NAME = 'dataFieldTest';
+
+    /** @var string */
+    protected const HOST_NAME = 'testHost';
+
+    /** @var ?DirectorDatafield */
+    protected $testDatafield = null;
+
+    /** @var ?IcingaCommand */
+    protected $testIcingaCommand = null;
+
+    /** @var IcingaHost */
+    private $testHost;
+
+    public function setUp(): void
+    {
+        parent::setUp();
+        if ($this->hasDb()) {
+            $db = $this->getDb();
+            $this->testDatafield = DirectorDatafield::create([
+                'varname'       => self::DATAFIELD_NAME,
+                'caption'       => 'Blah',
+                'description'   => '',
+                'datatype'      => 'Icinga\Module\Director\DataType\DataTypeArray',
+                'format'        => 'json'
+            ]);
+            $this->testDatafield->store($db);
+
+            $this->testIcingaCommand = IcingaCommand::create(
+                [
+                    'object_name' => self::COMMAND_NAME,
+                    'object_type' => 'object'
+                ],
+                $db
+            );
+            $this->testIcingaCommand->store($db);
+
+            $this->testHost = IcingaHost::create([
+                'object_name'  => self::HOST_NAME,
+                'object_type'  => 'object',
+                'display_name' => 'Whatever'
+            ], $this->getDb());
+        }
+    }
+
+    public function testFieldSuggestionsWithoutCheckCommand()
+    {
+        if ($this->skipForMissingDb()) {
+            return;
+        }
+
+        $db = $this->getDb();
+
+        $icingaHostFieldForm = (new IcingaObjectFieldForm())
+            ->setDb($db)
+            ->setIcingaObject($this->testHost);
+
+        $icingaHostFieldForm->setup();
+        /** @var array<string> $suggestions */
+        $suggestions = $icingaHostFieldForm->getElement('datafield_id')
+            ->getAttrib('options');
+
+        array_shift($suggestions);
+
+        $this->assertEquals(
+            [
+                'Other available fields' => [
+                    $this->testDatafield->get('id') => "Blah (dataFieldTest)"
+                ]
+            ],
+            $suggestions
+        );
+    }
+
+    public function testFieldSuggestionsWithCheckCommand()
+    {
+        if ($this->skipForMissingDb()) {
+            return;
+        }
+
+        $db = $this->getDb();
+
+        $this->testHost->check_command = self::COMMAND_NAME;
+        $icingaHostFieldForm = (new IcingaObjectFieldForm())
+            ->setDb($db)
+            ->setIcingaObject($this->testHost);
+
+        $icingaHostFieldForm->setup();
+
+        /** @var array<string> $suggestions */
+        $suggestions = $icingaHostFieldForm->getElement('datafield_id')
+            ->getAttrib('options');
+
+        array_shift($suggestions);
+        $this->assertEquals(
+            [
+                'Other available fields' => [
+                    $this->testDatafield->get('id') => "Blah (dataFieldTest)"
+                ]
+            ],
+            $suggestions
+        );
+    }
+
+    public function tearDown(): void
+    {
+        if ($this->hasDb()) {
+            $db = $this->getDb();
+            $this->deleteDatafields();
+
+            if (IcingaHost::exists(self::HOST_NAME, $db)) {
+                IcingaHost::load(self::HOST_NAME, $db)->delete();
+            }
+
+            if (IcingaCommand::exists(self::COMMAND_NAME, $db)) {
+                IcingaCommand::load(self::COMMAND_NAME, $db)->delete();
+            }
+        }
+
+        parent::tearDown();
+    }
+
+    protected function deleteDatafields()
+    {
+        $db = $this->getDb();
+        $dbAdapter = $db->getDbAdapter();
+
+        $query = $dbAdapter->select()
+            ->from('director_datafield')
+            ->where('varname = ?', self::DATAFIELD_NAME);
+        foreach (DirectorDatafield::loadAll($db, $query, 'id') as $datafield) {
+            $datafield->delete();
+        }
+    }
+}
diff --git a/test/php/library/Director/IcingaConfig/StateFilterTest.php b/test/php/library/Director/IcingaConfig/StateFilterTest.php
@@ -167,5 +167,7 @@ public function tearDown(): void
                 }
             }
         }
+
+        parent::tearDown();
     }
 }
diff --git a/test/php/library/Director/Objects/HostGroupMembershipResolverTest.php b/test/php/library/Director/Objects/HostGroupMembershipResolverTest.php
@@ -17,6 +17,7 @@ class HostGroupMembershipResolverTest extends BaseTestCase
 
     public function setUp(): void
     {
+        parent::setUp();
         IcingaTemplateRepository::clear();
     }
 

diff --git a/test/php/library/Director/Objects/IcingaCommandTest.php b/test/php/library/Director/Objects/IcingaCommandTest.php
@@ -212,5 +212,7 @@ public function tearDown(): void
         if (IcingaCommand::exists($this->testCommandName, $db)) {
             IcingaCommand::load($this->testCommandName, $db)->delete();
         }
+
+        parent::tearDown();
     }
 }
diff --git a/test/php/library/Director/Objects/IcingaHostTest.php b/test/php/library/Director/Objects/IcingaHostTest.php
@@ -747,7 +747,7 @@ public function tearDown(): void
                 }
             }
 
-            $kill = array('___TEST___zone');
+            $kill = ['___TEST___zone', '___TEST___zone2'];
             foreach ($kill as $name) {
                 if (IcingaZone::exists($name, $db)) {
                     IcingaZone::load($name, $db)->delete();
@@ -756,6 +756,8 @@ public function tearDown(): void
 
             $this->deleteDatafields();
         }
+
+        parent::tearDown();
     }
 
     protected function deleteDatafields()

diff --git a/test/php/library/Director/Objects/IcingaNotificationTest.php b/test/php/library/Director/Objects/IcingaNotificationTest.php
@@ -244,5 +244,7 @@ public function tearDown(): void
                 }
             }
         }
+
+        parent::tearDown();
     }
 }
diff --git a/test/php/library/Director/Objects/IcingaServiceSetTest.php b/test/php/library/Director/Objects/IcingaServiceSetTest.php
@@ -12,6 +12,7 @@ class IcingaServiceSetTest extends IcingaObjectTestCase
 
     public function setUp(): void
     {
+        parent::setUp();
         $this->assertNull($this->subject, 'subject must have been taken down before!');
 
         if ($this->hasDb()) {

diff --git a/test/php/library/Director/Objects/IcingaServiceTest.php b/test/php/library/Director/Objects/IcingaServiceTest.php
@@ -44,6 +44,7 @@ public function testFailsToStoreWithMissingLazyRelations()
 
     public function testAcceptsAssignRules()
     {
+        $this->expectNotToPerformAssertions();
         $service = $this->service();
         $service->object_type = 'apply';
         $service->assign_filter = 'host.address="127.*"';
@@ -287,5 +288,7 @@ public function tearDown(): void
                 }
             }
         }
+
+        parent::tearDown();
     }
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -88,5 +88,7 @@ public function tearDown(): void @@
                     $this->subject->delete();
                 }
             }
+            parent::tearDown();
         }
     }