Content Types: Fix property variation change when content exists only in non-default language#21182
Conversation
… in non-default language (closes #11771) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR fixes a PanicException that occurred when changing a property type's variation (e.g., from invariant to culture-variant) in scenarios where content exists only in non-default languages. The issue stemmed from the system attempting to update DocumentCultureVariationDto records that don't exist for languages in which content was never created.
Key Changes:
- Replaced a
PanicExceptionwith acontinuestatement to gracefully handle missingDocumentCultureVariationDtorecords - Added comprehensive test coverage for property and content type variation changes in edge-case scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs | Changed exception handling in RenormalizeDocumentEditedFlags to skip updating edited flags for non-existent language variant records instead of throwing PanicException |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceVariantsTests.cs | Added 10 new test cases covering property variation changes (invariant↔variant) and content type variation changes when content exists only in non-default languages |
|
This all looks great @nikolajlauridsen in terms of updates, analysis and tests. The only problem is I'm struggling to replicate the original issue to verify that this does indeed solve it. Please could you add manual replication steps, including details of the While I'm looking that, my only thought on the tests you might consider, was that would it be good to have variations with the two options for the |
|
The setup I used for testing was:
If I had AllowEditInvariantFromNonDefault set to false I get an error when I initially change the property to no longer be shared between cultures, if I set it to true I get the same error when I change it back to be invariant, so initially invariant then variant then invariant again -> error occurs I think it's a good idea to add tests with AllowEditInvariantFromNonDefault set to true, because it's actually the primary place it might happen, if it's set to false it'll only happen if you use something like checkbox becaues it has a default value, but if it's set to true it can also happen with a textstring because the editor can change it manually 😄 |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
AndyButland
left a comment
There was a problem hiding this comment.
Thanks @nikolajlauridsen - I've got it now. Did seem that the property type was important. Can confirm I no longer see the problem though with this PR included, and the integration tests look good to me now coverage wise.
… in non-default language (#21182) * Content Types: Fix property variation change when content exists only in non-default language (closes #11771) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * Add tests for AllowEditInvariantFromNonDefault enabled scenario 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]>
… in non-default language (#21182) * Content Types: Fix property variation change when content exists only in non-default language (closes #11771) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * Add tests for AllowEditInvariantFromNonDefault enabled scenario 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]>
|
Have cherry-picked for 16.5 and 17.1. |
Summary
Fixes #11771
This PR fixes a
PanicExceptionthat was thrown when changing a property type's variation setting (e.g., from invariant to "Vary by culture") when content exists only in a non-default language.Root Cause Analysis
The Scenario
PanicExceptionis thrownWhy This Happens
When a property type's variation changes,
ContentTypeRepositoryBase.RenormalizeDocumentEditedFlagsperforms two key operations:CopyPropertyData: Migrates property data to the appropriate locationLanguageId = defaultLanguageId)LanguageId = null)RenormalizeDocumentEditedFlags: Updates the "edited" flags for affected documents(nodeId, languageId)DocumentCultureVariationDtoto update its edited flagThe Bug
After
CopyPropertyDatamigrates invariant property data to the default language,RenormalizeDocumentEditedFlagstries to find aDocumentCultureVariationDtofor the default language. However:DocumentCultureVariationDtoexists for the default language because the content was never created in that languagePanicExceptionwhen it couldn't find the expected recordWhy
AllowEditInvariantFromNonDefaultAffected ThisThe
AllowEditInvariantFromNonDefaultsetting determines where invariant property data is stored:truefalseBoth scenarios ultimately hit the same issue: expecting a
DocumentCultureVariationDtothat doesn't exist.The Fix
Changed the
PanicExceptionto acontinuestatement inRenormalizeDocumentEditedFlags.Why this is safe: The purpose of this code is to update the "edited" flag on
DocumentCultureVariationDtorecords. If no such record exists for a language (because content was never created in that language), there's simply nothing to update. The property data migration itself succeeds - we just skip the edited flag update for non-existent language variants.Test Plan
Added 20 new integration test cases covering both
AllowEditInvariantFromNonDefault = false(default) andAllowEditInvariantFromNonDefault = true:With
AllowEditInvariantFromNonDefault = false(default)With
AllowEditInvariantFromNonDefault = trueAll 105 tests in
ContentTypeServiceVariantsTestspass (85 existing + 20 new regression tests).🤖 Generated with Claude Code