From 258e593b17def105586dbced786e6b780a76d49b Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Sat, 16 Mar 2024 17:26:09 +0000 Subject: [PATCH 01/11] fix: Add foreign id when creating a Model not via a relationship --- src/Concerns/MustBeApproved.php | 32 ++++++++++++----------- tests/Feature/MustBeApprovedTraitTest.php | 19 ++++++++++---- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/src/Concerns/MustBeApproved.php b/src/Concerns/MustBeApproved.php index 89b4244..be6ba07 100644 --- a/src/Concerns/MustBeApproved.php +++ b/src/Concerns/MustBeApproved.php @@ -12,14 +12,20 @@ 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: function ($model): ?bool { + if (! $model->user_id) { + $model->user_id = auth()->id(); + } + + return 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(); @@ -30,29 +36,23 @@ 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; } @@ -61,9 +61,11 @@ protected static function insertApprovalRequest($model) 'original_data' => $model->getOriginalMatchingChanges(), ]); - if ($noNeedToProceed) { + if (empty($noApprovalNeeded)) { return false; } + + return true; } /** diff --git a/tests/Feature/MustBeApprovedTraitTest.php b/tests/Feature/MustBeApprovedTraitTest.php index 6baa288..b14291f 100644 --- a/tests/Feature/MustBeApprovedTraitTest.php +++ b/tests/Feature/MustBeApprovedTraitTest.php @@ -139,7 +139,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 +175,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 +200,11 @@ // 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']), + 'user_id' => null, + ]), 'original_data' => json_encode([]), ]); @@ -230,10 +236,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'; @@ -256,6 +264,7 @@ 'title' => 'My First Post', 'content' => 'This is my first post', 'config' => ['checked' => true], + 'user_id' => null, ]), 'original_data' => json_encode([]), ]); From 97e6cae5d949b9094544da66d17b83d91d012da5 Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Sat, 16 Mar 2024 18:15:41 +0000 Subject: [PATCH 02/11] refactor: Extract the foreign key from the payload and store it separately --- ...d_foreign_id_column_to_approvals_table.php | 21 ++++++++++++ src/ApprovalServiceProvider.php | 1 + src/Concerns/MustBeApproved.php | 25 ++++++++++----- tests/Feature/MustBeApprovedTraitTest.php | 32 +++++++++++++++++-- tests/Pest.php | 1 - 5 files changed, 68 insertions(+), 12 deletions(-) create mode 100644 database/migrations/2024_03_16_173148_add_foreign_id_column_to_approvals_table.php diff --git a/database/migrations/2024_03_16_173148_add_foreign_id_column_to_approvals_table.php b/database/migrations/2024_03_16_173148_add_foreign_id_column_to_approvals_table.php new file mode 100644 index 0000000..25a2930 --- /dev/null +++ b/database/migrations/2024_03_16_173148_add_foreign_id_column_to_approvals_table.php @@ -0,0 +1,21 @@ +unsignedBigInteger('foreign_key')->nullable()->after('original_data'); + }); + } + + public function down(): void + { + Schema::table('approvals', function (Blueprint $table) { + $table->dropColumn('foreign_key'); + }); + } +}; diff --git a/src/ApprovalServiceProvider.php b/src/ApprovalServiceProvider.php index b361180..81a9e70 100644 --- a/src/ApprovalServiceProvider.php +++ b/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', ]); } } diff --git a/src/Concerns/MustBeApproved.php b/src/Concerns/MustBeApproved.php index be6ba07..336bba4 100644 --- a/src/Concerns/MustBeApproved.php +++ b/src/Concerns/MustBeApproved.php @@ -12,14 +12,8 @@ trait MustBeApproved public static function bootMustBeApproved(): void { - static::creating(callback: function ($model): ?bool { - if (! $model->user_id) { - $model->user_id = auth()->id(); - } - - return static::insertApprovalRequest($model); - }); - static::updating(callback: fn ($model): ?bool => static::insertApprovalRequest($model)); + static::creating(callback: fn($model): ?bool => static::insertApprovalRequest($model)); + static::updating(callback: fn($model): ?bool => static::insertApprovalRequest($model)); } /** @@ -29,6 +23,12 @@ protected static function insertApprovalRequest($model): ?bool { $filteredDirty = $model->getDirtyAttributes(); + $foreignKey = $model->getForeignKeyName(); + $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') { $filteredDirty[$key] = json_decode(json: $value, associative: true); @@ -59,6 +59,7 @@ protected static function insertApprovalRequest($model): ?bool $model->approvals()->create([ 'new_data' => $filteredDirty, 'original_data' => $model->getOriginalMatchingChanges(), + 'foreign_key' => $foreignKeyValue, ]); if (empty($noApprovalNeeded)) { @@ -87,6 +88,14 @@ public function getApprovalAttributes(): array return $this->approvalAttributes ?? []; } + /** + * Get the name of the foreign key for the model. + */ + public function getForeignKeyName(): string + { + return 'user_id'; + } + /** * Check is the approval can be bypassed. */ diff --git a/tests/Feature/MustBeApprovedTraitTest.php b/tests/Feature/MustBeApprovedTraitTest.php index b14291f..83eeb7a 100644 --- a/tests/Feature/MustBeApprovedTraitTest.php +++ b/tests/Feature/MustBeApprovedTraitTest.php @@ -25,7 +25,6 @@ 'new_data' => json_encode([ 'name' => 'Chris', 'meta' => 'red', - 'user_id' => null, ]), 'original_data' => json_encode([]), ]) @@ -203,7 +202,6 @@ 'new_data' => json_encode([ 'name' => 'Neo', 'data' => json_encode(['foo', 'bar']), - 'user_id' => null, ]), 'original_data' => json_encode([]), ]); @@ -264,7 +262,6 @@ 'title' => 'My First Post', 'content' => 'This is my first post', 'config' => ['checked' => true], - 'user_id' => null, ]), 'original_data' => json_encode([]), ]); @@ -291,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 getForeignKeyName(): 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, + ]); +}); diff --git a/tests/Pest.php b/tests/Pest.php index 695ecfd..b45abd6 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -18,7 +18,6 @@ $this->fakeModelData = [ 'name' => 'Chris', 'meta' => 'red', - 'user_id' => auth()->id(), ]; }) ->in(__DIR__); From f21ba58d5c9445058554d739d888d884becb8604 Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Sat, 16 Mar 2024 19:18:20 +0000 Subject: [PATCH 03/11] docs: Updated documentation to explain how models with foreign ids work. refactor: Renamed a method to avoid naming conflicts --- README.md | 43 +++++++++++++++++++++-- src/Concerns/MustBeApproved.php | 7 ++-- tests/Feature/MustBeApprovedTraitTest.php | 2 +- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 9db8f34..bf3588b 100644 --- a/README.md +++ b/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,6 +233,7 @@ 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. @@ -203,6 +241,7 @@ If you don't want Model data to be approved, you can bypass it with the `without ```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. diff --git a/src/Concerns/MustBeApproved.php b/src/Concerns/MustBeApproved.php index 336bba4..8342713 100644 --- a/src/Concerns/MustBeApproved.php +++ b/src/Concerns/MustBeApproved.php @@ -22,12 +22,11 @@ public static function bootMustBeApproved(): void protected static function insertApprovalRequest($model): ?bool { $filteredDirty = $model->getDirtyAttributes(); - - $foreignKey = $model->getForeignKeyName(); + $foreignKey = $model->getApprovalForeignKeyName(); $foreignKeyValue = $filteredDirty[$foreignKey] ?? null; // Remove the foreign key from the dirty attributes - unset($filteredDirty[$foreignKey]); + unset($filteredDirty[$foreignKey]); foreach ($filteredDirty as $key => $value) { if (isset($model->casts[$key]) && $model->casts[$key] === 'json') { @@ -91,7 +90,7 @@ public function getApprovalAttributes(): array /** * Get the name of the foreign key for the model. */ - public function getForeignKeyName(): string + public function getApprovalForeignKeyName(): string { return 'user_id'; } diff --git a/tests/Feature/MustBeApprovedTraitTest.php b/tests/Feature/MustBeApprovedTraitTest.php index 83eeb7a..d302fec 100644 --- a/tests/Feature/MustBeApprovedTraitTest.php +++ b/tests/Feature/MustBeApprovedTraitTest.php @@ -300,7 +300,7 @@ public $timestamps = false; - public function getForeignKeyName(): string + public function getApprovalForeignKeyName(): string { return 'user_id'; } From 445034d62402b73baa714b711265bb5b0d1fc07e Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Sun, 17 Mar 2024 15:00:04 +0000 Subject: [PATCH 04/11] style: Pint --- ...3_16_173148_add_foreign_id_column_to_approvals_table.php | 3 ++- src/Concerns/MustBeApproved.php | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/database/migrations/2024_03_16_173148_add_foreign_id_column_to_approvals_table.php b/database/migrations/2024_03_16_173148_add_foreign_id_column_to_approvals_table.php index 25a2930..3780c61 100644 --- a/database/migrations/2024_03_16_173148_add_foreign_id_column_to_approvals_table.php +++ b/database/migrations/2024_03_16_173148_add_foreign_id_column_to_approvals_table.php @@ -4,7 +4,8 @@ use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\Schema; -return new class extends Migration { +return new class extends Migration +{ public function up(): void { Schema::table('approvals', function (Blueprint $table) { diff --git a/src/Concerns/MustBeApproved.php b/src/Concerns/MustBeApproved.php index 8342713..fdc282f 100644 --- a/src/Concerns/MustBeApproved.php +++ b/src/Concerns/MustBeApproved.php @@ -12,8 +12,8 @@ trait MustBeApproved public static function bootMustBeApproved(): void { - static::creating(callback: fn($model): ?bool => static::insertApprovalRequest($model)); - static::updating(callback: fn($model): ?bool => static::insertApprovalRequest($model)); + static::creating(callback: fn ($model): ?bool => static::insertApprovalRequest($model)); + static::updating(callback: fn ($model): ?bool => static::insertApprovalRequest($model)); } /** @@ -26,7 +26,7 @@ protected static function insertApprovalRequest($model): ?bool $foreignKeyValue = $filteredDirty[$foreignKey] ?? null; // Remove the foreign key from the dirty attributes - unset($filteredDirty[$foreignKey]); + unset($filteredDirty[$foreignKey]); foreach ($filteredDirty as $key => $value) { if (isset($model->casts[$key]) && $model->casts[$key] === 'json') { From 692f8d18a8e2f67afcfe1b0059963a65a0398b66 Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Sun, 17 Mar 2024 17:41:10 +0000 Subject: [PATCH 05/11] docs: Added upgrade instructions --- UPGRADE.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/UPGRADE.md b/UPGRADE.md index dab249b..d72ef36 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,31 @@ # Upgrade Guide +## v1.4.3 -> v1.5.0 + +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'); +}); +``` + +> [!IMPORTANT] +> The namespace for the package has changed. + +Be sure to replace any instance of `Cjmellor\Approval` to `Approval\Approval` + ## 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. From 973a8b1a99b392d02e9e601df2aebadcbb0169c8 Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Mon, 18 Mar 2024 23:13:14 +0000 Subject: [PATCH 06/11] build: Add PHP constrains --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index f03f3c1..674a9ad 100644 --- a/composer.json +++ b/composer.json @@ -16,7 +16,7 @@ } ], "require": { - "php": "^8.1", + "php": "^8.1|^8.2", "illuminate/contracts": "^9.0|^10.0|^11.0", "spatie/laravel-package-tools": "^1.14.0" }, From aafe5ab035af51836aa6a4f3616db98e8e158470 Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Mon, 18 Mar 2024 23:18:51 +0000 Subject: [PATCH 07/11] build: Update PHP constrains --- .gitignore | 1 + composer.json | 14 ++++++++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 9a43686..9241c7d 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ .idea .php_cs .php_cs.cache +.phpunit.cache .phpunit.result.cache build composer.lock diff --git a/composer.json b/composer.json index 674a9ad..e7565e0 100644 --- a/composer.json +++ b/composer.json @@ -16,25 +16,27 @@ } ], "require": { - "php": "^8.1|^8.2", - "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": { From b4c1281008fa975afd983c0ca650dcf4469ecb3f Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Mon, 18 Mar 2024 23:19:48 +0000 Subject: [PATCH 08/11] build: Update GA workflow --- .github/workflows/run-pest.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/run-pest.yml b/.github/workflows/run-pest.yml index 4e95152..6d2033c 100644 --- a/.github/workflows/run-pest.yml +++ b/.github/workflows/run-pest.yml @@ -8,7 +8,7 @@ jobs: strategy: fail-fast: true matrix: - php: [ 8.1, 8.2 ] + php: [ 8.2 ] stability: [ prefer-lowest, prefer-stable ] name: "PHP: v${{ matrix.php }} [${{ matrix.stability }}]" From 0c2b58bdc97eb07f300ebd4ab559191cc6ad0008 Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Mon, 18 Mar 2024 23:20:35 +0000 Subject: [PATCH 09/11] build: Update GA workflow --- .github/workflows/run-pest.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/run-pest.yml b/.github/workflows/run-pest.yml index 6d2033c..ab8006e 100644 --- a/.github/workflows/run-pest.yml +++ b/.github/workflows/run-pest.yml @@ -8,7 +8,7 @@ jobs: strategy: fail-fast: true matrix: - php: [ 8.2 ] + php: [ 8.2, 8.3 ] stability: [ prefer-lowest, prefer-stable ] name: "PHP: v${{ matrix.php }} [${{ matrix.stability }}]" From 0bfeb4c061aa42b9636ff34c0e433d5fd3334cd8 Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Mon, 18 Mar 2024 23:23:45 +0000 Subject: [PATCH 10/11] docs: Update --- UPGRADE.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index d72ef36..319751d 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -21,11 +21,6 @@ Schema::table('approvals', function (Blueprint $table) { }); ``` -> [!IMPORTANT] -> The namespace for the package has changed. - -Be sure to replace any instance of `Cjmellor\Approval` to `Approval\Approval` - ## 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. From f925dd31e9c191430b8a33631af5655e2d7f3b7a Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Mon, 18 Mar 2024 23:25:54 +0000 Subject: [PATCH 11/11] docs: Update --- UPGRADE.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/UPGRADE.md b/UPGRADE.md index 319751d..513f50e 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -2,6 +2,9 @@ ## 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