[MGNLUI-3633] DelegatingMultiValueSubnodeTransformer behaves erratically Created: 22/Oct/15  Updated: 05/May/16  Resolved: 19/Nov/15

Status: Closed
Project: Magnolia UI
Component/s: forms
Affects Version/s: 5.4.2
Fix Version/s: 5.4.4

Type: Bug Priority: Neutral
Reporter: Mikaël Geljić Assignee: Oanh Thai Hoang
Resolution: Fixed Votes: 2
Labels: i18n, multifield, support, transformer
Remaining Estimate: 0d
Time Spent: 6d 2.5h
Original Estimate: 4d

Attachments: PDF File Testresultin3bundle5.pdf    
Issue Links:
Relates
relates to MGNLUI-3668 MultiValueFields must react on the ch... Closed
causality
caused by MGNLUI-3489 Support field default value for local... Closed
is causing MGNLUI-3631 I18N on nested MultiValue / Composite... 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)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Sprint: Basel 23
Story Points: 8

 Description   

The DelegatingMultiValueSubnodeTransformer doesn't work well with i18n, since 5.4.2.

Given the following MultiValueFieldDefinition:

class: info.magnolia.ui.form.field.definition.MultiValueFieldDefinition
transformerClass: info.magnolia.ui.form.field.transformer.multi.DelegatingMultiValueSubnodeTransformer
field:
  name: text
  class: info.magnolia.ui.form.field.definition.TextFieldDefinition
  i18n: true

When I add a new entry to the multi-field in English ("en") and I switch to German
Then

  1. the entry disappears (should be preserved, with blank value);
    I have to add it again to enter the value in German ("de")
  2. when saving the dialog, only the German property text_de = "de" is saved.

There seem to be similar quirks when removing entries as well.

Several notes:

  • DelegatingMultiValueSubnodeTransformer was working in 5.4.1.
  • DelegatingMultiValueFieldTransformer still works in 5.4.2.
    • It is the superclass of the subnode transformer
    • It has undergone major changes in 5.4.2 (MGNLUI-3489); these changes are most likely causing this issue.


 Comments   
Comment by Aleksandr Pchelintcev [ 28/Oct/15 ]

This should a problematic part:

Node rootJcrItem = rootItem.getJcrItem();
try {
 if (rootJcrItem.hasNode(definition.getName())) {
...

Since the new logic - the fields as well as transformers get re-created when locale is switched. Hence, the sub-node get also re-initialized. The snippet above proposes the following logic:
1) check the underlying JCR node - if it has a subnode with required name - pull it into adapter and use it.
2) create a new transient adapter otherwise

Now we can end-up in a third state - the child item might have been created already, but hasn't been yet persisted => step 0:
0) check the current not persisted children of the root item (rootItem.getChildren() would it be).

Checking that would anyway be a right/safer thing to do.

That's just a thought from top of my head, but hope it works/helps.

Comment by Eric Hechinger [ 02/Nov/15 ]

Hello mgeljic

Given the following MultiValueFieldDefinition:

class: info.magnolia.ui.form.field.definition.MultiValueFieldDefinition
transformerClass: info.magnolia.ui.form.field.transformer.multi.DelegatingMultiValueSubnodeTransformer
*i18n: true*
field:
  name: text
  class: info.magnolia.ui.form.field.definition.TextFieldDefinition

i18n: true on the multifield level.

generate another behavior. (correct in my opinion)

  • values are correctly stored
  • if you create two entry in 'en' and switch to 'de', no entry are present. This is for me a normal behavior. You can then add one 'de' entry. Why would you have to have the same number of entry for every language?
Comment by Oanh Thai Hoang [ 02/Nov/15 ]

After testing for this bug in versions: 5.4.1, 5.4.2 and 5.4.3-Snapshot (please refer to Testresultin3bundle5.pdf for more details) in 2 cases: i18n and no i18n for MultiValueFieldDefinition. 5.4.2 and 5.4.3-snapshot work differently from 5.4.1. This bug happen from 5.4.2. So I will do analysis why it happens and fix it to make it work correctly. Thanks

Comment by Mikaël Geljić [ 02/Nov/15 ]

Hi ehechinger, I agree your case works, but you're changing the "Given" .
As a matter of fact, since MGNLUI-3169, we support *both* use cases:

  • i18n set to true on a MultiField: stores a different set of entries per locale. No need to enable i18n on sub-fields (would be redundant). As you said, this still works.
  • i18n set to false on a MultiField: stores a single set of entries. Entries may be translated 1-to-1 by setting i18n to true on sub-fields. This is broken since 5.4.2.

At least we don't make any assumption, customers choose what suits them best.

Comment by Oanh Thai Hoang [ 05/Nov/15 ]

Thanks apchelintcev for your suggestion.

Hi mgeljic, apchelintcev

This is issue and I reproduced bug on version 5.4.2
There is related module need to be altered: ui

The issue happens when user switching languages in form has MultiValueFieldDefinition field using DelegatingMultiValueSubnodeTransformer transformer.

For my understanding:
Before major changes in (MGNLUI-3489), all the locales shared the same fields in one form. So the form didn't rebuilt when user switching languages.

Nowadays (from 5.4.2), it does not work as before. When user switch to a new language, a new form for a new language will be created, new transformer, new field... The form has many section instances per language but still access the same component instance. We can imagine that 2 languages: 2 forms (each form is built one time) and different fields due to UI events.

So for the case : adding new entry in "en" and switch to "de" -> the entry disappears. The reason is data for entry in "de" has been gone because we will create new transformer for de and data hasn't persisted to JCR.

For my approach:

1. What we should do is checking data in transformer, I intend the checking is here:

public DelegatingMultiValueSubnodeTransformer(Item relatedFormItem, ConfiguredFieldDefinition definition, Class<PropertysetItem> type, I18NAuthoringSupport i18NAuthoringSupport) {
        super(relatedFormItem, definition, type, i18NAuthoringSupport);

        // In case the child item has been create already but hasn't persisted yet
        if (subNode == null) {
            if (((JcrNodeAdapter) relatedFormItem).getChildren().containsKey(definition.getName())) {
                subNode = (JcrNodeAdapter) ((JcrNodeAdapter) relatedFormItem).getChildren().get(definition.getName());
            }
        }
    }

For my site, I don't think persist data to JCR before switching is good way, so continue to using that data for transformer that is acceptable.

2. Another problem come from transformer is how can we deal with storing a single set of entries. We should check the data exist before creating new data:

        final JcrNodeAdapter child;

        // Should be check the new Item Name existed because it can be created already from other language form.
        if (getRootItem().getChild(newItemName) != null) {
            child = (JcrNodeAdapter) getRootItem().getChild(newItemName);
        } else {
            child = new JcrNewNodeAdapter(getRootItem().getJcrItem(), childNodeType, newItemName);
        }

There are 2 parts I found out that it make data can not store correctly. Here is my patch https://gist.github.com/oanhthai/32ab495aa6bf02dee6ae. Please feel free to review it.

Cause the form presenter does not work as before. So keeping the same fields for

i18n set to false on a MultiField and i18n to true on sub-fields

is impossible but if we somehow make the FormBuilder can build again while switching language can help to build the same fields for each form of locale.

For my point of view about user exp:
1. Don't support

i18n set to false on a MultiField and i18n to true on sub-fields

2. Make the Form Ui run the same as 5.4.1 if we provide the way to force the line has '*' can run

if (currentFormSection != null) {
                    FormSection newFormSection = Iterables.tryFind(newFormSections.values(), new FormSectionNameMatches(currentFormSection.getName())).orNull();
                    if (newFormSection == null) {
                        *newFormSection = formBuilder.buildFormTab(tabDefinition, itemDatasource, null).getContainer();*
                        *newFormSections.put(tabDefinition, newFormSection);*
                    }

                    ((SingleComponentContainer) currentFormSection.getParent()).setContent(newFormSection);
                }

this code come from FormPresenterImpl class

Comment by Oanh Thai Hoang [ 05/Nov/15 ]

Another patch related to Form presenter https://gist.github.com/oanhthai/d4bf5c3a8f16c45aa647. It will help the activeLocale always get correct locale at the first time (While checking source code, I found that the activeLocale always null at the first time).

Comment by Aleksandr Pchelintcev [ 13/Nov/15 ]

oanh.thai I guess it is a good time to transform these comments and patches into a branch and a pull-request because it'll be easier to carry on with the review and discussion. Anyway from what I've read so far - it seems to mostly go in line with what I pointed out initially, so I guess you can go ahead.

As for the form presenter issue - we should extract it into a separate ticket.

Comment by Jennylyn Sze [ 29/Apr/16 ]

Is this issue really fixed in 5.4.4?

I'm using 5.4.6 and still have this issue.

Thanks!

Comment by Oanh Thai Hoang [ 05/May/16 ]

HI jenny_87],

I'm sorry for late reply.

The real problem is that the multi field doesn't listen to data-source changes so ideally as you switch the language - the field should already look the same way (given it's not i18n-ized itself).
And this ticket just supported to improve the case that the sub-field in multi field always re-initialized.

So the actual issue doesn't fix yet. And it belongs to https://jira.magnolia-cms.com/browse/MGNLUI-3668.

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