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

Allow forced deletion of soft-delete models #1212

Open
lex0r opened this issue Sep 24, 2024 · 14 comments
Open

Allow forced deletion of soft-delete models #1212

lex0r opened this issue Sep 24, 2024 · 14 comments

Comments

@lex0r
Copy link
Contributor

lex0r commented Sep 24, 2024

Package targeted

Winter CMS

Description

By definition, soft-delete enabled models are not expected to be permanently deleted. There's an exception to this rule, when developers rely on soft-delete as a sort of "inactive" attribute. Having such an attribute implies a possibility to "activate" at some point later (i.e. restore). Another possible operation for an "inactive" content may be actually a permanent deletion for the sake of good data housekeeping.

Of course the above can be achieved using an extra field like "status" or similar. This comes with some extra work needed to implement this, including a custom scope to hide inactive records. The amount of work needed is probably 95% repeats what is already available with soft deletion feature. So it comes down to the question - can soft delete cover the 5% change or that would be a form of featuritis?

It makes sense to extend the existing handling of soft-delete models to allow for a forceDelete() call on the model being deleted. This could be done with minimum modifications using an extra data parameter on the delete request (like post('permanentDelete')) inside FormController's respective method.

Will this change be backwards-compatible?

No response

@lex0r lex0r added needs review Issues/PRs that require a review from a maintainer Type: Conceptual Enhancement labels Sep 24, 2024
@mjauvin
Copy link
Member

mjauvin commented Sep 24, 2024

@mjauvin
Copy link
Member

mjauvin commented Sep 24, 2024

Also see the documentation:
https://wintercms.com/docs/v1.2/docs/database/traits#soft-deleting

@mjauvin mjauvin closed this as completed Sep 24, 2024
@mjauvin mjauvin added Type: Question and removed Type: Conceptual Enhancement needs review Issues/PRs that require a review from a maintainer labels Sep 24, 2024
@lex0r
Copy link
Contributor Author

lex0r commented Sep 24, 2024

@mjauvin I'm sorry I wasn't clear enough about what I would like to see as a feature in Winter CMS.

You are right about the existence of forceDelete(), which I did mention in the issue.

My request is about the gap that exists between forceDelete() and backend UI backed by FormController. Current FormController implementation doesn't care about whether the model which data it handles is a soft delete or non-soft delete one. As the result, it's impossible for a developer to easily (for example by adding a relevant button parameter) ensure permanent deletion of soft-delete content. One has to either come up with a solution based on model.afterDelete event to actually force delete the model, or override update_onDelete() implementation of FormController to do the same (or maybe use formAfterDelete()).

There's an easy win to bridge this gap out of the box by allowing to forceDelete() using declarative approach of defining an extra data-request-data on the deletion button.

FYI @bennothommo @LukeTowers

@mjauvin
Copy link
Member

mjauvin commented Sep 24, 2024

You can easily add a button to your controller's update.php view like below:

    <button
        class="btn btn-default oc-icon-refresh"
        data-request="onPurgeDeleted"
    >REALLY DELETE
    </button>   

And just add a onPurgeDeleted() method to your controller.

public function onPurgeDeleted($id)
{
    $this->formFindModelObject($id)->forceDelete();
}

Am I missing something here?

@lex0r
Copy link
Contributor Author

lex0r commented Sep 24, 2024

Am I missing something here?

FormController's implementation of deletion is a bit longer and I'm afraid developers risk breaking compatibility with it unless they duplicate update_onDelete() and add own custom handling like above. Which is of course an option but I thought this could be extended a little mainly because it's a rather typical use case (SoftDelete trait has forceDelete() for a reason).

It's about developer productivity mostly. Imagine having to do the above in 5+ controllers, or instead have it out of the box accessible with:

data-request-data="forceDeleting: 1"

@LukeTowers
Copy link
Member

@lex0r you could always use a trait to add it to multiple controllers

@lex0r
Copy link
Contributor Author

lex0r commented Sep 25, 2024

@lex0r you could always use a trait to add it to multiple controllers

Well yes, just that it becomes a little clumsy because I will have to override two methods from FormController (the action handler and getLang() which is protected), and keep an eye on both in the unlikely case they change...

@lex0r
Copy link
Contributor Author

lex0r commented Sep 25, 2024

I thought would be nice for FormController to become aware of soft-delete models. They are not uncommon after all.

@mjauvin
Copy link
Member

mjauvin commented Sep 25, 2024

Feel free to submit a PR if you think this can be generic enough to be helpful to the masses

@mjauvin mjauvin reopened this Sep 25, 2024
@bennothommo
Copy link
Member

I'm not opposed to us adding in a check for a forceDelete parameter that could be defined with the data request and interpreted by the onDelete action in the FormController behaviour to run forceDelete as opposed to delete. I think that is what @lex0r is suggesting.

@LukeTowers
Copy link
Member

@bennothommo any ideas for how to protect it from misuse? I wouldn't want to add it and allow it to be triggered by users without the developers realizing by simply including an extra parameter in the delete requests.

@bennothommo
Copy link
Member

The alternative would be to allow it to be configurable in the form config, if you didn't want it to be triggered by a parameter in the request.

@LukeTowers
Copy link
Member

Can anyone provide some sample use cases here for us to consider?

@bennothommo
Copy link
Member

bennothommo commented Sep 27, 2024

A potential use case would be for things that could be deleted (softly) elsewhere, but you wanted the Backend to be a place where an admin could permanently delete it, and instead of having to create a custom delete action or override the Form Controller's method, you just define that parameter in the form config.

For example, I create a Comments plugin to leave comments on a page. I could allow a "delete" action for users to delete their own comments on the front-end, leaving a "deleted message" placeholder in its place, but I could also have a Backend page to review all comments (including deleted ones) with the ability to completely delete the comment if I wish.

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

No branches or pull requests

4 participants