Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add pan:delete command with ID-specific deletion and test case. #10

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file added database/database.sqlite
Empty file.
41 changes: 41 additions & 0 deletions src/Adapters/Laravel/Console/Commands/PanDeleteCommand.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php

declare(strict_types=1);

namespace Pan\Adapters\Laravel\Console\Commands;

use Illuminate\Console\Command;
use Pan\Contracts\AnalyticsRepository;

class PanDeleteCommand extends Command
{
/**
* The name and signature of the console command.
*
* @var string
*/
protected $signature = 'pan:delete {id : The ID of the analytic to delete}';

/**
* The console command description.
*
* @var string
*/
protected $description = 'Delete analytics by ID.';

/**
* Execute the console command.
*/
public function handle(AnalyticsRepository $repository): void
nunomaduro marked this conversation as resolved.
Show resolved Hide resolved
{
$id = (int) $this->argument('id');

if ($id > 0) {
$this->info(
$repository->delete($id)
);
} else {
$this->error('Analytic ID must be a positive integer.');
}
}
}
2 changes: 2 additions & 0 deletions src/Adapters/Laravel/Providers/PanServiceProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Illuminate\Support\ServiceProvider;
use Pan\Adapters\Laravel\Console\Commands\InstallPanCommand;
use Pan\Adapters\Laravel\Console\Commands\PanCommand;
use Pan\Adapters\Laravel\Console\Commands\PanDeleteCommand;
use Pan\Adapters\Laravel\Console\Commands\PanFlushCommand;
use Pan\Adapters\Laravel\Http\Controllers\EventController;
use Pan\Adapters\Laravel\Http\Middleware\InjectJavascriptLibrary;
Expand Down Expand Up @@ -83,6 +84,7 @@ private function registerCommands(): void
InstallPanCommand::class,
PanCommand::class,
PanFlushCommand::class,
PanDeleteCommand::class,
]);
}
}
Expand Down
10 changes: 10 additions & 0 deletions src/Adapters/Laravel/Repositories/DatabaseAnalyticsRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,14 @@ public function flush(): void
{
DB::table('pan_analytics')->truncate();
}

/**
ruchit288 marked this conversation as resolved.
Show resolved Hide resolved
* Delete a specific analytic by ID.
*/
public function delete(int $id): string
{
return DB::table('pan_analytics')->where('id', $id)->delete()
? "Analytic has been deleted."
Copy link

@taghwo taghwo Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the client i.e the command should be the one handling this. Returning int here would be better than this string.

: "Analytic not found or already deleted.";
}
}
5 changes: 5 additions & 0 deletions src/Contracts/AnalyticsRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,9 @@ public function increment(string $name, EventType $event): void;
* Flush all analytics.
*/
public function flush(): void;

/**
* Delete a specific analytic by ID.
*/
public function delete(int $id): string;
}
38 changes: 38 additions & 0 deletions tests/Feature/Laravel/Console/PanDeleteTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php

use Pan\Contracts\AnalyticsRepository;

beforeEach(function (): void {
$this->repository = mock(AnalyticsRepository::class)->makePartial();
Copy link

@taghwo taghwo Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the test here, the way it's broken down.
What is the performance gain in using a mock versus hitting the DB here?
I would suggest hitting the DB and confirming the row is actually deleted or test the repository method in isolation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ruchit288 can you assert the database is missing the ID after deleting in these tests instead of the mock? Another way would be, leave the command class test as it is, and have a separate test for the delete of method of the repository (assert database missing the ID deleted).

You can check the merged branches, look at the feature test cases on how you can insert some data into the table. That would even prevent hard coding the id like ['id' => 1]. You can use insertGetId, which would make the tests more stable and less prone to failure.

app()->instance(AnalyticsRepository::class, $this->repository);
});

it('deletes a specific analytic by ID', function (): void {
$this->repository
->expects('delete')
->with(1)
->once()
->andReturn('Analytic has been deleted.');

$this->artisan('pan:delete', ['id' => 1])
->assertExitCode(0)
->expectsOutput('Analytic has been deleted.');
});

it('fails when no argument is provided', function (): void {
$this->artisan('pan:delete')
->assertExitCode(1)
->expectsOutput('Not enough arguments (missing: "id").');
});

it('handles non-existent analytic gracefully', function (): void {
$this->repository
->expects('delete')
->with(26)
->once()
->andReturn('Analytic not found or already deleted.');

$this->artisan('pan:delete', ['id' => 26])
->assertExitCode(0)
->expectsOutput('Analytic not found or already deleted.');
});