From c81d9b7e8ad05118589a6be9b1d5d44520b1041e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= <mvorisek@mvorisek.cz>
Date: Wed, 16 Aug 2023 01:07:45 +0200
Subject: [PATCH 1/4] improve UA tests

---
 src/Model/UserAction.php |  5 +---
 tests/UserActionTest.php | 62 ++++++++++++++++++----------------------
 2 files changed, 29 insertions(+), 38 deletions(-)

diff --git a/src/Model/UserAction.php b/src/Model/UserAction.php
index 2922424ad..bdc7163d8 100644
--- a/src/Model/UserAction.php
+++ b/src/Model/UserAction.php
@@ -157,7 +157,7 @@ protected function validateBeforeExecute(): void
         }
 
         // Verify that model fields wouldn't be too dirty
-        if (is_array($this->fields)) {
+        if (!is_bool($this->fields)) {
             $tooDirty = array_diff(array_keys($this->getEntity()->getDirtyRef()), $this->fields);
 
             if ($tooDirty) {
@@ -166,9 +166,6 @@ protected function validateBeforeExecute(): void
                     ->addMoreInfo('dirty', array_keys($this->getEntity()->getDirtyRef()))
                     ->addMoreInfo('permitted', $this->fields);
             }
-        } elseif (!is_bool($this->fields)) { // @phpstan-ignore-line
-            throw (new Exception('Argument `fields` for the user action must be either array or boolean'))
-                ->addMoreInfo('fields', $this->fields);
         }
 
         // Verify some records scope cases
diff --git a/tests/UserActionTest.php b/tests/UserActionTest.php
index 41318ac19..b12200686 100644
--- a/tests/UserActionTest.php
+++ b/tests/UserActionTest.php
@@ -4,7 +4,7 @@
 
 namespace Atk4\Data\Tests;
 
-use Atk4\Core\Exception as CoreException;
+use Atk4\Data\Exception;
 use Atk4\Data\Model;
 use Atk4\Data\Persistence;
 use Atk4\Data\Schema\TestCase;
@@ -108,6 +108,17 @@ public function testCustomSeedClass(): void
         self::assertSame($customClass, get_class($client->getUserAction('foo')));
     }
 
+    public function testExecuteUndefinedMethodException(): void
+    {
+        $client = new UaClient($this->pers);
+        $client->addUserAction('new_client');
+        $client = $client->load(1);
+
+        $this->expectException(\Error::class);
+        $this->expectExceptionMessage('Call to undefined method');
+        $client->executeUserAction('new_client');
+    }
+
     public function testPreview(): void
     {
         $client = new UaClient($this->pers);
@@ -116,12 +127,13 @@ public function testPreview(): void
         });
 
         $client = $client->load(1);
+
         self::assertSame('John', $client->getUserAction('say_name')->execute());
 
-        $client->getUserAction('say_name')->preview = function (UaClient $m, string $arg) {
+        $client->getUserAction('say_name')->preview = function (UaClient $m) {
             return 'will say ' . $m->get('name');
         };
-        self::assertSame('will say John', $client->getUserAction('say_name')->preview('x'));
+        self::assertSame('will say John', $client->getUserAction('say_name')->preview());
 
         $client->getModel()->addUserAction('also_backup', ['callback' => 'backupClients']);
         self::assertSame('backs up all clients', $client->getUserAction('also_backup')->execute());
@@ -132,55 +144,49 @@ public function testPreview(): void
         self::assertSame('Also Backup UaClient', $client->getUserAction('also_backup')->getDescription());
     }
 
-    public function testAppliesTo1(): void
+    public function testAppliesToSingleRecordNotLoadedException(): void
     {
         $client = new UaClient($this->pers);
         $client = $client->createEntity();
 
+        $this->expectException(Exception::class);
         $this->expectExceptionMessage('load existing record');
         $client->executeUserAction('sendReminder');
     }
 
-    public function testAppliesTo2(): void
+    public function testAppliesToNoRecordsLoadedRecordException(): void
     {
         $client = new UaClient($this->pers);
         $client->addUserAction('new_client', ['appliesTo' => Model\UserAction::APPLIES_TO_NO_RECORDS]);
         $client = $client->load(1);
 
+        $this->expectException(Exception::class);
         $this->expectExceptionMessage('can be executed on non-existing record');
         $client->executeUserAction('new_client');
     }
 
-    public function testAppliesTo3(): void
-    {
-        $client = new UaClient($this->pers);
-        $client->addUserAction('new_client', ['appliesTo' => Model\UserAction::APPLIES_TO_NO_RECORDS, 'atomic' => false]);
-        $client = $client->createEntity();
-
-        $this->expectExceptionMessage('undefined method');
-        $client->executeUserAction('new_client');
-    }
-
-    public function testException1(): void
+    public function testNotDefinedException(): void
     {
         $client = new UaClient($this->pers);
 
-        $this->expectException(CoreException::class);
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('User action is not defined');
         $client->getUserAction('non_existent_action');
     }
 
-    public function testDisabled1(): void
+    public function testDisabledBoolException(): void
     {
         $client = new UaClient($this->pers);
         $client = $client->load(1);
 
         $client->getUserAction('sendReminder')->enabled = false;
 
+        $this->expectException(Exception::class);
         $this->expectExceptionMessage('disabled');
         $client->getUserAction('sendReminder')->execute();
     }
 
-    public function testDisabled2(): void
+    public function testDisabledClosureException(): void
     {
         $client = new UaClient($this->pers);
         $client = $client->load(1);
@@ -194,6 +200,7 @@ public function testDisabled2(): void
             return false;
         };
 
+        $this->expectException(Exception::class);
         $this->expectExceptionMessage('disabled');
         $client->getUserAction('sendReminder')->execute();
     }
@@ -211,7 +218,7 @@ public function testFields(): void
         self::assertSame('Peter', $client->get('name'));
     }
 
-    public function testFieldsTooDirty1(): void
+    public function testFieldsTooDirtyException(): void
     {
         $client = new UaClient($this->pers);
         $client->addUserAction('change_details', ['callback' => 'save', 'fields' => ['name']]);
@@ -222,24 +229,11 @@ public function testFieldsTooDirty1(): void
         $client->set('name', 'Peter');
         $client->set('reminder_sent', true);
 
+        $this->expectException(Exception::class);
         $this->expectExceptionMessage('dirty fields');
         $client->getUserAction('change_details')->execute();
     }
 
-    public function testFieldsIncorrect(): void
-    {
-        $client = new UaClient($this->pers);
-        $client->addUserAction('change_details', ['callback' => 'save', 'fields' => 'whops_forgot_brackets']);
-
-        $client = $client->load(1);
-
-        self::assertNotSame('Peter', $client->get('name'));
-        $client->set('name', 'Peter');
-
-        $this->expectExceptionMessage('must be either array or boolean');
-        $client->getUserAction('change_details')->execute();
-    }
-
     public function testConfirmation(): void
     {
         $client = new UaClient($this->pers);

From 54ca9ec84e5d1044ad9a9e18548921ebb523a0b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= <mvorisek@mvorisek.cz>
Date: Wed, 16 Aug 2023 01:23:36 +0200
Subject: [PATCH 2/4] improve UA exception messages

---
 src/Model/UserAction.php | 20 +++++++++-----------
 tests/UserActionTest.php | 19 ++++++++++++++-----
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/src/Model/UserAction.php b/src/Model/UserAction.php
index bdc7163d8..dd0f588fc 100644
--- a/src/Model/UserAction.php
+++ b/src/Model/UserAction.php
@@ -153,33 +153,31 @@ public function execute(...$args)
     protected function validateBeforeExecute(): void
     {
         if ($this->enabled === false || ($this->enabled instanceof \Closure && ($this->enabled)($this->getEntity()) === false)) {
-            throw new Exception('This action is disabled');
+            throw new Exception('User action is disabled');
         }
 
-        // Verify that model fields wouldn't be too dirty
         if (!is_bool($this->fields)) {
-            $tooDirty = array_diff(array_keys($this->getEntity()->getDirtyRef()), $this->fields);
+            $dirtyFields = array_keys($this->getEntity()->getDirtyRef());
+            $tooDirtyFields = array_diff($dirtyFields, $this->fields);
 
-            if ($tooDirty) {
-                throw (new Exception('Calling user action on a Model with dirty fields that are not allowed by this action'))
-                    ->addMoreInfo('too_dirty', $tooDirty)
-                    ->addMoreInfo('dirty', array_keys($this->getEntity()->getDirtyRef()))
-                    ->addMoreInfo('permitted', $this->fields);
+            if ($tooDirtyFields !== []) {
+                throw (new Exception('User action cannot be executed as unrelated fields are dirty'))
+                    ->addMoreInfo('tooDirtyFields', $tooDirtyFields)
+                    ->addMoreInfo('otherDirtyFields', array_diff($dirtyFields, $tooDirtyFields));
             }
         }
 
-        // Verify some records scope cases
         switch ($this->appliesTo) {
             case self::APPLIES_TO_NO_RECORDS:
                 if ($this->getEntity()->isLoaded()) {
-                    throw (new Exception('This user action can be executed on non-existing record only'))
+                    throw (new Exception('User action can be executed on new entity only'))
                         ->addMoreInfo('id', $this->getEntity()->getId());
                 }
 
                 break;
             case self::APPLIES_TO_SINGLE_RECORD:
                 if (!$this->getEntity()->isLoaded()) {
-                    throw new Exception('This user action requires you to load existing record first');
+                    throw new Exception('User action can be executed on loaded entity only');
                 }
 
                 break;
diff --git a/tests/UserActionTest.php b/tests/UserActionTest.php
index b12200686..c361b682d 100644
--- a/tests/UserActionTest.php
+++ b/tests/UserActionTest.php
@@ -144,13 +144,22 @@ public function testPreview(): void
         self::assertSame('Also Backup UaClient', $client->getUserAction('also_backup')->getDescription());
     }
 
+    public function testAppliesToSingleRecordNotEntityException(): void
+    {
+        $client = new UaClient($this->pers);
+
+        $this->expectException(\TypeError::class);
+        $this->expectExceptionMessage('Expected entity, but instance is a model');
+        $client->executeUserAction('sendReminder');
+    }
+
     public function testAppliesToSingleRecordNotLoadedException(): void
     {
         $client = new UaClient($this->pers);
         $client = $client->createEntity();
 
         $this->expectException(Exception::class);
-        $this->expectExceptionMessage('load existing record');
+        $this->expectExceptionMessage('User action can be executed on loaded entity only');
         $client->executeUserAction('sendReminder');
     }
 
@@ -161,7 +170,7 @@ public function testAppliesToNoRecordsLoadedRecordException(): void
         $client = $client->load(1);
 
         $this->expectException(Exception::class);
-        $this->expectExceptionMessage('can be executed on non-existing record');
+        $this->expectExceptionMessage('User action can be executed on new entity only');
         $client->executeUserAction('new_client');
     }
 
@@ -182,7 +191,7 @@ public function testDisabledBoolException(): void
         $client->getUserAction('sendReminder')->enabled = false;
 
         $this->expectException(Exception::class);
-        $this->expectExceptionMessage('disabled');
+        $this->expectExceptionMessage('User action is disabled');
         $client->getUserAction('sendReminder')->execute();
     }
 
@@ -201,7 +210,7 @@ public function testDisabledClosureException(): void
         };
 
         $this->expectException(Exception::class);
-        $this->expectExceptionMessage('disabled');
+        $this->expectExceptionMessage('User action is disabled');
         $client->getUserAction('sendReminder')->execute();
     }
 
@@ -230,7 +239,7 @@ public function testFieldsTooDirtyException(): void
         $client->set('reminder_sent', true);
 
         $this->expectException(Exception::class);
-        $this->expectExceptionMessage('dirty fields');
+        $this->expectExceptionMessage('User action cannot be executed as unrelated fields are dirty');
         $client->getUserAction('change_details')->execute();
     }
 

From 84ef169542ad744c7dd93c859b41fccdd7c1cf05 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= <mvorisek@mvorisek.cz>
Date: Wed, 16 Aug 2023 01:24:13 +0200
Subject: [PATCH 3/4] unify UA execute/preview

---
 src/Model/UserAction.php | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/src/Model/UserAction.php b/src/Model/UserAction.php
index dd0f588fc..d52587fd6 100644
--- a/src/Model/UserAction.php
+++ b/src/Model/UserAction.php
@@ -126,12 +126,13 @@ public function getActionForEntity(Model $entity): self
      */
     public function execute(...$args)
     {
+        $passEntity = false;
         if ($this->callback === null) {
             $fx = \Closure::fromCallable([$this->getEntity(), $this->shortName]);
         } elseif (is_string($this->callback)) {
             $fx = \Closure::fromCallable([$this->getEntity(), $this->callback]);
         } else {
-            array_unshift($args, $this->getEntity());
+            $passEntity = true;
             $fx = $this->callback;
         }
 
@@ -140,6 +141,10 @@ public function execute(...$args)
         try {
             $this->validateBeforeExecute();
 
+            if ($passEntity) {
+                array_unshift($args, $this->getEntity());
+            }
+
             return $this->atomic === false
                 ? $fx(...$args)
                 : $this->getModel()->atomic(static fn () => $fx(...$args));
@@ -193,14 +198,27 @@ protected function validateBeforeExecute(): void
      */
     public function preview(...$args)
     {
+        $passEntity = false;
         if (is_string($this->preview)) {
             $fx = \Closure::fromCallable([$this->getEntity(), $this->preview]);
         } else {
-            array_unshift($args, $this->getEntity());
+            $passEntity = true;
             $fx = $this->preview;
         }
 
-        return $fx(...$args);
+        try {
+            $this->validateBeforeExecute();
+
+            if ($passEntity) {
+                array_unshift($args, $this->getEntity());
+            }
+
+            return $fx(...$args);
+        } catch (CoreException $e) {
+            $e->addMoreInfo('action', $this);
+
+            throw $e;
+        }
     }
 
     /**

From e1174252302508fd65529c3473c070b14aaa23b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= <mvorisek@mvorisek.cz>
Date: Wed, 16 Aug 2023 01:55:01 +0200
Subject: [PATCH 4/4] add full UA support for model

---
 src/Model/UserAction.php | 58 ++++++++++++++++++++++------------------
 tests/UserActionTest.php | 12 ++++++++-
 2 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/src/Model/UserAction.php b/src/Model/UserAction.php
index d52587fd6..c79af9253 100644
--- a/src/Model/UserAction.php
+++ b/src/Model/UserAction.php
@@ -32,15 +32,15 @@ class UserAction
     public const APPLIES_TO_MULTIPLE_RECORDS = 'multiple'; // e.g. delete
     public const APPLIES_TO_ALL_RECORDS = 'all'; // e.g. truncate
 
-    /** @var string by default action is for a single record */
-    public $appliesTo = self::APPLIES_TO_SINGLE_RECORD;
-
     /** Defining action modifier */
     public const MODIFIER_CREATE = 'create'; // create new record(s)
     public const MODIFIER_UPDATE = 'update'; // update existing record(s)
     public const MODIFIER_DELETE = 'delete'; // delete record(s)
     public const MODIFIER_READ = 'read'; // just read, does not modify record(s)
 
+    /** @var string by default action is for a single record */
+    public $appliesTo = self::APPLIES_TO_SINGLE_RECORD;
+
     /** @var string How this action interact with record */
     public $modifier;
 
@@ -74,26 +74,28 @@ class UserAction
     /** @var bool Atomic action will automatically begin transaction before and commit it after completing. */
     public $atomic = true;
 
+    private function _getOwner(): Model
+    {
+        return $this->getOwner(); // @phpstan-ignore-line;
+    }
+
     public function isOwnerEntity(): bool
     {
-        /** @var Model */
-        $owner = $this->getOwner(); // @phpstan-ignore-line
+        $owner = $this->_getOwner();
 
         return $owner->isEntity();
     }
 
     public function getModel(): Model
     {
-        /** @var Model */
-        $owner = $this->getOwner(); // @phpstan-ignore-line
+        $owner = $this->_getOwner();
 
         return $owner->getModel(true);
     }
 
     public function getEntity(): Model
     {
-        /** @var Model */
-        $owner = $this->getOwner(); // @phpstan-ignore-line
+        $owner = $this->_getOwner();
         $owner->assertIsEntity();
 
         return $owner;
@@ -104,8 +106,7 @@ public function getEntity(): Model
      */
     public function getActionForEntity(Model $entity): self
     {
-        /** @var Model */
-        $owner = $this->getOwner(); // @phpstan-ignore-line
+        $owner = $this->_getOwner();
 
         $entity->assertIsEntity($owner);
         foreach ($owner->getUserActions() as $name => $action) {
@@ -126,13 +127,13 @@ public function getActionForEntity(Model $entity): self
      */
     public function execute(...$args)
     {
-        $passEntity = false;
+        $passOwner = false;
         if ($this->callback === null) {
-            $fx = \Closure::fromCallable([$this->getEntity(), $this->shortName]);
+            $fx = \Closure::fromCallable([$this->_getOwner(), $this->shortName]);
         } elseif (is_string($this->callback)) {
-            $fx = \Closure::fromCallable([$this->getEntity(), $this->callback]);
+            $fx = \Closure::fromCallable([$this->_getOwner(), $this->callback]);
         } else {
-            $passEntity = true;
+            $passOwner = true;
             $fx = $this->callback;
         }
 
@@ -141,13 +142,13 @@ public function execute(...$args)
         try {
             $this->validateBeforeExecute();
 
-            if ($passEntity) {
-                array_unshift($args, $this->getEntity());
+            if ($passOwner) {
+                array_unshift($args, $this->_getOwner());
             }
 
             return $this->atomic === false
                 ? $fx(...$args)
-                : $this->getModel()->atomic(static fn () => $fx(...$args));
+                : $this->_getOwner()->atomic(static fn () => $fx(...$args));
         } catch (CoreException $e) {
             $e->addMoreInfo('action', $this);
 
@@ -157,11 +158,11 @@ public function execute(...$args)
 
     protected function validateBeforeExecute(): void
     {
-        if ($this->enabled === false || ($this->enabled instanceof \Closure && ($this->enabled)($this->getEntity()) === false)) {
+        if ($this->enabled === false || ($this->enabled instanceof \Closure && ($this->enabled)($this->_getOwner()) === false)) {
             throw new Exception('User action is disabled');
         }
 
-        if (!is_bool($this->fields)) {
+        if (!is_bool($this->fields) && $this->fields !== []) {
             $dirtyFields = array_keys($this->getEntity()->getDirtyRef());
             $tooDirtyFields = array_diff($dirtyFields, $this->fields);
 
@@ -185,6 +186,11 @@ protected function validateBeforeExecute(): void
                     throw new Exception('User action can be executed on loaded entity only');
                 }
 
+                break;
+            case self::APPLIES_TO_MULTIPLE_RECORDS:
+            case self::APPLIES_TO_ALL_RECORDS:
+                $this->_getOwner()->assertIsModel();
+
                 break;
         }
     }
@@ -198,19 +204,19 @@ protected function validateBeforeExecute(): void
      */
     public function preview(...$args)
     {
-        $passEntity = false;
+        $passOwner = false;
         if (is_string($this->preview)) {
-            $fx = \Closure::fromCallable([$this->getEntity(), $this->preview]);
+            $fx = \Closure::fromCallable([$this->_getOwner(), $this->preview]);
         } else {
-            $passEntity = true;
+            $passOwner = true;
             $fx = $this->preview;
         }
 
         try {
             $this->validateBeforeExecute();
 
-            if ($passEntity) {
-                array_unshift($args, $this->getEntity());
+            if ($passOwner) {
+                array_unshift($args, $this->_getOwner());
             }
 
             return $fx(...$args);
@@ -245,7 +251,7 @@ public function getConfirmation()
         } elseif ($this->confirmation === true) {
             $confirmation = 'Are you sure you wish to execute '
                 . $this->getCaption()
-                . ($this->getEntity()->getTitle() ? ' using ' . $this->getEntity()->getTitle() : '')
+                . ($this->isOwnerEntity() && $this->getEntity()->getTitle() ? ' using ' . $this->getEntity()->getTitle() : '')
                 . '?';
 
             return $confirmation;
diff --git a/tests/UserActionTest.php b/tests/UserActionTest.php
index c361b682d..a7e95af3a 100644
--- a/tests/UserActionTest.php
+++ b/tests/UserActionTest.php
@@ -85,7 +85,7 @@ public function testBasic(): void
         $client->unload();
 
         // test system action
-        $act2 = $client->getUserAction('backupClients');
+        $act2 = $client->getModel()->getUserAction('backupClients');
 
         // action takes no arguments. If it would, we should be able to find info about those
         self::assertSame([], $act2->args);
@@ -153,6 +153,16 @@ public function testAppliesToSingleRecordNotEntityException(): void
         $client->executeUserAction('sendReminder');
     }
 
+    public function testAppliesToAllRecordsEntityException(): void
+    {
+        $client = new UaClient($this->pers);
+        $client = $client->load(1);
+
+        $this->expectException(\TypeError::class);
+        $this->expectExceptionMessage('Expected model, but instance is an entity');
+        $client->executeUserAction('backupClients');
+    }
+
     public function testAppliesToSingleRecordNotLoadedException(): void
     {
         $client = new UaClient($this->pers);