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

Set the current workflowPlace of the ContentWorkflow only on a specific DimensionContent #92

Closed
niklasnatter opened this issue Feb 25, 2020 · 4 comments · Fixed by #144
Labels
Technical Debt Impacts only code quality, no or just small impact on end developers and users
Milestone

Comments

@niklasnatter
Copy link
Contributor

niklasnatter commented Feb 25, 2020

The ContentWorkflow service allows to apply a transition on a contentRichEntity and given dimensionAttributes. Because the currentPlace of an contentRichEntity might differ based on the locale, the currentPlace is stored on the DimensionContent entity. This makes sense, as the english version of a page might be published while the german version is still in review.

In most setups, there are 4 DimensionContent entities that are related to a specific locale:

  • Draft Stage / Unlocalized
  • Draft Stage / Localized
  • Live Stage / Unlocalized
  • Live Stage / Localized

Right now, all of these DimensionContent entities contain a currentPlace property, but the ContentWorkflow only updates the currentPlace property of the Draft Stage / Localized DimensionContent. This means the database will contain the following values for the currentPlace attributes, if a contentRichEntity is published in a specific locale:

  • Draft Stage / Unlocalized: unpublished
  • Draft Stage / Localized: published
  • Live Stage / Unlocalized: unpublished
  • Live Stage / Localized: unpublished

In my opinion, it is very confusing that the Draft Stage / Localized DimensionContent is the only entity that contains the correct currentPlace. We should adjust this behaviour to one of the following strategies:

  • Update the currentPlace of all entities. This is problematic because there is no "right" value for the unlocalized DimensionContent
  • Set the currentPlace only on the Draft Stage / Localized DimensionContent. Set the currentPlace of all other DimensionContent entities to null
@niklasnatter niklasnatter added the Technical Debt Impacts only code quality, no or just small impact on end developers and users label Feb 25, 2020
@wachterjohannes
Copy link
Member

wachterjohannes commented Feb 26, 2020

in my opinion it should look like this:

  • Draft Stage / Unlocalized: null
  • Draft Stage / Localized: draft
  • Live Stage / Unlocalized: null
  • Live Stage / Localized: published

@wachterjohannes wachterjohannes added this to the 0.3 milestone Mar 23, 2020
@niklasnatter
Copy link
Contributor Author

niklasnatter commented Mar 23, 2020

After thinking through this a second time, I have the opinion that we should save the currentPlace only on the Draft Stage / Localized DimensionContent and set the currentPlace of all other DimensionContent entities to null.

I think setting an additional currentPlace on the Live Stage / Localized DimensionContent does not provide any value and makes things harder to understand because of the following:

  • If something is unpublished, we delete the Live Stage / Localized DimensionContent. Therefore the currentPlace Live Stage / Localized would always be published at the moment.
  • We need a single source of truth about the currentPlace for given dimensionAttributes. The ContentWorkflow already uses the Draft Stage / Localized DimensionContent as single source of truth. It would be confusing that the currentPlace of the Live Stage / Localized is set even if it is never used.
  • In my opinion, the currentPlace should be used only in the context of the ContentWorkflow. No other service should be allowed to set/change this property. We should not determine if something is published based on the currentPlace.

@luca-rath
Copy link
Contributor

It's necessary to have workflowPublished on the Live Stage / Localized DimensionContent to allow sorting by published date.

I'm using following code to evaluate publishedState of a DimensionContent.
That code should work with both, DRAFT and LIVE dimensions.

  • getPublishedState() === true means published
  • getPublishedState() === false && getPublished() !== null means unpublished, but a published version exists.
  • getPublishedState() === false && getPublished() === null means unpublished, without a published version.

The SmartContentItem js container uses that logic to display the published indicator for each item.

public function getPublished(): ?\DateTimeInterface
{
    $contentProjection = $this->getContentProjection();
    $dimension = $contentProjection->getDimension();

    if (null === $dimension->getLocale()) {
        return null;
    }

    if (!$contentProjection instanceof WorkflowInterface) {
        return null;
    }

    return $contentProjection->getWorkflowPublished();
}

public function getPublishedState(): bool
{
    $contentProjection = $this->getContentProjection();
    $dimension = $contentProjection->getDimension();

    if (null === $dimension->getLocale()) {
        return false;
    }

    if (DimensionInterface::STAGE_LIVE === $dimension->getStage()) {
        return true;
    }

    if (!$contentProjection instanceof WorkflowInterface) {
        return true;
    }

    return $contentProjection->getWorkflowPlace() === WorkflowInterface::WORKFLOW_PLACE_PUBLISHED;
}

Therefore it's necessary to have workflowPlace and workflowPublished set on both, Draft Stage / Localized and Live Stage / Localized.

The workflowPlace and workflowPublished on Live Stage / Localized should NOT always be a copy of Draft Stage / Localized, because the following should be a valid state:

  • Draft Stage / Localized: workflowPlace: draft, workflowPublished: 2020-04-19
  • Live Stage / Localized: workflowPlace: published, workflowPublished: 2020-04-19

One way to achieve this would be a PublishCompletedSubscriber, that copies workflowPlace and workflowPublished from Draft Stage / Localized to Live Stage / Localized.

@niklasnatter
Copy link
Contributor Author

niklasnatter commented Apr 20, 2020

I feel like your comment is related to different issues, so i am trying to split the answer into different parts:

  1. You are stating that your code should work for draft and live dimensions? Why is this the case? Do you need the information on the website? In general, a summary of what you are trying to do would be helpful.

  2. The only place your code uses the workflowPlace is when you already know that the locale of the dimension is not null and the stage is not live. Hence you are only accessing it on the Draft / Localized dimension, right? Furthermore, I think you are using the same logic as in the WorkflowNormalizer in that line, right?

  3. I think I agree with you that there should be a workflowPublished set on the Live / Localized dimension. But this should be discussed in a different issue in my opinion 🙂

  4. Unfortunately, a PublishCompletedSubscriber is not possible easily. This is because the Live / Localized dimension is not flushed and therefore might not exist at this point (for example, when the page is first published). This is even more complex than Emit an event when a ContentWorkflow transition is flushed #129 because we would need a second flush to flush the changes to the Live / Localized dimension 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Technical Debt Impacts only code quality, no or just small impact on end developers and users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants