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

Added withTrashed to auditable relation and avoid logging item as updated when restoring #222

Closed
wants to merge 4 commits into from

Conversation

gmedeiros
Copy link

When retreiving a softdeleted item, I was getting an error, so I added withTrashed when at the auditable() relation. This is because I am auditing a model and some related models using many to many relationships (which required a few changes and not using the sync() method.

Also, when restoring a softdeleted item, an updated audit was being created. I added some logic to check if only the updated_at and deleted_at are changing (with deleted_at being not null) before saving an updated audit.

@@ -40,7 +40,16 @@ public function created(AuditableContract $model)
*/
public function updated(AuditableContract $model)
{
Auditor::execute($model->setAuditEvent('updated'));
if (

Choose a reason for hiding this comment

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

Expected 0 spaces after opening bracket; newline found

$model->isDirty($model->getDeletedAtColumn()) &&
!$model->deleted_at &&
count($model->getDirty()) == 2 //deleted_at and updated_at
){

Choose a reason for hiding this comment

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

Expected 1 space after closing parenthesis; found 0

count($model->getDirty()) == 2 //deleted_at and updated_at
){
// only restoring softdeleted item, no values changed other than deleted_at an updated_at
}

Choose a reason for hiding this comment

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

Expected 1 space after closing brace; newline found

Copy link
Contributor

@quetzyg quetzyg left a comment

Choose a reason for hiding this comment

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

Thanks for contributing, but your code needs improvements.

method_exists($model, 'getDeletedAtColumn') &&
$model->isDirty($model->getDeletedAtColumn()) &&
!$model->deleted_at &&
count($model->getDirty()) == 2 //deleted_at and updated_at
Copy link
Contributor

Choose a reason for hiding this comment

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

While this may work, I don't like how the code is written. Specially the empty if part.

@@ -93,7 +93,7 @@ public function getTable()
*/
public function auditable()
{
return $this->morphTo();
return $this->morphTo()->withTrashed();
Copy link
Member

@anteriovieira anteriovieira Apr 12, 2017

Choose a reason for hiding this comment

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

You can do this in your own code $audit->auditable()->withTrashed() 😉

Copy link
Author

Choose a reason for hiding this comment

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

Sounds like a good idea, thanks!

@anteriovieira
Copy link
Member

@gmedeiros, and if you ignore these properties, could it help?

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;
use OwenIt\Auditing\Auditable;
use OwenIt\Auditing\Contracts\Auditable as AuditableContract;

class Post extends Model implements AuditableContract;
{
    use Auditable;

    /**
     * Attributes to exclude from the Audit.
     *
     * @var array
     */
    protected $auditExclude = [
        'updated_at', 'deleted_at'
    ];
}

@anteriovieira anteriovieira reopened this Apr 12, 2017
@quetzyg quetzyg mentioned this pull request Apr 30, 2017
@quetzyg
Copy link
Contributor

quetzyg commented Apr 30, 2017

Thanks for the pull request @gmedeiros.

A cleaner implementation has been done in #233.
While your code didn't get merged, your PR was still helpful as a bug report.

This PR is now closed.

@quetzyg quetzyg closed this Apr 30, 2017
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.

4 participants