Node in the Recycle Bin keeps a Published status

wtct commented 1 year ago

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

10.4.0, 11.2.0

Bug summary

Hi Guys,

We noticed an issue a long time ago. Finally, we figured out that we should report an issue :)

Deleted node (moved to the Recycle Bin) has the Published status, if it was previously published, as shown below:

Probably, it could make sense for some reason. Unfortunately, the same restored node has the Unpublished status. Then, it looks like, it no longer makes any sense :)

It's a little bit weird for us, because this behavior introduces the "Published and Trashed" state.

Hopefully, the "Published and Trashed" node is not accessible through IPublishedContentCache/IPublishedContentQuery.

However, it looks like you need to check the IPublishedContentCache in order to determine if it is really published or not, because IContent.Published is still true.

Please consider if the "Published + Trashed" state makes any sense.

Probably, when deleting a node, it should be unpublished first, then moved to the Recycle Bin.

We can see a comment in the ContentService:

https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Core/Services/ContentService.cs#L2398-L2400

Unfortunately, it's hard to understand it :)

Best!

cc: @piotrbach

Specifics

No response

Steps to reproduce

Publish a node.
Delete this node/trash/move to the Recycle Bin.
Open a node in the Recycle Bin.
Check status.
Restore this node.
Open this node.
Check status once again.

Expected result / actual result

No response

github-actions[bot] commented 1 year ago

Hi there @wtct!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
We'll replicate the issue to ensure that the problem is as described.
We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

elit0451 commented 1 year ago

Hi @wtct 👋

Thanks for reaching out! I've tested this and the deleted node with the "Published + Trashed" state is not accessible through either IPublishedContentCache or IPublishedContentQuery, however, to make it more intuitive, it would make sense to change the state to unpublished when deleting a node as it says in the ContentService.

So I will go ahead and mark this as up for grabs as we would like some help with it 😊

github-actions[bot] commented 1 year ago

Hi @wtct,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

mennomout commented 1 year ago

@elit0451 @wtct I've made a pull request for this issue: https://github.com/umbraco/Umbraco-CMS/pull/13998

I do believe this change might require some more discussion. I'll see what feedback I get from the core team and update this issue with relevant information if necessary.

nul800sebastiaan commented 1 year ago

I believe the way it works currently is quite on purpose to help editors when they accidentally delete something, they restore it from the recycle bin and it's back exactly like it was before, without having to remember to also publish it again.

To make matters worse, accidentally deleting a tree of nodes, I believe with the updated code from the PR will not unpublish any descendants. Also.. do you REALLY want to do a heavy unpublish operation on a tree with possibly hundreds of descendants? And also, again, if the editor made a mistake and needs to restore the deleted tree, they'd again have to wait for hundreds of descendants to go through the heavy publish operation again.

wtct commented 1 year ago

Hi @nul800sebastiaan,

I believe the way it works currently is quite on purpose to help editors when they accidentally delete something, they restore it from the recycle bin and it's back exactly like it was before, without having to remember to also publish it again.

Unfortunately, a restored node will not be in the same state after restoring. Please check the bug summary.

cc: @piotrbach

nul800sebastiaan commented 1 year ago

Ah yes I see, but the second part of my comment still applies.

mennomout commented 1 year ago

@nul800sebastiaan Thank your for the reply.

Unpublishing descendants is something that can be added to the PR of course. However I do agree that this will impact the speed of deleting items due to the unpublish action. Currently that action is still done when recovering a trashed item.

One option would be to change the 'status' label for content in the bin from "published" or "unpublished" to "trashed". Correct me if I'm wrong but the published or unpublished statuses are not relevant when a content item is in the recycle bin. Mainly because currently when you restore a trashed item it is unpublished by default. If it's about the last state an item was in before it was trashed then you can still see it's history and derive if it was published or unpublished.

nul800sebastiaan commented 1 year ago

Thanks again all for the discussion. I understand that this is not quite expected and as noted, it works the way it works right now to keep things working in a performant way. As this is not really causing any issues for most people, we would love to hear more about the real-life use cases where people run into this problem before we consider if and what we should do about it.

Of course I totally understand the "something unexpected happened and I don't like it argument" but we'd love to hear some more actual scenarios to figure out what direction we should go, if any direction.

Thanks in advance!

Migaroez commented 3 months ago

This issue is being closed as no new insights have been given in over a year nor have we had any major requests to change the labeling as was suggested by @mennomout

If anyone still feels strongly about this then feel free to open up a discussion.

umbraco / Umbraco-CMS