[MGNLUI-3715] Only values for selected options should be saved when using SwitchableFieldDefinition Created: 11/Dec/15  Updated: 27/Jan/17  Resolved: 17/Mar/16

Status: Closed
Project: Magnolia UI
Component/s: forms
Affects Version/s: 5.4.3
Fix Version/s: 5.4.6

Type: Improvement Priority: Major
Reporter: Richard Gange Assignee: Oanh Thai Hoang
Resolution: Fixed Votes: 0
Labels: forms, support, ux
Remaining Estimate: 0d
Time Spent: 7d 2.75h
Original Estimate: 5d

Attachments: Java Source File ComplexFieldFriendlySwitchableTransformer.java    
Issue Links:
causality
is causing MGNLPN-333 CookieFieldTransformer doesn't store ... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Release notes required:
Yes
Documentation update required:
Yes
Date of First Response:
Sprint: Saigon 35
Story Points: 8

 Description   

When we change this, care should be taken that values in all options remain available while the user mainly switches between them. The values for all other options but the selected one should only be cleared once you save the form. Not all systems behave this way, but most do - it's commonly seen as good and expected behavior.

So, I fill in a value for option 1, switch to option 2, fill in another value there, then switch back to option 1. My initial value I've set in option 1 should still be there. Once I save the form, only the value for option 1 is stored. The value for option 2 is discarded.

Update: This ticket just support for basic Switchable field using SwitchableTransformer, not providing any delegating transformer for the switchable field for now.
I did provide approach to handle more complex sub-fields than SwitchableTransformer, just find the attached ComplexFieldFriendlySwitchableTransformer or get it from here. It help to alters JCR adapters directly



 Comments   
Comment by Oanh Thai Hoang [ 22/Jan/16 ]

Hi mgeljic],
This is my investigating.

Why do all value of all options still show after clicking save ?

  • This is behaviour of system. All values for all fields will be kept in JcrAdapter and petsit to JCR when clicking save button.

Approach:

  • Modify SwitchableTransformer to ignore all value that do not belong to select field
@Override
    public void writeToItem(PropertysetItem newValues) {
        // Alter newsValues to clear all data that not belong to selections
        String currentSelection = (String) newValues.getItemProperty(propertyPrefix).getValue();

        // Get iterator.
        Iterator<?> propertyNames = newValues.getItemPropertyIds().iterator();

        while (propertyNames.hasNext()) {
            String propertyName = (String) propertyNames.next();
            String compositePropertyName = getCompositePropertyName(propertyName);
            if (!propertyName.equalsIgnoreCase(currentSelection) && !propertyName.equalsIgnoreCase(propertyPrefix)){
                relatedFormItem.removeItemProperty(compositePropertyName);
            } else{
                if (newValues.getItemProperty(propertyName) != null) {
                    relatedFormItem.addItemProperty(compositePropertyName, newValues.getItemProperty(propertyName));
                    // Need to remove it from the removedProperties map
                    ((JcrNodeAdapter)relatedFormItem).unRemoveItemProperty(compositePropertyName);
                }
            }
        }
    }
  • Modify JcrNodeAdapter to support new API unRemoveItemProperty for providing mechanism to remove property in removedProperties Map. We need this API because we have one case while switching around options, all value fields will be kept in removedProperties Map and no fields will be persisted to JCR
    /**
     * Remove a property from a removedProperties map.
     * If the property was already remove, remove it for the removedProperties Map.
     */
    public boolean unRemoveItemProperty(Object id) {
        boolean res = false;
        if (getRemovedProperties().containsKey(id)) {
            getRemovedProperties().remove(id);
            res = true;
        }
        return res;
    }

Disadvantage: just support for default transformer SwitchableTransformer

Git patch: https://gist.github.com/oanhthai/ba7614a110b865b073ff

Opinion:
I think keep values for all options are ok with me. Because this field is switchable field, it means user can switch to other options if they want. And if switchable just shows one value for selected option, there is no meaningful for switchable. How about your ideal?

Comment by Oanh Thai Hoang [ 25/Jan/16 ]

I have been done this ticket and created PR. This PR is the fix for SwitchableTransformer but doesn't include delegating transformers classes.

Comment by Mikaël Geljić [ 26/Jan/16 ]

We've been keeping those switchable values historically, also because there was no reason to wipe them out; besides, if user changed his mind at some point, he could easily switch back with pre-filled value (Although this is more a duty for versioning). It's unclear to me if users trust this at the moment, but we can easily document the change.

Now that we ignore validation for invisible fields, it means one can save invalid values into those.
Note that it is mitigated by the fact that whatever the selected option, visible sub-fields *must* be valid at saving time.
So, unless templates do not check the actual value of the switch correctly, this isn't causing any real problem at the moment.

To some extent, it's a similar question as MGNLUI-3491 too.

Thanks oanh.thai, I'll check it out—ideally delegating transformers should be covered as well, but I've observed inconsistent cases with validation too.

Comment by Mikaël Geljić [ 21/Mar/16 ]

Definitely worth release notes, and maybe a note in Switchable field docu as well:

Since Magnolia 5.4.6, when used with the SwitchableTransformer, Switchable field now discards field values for the unselected options.

Comment by Vincent Gombert [ 27/May/16 ]

Hello,
I have some dialogs which need to keep all values so the component can be quickly switched.
Is there a way to configure the switchable field in order to not discard the unselected options values ?
Regards

Comment by Richard Gange [ 27/May/16 ]

Hi vgombert-

To answer your question, no. Can you expand on the use case? Are we talking about a manual switch? It's possible to store some predefined settings in configuration or a content app or something and then reference the settings using a name/id. Basically you link to the settings which are in the form of a set or record.

HTH
Rich

Comment by Vincent Gombert [ 31/May/16 ]

Hi Richard,

A simple use case is a link with different origins, let's say internal or external to keep it simple (actually there are more). The switchable field allows us to save properties for several origins and later change the origin without having to type again properties for other origin. If I understand this JIRA well, that won't be possible anymore in 5.4.6.

Regards,
Vincent

Comment by Mikaël Geljić [ 02/Jun/16 ]

Hi vgombert,

With the plain SwitchableTransformer, indeed this is no longer possible (I was never too fond of this change either, but here's the tip for now: if you use the DelegatingCompositeFieldTransformer it will keep working, as the parent field has no control over property changes within its sub-fields).

Cheers,
Mika

Generated at Mon Feb 12 09:09:23 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.