spatie / laravel-activitylog

Log activity inside your Laravel app
https://docs.spatie.be/laravel-activitylog
MIT License
5.36k stars 714 forks source link

When updating deleted_at column no activity is logged #979

Closed SudoGetBeer closed 2 years ago

SudoGetBeer commented 3 years ago

Describe the bug We have a API which takes an JSON and maps it into the database. But we want to unset the soft delete when updating.

The code is something like that:

                    $customer = Customer::withTrashed()->updateOrCreate(
                        ['username' => $dataCustomer['username']],
                        [
                            'name' => $dataCustomer['name'] ?? null,
                            'street' => $dataCustomer['street'] ?? null,
                            'postal_code' => $dataCustomer['postal_code'] ?? null,
                            'city' => $dataCustomer['city'] ?? null,
                            'deleted_at' => null,
                        ]
                    );

But then nothing is logged.

It seems the code at https://github.com/spatie/laravel-activitylog/blob/a23de07a6160897559f7f9ac14fa98d2676d450a/src/Traits/LogsActivity.php#L178-L182 is preventing this.

Expected behavior I think it should be logged. But maybe I misunderstood something.

Gummibeer commented 3 years ago

That's totally reasonable as you should use $model->restore() to un-delete a soft-deleted entity instead of mass-assignment updating the deleted_at column. https://github.com/spatie/laravel-activitylog/blob/a23de07a6160897559f7f9ac14fa98d2676d450a/src/Traits/LogsActivity.php#L155-L157

Same for deleting - as we track the deleted event - so the more precise bug, if any, is that the deleted_at column should be ignored at all during update events.

SudoGetBeer commented 3 years ago

Same for deleting - as we track the deleted event - so the more precise bug, if any, is that the deleted_at column should be ignored at all during update events.

I know it is maybe not a real bug. But I had to dig into the code to find out why it was not logged.

I already thought about submitting a PR. Would this be a good solution for you?

        if (Arr::has($this->getDirty(), $deletedAtColumn) && count($this->getDirty()) === 1) {
            if ($this->getDirty()[$deletedAtColumn] === null) {
                return false;
            }
        }
Gummibeer commented 3 years ago

The problem is that, in theory, every change there is a breaking change. Some of them could be argued as fixes regarding wanted but badly implemented behavior.

The intention there was to don't log soft-delete events as updates - like said above there are dedicated events for these. For the most customization I would move the current logic as-is to a new method shouldLogSoftDeleteChanges(string $eventName).

That way you can customize and override it without caring about the other checks and it's not a breaking change to anyone.

SudoGetBeer commented 3 years ago

@Gummibeer I made a PR #981.

Hope it works the way you suggested. Thanks for your help and this great package.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 7 days