From 615425a2a3629e317e956365e5023138e40a639a Mon Sep 17 00:00:00 2001 From: Ziad Talaat Date: Wed, 15 Jan 2025 04:02:12 +0200 Subject: [PATCH 1/2] test: add comprehensive tests for table record action and url configuration --- tests/Tables/TableConfigurationTest.php | 218 ++++++++++++++++++++++++ 1 file changed, 218 insertions(+) create mode 100644 tests/Tables/TableConfigurationTest.php diff --git a/tests/Tables/TableConfigurationTest.php b/tests/Tables/TableConfigurationTest.php new file mode 100644 index 00000000000..34e9201117c --- /dev/null +++ b/tests/Tables/TableConfigurationTest.php @@ -0,0 +1,218 @@ +hasMany(TestItemModel::class, 'owner_id'); + } +} + +class TestItemModel extends Model +{ + protected $table = 'test_items'; + public $timestamps = false; + protected $fillable = ['name', 'owner_id']; +} + +uses(TestCase::class)->group('tables'); + +beforeEach(function () { + $this->livewire = Mockery::mock(HasTable::class); + + Schema::create('test_owners', function (Blueprint $table) { + $table->id(); + $table->string('name')->nullable(); + }); + + Schema::create('test_items', function (Blueprint $table) { + $table->id(); + $table->foreignId('owner_id')->nullable()->constrained('test_owners')->cascadeOnDelete(); + $table->string('name')->nullable(); + }); +}); + +afterEach(function () { + Mockery::close(); + Schema::dropIfExists('test_items'); + Schema::dropIfExists('test_owners'); +}); + +it('can nullify table record action', function () { + Table::configureUsing(fn (Table $table) => $table + ->recordAction(null) + ); + + $table = Table::make($this->livewire); + + expect($table) + ->hasRecordActionBeenSet()->toBeTrue() + ->getRecordAction(new TestItemModel())->toBeNull(); +}); + +it('can nullify table record url', function () { + Table::configureUsing(fn (Table $table) => $table + ->recordUrl(null) + ); + + $table = Table::make($this->livewire); + + expect($table) + ->hasRecordUrlBeenSet()->toBeTrue() + ->getRecordUrl(new TestItemModel())->toBeNull(); +}); + +it('can set table record action and url together', function () { + Table::configureUsing(fn (Table $table) => $table + ->recordAction('edit') + ->recordUrl('/edit/1') + ); + + $table = Table::make($this->livewire); + + expect($table) + ->hasRecordActionBeenSet()->toBeTrue() + ->hasRecordUrlBeenSet()->toBeTrue() + ->getRecordAction(new TestItemModel())->toBe('edit') + ->getRecordUrl(new TestItemModel())->toBe('/edit/1'); +}); + +it('can nullify table record action and url together', function () { + Table::configureUsing(fn (Table $table) => $table + ->recordAction(null) + ->recordUrl(null) + ); + + $table = Table::make($this->livewire); + + expect($table) + ->hasRecordActionBeenSet()->toBeTrue() + ->hasRecordUrlBeenSet()->toBeTrue() + ->getRecordAction(new TestItemModel())->toBeNull() + ->getRecordUrl(new TestItemModel())->toBeNull(); +}); + +it('respects closure values for record action and url', function () { + $model = new TestItemModel(); + + Table::configureUsing(fn (Table $table) => $table + ->recordAction(fn (Model $record) => 'edit-' . $record::class) + ->recordUrl(fn (Model $record) => '/edit/' . $record::class) + ); + + $table = Table::make($this->livewire); + + expect($table) + ->hasRecordActionBeenSet()->toBeTrue() + ->hasRecordUrlBeenSet()->toBeTrue() + ->getRecordAction($model)->toBe('edit-' . $model::class) + ->getRecordUrl($model)->toBe('/edit/' . $model::class); +}); + +it('respects nullified record url in relation manager', function () { + $owner = TestOwnerModel::create(['name' => 'Test Owner']); + + $relationManager = Mockery::mock(RelationManager::class) + ->makePartial() + ->shouldAllowMockingProtectedMethods(); + + $relationManager->shouldReceive('getRelationship') + ->andReturn($owner->items()); + + $relationManager->shouldReceive('table') + ->andReturnUsing(function (Table $table) { + return $table->recordUrl(null); + }); + + $table = Table::make($relationManager); + $table = $relationManager->table($table); + + expect($table) + ->hasRecordUrlBeenSet()->toBeTrue() + ->getRecordUrl(new TestItemModel())->toBeNull(); +}); + +it('respects nullified record action in relation manager', function () { + $owner = TestOwnerModel::create(['name' => 'Test Owner']); + + $relationManager = Mockery::mock(RelationManager::class) + ->makePartial() + ->shouldAllowMockingProtectedMethods(); + + $relationManager->shouldReceive('getRelationship') + ->andReturn($owner->items()); + + $relationManager->shouldReceive('table') + ->andReturnUsing(function (Table $table) { + return $table->recordAction(null); + }); + + $table = Table::make($relationManager); + $table = $relationManager->table($table); + + expect($table) + ->hasRecordActionBeenSet()->toBeTrue() + ->getRecordAction(new TestItemModel())->toBeNull(); +}); + +it('respects custom record url in relation manager', function () { + $owner = TestOwnerModel::create(['name' => 'Test Owner']); + + $relationManager = Mockery::mock(RelationManager::class) + ->makePartial() + ->shouldAllowMockingProtectedMethods(); + + $relationManager->shouldReceive('getRelationship') + ->andReturn($owner->items()); + + $relationManager->shouldReceive('table') + ->andReturnUsing(function (Table $table) { + return $table->recordUrl('/custom/url'); + }); + + $table = Table::make($relationManager); + $table = $relationManager->table($table); + + expect($table) + ->hasRecordUrlBeenSet()->toBeTrue() + ->getRecordUrl(new TestItemModel())->toBe('/custom/url'); +}); + +it('respects custom record action in relation manager', function () { + $owner = TestOwnerModel::create(['name' => 'Test Owner']); + + $relationManager = Mockery::mock(RelationManager::class) + ->makePartial() + ->shouldAllowMockingProtectedMethods(); + + $relationManager->shouldReceive('getRelationship') + ->andReturn($owner->items()); + + $relationManager->shouldReceive('table') + ->andReturnUsing(function (Table $table) { + return $table->recordAction('custom-action'); + }); + + $table = Table::make($relationManager); + $table = $relationManager->table($table); + + expect($table) + ->hasRecordActionBeenSet()->toBeTrue() + ->getRecordAction(new TestItemModel())->toBe('custom-action'); +}); From 0e1c0498c48ab599d4f7f4eebbaa20eb8e7e57a7 Mon Sep 17 00:00:00 2001 From: Ziad Talaat Date: Wed, 15 Jan 2025 04:02:49 +0200 Subject: [PATCH 2/2] fix: respect nullified record action and url in tables Added tracking of whether recordAction and recordUrl have been explicitly set by the user. This ensures that default values are only applied when the user hasn't set these values explicitly, allowing nullification to work properly. This fixes an issue where setting recordAction or recordUrl to null in a table configuration would be overridden by the default values. --- .../InteractsWithRelationshipTable.php | 22 ++++++++++------ .../src/Resources/Pages/ListRecords.php | 25 +++++++++++++------ .../src/Table/Concerns/HasRecordAction.php | 8 ++++++ .../src/Table/Concerns/HasRecordUrl.php | 8 ++++++ 4 files changed, 48 insertions(+), 15 deletions(-) diff --git a/packages/panels/src/Resources/Concerns/InteractsWithRelationshipTable.php b/packages/panels/src/Resources/Concerns/InteractsWithRelationshipTable.php index 4d117a4f812..c75e82526b0 100644 --- a/packages/panels/src/Resources/Concerns/InteractsWithRelationshipTable.php +++ b/packages/panels/src/Resources/Concerns/InteractsWithRelationshipTable.php @@ -54,11 +54,14 @@ public static function getRelationshipName(): string protected function makeTable(): Table { - return $this->makeBaseTable() + $table = $this->makeBaseTable() ->relationship(fn (): Relation | Builder => $this->getRelationship()) ->modifyQueryUsing($this->modifyQueryWithActiveTab(...)) - ->queryStringIdentifier(Str::lcfirst(class_basename(static::class))) - ->recordAction(function (Model $record, Table $table): ?string { + ->queryStringIdentifier(Str::lcfirst(class_basename(static::class))); + + // Only set recordAction and recordUrl if they haven't been explicitly set + if (! $table->hasRecordActionBeenSet()) { + $table->recordAction(function (Model $record, Table $table): ?string { foreach (['view', 'edit'] as $action) { $action = $table->getAction($action); @@ -80,8 +83,11 @@ protected function makeTable(): Table } return null; - }) - ->recordUrl(function (Model $record, Table $table): ?string { + }); + } + + if (! $table->hasRecordUrlBeenSet()) { + $table->recordUrl(function (Model $record, Table $table): ?string { foreach (['view', 'edit'] as $action) { $action = $table->getAction($action); @@ -105,7 +111,9 @@ protected function makeTable(): Table } return null; - }) - ->authorizeReorder(fn (): bool => $this->canReorder()); + }); + } + + return $table->authorizeReorder(fn (): bool => $this->canReorder()); } } diff --git a/packages/panels/src/Resources/Pages/ListRecords.php b/packages/panels/src/Resources/Pages/ListRecords.php index 485cfa0feb9..963b6d60f39 100644 --- a/packages/panels/src/Resources/Pages/ListRecords.php +++ b/packages/panels/src/Resources/Pages/ListRecords.php @@ -239,12 +239,15 @@ public function getPluralModelLabel(): ?string protected function makeTable(): Table { - return $this->makeBaseTable() + $table = $this->makeBaseTable() ->query(fn (): Builder => $this->getTableQuery()) ->modifyQueryUsing($this->modifyQueryWithActiveTab(...)) ->modelLabel($this->getModelLabel() ?? static::getResource()::getModelLabel()) - ->pluralModelLabel($this->getPluralModelLabel() ?? static::getResource()::getPluralModelLabel()) - ->recordAction(function (Model $record, Table $table): ?string { + ->pluralModelLabel($this->getPluralModelLabel() ?? static::getResource()::getPluralModelLabel()); + + // Only set recordAction and recordUrl if they haven't been explicitly set + if (! $table->hasRecordActionBeenSet()) { + $table->recordAction(function (Model $record, Table $table): ?string { foreach (['view', 'edit'] as $action) { $action = $table->getAction($action); @@ -270,9 +273,13 @@ protected function makeTable(): Table } return null; - }) - ->recordTitle(fn (Model $record): string => static::getResource()::getRecordTitle($record)) - ->recordUrl($this->getTableRecordUrlUsing() ?? function (Model $record, Table $table): ?string { + }); + } + + $table->recordTitle(fn (Model $record): string => static::getResource()::getRecordTitle($record)); + + if (! $table->hasRecordUrlBeenSet()) { + $table->recordUrl($this->getTableRecordUrlUsing() ?? function (Model $record, Table $table): ?string { foreach (['view', 'edit'] as $action) { $action = $table->getAction($action); @@ -314,8 +321,10 @@ protected function makeTable(): Table } return null; - }) - ->authorizeReorder(static::getResource()::canReorder()); + }); + } + + return $table->authorizeReorder(static::getResource()::canReorder()); } /** diff --git a/packages/tables/src/Table/Concerns/HasRecordAction.php b/packages/tables/src/Table/Concerns/HasRecordAction.php index 01c616d9bf5..c7c7ebc3c5b 100644 --- a/packages/tables/src/Table/Concerns/HasRecordAction.php +++ b/packages/tables/src/Table/Concerns/HasRecordAction.php @@ -10,13 +10,21 @@ trait HasRecordAction { protected string | Closure | null $recordAction = null; + protected bool $hasRecordActionBeenSet = false; + public function recordAction(string | Closure | null $action): static { $this->recordAction = $action; + $this->hasRecordActionBeenSet = true; return $this; } + public function hasRecordActionBeenSet(): bool + { + return $this->hasRecordActionBeenSet; + } + public function getRecordAction(Model $record): ?string { $action = $this->evaluate( diff --git a/packages/tables/src/Table/Concerns/HasRecordUrl.php b/packages/tables/src/Table/Concerns/HasRecordUrl.php index 7e1e5b3d433..6a38eddf7ee 100644 --- a/packages/tables/src/Table/Concerns/HasRecordUrl.php +++ b/packages/tables/src/Table/Concerns/HasRecordUrl.php @@ -11,6 +11,8 @@ trait HasRecordUrl protected string | Closure | null $recordUrl = null; + protected bool $hasRecordUrlBeenSet = false; + public function openRecordUrlInNewTab(bool | Closure $condition = true): static { $this->shouldOpenRecordUrlInNewTab = $condition; @@ -22,10 +24,16 @@ public function recordUrl(string | Closure | null $url, bool | Closure $shouldOp { $this->openRecordUrlInNewTab($shouldOpenInNewTab); $this->recordUrl = $url; + $this->hasRecordUrlBeenSet = true; return $this; } + public function hasRecordUrlBeenSet(): bool + { + return $this->hasRecordUrlBeenSet; + } + public function getRecordUrl(Model $record): ?string { return $this->evaluate(