U4-11286 - DIscuss: Update the variant/invariant name on each content save or on each read

Created by Shannon Deminick 30 Apr 2018, 10:17:42 Updated by Shannon Deminick 03 May 2018, 05:20:42

Subtask of: U4-11278

This should be done at the DocumentRepository level

*When you have a variant content item and you save it, we need to update the Invariant name with the default language variant name *When you have an invariant content item and you save it, we need to update the Default language Variant name with the Invariant name BUT only if more than one language exists

OR

*When we read an IContent or IEntity via the content/entity service we update the appropriate 'name' so that it is not null which will happen when languages are added/removed/changed or doc types are changed by variation ... probably a more stable approach since it solves the complicated cases at least for editors to continue editing content.

TODO: Create a task to complete this

Comments

Stephan 01 May 2018, 09:04:07

The first choice means we are going to create database rows that we don't really need. And... it's creating "magic" in the database which could be hard to deal with later on. I'd much rather "sanitize" things when reading.


Stephan 01 May 2018, 09:09:17

Copying discussion from Slack: if we just SetName(name, culture), then there is no invariant name, and saving will throw - so, should it be a rule that an invariant name is required? this is weird when the content is known to vary by culture - so would it be fine, when setting a variant name, to ensure that the invariant name is not null? and at what level should it happen?


Shannon Deminick 02 May 2018, 03:09:43

Something else to add to this discussion is that if Name is null, we get other issues. For example in the PublishedContent ctor: public PublishedContent(ContentNode contentNode, ContentData contentData, IPublishedSnapshotAccessor publishedSnapshotAccessor) we do this: _urlName = _contentData.Name.ToUrlSegment(); to store a UrlName but this will throw because Name is null. So then the next question is, if we allow a null name, then we would need to allow a null UrlName, else, if we don't allow a null invariant Name, then we'll always have a non null invariant UrlName even if the content item is variant.


Shannon Deminick 02 May 2018, 05:44:21

@zpqrtbnk I had to 'fix' a few things to continue working which i'd like you to have a look at, we can make adjustments later but is part of this discussion.

*In rev: 9843f3a5fd6fb2f4e2d017a37e6d149984254595 - There's some notable things I've fixed: **not being able to empty the recycle bin **making sure Name can never be empty at the DocumentRepository level (in the next commit another update is required for the PublishName property) **ensures domains cache is refreshed when changing languages or deleting content that has a domain assigned **PublishedRouter is updated to filter out any domains assigned to content that is not published *In rev: 6fe9c4d4dfbacba978d2b040f0471dc504327722 **Fixed PublishName from never being null, without this you cannot actually create any variant content since it will try to generate a URL which is always based on the invariant PublishName **A mistake was found in DocumentRepository where contentVersionDto.Text = content.Name when it should be contentVersionDto.Text = content.PublishName since it's being published when persisting new content

I've added a lot of comments and fixme to all of this code


Stephan 02 May 2018, 15:45:18

Reviewed. Happy with most changes. No, all actually.

It feels a bit weird to manage invariant Name in the repository, and invariant PublishName in the content item, but with appropriate comments cross-referencing each other (which I'm adding) I think its fine for now. So the idea would be:

  • when we save, ensure we have an invariant name, get it from variant names if necessary (in repository)
  • when we set publish infos (ie when we set publish names), ensure we set an invariant name (in content)

I have also cleared a few //fixme and pushed a cleanup commit.

At that point, all happy. Moving to review.


Stephan 02 May 2018, 15:59:29

PR https://github.com/umbraco/Umbraco-CMS/pull/2611


Shannon Deminick 03 May 2018, 05:20:38

OK i will close this one then, will assume you have added appropriate comments.


Priority: Task - Pri 1

Type: Task

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version:

Sprint: Sprint 84

Story Points: 1

Cycle: 10