-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Service refactoring to "fully" enable segments #19114
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
Conversation
…ment-serverside-refactor
…ment-serverside-refactor
…ment-serverside-refactor
…ment-serverside-refactor
…ment-serverside-refactor
…ment-serverside-refactor
AndyButland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments for consideration from visual inspection - I'll move on now to testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating, editing and saving all seems to work as expected. Have tested creation and update of the following content:
- Fully invariant documents - ✅
- Culture-only variant documents - ✅
- Segment-only variant documents - ✅
- Culture and segment variant documents with
- Invariant properties - ✅
- Culture-only variant properties - ✅
- Segment-only variant properties- ✅
- Culture and segment variant properties - ✅
I found the following small issues:
- When publishing segmented content, we get these messages, which are a little weird. Should probably use the segment names if we could (like we would have the language names for culture variant content). If I understand correctly, we don't actually independently publish segments like we do cultures? If that's the case, probably shouldn't mention the segments at all.
Similarly for culture and segment variant:
- Status lozenges in the content editor aren't quite right. E.g. for this setup:
In the content section the first two are indicated as "Shared" with no indication on the third.
Then when you edit a particular segment, the third has a "Shared across culture" indication (and not, as you'd expect, "Shared across segments").
|
Thanks for testing @AndyButland 👍 I'll add some follow-up tasks to amend your findings, unless you find them blocking enough to postpone merging this PR in? |
AndyButland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, as discussed these are fine to consider as later improvements.



Prerequisites
Description
#19060 enables editing segmented documents. At least - to a point. The server-side modelling for document create and update operations do not fully support segments; specifically, the service layer struggle with properties that only varies by either culture or segment for doctypes with both culture and segment variance enabled.
There is no subtle fix here - we need to break things 😞
With this PR, the service layer models now align more with the API request models, which they probably should have done from the beginning.
This means that the
InvariantNameandInvariantPropertieshave been removed from the create/update models, and likewise thePropertiescollection on the variant models.To replace these:
Variantscollection, withnullvalues forCultureandSegment.Propertiescollection has been introduced on the create/update models. This collection now holds both the invariant and variant properties.Testing this PR
Test that any combination of invariance, culture and segment variance can be created and edited - that is:
Retrofitting custom code to counter the breakage
If you're using any of the "new" content editing services from V14 (
IContentEditingService,IMediaEditingService,IMemberEditingService) you will likely be affected by these changes.When managing invariant content, you'll need to add an "invariant variant" (with
nullvalues forCultureandSegment) to theVariantscollection on the create/update models, instead of usingInvariantNameandInvariantProperties(which have been removed).Any and all properties (both invariant and variant) go into the newly introduced
Propertiescollection on the create/update models. Notice that thePropertyValueModelnow featuresCultureandSegmentproperties for variant property values.