Merge pull request #57 from cjmellor/fix/foreign-key-constraint-on-qu…

…ery-builder

feat: Extract Foreign Key from Payload

.github/workflows/run-pest.yml

@@ -8,7 +8,7 @@ jobs:
     strategy:
       fail-fast: true
       matrix:
-        php: [ 8.1, 8.2 ]
+        php: [ 8.2, 8.3 ]
         stability: [ prefer-lowest, prefer-stable ]
 
     name: "PHP: v${{ matrix.php }} [${{ matrix.stability }}]"

.gitignore

@@ -1,6 +1,7 @@
 .idea
 .php_cs
 .php_cs.cache
+.phpunit.cache
 .phpunit.result.cache
 build
 composer.lock

README.md

@@ -49,7 +49,7 @@ The config allows you to change the polymorphic pivot name. It should end with `
 ## Usage
 
 > The package utilises Enums, so both PHP 8.1 and Laravel 9 must be used.
-> 
+>
 > **Note** This package does not approve/deny the data for you, it just stores the new/amended data into the database. It is up to you to decide how you implement a function to approve or deny the Model.
 
 Add the `MustBeApproved` trait to your Model and now the data will be stored in an `approvals` table, ready for you to approve or deny.
@@ -87,17 +87,54 @@ Here is some info about the columns in the `approvals` table:
 
 `audited_at` => The ID of the User who set the state
 
+`foreign_key` => A foreign key to the Model that the approval is for
+
+### Bypassing Approval Check
+
 If you want to check if the Model data will be bypassed, use the `isApprovalBypassed` method.
 
 ```php
 return $model->isApprovalBypassed();
 ```
 
+### Foreign Keys for New Models
+
+> [!NOTE]
+> It is recommended to read the below section on how foreign keys work in this package.
+
+> [!IMPORTANT]
+> By default, the foreign key will always be `user_id` because this is the most common foreign key used in Laravel.
+
+If you create a new Model directly via the Model, e.g.
+
+```php
+Post::create(['title' => 'Some Title']);
+```
+
+be sure to also add the foreign key to the Model, e.g.
+
+```php
+Post::create(['title' => 'Some Title', 'user_id' => 1]);
+```
+
+Now when the Model is sent for approval, the foreign key will be stored in the `foreign_key` column.
+
+### Customise the Foreign Key
+
+Your Model might not use the `user_id` as the foreign key, so you can customise it by adding this method to your Model:
+
+```php
+public function getApprovalForeignKeyName(): string
+{
+    return 'author_id';
+}
+```
+
 ## Scopes
 
 The package comes with some helper methods for the Builder, utilising a custom scope - `ApprovalStateScope`
 
-By default, all queries to the `approvals` table will return all the Models' no matter the state. 
+By default, all queries to the `approvals` table will return all the Models' no matter the state.
 
 There are three methods to help you retrieve the state of the Approval.
 
@@ -196,13 +233,15 @@ When a Model has been rolled back, a `ModelRolledBack` event will be fired with
 public Model $approval,
 public Authenticatable|null $user,
 ````
+
 ## Disable Approvals
 
 If you don't want Model data to be approved, you can bypass it with the `withoutApproval` method.
 
 ```php
 $model->withoutApproval()->update(['title' => 'Some Title']);
 ```
+
 ## Specify Approvable Attributes
 
 By default, all attributes of the model will go through the approval process, however if you only wish certain attributes to go through this process, you can specify them using the `approvalAttributes` property in your model.

UPGRADE.md

@@ -1,5 +1,29 @@
 # Upgrade Guide
 
+## v1.4.3 -> v1.5.0
+
+> [!IMPORTANT]
+> v1.5.0 will now only work with PHP versions >= 8.2. If you are using a version of PHP < 8.2, please use v1.4.5
+
+A new migration needs to be run. Run:
+
+```shell
+php artisan vendor:publish
+```
+then
+
+```shell
+php artisan migrate
+```
+
+or you add the migration manually:
+
+```php
+Schema::table('approvals', function (Blueprint $table) {
+    $table->unsignedBigInteger('foreign_key')->nullable()->after('original_data');
+});
+```
+
 ## v1.4.2 -> 1.4.3
 
 If you wish to audit which User set the state for the Model, you need to publish and run a new Migration.

composer.json

@@ -16,25 +16,27 @@
         }
     ],
     "require": {
-        "php": "^8.1",
-        "illuminate/contracts": "^9.0|^10.0|^11.0",
-        "spatie/laravel-package-tools": "^1.14.0"
+        "php": "^8.2",
+        "illuminate/contracts": "^10.0|^11.0"
     },
     "require-dev": {
         "laravel/pint": "^1.0",
         "nunomaduro/collision": "^7.0|^8.0",
         "orchestra/testbench": "^7.0|^8.0|^9.0",
         "pestphp/pest": "^2.0",
-        "pestphp/pest-plugin-type-coverage": "^2.0"
+        "pestphp/pest-plugin-arch": "^2.0",
+        "pestphp/pest-plugin-laravel": "^2.0",
+        "pestphp/pest-plugin-type-coverage": "^2.0",
+        "spatie/laravel-package-tools": "^1.14.0"
     },
     "autoload": {
         "psr-4": {
-            "Cjmellor\\Approval\\": "src"
+            "Cjmellor\\Approval\\": "src/"
         }
     },
     "autoload-dev": {
         "psr-4": {
-            "Cjmellor\\Approval\\Tests\\": "tests"
+            "Cjmellor\\Approval\\Tests\\": "tests/"
         }
     },
     "scripts": {

database/migrations/2024_03_16_173148_add_foreign_id_column_to_approvals_table.php

@@ -0,0 +1,22 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+return new class extends Migration
+{
+    public function up(): void
+    {
+        Schema::table('approvals', function (Blueprint $table) {
+            $table->unsignedBigInteger('foreign_key')->nullable()->after('original_data');
+        });
+    }
+
+    public function down(): void
+    {
+        Schema::table('approvals', function (Blueprint $table) {
+            $table->dropColumn('foreign_key');
+        });
+    }
+};

src/ApprovalServiceProvider.php

@@ -16,6 +16,7 @@ public function configurePackage(Package $package): void
                 '2022_02_12_195950_create_approvals_table',
                 '2023_10_09_204810_add_rolled_back_at_column_to_approvals_table',
                 '2023_11_17_002135_add_audited_by_column_to_approvals_table',
+                '2024_03_16_173148_add_foreign_id_column_to_approvals_table',
             ]);
     }
 }

src/Concerns/MustBeApproved.php

@@ -12,16 +12,21 @@ trait MustBeApproved
 
     public static function bootMustBeApproved(): void
     {
-        static::creating(callback: fn ($model) => static::insertApprovalRequest($model));
-        static::updating(callback: fn ($model) => static::insertApprovalRequest($model));
+        static::creating(callback: fn ($model): ?bool => static::insertApprovalRequest($model));
+        static::updating(callback: fn ($model): ?bool => static::insertApprovalRequest($model));
     }
 
     /**
      * Create an Approval request before committing to the database.
      */
-    protected static function insertApprovalRequest($model)
+    protected static function insertApprovalRequest($model): ?bool
     {
         $filteredDirty = $model->getDirtyAttributes();
+        $foreignKey = $model->getApprovalForeignKeyName();
+        $foreignKeyValue = $filteredDirty[$foreignKey] ?? null;
+
+        // Remove the foreign key from the dirty attributes
+        unset($filteredDirty[$foreignKey]);
 
         foreach ($filteredDirty as $key => $value) {
             if (isset($model->casts[$key]) && $model->casts[$key] === 'json') {
@@ -30,40 +35,37 @@ protected static function insertApprovalRequest($model)
         }
 
         if ($model->isApprovalBypassed() || empty($filteredDirty)) {
-            return;
+            return null;
         }
 
-        $noNeedToProceed = true;
         $approvalAttributes = $model->getApprovalAttributes();
 
         if (! empty($approvalAttributes)) {
-            $noNeedToProceed = collect($model->getDirty())
+            $noApprovalNeeded = collect($model->getDirty())
                 ->except($approvalAttributes)
-                ->isEmpty();
-
-            if (! $noNeedToProceed) {
-                $noApprovalNeeded = collect($model->getDirty())
-                    ->except($approvalAttributes)
-                    ->toArray();
+                ->toArray();
 
+            if (! empty($noApprovalNeeded)) {
                 $model->discardChanges();
-
                 $model->forceFill($noApprovalNeeded);
             }
         }
 
-        if (self::approvalModelExists($model) && $noNeedToProceed) {
+        if (self::approvalModelExists($model) && empty($noApprovalNeeded)) {
             return false;
         }
 
         $model->approvals()->create([
             'new_data' => $filteredDirty,
             'original_data' => $model->getOriginalMatchingChanges(),
+            'foreign_key' => $foreignKeyValue,
         ]);
 
-        if ($noNeedToProceed) {
+        if (empty($noApprovalNeeded)) {
             return false;
         }
+
+        return true;
     }
 
     /**
@@ -85,6 +87,14 @@ public function getApprovalAttributes(): array
         return $this->approvalAttributes ?? [];
     }
 
+    /**
+     * Get the name of the foreign key for the model.
+     */
+    public function getApprovalForeignKeyName(): string
+    {
+        return 'user_id';
+    }
+
     /**
      * Check is the approval can be bypassed.
      */

tests/Feature/MustBeApprovedTraitTest.php

@@ -25,7 +25,6 @@
         'new_data' => json_encode([
             'name' => 'Chris',
             'meta' => 'red',
-            'user_id' => null,
         ]),
         'original_data' => json_encode([]),
     ])
@@ -139,7 +138,8 @@
 });
 
 test(description: 'an approvals model is created when a model is created with MustBeApproved trait set and has the approvalInclude array set', closure: function () {
-    $model = new class extends Model {
+    $model = new class extends Model
+    {
         use MustBeApproved;
 
         protected $table = 'fake_models';
@@ -174,11 +174,12 @@
         $table->id();
         $table->string('name')->nullable();
         $table->string('meta')->nullable();
-
         $table->json('data')->nullable();
+        $table->foreignId('user_id')->nullable();
     });
 
-    $model = new class extends Model {
+    $model = new class extends Model
+    {
         use MustBeApproved;
 
         protected $table = 'fake_models_with_array';
@@ -198,7 +199,10 @@
 
     // check if the data is stored correctly in the approval table
     $this->assertDatabaseHas(table: Approval::class, data: [
-        'new_data' => json_encode(['name' => 'Neo', 'data' => json_encode(['foo', 'bar'])]),
+        'new_data' => json_encode([
+            'name' => 'Neo',
+            'data' => json_encode(['foo', 'bar']),
+        ]),
         'original_data' => json_encode([]),
     ]);
 
@@ -230,10 +234,12 @@
         $table->string('title');
         $table->string('content');
         $table->json('config');
+        $table->foreignId('user_id')->nullable();
         $table->timestamps();
     });
 
-    $model = new class extends Model {
+    $model = new class extends Model
+    {
         use MustBeApproved;
 
         protected $table = 'posts';
@@ -282,3 +288,32 @@
     expect($modelFromDatabase->config)
         ->toBe(['checked' => true]);
 });
+
+test('the foreign key is extracted from the payload and stored in a separate column', function () {
+    $model = new class extends Model
+    {
+        use MustBeApproved;
+
+        protected $table = 'fake_models';
+
+        protected $guarded = [];
+
+        public $timestamps = false;
+
+        public function getApprovalForeignKeyName(): string
+        {
+            return 'user_id';
+        }
+    };
+
+    // create a model
+    $model->create([
+        'name' => 'Neo',
+    ]);
+
+    $this->assertDatabaseHas(table: Approval::class, data: [
+        'new_data' => json_encode(['name' => 'Neo']),
+        'original_data' => json_encode([]),
+        'foreign_key' => null,
+    ]);
+});

tests/Pest.php

@@ -18,7 +18,6 @@
         $this->fakeModelData = [
             'name' => 'Chris',
             'meta' => 'red',
-            'user_id' => auth()->id(),
         ];
     })
     ->in(__DIR__);