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

Ability to disable cascading for trash/restore #69

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

kolorafa
Copy link
Contributor

Pull request give the ability to disable cascading for trash/restore only and can still cascade the normal delete/purge.


We decided to only cascade deletion (with cascade enabled for audit log) , but never cascade trash or restore.

The reason for that is, that if you restore entity in hasMany relation, you might not want to restore 'child' entities that was intentionally trashed before you accidentally trash and restore the 'parent'.

For example, you have Users hasMany UserNotes.
User create 2 notes, User trashes one of the note.
Admin trashes User (it does trash the one not yet trashed note), then admin restore User (for some reason), the code can't restore the UserNotes to the 'previous state' like you would expect of "restore" to do, it will restore Both UserNotes even if one of the notes needs to be still trashed.

So you want to delete UserNotes if you delete User, but you don't want to trash/restore User notes when you trash/restore User.

@kolorafa kolorafa marked this pull request as ready for review August 28, 2024 16:03
@kolorafa
Copy link
Contributor Author

The PR will not change the default behavior if the user not intentionally change this variable.

@ADmad Should I create some additional test cases?

The only one more test that came to my mind is for cascadingRestoreTrash(), but in that case if you switch cascading off, then this function will behave exactly like restoreTrash() making it pointless to use cascadingRestoreTrash() in the first place, but might be good to document it if someone might try to use it without checking manually at least once.

@ADmad
Copy link
Member

ADmad commented Aug 29, 2024

...but might be good to document it if someone might try to use it without checking manually at least once.

Adding tests for documenting the behavior is a good idea. I am fine with the patch in general.

@kolorafa
Copy link
Contributor Author

@ADmad now I'm thinking about test for cascadingRestoreTrash(),

If someone disable cascading, and then still execute cascadingRestoreTrash(), should it
a) (just silently) not do cascading - that should be enough for now - less changes, smaller chance for error :)
b) ignore the new variable - making it only affect trash() so still cascade restores using this function (not going to use in our project but...)
c) throw exception - telling you are doing something wrong?

@ADmad
Copy link
Member

ADmad commented Aug 29, 2024

Silent no-ops are not nice. It's better to inform the dev/user when they do something that doesn't make sense.

@kolorafa
Copy link
Contributor Author

kolorafa commented Aug 29, 2024

Now is more consistent (only changing the trash, as you can already restore without cascading) :)

@ADmad
Copy link
Member

ADmad commented Aug 29, 2024

Simplicity, nice!

@ADmad ADmad merged commit f48a569 into UseMuffin:master Aug 29, 2024
7 checks passed
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