search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[D8][UX] Add option to delete content when deleting a content type. #2461

Open docwilmot opened 7 years ago

docwilmot commented 7 years ago

A checkbox on the confirm form would do.

PR by @klonos (disallows deleting a content type while content of that type exists): https://github.com/backdrop/backdrop/pull/2417 Alternative PR by @klonos (adds a confirmation checkbox, to allow deleting all content of the type to be deleted): https://github.com/backdrop/backdrop/pull/2420

docwilmot commented 7 years ago

More info:

cntnt type delete

foo deleted

This long discussion on Drupal led to the decision to actually block deletion of entity types with remaining content (link to a specific suggestion, same as this issue): https://www.drupal.org/node/232327#comment-3274338

We probably need to repurpose this issue to what to do about deleting content types with remaining content, but for now I'll leave the title as this subset of that discussion.

Dinornis commented 7 years ago

Great idea.

klonos commented 7 years ago

It would be really great if we offered the option to export/save the data before deletion. Sorry, I come from a data-security background and I am always (over)conscious when destructive operations are being performed over data: #2498

docwilmot commented 7 years ago

I've been a bit conflicted over this issue actually. The Drupal way of preventing deletion of the content type if there are remaining nodes makes sense; it would prevent data loss better than the option presented here.

2498 is a good idea long term, but still doesnt help this issue. Even if you trash/unpublish the nodes, you cant delete the content type as you would still be left with ghost nodes. Except if we also find a way to trash content types and fields instead of deleting them.

Similarly, exporting will only work if we can also export the content types and fields too.

jenlampton commented 7 years ago

I'd love to see a UI Improvement that prevents deleting a content type until all the nodes have been deleted, but since we are only 1 day from feature freeze, it's unlikely the new feature "option to delete content" will make it into this release.

I'm going to bump the milestone on this feature to 1.8, but if someone remains motivated enough to fix the content deletion bug on 1.7 without the new feature, please create a new issue and add the milestone.

klonos commented 5 years ago

I have reread the comments in this issue, and here are my thoughts:

...Except if we also find a way to trash content types and fields instead of deleting them.

I like this idea. Here's what we could do:

...the above is not a prerequisite for moving this ticket forward (it is in fact a separate issue on its own), but would limit the use cases where a content type would need to be deleted to only when people are actually determined to get rid of the content type + all the respective content. So then we'd then only need to decide to either:

...or:

jenlampton commented 5 years ago

@klonos that sounds like a lot of work. Sanity check: is this an 80% use-case feature? I'd rather see the fix that prevents types from being deleted while there is still content, and the rest of this be handled in contrib.

klonos commented 5 years ago

OK, I took a stab at a PR...

klonos commented 5 years ago

...here's current D8 for reference btw:

screen shot 2018-12-12 at 11 15 41 am screen shot 2018-12-12 at 11 16 12 am
olafgrabienski commented 5 years ago

To remove this content type, you first need to remove all content of that type.

Great, that there's a link to the admin/content page on the "remove" part of the sentence. I'd suggest to filter the page to only display items of the respective content type (as in the result of admin/content/node?status=All&type=MY-TYPE).

klonos commented 5 years ago

...after popular demand (😜), an alternative PR that allows deleting content of the type to be deleted (with confirmation via a checkbox):

Screenshots (include fixes for #3422 and #3425):

...after confirming and deleting:

screen shot 2018-12-12 at 4 44 56 pm
docwilmot commented 5 years ago

i like this best

docwilmot commented 5 years ago

I would use "delete" instead of "remove" though, just to be perfectly clear.

klonos commented 5 years ago

...I'd suggest to filter the page to only display items of the respective content type...

Great suggestion @olafgrabienski 👍 ...I've updated the PR to include that.

I would use "delete" instead of "remove" though, just to be perfectly clear.

Yep, I missed that @docwilmot. Fixed in both PRs 😉

olafgrabienski commented 5 years ago

I've tested https://github.com/backdrop/backdrop/pull/2420 in the sandbox. After clicking the button Delete all content and the content type I got this message:

  • Deleted 2 pieces of content of type Article.
  • The content type Article has been deleted.

As a result, the articles have been deleted but the content type not.

Edit:

olafgrabienski commented 5 years ago

Small wording suggestion, to not use "of" two times:

Deleted 2 pieces of content of type Article.

Deleted 2 content items of type Article.

klonos commented 5 years ago

Hmm, you are right @olafgrabienski. This was working on my local, but is not working in the PR sandbox. I'll take a look and see if I can figure out what's wrong.

Thanks

jenlampton commented 5 years ago

I'm not able to get this PR working: https://github.com/backdrop/backdrop/pull/2420

jenlampton commented 5 years ago

Removing the 1.12 milestone, but adding the tag for review later.

jenlampton commented 5 years ago

removing [D8 feature parity] from title, adding into project.

klonos commented 4 years ago

I started working on this again...

@olafgrabienski you are right, content item(s) of type ... sounds better than piece(s) of content of type... in this instance. Updated the PR accordingly.

I'm not able to get this PR working: backdrop/backdrop#2420

I think that it needed to be rebased @jenlampton. Can you please give it another go now?

klonos commented 4 years ago

...leaving the NW label, as I plan to implement the following:

Would still like feedback on current implementation though.

alanmels commented 4 years ago

I've just tested this on http://2420.backdrop.backdrop.qa.backdropcms.org/admin/structure/types and unfortunately it still does not delete the content type itself. The website's log shows: 

Error
Argument 2 passed to t() must be an array, integer given, called in /home/qa/github/backdrop/backdrop/pr/2420/core/modules/dblog/dblog.admin.inc on line 310 and defined

Is the version on http://2420.backdrop.backdrop.qa.backdropcms.org the most up-to-date or should I download the PR to localhost to test?

jenlampton commented 4 years ago

~@alanmels The current PR (at http://2420.backdrop.backdrop.qa.backdropcms.org/) needs to be rebased.~ I'm not sure if that will fix the problem you are seeing, but the results will be the same if you download the PR to test locally.

edit: Oh I read that wrong. Looks like this PR was just rebased, so the problem you are seeing is probably real, and still needs to be addressed.

alanmels commented 4 years ago

@jenlampton, thanks! Then I'll try later.

philsward commented 3 years ago

Any progress on this?

I don't think this issue is a show-stopper but it does create the potential for a lot of bad UX for users. Most content types are deleted early during restructuring, however if there is a content type that is deleted and other nodes link to pages of the deleted content type, you're presented at most with a bunch of PHP errors instead of a proper 404.

At the very least you get:

site.com
**Error**
Call to a member function fetchCol() on null

This isn't very welcoming if a normal user navigates to this page.

One thought as a temporary workaround until this can be fixed proper is to present an error that is more friendly, wraps the error in the sites theme and says "The page you are looking for is pending deletion.". Put a simple check in the code that says "if content_type = null, show pending deletion message". (I'm not a coder, but hopefully that makes sense)

The other temporary workaround thought is to move all of the content to a reserved (hidden) content type and unpublish them. This should give an access denied error I believe which is better than the current error UX.

As far as what @klonos has suggested in https://github.com/backdrop/backdrop-issues/issues/2461#issuecomment-446612198, I personally love that implementation. It gives an actionable confirmation to the user and it is clear on what needs to happen.

I will say, it might be really cool if it could traverse through all the other modules that touch the content type. I don't think it needs to hard stop, just show the number of entities that are being deleted and then show something like:

In addition, deleting this content type will affect the following:

Maybe add a checkbox to confirm?

In addition, deleting this content type will affect the following:

stpaultim commented 3 years ago

Adding this to my watch list. Seems like a problem that needs a solution and it seems like we've gotten close to solving it in the past.

As far as I can tell, the next step is for someone to look at the most recent PR and see if they can determine why it's not working as expected.

indigoxela commented 3 years ago

@klonos I've left two comments at your PR.

philsward commented 3 years ago

One thing I've wondered regarding this particular issue, how are entities in general handled in this regard?

Here we're talking about content types and nodes, what about paragraphs, or field collections or commerce products as examples? Should the same or a similar approach be taken with all entities or am I over thinking it?

Paragraphs probably won't really be an issue because there isn't an entity type container, but products with commerce (for example) will because the products are housed within an entity type. I say that... The more I think about it, commerce may be a bad example because a product is locked from deletion once it's been added to an order. I have very little experience with field collection as Paragraphs became my primary go-to.

Mainly just bringing up the idea that this may need to extend beyond nodes or at least have the ability for other modules to tie into its functionality.

indigoxela commented 3 years ago

Mainly just bringing up the idea that this may need to extend beyond nodes

If your concern's about things attached to nodes - that's a job for hook_node_delete/hook_entity_delete which (contrib) modules are supposed to implement.

Custom file types might deserve some attention. But I suppose, that could/should be a follow-up issue.

philsward commented 3 years ago

If your concern's about things attached to nodes ...

No, the concern is with contribs that create their own entities inside their own entity types.

ECK should be an excellent example though I don't know if it's been ported yet.

klonos commented 3 years ago

Right. Thanks for reviving this @philsward 👍 ...I should get back to it then.

klonos commented 3 years ago

...also thanks @indigoxela for the PR review, and @jenlampton, @alanmels, and @stpaultim for the feedback, testing, and ideas 🙏

I also find this a handy UX improvement, and in the meantime one of the 2 issues that were "supplementing" this has been fixed/merged (talking about #3422 and #3425). I'll get back to working on this over the next few days (the weekend most likely). Thanks again all.

stpaultim commented 3 years ago

@klonos - is there room for this on your to-do list?

klonos commented 1 year ago

A belated "yes!" to this one ...I almost got bitten by it again while on a content type deleting spree. When performing such a repetitive task, it is easy to miss the fact that a type has existing content, and instead treat it the same as the other content types (blindly hitting the "delete" button). I'll need to work on finalizing #3422 first though.