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

v8: Unpublishing a variant does not trigger Unpublished event #5410

Closed
natashadecanha opened this issue May 7, 2019 · 10 comments
Closed

v8: Unpublishing a variant does not trigger Unpublished event #5410

natashadecanha opened this issue May 7, 2019 · 10 comments

Comments

@natashadecanha
Copy link

natashadecanha commented May 7, 2019

Bug summary

The published event triggered every time I unblished a page. So I took a look in the code and saw that in the UnpublishCulture the PublishedState is set to Publishing instead of Unpublishing. So likely this is the problem. I tested it and it works on my machine :)

So I propose the following pull request for change. But I cannot be sure if this is on purpose or an accidental oversight or wat the repercussions are.
PR here

Specifics

Steps to reproduce

public void Initialize()
{
ContentService.Published += ContentService_Published;
ContentService.Unpublished += ContentService_Unpublished;
}

  • Initialize the published and unpublished event.
  • unpublish a page

Expected result

Unpublished event triggers

Actual result

Published event triggers

*Edit:
After some more testing, I noticed that it only fails if you have a multilingual doctype ("Allow varying by culture"). See added screenshots

Without "allow varying by culture"
You can see that when I unpublish a page, the unpublish event gets triggered, as expected.
2019-05-28 10_17_50-Window
With "allow varying by culture"
You can see that when I unpublish a page, the publish event gets triggered, as not expected.
2019-05-28 10_24_09-Window

@linuselander
Copy link

I agree with @natashadecanha that it is a bit confusing that the act of unpublishing a culture variant triggers the Published event. However, it would also be confusing (and possibly even worse) if an Unpublished event was triggered. The event is related to the IContent rather than the individual culture variants so the Unpublished event could be misleading.

With "Allow varying by culture" the ContentService.Unpublished seems to be triggered when all culture variants are unpublished at the same time. I think this is correct since this means that the entire IContent is in fact Unpublished. However, this should also be the case when the last culture variant is unpublished. Currently, unpublishing the last variant triggers the Published event.

Maybe there is a need for specific events such as PublishedCulture and UnpublishedCulture with dedicated EventArgs?

@PerplexDaniel
Copy link
Contributor

@linuselander Makes a valid point about the whole IContent not being unpublished when only unpublishing a subset of cultures. However, I do not agree it is therefore confusing to trigger an Unpublish event. It literally is an unpublish action, it just happens to unpublish only part of the content.

Like @linuselander says, I think Umbraco should just expand the PublishEventArgs to include information about the current state of cultures on the IContent (published/unpublished) and probably also a PublishedState of some kind (e.g. Unpublished / Partially Published / Published or so) as a shortcut for the developer.

It should actually also be renamed to UnpublishEventArgs, as the as the Published / Publishing events do not even use it -- they use Content{Published,Publishing}EventArgs.

@rekony
Copy link

rekony commented Mar 10, 2020

Hi Team, any updates on this issue?

@TimGeyssens
Copy link
Contributor

Also hit by this, how can one see if a variant is being unpublished to do some custom stuff?

@mzajkowski
Copy link

We've been hit by this issue too :( Would be lovely to get any update on IF/WHEN it can be looked at? Seems like working with varianted content and processing it outside of the "standard" way is impossible (at least without extreme hacking what is risky as usual..).

@Shazwazza
Copy link
Contributor

Shazwazza commented Dec 2, 2020

"Unpublishing" a variant doesn't unpublish IContent, it just saves the IContent (which contains variants) to exclude that variant from having a published version.

There's a lot of design things going on here. PublishedState works in many cases as a 'transitory' value to indicate what is happening to that instance of the model, not what the persisted snapshot is. There are many notes here https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Core/Models/PublishedState.cs#L9

Unpublishing one single cuture happens via a re-publishing of the document, without that culture.

(see #5409 (comment))

Unpublishing the entire IContent item is the only time Unpublishing triggers. Variants are not IContent, variants are just sub data items of IContent. We cannot 'just' have the Unpublishing event fire for changing a variants published version state, this will have huge repercussions and breaking changes. Though there's a lot of 'rules' around how an IContent gets unpubished that I can't recall off the top of my head but taking into account things like unpublishing mandatory cultures and default cultures, etc...

What can you do now? Possibly look at ContentServiceEventTests, perhaps what you need to do is in there with things like IsCulturePublished, HasPublishedCulture and HasUnpublishedCulture (part of ContentPublishedEventArgs which has notes: https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Core/Events/ContentPublishedEventArgs.cs).

Please reply here if that does/doesn't do what you require. If it doesn't, If you want to dig deeper into the code to see what can/can't/should be done. Most of this stuff happens in ContentService. You'll see a lot of notes in the CommitDocumentChangesInternal which needs to cater for a lot of the rules imposed by having mandatory and default cultures. There are lots of tests for this: ContentServiceTests, ContentServiceEventTests, ContentServicePublishBranchTests, DocumentRepositoryTest, VariationTests

Is this confusing? Yes I agree that it is and there should be a nicer and documented way to know when and which cultures have been 'unpublished'. Is this design weird? Yes I agree but dealing with breaking changes and trying to minimize impacts of moving between major versions comes with it's own pitfalls and weird things need to be done without vastly overhauling the DB schema, services and events (which we'll need to do at some point). I'm sure in the meantime it's possible to make this easier, or at least just documented if what you need already exists as examples in those tests.

@Shazwazza Shazwazza changed the title v8: Unpublishing content does not trigger Unpublished event v8: Unpublishing a variant does not trigger Unpublished event Dec 2, 2020
@TimGeyssens
Copy link
Contributor

Yup, some docs to make sure people don't end up spending time on debugging this would be ace!

@marcemarc
Copy link
Contributor

Just to add to this, by setting a Mandatory language - then unpublishing that, unpublishes everything, and the Unpublish event 'does fire'...
and for my circumstance, although I was about to suggest an unpublishing variant event - it's actually really good that unpublishing of the variant triggers the publish - I just didn't realise it til now!

Have added some docs on how you work with variants in events here - if that makes sense to people?

umbraco/UmbracoDocs#3019

@Shazwazza
Copy link
Contributor

The reason that occurs is because unpublishing a mandatory language effectively unpublishes the entire node because nothing can be published if a mandatory language is not published. Variants don't have the notion of published or unpublished, only the node does. Variants are either:

  • not created
  • created + available
  • created + not available

They cannot be un-created.

@nul800sebastiaan nul800sebastiaan added state/needs-investigation This requires input from HQ or community to proceed and removed state/hq-discussion labels Mar 25, 2021
@emma-hq
Copy link
Contributor

emma-hq commented Mar 30, 2021

Thanks for all the chat on this issue, peeps. I think there's a good case for a docs update here but the issue itself can be closed.

Emma

@emma-hq emma-hq closed this as completed Mar 30, 2021
@umbrabot umbrabot removed the state/needs-investigation This requires input from HQ or community to proceed label Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests