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

Support for auditSync and auditDetach on Auditable custom Pivot class #954

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

txdFrancesco
Copy link

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.

Copy link
Contributor

@willpower232 willpower232 left a 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?

@txdFrancesco
Copy link
Author

For the spaces sure, for the tests I'll need to add a couple of models to reproduce.
Users -> GroupMembers (with 'role' column) -> Groups could be an example of my case. Let me know if you have a better/preferred example structure. I'll update when I'm done, thanks

@txdFrancesco
Copy link
Author

I added some tests that cover sync, attach and detach with an Auditable custom Pivot class.
AuditAttach is not affected by my code change, I put in the test anyway for completeness. Let me know if it is better to remove it.

The other tests are failing without the code change and are passing now.

Copy link
Contributor

@willpower232 willpower232 left a 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

src/Auditable.php Outdated Show resolved Hide resolved
src/Auditable.php Outdated Show resolved Hide resolved
src/Auditable.php Outdated Show resolved Hide resolved
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.

2 participants