-
Notifications
You must be signed in to change notification settings - Fork 390
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
Support for auditSync and auditDetach on Auditable custom Pivot class #954
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you able to correct your spaces to match the rest of the code and add some tests for the functionality you've added?
For the spaces sure, for the tests I'll need to add a couple of models to reproduce. |
I added some tests that cover sync, attach and detach with an Auditable custom Pivot class. The other tests are failing without the code change and are passing now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some final spaces and maybe wake up github actions
I think |
@willpower232 Sorry for the inconvenience, I'm not sure what should I do to make this pull request go through. |
The bug was caused by the way the detach method creates the query to delete the pivot class (creating a new instance and not retrieving the one in the database). Attaching is not affected by it (there is no instance to retrieve), applying the changes would just remove the "created" audit on the pivot class, which is against the idea of having an auditable pivot class. A full solution would be to rework how the detach method builds the query instead of wrapping with withoutAuditing, but frankly I don't know the codebase enough to make such an impactful change for what is an edge usecase. |
I was confused as to why github actions wasn't working but it seems to be behaving now, I'll re read the PR shortly |
Allow the use of auditSync and auditDetach on BelongsToMany relations where the pivot class is an Auditable class.
This is useful in cases where the relation model is itself a Laravel model and may be updated (and thus audited) indipendently from the related models, without requring dedicated code around every ->auditSync call on the related models.