owen-it / laravel-auditing

Record the change log from models in Laravel
https://laravel-auditing.com
MIT License
2.94k stars 382 forks source link

feat (Auditable): customized relationship for Attach, Detach and Sync #899

Closed wilianx7 closed 2 months ago

wilianx7 commented 4 months ago

Hello guys.

In my API I have a situation where I need to provide a BelongsToMany relationship with some conditions to Audit Attach, Detach and Sync.

This situation occurs because my pivot table has many pivot columns that need to be conditioned when I do an Attach/Detach/Sync.

Nowadays I can't do this, as these methods only accept the name of the relationship (without conditions). So I made this PR with a new optional parameter $relationship.

Example use case:

image

In this case, I just want to sync the serviceTypes relationship in my pivot table, where the pivot columns match the respective company_id and tenancy_id values. The other data in the pivot table remained unchanged.

parallels999 commented 4 months ago

Also needs unit tests

wilianx7 commented 4 months ago

I understand. However, in the way you are suggesting, the method call becomes much more verbose taking into account the addition of an optional parameter at the end, which forces the passage of all other parameters so that Closure can be used.

The implementation I made is more dynamic, as it does not require an additional parameter and allows the method to be used from a Closure or the name of the relationship (as it is today).

Suggested implementation:

$model->auditAttach('my_existing_relation', ['ids'], null, null, null, fn ($rel) => $rel->wherePivot());

My implementation:

$model->auditAttach(fn () => $rel->wherePivot(), ['ids']);

$model->auditAttach('my_existing_relation', ['ids']);

What would be the negative point of my implementation?

wilianx7 commented 4 months ago

Here, where $firstCategory is attached? $article1 or $article2?

Sorry, I didn't see any comments in my tests. But you're right, in this situation my implementation will fail.

I'll do it the way you suggested ASAP.

wilianx7 commented 4 months ago

@parallels999 could you review, please?

parallels999 commented 4 months ago

LGTM 👍 But @MortenDHansen review is needed

wilianx7 commented 3 months ago

@MortenDHansen could you review, please?