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

Conversation

ruchit288
Copy link

@ruchit288 ruchit288 commented Oct 15, 2024

This PR contain:

  • Implement a new Artisan command pan delete to remove specific analytic by ID.
  • Command e.g. -> php artisan pan:delete 1

Copy link

@taghwo taghwo left a comment

Choose a reason for hiding this comment

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

Nice work here, simple and clear 🚀

*
* @var string
*/
protected $description = 'Delete analytics by ID or all at once';
Copy link

Choose a reason for hiding this comment

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

I don't think this is deleting all? The where clause in the repository says it's removing one entity or its not all together.

Copy link
Author

Choose a reason for hiding this comment

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

You are correct. I just update the description.

src/Adapters/Laravel/Console/Commands/PanDeleteCommand.php Outdated Show resolved Hide resolved
<?php

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.

ruchit288 and others added 3 commits October 17, 2024 10:51
Co-authored-by: Taghwo Millionaire <40868373+taghwo@users.noreply.github.com>
Co-authored-by: Taghwo Millionaire <40868373+taghwo@users.noreply.github.com>
*/
public function handle(AnalyticsRepository $repository): void
{
$id = $this->option('id');

Choose a reason for hiding this comment

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

As per @nunomaduro pan:delete {id}, I think we need an argument here at the place of option.

Suggested change
$id = $this->option('id');
$id = $this->argument('id');

Copy link
Author

@ruchit288 ruchit288 Oct 17, 2024

Choose a reason for hiding this comment

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

Done. Thanks @MrPunyapal for clarifying.

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.

taghwo

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants