Sulu: Allow to use relative visibleCondition (or disableCondition) in block

[jordygroote]

| Q | A
| --- | ---
| Sulu version | 2.1.0-RC1
| Browser version | Chrome for Mac versie 81.0.4044.138 (Officiële build) (64-bits)

Actual Behavior

We're trying to hide/show a field on condition of a checkbox in SULU, but nothing happens when toggling the checkbox. This is 2 levels into nested blocks.

<property name="loadSubPages" type="checkbox">
    <meta>
        <title lang="nl">Alle onderliggende pagina's laden</title>
    </meta>
</property>

<block name="subMenu" default-type="subMenuItem" minOccurs="0" visibleCondition="!loadSubPages">

Expected Behavior

When the checkbox is checked the block subMenu should be hidden, and vice versa.

Steps to Reproduce

Configure a XML with the above mentioned properties, nest it in another block. Then try to toggle the loadSubPages, nothing happens.

Possible Solutions

When the checkbox is toggled, hide the other property.

[alexander-schranz]

Sorry currently the conditions gets only the data from the whole form and it will search for loadSubPages at the root form data. We hope we can provide a solution allow relative conditions inside blocks.

A solution I could think of is without breaking exist forms we add a prefix maybe if we go with a json json path a prefilx like: !./my-relative-property could be used for relative variables or something else but we need some identicator for relative props else exist forms could be broken. Another solution could be a magic variable _relative.my-relative-property.

Advantage of ./my-relative-property would be that something like ../my-relative-property could maybe also be supported in the block to access the parent block properties.

/cc @sulu/core-team

[danrot]

I like the idea of using ./my-relative-property, but I don't know if the jsonpointer package we use supports something like this :shrug:

[alexandersch]

I would really like to see this feature. I am trying to do conditional rendering inside a block. Currently visibleCondition/disabledCondition does not work here.

[danrot]

Just started looking at this issue. The first problem we have to solve is the impossibility to have different visible and disabled results per block. The following diagram should give an idea of the way the forms are currently structured and why this is a problem.

The thing is that the AbstractFormStore currently receives the schema from the server, which contains the visibleCondition and the disabledCondition. It already resolves these conditions and replaces the visibleCondition and disabledCondition with visible and disabled boolean values, so that the other parts of the system further down don't have to care about evaluating the conditions anymore.

Unfortunately the schema does not share the exact same structure as the data, especially when it comes to blocks. In the schema representation there is only one schema for a block, and only in the data representation there can be multiple. Since we already resolving the conditions in the schema representation, it is not possible to have a different value for each block, i.e. if a field is disabled or hidden in one of the blocks, it will be the same for all other blocks.

So the solution to this problem is to move the evaluation of the conditions away from the AbstractFormStore. I think a good place for the evaluation of the visibleCondition would be the Renderer, because it already returns null if the field is not visible. The disabledCondition could be
evaluated in the same file, or by applying the same logic it could be evaluated in the Field, in which case we would not even have to change any react interfaces.

The only other question we have left to clarify then, is how the notation for refering to a field within the same block looks like. In general I like the idea from @alexander-schranz with ./property and ../property, but this is not a valid json pointer and therefore also not supported by the library we are using for json pointers. So in order to do that we would have to implement that on our own and make these dots only allowed at the very beginning of the pointer or use something like the canonical-path package. From that point of view I would prefer a special variable. Since we already have __locale, we could also introduce __current, __parent or __block (would avoid the last one because it is very specific, and does not work if we have other fields like blocks at some point). Also, it is questionable how to handle nested blocks. With the .. syntax it would be pretty clear, maybe with the variables we could introduce something like this __parent.__parent.property? Otherwise it won't be possible to base the condition on the parent block.

[nnatter]

Thanks a lot for working on this! I think __current and __parent sounds good.

Would it make things easier if conditions inside of blocks would always be evaluated relative to the data of the current block (block-property for a property in the current block, /root-property for a property in the root of a template)? I know this would be a small BC break and there is also the question how to access the parent in the case of nested blocks.

To use something like the canonical-path package, we would first need to extract all paths of a given condition, right?

[danrot]

The idea with differentiating that by a leading slash might work, but as you already said, there will still be an issue with nested blocks :confused:

The canonical-path package might be useful to simplify some paths that can then be passed to these conditions, and yeah, we could do that in advance once, would save us some processing time later.

[luca-rath]

There exists a draft about relative json pointers here https://tools.ietf.org/id/draft-handrews-relative-json-pointer-00.html, so maybe we should stick to that syntax? The downside, it's also not implemented in the json-pointer library we use and the existing issue https://github.com/manuelstofer/json-pointer/issues/29 is not really active 😬

[danrot]

It's probably a good idea to support the existing standard anyway :-)

[alexander-schranz]

I think I would go with __current and __parent first I thought about maybe converting ../ and ./ into this variables but this magic I think will make more problems as what when I want todo something like "path != "./test" here the ./ or ../ should not getting replaced with __current or __parent. So I would here say we provide the variables instead directly and they can use them.

[nnatter]

The downside, it's also not implemented in the json-pointer library we use and the existing issue manuelstofer/json-pointer#29 is not really active 😬

Have not looked at the code, but I think we dont use the json-pointer library to resolve the paths inside of conditions. I guess we just pass the condition and the data to the jexl library: https://www.npmjs.com/package/jexl

[luca-rath]

I just saw json-pointer in the package.json and thought, this library would be used 🙄

[danrot]

@luca-rath This library is used, but json pointers are not used in combination with jexl (yet). Alex's message above has also confused me a bit :see_no_evil:

[danrot]

I have just implemented the __current variable in the most straightforward way (at least IMO) and that didn't result as we've imagined that above. I would have set the __current variable to the value passed to the current field. That means that a text_line field would receive the value of the text_line, ergo a string instead of an expected object.

It would be easy to implement as we've discussed above, but this hickup made me thinking if this naming is really so logical. I think it is easily possible, that somebody would imagine __current to be the field itself. So I thought maybe it makes more sense to name it __parent already instead. That would mean we would only have one variable to declare, explain and document. Also, when thinking about it, it makes sense to consider the blocks field being the __parent of the text_editor field in the example below.

        <block name="blocks" default-type="editor" minOccurs="0">
            <types>
                <type name="editor">
                    <meta>
                        <title lang="en">Editor</title>
                        <title lang="de">Editor</title>
                    </meta>
                    <properties>
                        <property name="title" type="text_line" />
                        <property name="text_editor" type="text_editor" visibleCondition="__parent.title == 'Test'" />
                    </properties>
                </type>
            </types>
        </block>

So I wouldn't be too surprised if the text_editor field would be invisible if the title field within the block would be set to "Test". Therefore I would now lean towards that solution, and not implement the __current variable at all.

[nnatter]

This should be fixed with #5705 - Sulu 2.3 will finally allow to use a __parent variable inside of conditions to access the current block 🎉

See the documentation PR for an example how to use this: https://github.com/sulu/sulu-docs/pull/609