From 50340a2fd89f425bd1ce6fc7e33af7b13e92389d Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Mon, 18 Mar 2024 21:55:46 +0000 Subject: [PATCH 1/3] refactor: Extract the foreign key from the payload and store it separately fix: Add foreign id when creating a Model not via a relationship docs: Updated documentation to explain how models with foreign ids work. refactor: Renamed a method to avoid naming conflicts docs: Added upgrade instructions --- .gitignore | 1 + README.md | 43 ++++++++++++++++- UPGRADE.md | 26 ++++++++++ ...d_foreign_id_column_to_approvals_table.php | 22 +++++++++ src/ApprovalServiceProvider.php | 1 + src/Concerns/MustBeApproved.php | 40 ++++++++++------ tests/Feature/MustBeApprovedTraitTest.php | 47 ++++++++++++++++--- tests/Pest.php | 1 - 8 files changed, 157 insertions(+), 24 deletions(-) create mode 100644 database/migrations/2024_03_18_214515_add_foreign_id_column_to_approvals_table.php 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/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/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. diff --git a/database/migrations/2024_03_18_214515_add_foreign_id_column_to_approvals_table.php b/database/migrations/2024_03_18_214515_add_foreign_id_column_to_approvals_table.php new file mode 100644 index 0000000..3780c61 --- /dev/null +++ b/database/migrations/2024_03_18_214515_add_foreign_id_column_to_approvals_table.php @@ -0,0 +1,22 @@ +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..a884160 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_18_214515_add_foreign_id_column_to_approvals_table.php', ]); } } diff --git a/src/Concerns/MustBeApproved.php b/src/Concerns/MustBeApproved.php index 89b4244..fdc282f 100644 --- a/src/Concerns/MustBeApproved.php +++ b/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. */ diff --git a/tests/Feature/MustBeApprovedTraitTest.php b/tests/Feature/MustBeApprovedTraitTest.php index 6baa288..d302fec 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([]), ]) @@ -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, + ]); +}); 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 2bb5c9b80f92b004bbb3cebe60982bb09a4e5c81 Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Mon, 18 Mar 2024 22:01:36 +0000 Subject: [PATCH 2/3] build: Add missing PHP constraint --- 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 ba4114be71d841d0f5e4e8ed6ada18674a07c963 Mon Sep 17 00:00:00 2001 From: Chris Mellor Date: Mon, 18 Mar 2024 22:04:33 +0000 Subject: [PATCH 3/3] build: Remove PHP 8.1 constraint --- .github/workflows/run-pest.yml | 2 +- composer.json | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) 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 }}]" diff --git a/composer.json b/composer.json index 674a9ad..ff4f180 100644 --- a/composer.json +++ b/composer.json @@ -16,14 +16,14 @@ } ], "require": { - "php": "^8.1|^8.2", - "illuminate/contracts": "^9.0|^10.0|^11.0", + "php": "^8.2", + "illuminate/contracts": "^10.0|^11.0", "spatie/laravel-package-tools": "^1.14.0" }, "require-dev": { "laravel/pint": "^1.0", "nunomaduro/collision": "^7.0|^8.0", - "orchestra/testbench": "^7.0|^8.0|^9.0", + "orchestra/testbench": "^8.0|^9.0", "pestphp/pest": "^2.0", "pestphp/pest-plugin-type-coverage": "^2.0" },