Jumoo / uSync.Complete.Issues

Public Issue tracker and roadmap for uSync.Complete
https://jumoo.co.uk/usync/complete/
2 stars 1 forks source link

Weird (lack of?) Double Escaping of Block List Content when A Contentment Block editor is inside a core Block List. #226

Open marcemarc opened 3 months ago

marcemarc commented 3 months ago

Describe the bug When a Block List is used inside a Block List (I know how awful)

the inner block list needs to be 'double escaped', because it's JSON inside JSON

And if you have a Block List within that Block List within a Block List

You need it to be 'triple escaped'

What we are seeing is that for an Editor that has this lovely triple depth of nesting is that Umbraco saves the content to the database in this triple escaped fashion and all renders ok on the front end...

but when we publish this page to another server via uSync Complete

Then the JSON stored in the database for the item is no longer Triple Escaped...

But, and this is the weird thing, if you open up the page in the backoffice on the instance that has been published to, the backoffice shows all the nested block lists, with all the content at each level intact!

Isn't that bloody brilliant?

But, on the front end, when you use Modelsbuilder to write out the nested blocks, looping through each level it can't work out the nested blocks, because the lack of escaping, and so you don't see any child blocks when looping through them, literally they are not there.

When you press Save and Publish in the backoffice, then Umbraco updates the saved data in the database entry to have the triple level of escaping and Modelsbuilder is happy to deserialise again, and show the blocks.

Here is a screenshot showing, how uSync has sent the data on top, and how Umbraco has stored it in the first place and what it gets changed to when you press save and publish - see the extra \\\\\\\\\\\\\\\\

image

But if you were only ever looking in the backoffice you'd never know that it hadn't worked as expected!

Version (please complete the following information):

To Reproduce Steps to reproduce the behavior: Create a Block List Property Editor on a Page Allow within that another Block List of Repeating Items And allow that to have a Block List that adds further blocks...

Write a View that loops through each level of blocks writing out the content.

Now fill in content for these blocks - save and publish, see how each level is saved in db with multiple levels of escaped-ness See how the nested content blocks are written out in the view.

then push this using uSync Complete to another environment

see in the backoffice of your 'production environment' that the nested blocks have been pushed successfully

then look at the front end of the site and stare in disappointment that only the top level blocks have been rendered

Look in database to see how the saved content no longer has the multi levels of escaped-ness

Save and publish the page in production, marvel how the view now writes out the nested block content and see in the database how this updated version now has multi levels of escaped-ness...

Expected behavior I think I'd expect uSync to persist the multi-level of escaped-ness so when you publish the page in staging, it updates in production, without having to save and publish it in production to correct.

Screenshots Another screenshot...

image

At least it's not nested block list within nested block list within nexted block list within vorto!

KevinJump commented 3 months ago

Sad face emoji.

feels like i have escaped / not-escaped / escaped blocks and nested content and grid elements for past 5 years, there is always one way where it doesn't like it.

we de-escape everything when we process it, so we can process all the subitems, but putting it back obs in this instance isn't right.

This will be a core uSync issue. (its the block list mappers that are not doing things). So i will see what we can do/break/fix.

KevinJump commented 3 months ago

Can you save me the pain of having to get models builder working and share some recursive code. ?

our test pages we use don't use models builder and they are rendering child blocks (and grandchild blocks in side blocks) image

so i need to convert this to models builder. (first step if breaking it to fix it!)

marcemarc commented 3 months ago

I'll try...

Just for replicating all the things

the project has

        "ModelsBuilder": {
            "ModelsMode": "SourceCodeAuto",
            "AcceptUnsafeModelsDirectory": true,
            "ModelsDirectory": "~/../ClientName.Web.Core/Models/CmsModels",
            "ModelsNamespace": "ClientName.Web.Core.Models.CmsModels",
            "DebugLevel": 0
        },

and this generates the models to a class library project folder which is included in the Umbraco Project (I know like dot net framework...)

Then in your Page Document Type that has the BlockListProperty, you will have @inherits Umbraco.Cms.Web.Common.Views.UmbracoViewPage<PageDocumentType>

at the top

Then in your view, this project loops through each of the Blocks...

@if (Model.ContentBlocks != null && Model.ContentBlocks.Any())
{
    @foreach (var block in Model.ContentBlocks)
    {
        @await Component.InvokeAsync("ContentBlock", new { contentBlock = block, siteTheme = SiteThemesOptions.ClientTheme })
    }
}

I'm not sure why it's calling a ViewComponent for each block... something to do with having different themes I think

ahh yes, the ContentBlock ViewComponent does this: so it looks for a themed View

using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.ViewEngines;
using Microsoft.Extensions.Logging;
using RhodesTrust.Web.Core.Extensions;
using Umbraco.Cms.Core.Models.Blocks;
    public class ContentBlockViewComponent : ViewComponent
    {
        private readonly ICompositeViewEngine _compositeViewEngine;
        private readonly ILogger<ContentBlockViewComponent> _logger;

        public ContentBlockViewComponent(ICompositeViewEngine compositeViewEngine, ILogger<ContentBlockViewComponent> logger)
        {
            _compositeViewEngine = compositeViewEngine;
            _logger = logger;
        }

        public IViewComponentResult Invoke(BlockListItem contentBlock, string siteTheme)
        {
            //get partial view name, this will either be the alias of the element type for the block
            // or if it's a shared content type the alias of the picked block
            var publishedContent = contentBlock.Content;

            var blockViewPath = $"Views/Partials/Shared/ContentBlocks/{publishedContent?.ContentType.Alias.FirstCharToUppercase()}.cshtml";
            var blockView = _compositeViewEngine.GetView(null, blockViewPath, false);

            if(!string.IsNullOrEmpty(siteTheme))
            {
                var blockViewPathForTheme = $"Views/Partials/Shared/ContentBlocks/{siteTheme}/{publishedContent?.ContentType.Alias.FirstCharToUppercase()}.cshtml";
                var blockViewForTheme = _compositeViewEngine.GetView(null, blockViewPathForTheme, false);
                if (blockViewForTheme.Success)
                {
                    return View($"~/{blockViewPathForTheme}", contentBlock);
                }
                else if (blockView.Success)
                {
                    return View($"~/{blockViewPath}", contentBlock);
                }
            } else if (blockView.Success) {
                return View($"~/{blockViewPath}", contentBlock);
            }

            _logger.LogError("No partial view found for {blockViewPath}", blockViewPath);
            return Content(string.Empty);
        }
    }

So for each block it renders a View, so for this Accordion Block it's like this

@inject Umbraco.Cms.Core.Strings.IShortStringHelper _shortStringHelper;
@inherits UmbracoViewPage<BlockListItem>
@{
    var contentBlockSettings = Model.Settings as CommonBlockRowSettings;
    var theme = "theme-none";
    if (contentBlockSettings != null)
    {
        if (contentBlockSettings.BackgroundColorTheme != null && !string.IsNullOrWhiteSpace(contentBlockSettings.BackgroundColorTheme))
        {
            theme = "theme-" + contentBlockSettings.BackgroundColorTheme.ToLowerInvariant();
        }
    }
    if (Model.Content is AccordionWithTextOnly accordion)
    {
        if (accordion.AccordionItems != null && accordion.AccordionItems.Any())
        {
            <section class="accordion-block accordion-block-text @theme py-10 py-6-mobile">
                <div class="container is-max-widescreen">
                    @if (!accordion.Title.IsNullOrWhiteSpace())
                    {
                        <h2 class="common-block-title semicircle-large">@accordion.Title</h2>
                    }
                    <div class="accordion" data-component="accordion">
                        @foreach (var item in accordion.AccordionItems)
                        {
                            var accordionItem = item as AccordionWithTextOnlyItem;
                            @if(accordionItem != null)
                            {
                                var uniqId = accordionItem?.KeyStr();
                                var accordionHasContentItems = accordionItem.ContentItems != null && accordionItem.ContentItems.Any();
                                <div class="accordion-box">
                                    <div class="columns">
                                        <div class="column">
                                            @if(accordionHasContentItems)
                                            {
                                                <button class="is-block is-clickable pl-0"
                                                        id="accord-@uniqId"
                                                        aria-expanded="false"
                                                        aria-controls="sect-@uniqId">
                                                    <p class="accordion-title mb-0">@accordionItem.Title</p>
                                                    @if (!accordionItem.IntroParagraph.IsNullOrWhiteSpace())
                                                    {
                                                        <div class="content content pl-7 mt-4">
                                                            @Html.Raw(accordionItem.IntroParagraph)
                                                        </div>
                                                    }
                                                </button>
                                            }
                                            else
                                            {
                                                <p class="accordion-title mb-0">@accordionItem.Title</p>
                                                @if (!accordionItem.IntroParagraph.IsNullOrWhiteSpace())
                                                {
                                                    <div class="content content pl-7 mt-4">
                                                        @Html.Raw(accordionItem.IntroParagraph)
                                                    </div>
                                                }
                                            }
                                        </div>
                                    </div>
                                    <div id="sect-@uniqId"
                                            role="region"
                                            aria-labelledby="accord-@uniqId"
                                            class="accordion-panel hidden"
                                            aria-hidden="true">
                                        <div class="content">
                                            @if (accordionHasContentItems)
                                            {
                                                @foreach (var contentItem in accordionItem.ContentItems)
                                                {
                                                    @await Html.PartialAsync($"Shared/GenericBlocks/{contentItem.ContentType.Alias.FirstCharToUppercase()}", contentItem)
                                                }
                                            }
                                        </div>
                                    </div>
                                </div>
                            }
                        }
                    </div>
                </div>
            </section>
        }
    }
}

where you can see the check to see if it has child items and it renders the third level...

You can see in the HTML of the page that

                    <div class="accordion-box">
                                    <div class="columns">
                                        <div class="column">

is rendered ok

so it is this reading of whether the nested item has nested items that is the issue... var accordionHasContentItems = accordionItem.ContentItems != null && accordionItem.ContentItems.Any();

if that helps... I'm not sure if this is the correct 'modelsbuilder' way, I didn't write it, but basically Modelsbuilder should give you a strongly typed property for each nested property, that you can foreach through...

hmmm, wonder if issue is in modelsbuilder then if it works with StringlyTypedModels?

KevinJump commented 3 months ago

Works for me 😞

i have this view :

@using Umbraco.Cms.Web.Common.PublishedModels;
@inherits Umbraco.Cms.Web.Common.Views.UmbracoViewPage<ContentModels.BlockPage>
@using ContentModels = Umbraco.Cms.Web.Common.PublishedModels;
@{
    Layout = null;

    <h1>@Model.PageTitle</h1>

    @foreach(var block in Model.Blocks)
    {
        RenderBlock((ContentModels.BlockTypeOne)block.Content);
    }
}

@{
    void RenderBlock(ContentModels.BlockTypeOne block) {

        <h2>@block.PageTitle</h2>
        <div>
            @block.BlockContent
        </div>
        <div>
            @foreach(var child in block.Blocks) {
                RenderBlock((ContentModels.BlockTypeOne)child.Content);
            }
        </div>

    }
}

tripple nested list content, pushed between two servers (on to a blank new install, not config changes (so InMemory models builder?).

renders this:

image

marcemarc commented 3 months ago

I wonder if there is something with the specific content then... or in this slightly elaborate approach, I'll try putting a basic loop through the properties as you have to see tomorrow... but we has spent a few hours coming to this conclusion :-( so sad face emoji if issue is in the code!

marcemarc commented 3 months ago

Aha! curveball,

image

Of course, the Inner Block is using ContentBlocks! of course you'd have Block list on the outside, but Contentment Content blocks on the inside... doh

(if only Lee made it clear somehow in the backoffice)

Ok soooooooooo the Property Value Converter for Contentment Blocks has this

        public override object ConvertSourceToIntermediate(IPublishedElement owner, IPublishedPropertyType propertyType, object source, bool preview)
        {
            if (source is string value)
            {
                return JsonConvert.DeserializeObject<IEnumerable<ContentBlock>>(value);
            }

            return base.ConvertSourceToIntermediate(owner, propertyType, source, preview);
        }

When Umbraco publishes the page it is a 'string', but when uSync publishes it, it's a JArray that comes into the property value converter and so the Contentment Blocks Property Value Converter 'skips it'...

sigh... this fixes it:

        if (source is string value)
            {
                return JsonConvert.DeserializeObject<IEnumerable<ContentBlock>>(value);
            }
            else if (source is JArray jValue)
            {
                return JsonConvert.DeserializeObject<IEnumerable<ContentBlock>>(jValue.ToString());
            }

but my guess must be it's an issue in ContentmentContentBlocks mapper?

https://github.com/KevinJump/uSync/blob/v13/main/uSync.Community.Contrib/Mappers/ContentmentContentBlocks.cs

But I can't see how! why the stored value pushed by uSync would be considered a JArray and not a string?

Unless your cleaning of the escaping makes it a valid JArray, but the Umbraco Save and Publish adds lots of escaping and it's no longer a valid JArray and so is determined to be a string instead...

KevinJump commented 3 months ago

Ahhh Soo I had conversations with Lee a long time ago about Contentment Content blocks and escaping (or the lack off) and how it worked in the Backoffice and then not on the site. and we did something (it was a long time ago). So i suspect that is why the code does something odd, and why we are playing the is it a JArray card.

I will have to look back and see what if anything i can recall about why we did what we did.