Update for scrutinizer issues. pt. 1 (#65)

* Update for scrutinizer issues. pt. 1

* Add check for Synonyms being pushed/deleted in Elastic

* Ensure the set-up of synonym sets before testing

* The rule ID is not the same as the set ID

* Better testing of CRUD for synonyms

Author: Firesphere

composer.json

@@ -15,11 +15,6 @@
       "email": "github@casa-laguna.net",
       "homepage": "https://firesphere.dev",
       "role": "Lead developer"
-    },
-    {
-      "name": "Marco `Sheepy` Hermo",
-      "email": "marco@silverstripe.com",
-      "role": "Lead developer"
     }
   ],
   "require": {

src/Extensions/ElasticSynonymExtension.php

@@ -16,6 +16,7 @@
 use Firesphere\ElasticSearch\Models\SynonymSet;
 use Firesphere\ElasticSearch\Services\ElasticCoreService;
 use Firesphere\SearchBackend\Models\SearchSynonym;
+use Psr\Container\NotFoundExceptionInterface;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\ORM\DataExtension;
@@ -27,48 +28,42 @@
  */
 class ElasticSynonymExtension extends DataExtension
 {
-    public function updateCMSFields(FieldList $fields)
-    {
-        $fields->removeByName('SynonymSets');
-        parent::updateCMSFields($fields);
-    }
-
     /**
      * Add or update this synonym in Elastic
      *
-     * @throws ClientResponseException
-     * @throws ServerResponseException
-     * @throws MissingParameterException
+     * @throws NotFoundExceptionInterface
      */
     public function onAfterWrite()
     {
         $service = Injector::inst()->get(ElasticCoreService::class);
+        /** @var SearchSynonym|ElasticSynonymExtension $owner */
+        $owner = $this->owner;
         $syn = $service->getClient()->synonyms();
         /** @var SynonymSet $set */
         $set = SynonymSet::get()->first();
         $syn->putSynonymRule([
             'set_id'  => $set->Key,
-            'rule_id' => $this->owner->getModifiedID(),
+            'rule_id' => $owner->getModifiedID(),
             'body'    => [
-                'synonyms' => $this->owner->getCombinedSynonym()
+                'synonyms' => $owner->getCombinedSynonym()
             ]
         ]);
     }
 
     /**
      * When deleting a synonym from the CMS, delete it as a rule
      *
-     * @throws ClientResponseException
-     * @throws ServerResponseException
-     * @throws MissingParameterException
+     * @throws NotFoundExceptionInterface
      */
     public function onAfterDelete()
     {
         $service = Injector::inst()->get(ElasticCoreService::class);
         $syn = $service->getClient()->synonyms();
+        /** @var SearchSynonym $owner */
+        $owner = $this->owner;
         /** @var SynonymSet $set */
         $set = SynonymSet::get()->first();
-        $syn->deleteSynonymRule(['set_id' => $set->Key, 'rule_id' => $this->owner->getModifiedId()]);
+        $syn->deleteSynonymRule(['set_id' => $set->Key, 'rule_id' => $owner->getModifiedId()]);
         parent::onAfterDelete();
     }
 }

src/Tasks/ElasticIndexTask.php

@@ -88,6 +88,11 @@ class ElasticIndexTask extends BuildTask
      */
     protected $index;
 
+    /**
+     * @var int
+     */
+    protected $groups = 0;
+
     /**
      * ElasticIndexTask constructor. Sets up the document factory
      *
@@ -142,7 +147,7 @@ public function run($request)
         $time = gmdate('H:i:s', (time() - $start));
         $this->getLogger()->info(sprintf('Time taken: %s', $time));
 
-        return $groups;
+        $this->groups = $groups;
     }
 
     /**
@@ -217,15 +222,18 @@ public function setDebug(bool $debug, bool $force = false): self
         return $this;
     }
 
+    public function getGroups(): int
+    {
+        return $this->groups;
+    }
+
     /**
      * Index a single class for a given index. {@link static::indexClassForIndex()}
      *
      * @param bool $isGroup Is a specific group indexed
      * @param string $class Class to index
      * @param int $group Group to index
      * @return int|bool
-     * @throws HTTPException
-     * @throws ValidationException
      */
     private function indexClass(bool $isGroup, string $class, int $group)
     {

tests/unit/Extensions/ElasticSynonymExtensionTest.php

@@ -0,0 +1,74 @@
+<?php
+
+namespace Firesphere\ElasticSearch\Tests\unit\Extensions;
+
+use Elastic\Elasticsearch\Client;
+use Elastic\Elasticsearch\Exception\ClientResponseException;
+use Firesphere\ElasticSearch\Extensions\ElasticSynonymExtension;
+use Firesphere\ElasticSearch\Models\SynonymSet;
+use Firesphere\ElasticSearch\Services\ElasticCoreService;
+use Firesphere\ElasticSearch\Tasks\ElasticConfigureSynonymsTask;
+use Firesphere\SearchBackend\Models\SearchSynonym;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Core\Injector\Injector;
+use SilverStripe\Dev\SapphireTest;
+
+class ElasticSynonymExtensionTest extends SapphireTest
+{
+
+    public function testWriteUpdateDelete()
+    {
+        (new SynonymSet())->requireDefaultRecords();
+        $request = new HTTPRequest('GET', 'dev/tasks/ElasticSynonymTask');
+        $task = new ElasticConfigureSynonymsTask();
+
+        $task->run($request);
+
+        SynonymSet::singleton()->requireDefaultRecords();
+        /** @var SynonymSet $set */
+        $set = SynonymSet::get()->first();
+        /** @var SearchSynonym $synonym */
+        $synonym = SearchSynonym::create(['Keyword' => 'Simon', 'Synonym' => 'Firesphere']);
+        $extension = new ElasticSynonymExtension();
+        $extension->setOwner($synonym);
+
+        $synonym->write();
+
+        /** @var Client $client */
+        $client = Injector::inst()->get(ElasticCoreService::class)->getClient();
+        $synonymCheck = $client->synonyms()->getSynonymRule([
+            'set_id'  => $set->Key,
+            'rule_id' => $synonym->getModifiedID()
+        ]);
+
+        $check = $synonymCheck->asArray();
+
+        $this->assertEquals(['id' => $synonym->getModifiedID(), 'synonyms' => $synonym->getCombinedSynonym()], $check);
+
+        $synonym->Synonym = 'Firesphere,Hans';
+        $synonym->write();
+
+        $synonymCheck = $client->synonyms()->getSynonymRule([
+            'set_id'  => $set->Key,
+            'rule_id' => $synonym->getModifiedID()
+        ]);
+
+        $check = $synonymCheck->asArray();
+
+        $this->assertEquals(['id' => $synonym->getModifiedID(), 'synonyms' => $synonym->getCombinedSynonym()], $check);
+
+
+        $synonym->delete();
+
+        try {
+            $client->synonyms()->getSynonymRule([
+                'set_id'  => $set->Key,
+                'rule_id' => $synonym->getModifiedID()
+            ]);
+        } catch (ClientResponseException $e) {
+            $this->assertEquals(404, $e->getCode());
+        }
+
+
+    }
+}

tests/unit/Tasks/ElasticIndexTaskTest.php

@@ -33,14 +33,14 @@ public function testRun()
         $page->publishSingle();
         $task = new ElasticIndexTask();
         $request = new HTTPRequest('GET', 'dev/tasks/ElasticIndexTask');
-        $result = $task->run($request);
+        $task->run($request);
 
-        $this->assertGreaterThan(0, $result);
+        $this->assertGreaterThan(0, $task->getGroups());
         $this->assertinstanceOf(ElasticIndex::class, $task->getIndex());
         $request = new HTTPRequest('GET', 'dev/tasks/ElasticIndexTask', ['clear' => true]);
-        $result = $task->run($request);
+        $task->run($request);
 
-        $this->assertGreaterThan(0, $result);
+        $this->assertGreaterThan(0, $task->getGroups());
         $this->assertinstanceOf(ElasticIndex::class, $task->getIndex());
     }
 }