[MGNLUI-6948] Fields within a switchable cannot be internationalized Created: 10/Nov/21  Updated: 07/Dec/21  Resolved: 07/Dec/21

Status: Closed
Project: Magnolia UI
Component/s: None
Affects Version/s: 6.2.12
Fix Version/s: 6.2.14

Type: Bug Priority: Neutral
Reporter: Richard Gange Assignee: Adam Siska
Resolution: Fixed Votes: 0
Labels: i18n
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Java Source File RealOptionBuilder.java    
Issue Links:
relation
Template:
Acceptance criteria:
Empty
Task DoD:
[X]* Doc/release notes changes? Comment present?
[X]* Downstream builds green?
[X]* Solution information and context easily available?
[X]* Tests
[X]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[X]* Steps to reproduce, expected, and actual results filled
[X]* Affected version filled
Date of First Response:
Story Points: 2

 Description   

Given a 5 UI Switchable field with DelegatingCompositeFieldTransformer, one sub-field set with i18n and one without;
this field is not converted properly to the equivalent 6 UI definition. The i18n sub-field is not set with i18n.

Original description (Blossom)

Example dialog with one field inside the switchable set to i18n:

@TabFactory("buttonLinkDialog.button.tab.label")
public void buttonLinkTab(Node node, UiConfig uiConfig, TabBuilder tabBuilder) {
   tabBuilder.fields(
         uiConfig.fields.text("keepitsimple").label("Simple Field"),
         uiConfig.fields.text("keepitlocalized").label("Locale Field").i18n().required(),
         uiConfig.fields.checkbox("checkit").label("Check it").defaultValue("true").buttonLabel("Check Button"),
         uiConfig.fields.switchable("testme").options(
               new RealOptionBuilder().value("test1_text1").label("test1").selected(),
               new RealOptionBuilder().value("test2_text2").label("test2")
               ).fields(
                     uiConfig.fields.text("test1_text1").label("Text 1").required(),
                     
                     // inner field set to i18n()
                     uiConfig.fields.text("test2_text2").label("Text 2").required().i18n())
         // EDIT: original report didn't include this line
         .transformerClass(DelegatingCompositeFieldTransformer.class)
         .required()
   );
} 

An equivalent dialog in YAML is working:

form:
  properties:
    switchable:
      $type: switchableField
      label: Switchable
      field:
        $type: radioButtonGroupField
        layout: horizontal
        datasource:
          $type: optionListDatasource
          options:
            - name: foo
              value: foo
              label: Foo
            - name: bar
              value: bar
              label: Bar
      itemProvider:
        $type: jcrChildNodeProvider
      forms:
        - name: foo
          properties:
            foo:
              label: Foo
              $type: textField
            bar:
              label: Bar
              $type: richTextField
        - name: bar
          properties:
            foo:
              label: Foo
              $type: textField
            bar:
              label: Bar
              $type: richTextField
              i18n: true


 Comments   
Comment by Jörg Wirsig [ 11/Nov/21 ]

Attached the RealOptionBuilder to make it easier to reproduce.

Comment by Mikaël Geljić [ 11/Nov/21 ]

Hi joergwirsig,

Thank you for reporting. At first, I'd like to point out that 5 UI switchable fields require to use the DelegatingCompositeFieldTransformer in order to support nesting fields or—as it turns out— i18n per sub-fields.
There was a hint (best practice) in documentation, but not about i18n I must say, and it's almost fine print.

So for me, the @TabFactory becomes:

@TabFactory("Secondary")
public void buttonLinkTab(Node node, UiConfig uiConfig, TabBuilder tabBuilder) {
    tabBuilder.fields(
            uiConfig.fields.text("keepitsimple").label("Simple Field"),
            uiConfig.fields.text("keepitlocalized").label("Locale Field")
                    .i18n()
                    .required(),
            uiConfig.fields.checkbox("checkit").label("Check it")
                    .defaultValue("true")
                    .buttonLabel("Check Button"),
            uiConfig.fields.switchable("testme").label("Test me")
                    .options(new OptionBuilder().value("test1_text1").label("test1").selected(),
                            new OptionBuilder().value("test2_text2").label("test2"))
                    .fields(uiConfig.fields.text("test1_text1").label("Text 1")
                                    .required(),
                            // inner field set to i18n()
                            uiConfig.fields.text("test2_text2").label("Text 2")
                                    .required()
                                    .i18n())
                    .transformerClass(DelegatingCompositeFieldTransformer.class)
                    .required()
    );
}

For the story, SwitchableTransformer is the default; but we never got to change this default to the delegating impl, mostly because the delegating impl does not prefix sub-fields' properties with the switchable's own field name.

Unfortunately, I cannot confirm a working result after the definition-conversion (despite traces of this transformer's support in SwitchableFieldDefinitionConverter), while it works like a charm in the compatibility Pages app.
I'll file a MGNLUI ticket and attach a test. Ideally the conversion should result in something close to the YAML you posted.

Cheers,
Mika

Edit: The RealOptionBuilder helps a bit with selection, but still no i18n support.

Comment by Jörg Wirsig [ 12/Nov/21 ]

Hi mgeljic,
thank you very much for the update on that issue.

At first, I'd like to point out that 5 UI switchable fields require to use the DelegatingCompositeFieldTransformer in order to support nesting fields or—as it turns out— i18n per sub-fields.

Yes sure, in real world code we are using that transformer, but we are using several Helper classes to conviently generate some kind of fields so it would be very difficult to share the real world code. Instead i build that demo dialog to make it easier to strip the problem down to that what matters.

Is there already a link to the MGNLUI Ticket you can share?

Comment by Mikaël Geljić [ 13/Nov/21 ]

Moved this one to MGNLUI.

Comment by Jörg Wirsig [ 15/Nov/21 ]

Hi mgeljic

I'm pretty sure i found the reason, for the behaviour. 

See SwitchableFieldDefinitionConverter.java Line 73

 public ConfiguredSwitchableFieldDefinition<Option> convert(SwitchableFieldDefinition oldDefinition) {
    ConfiguredSwitchableFieldDefinition<Option> newDefinition = super.convert(oldDefinition);

    OptionGroupFieldDefinition oldOptionField = new OptionGroupFieldDefinition();
    oldOptionField.setName(oldDefinition.getName());
    oldOptionField.setOptions(oldDefinition.getOptions());
    oldOptionField.setI18n(oldDefinition.isI18n());

    RadioButtonGroupFieldDefinition optionField = (RadioButtonGroupFieldDefinition) DefinitionConverter.FIELD.convert(oldOptionField);
    newDefinition.setField(optionField);

    List<FormDefinition<Option>> formList = new ArrayList<>();
    ConfiguredFormDefinition form;
    for (ConfiguredFieldDefinition fieldDefinition : oldDefinition.getFields()) {
        fieldDefinition.setI18n(oldDefinition.isI18n());
        form = new ConfiguredFormDefinition();
        form.setName(fieldDefinition.getName());
        form.setProperties(Collections.singletonList(DefinitionConverter.FIELD.convert(fieldDefinition)));
        formList.add(form);
    }
    newDefinition.setForms(formList);
    convertTransformerStrategy(oldDefinition, newDefinition);
    return newDefinition;
}

Where 

fieldDefinition.setI18n(oldDefinition.isI18n()); 

is setting i18n of subfields to the same as the i18n setting of the switchable itself. It seems to be done on purpose but i don't know why.

But replacing the converter with a different implementation without that line everything seems to be working properly. 

Can you or someone else give me short feedback, if that line of code is just a bug or if it has a special purpose which I'm currently missing. 

Comment by Richard Gange [ 16/Nov/21 ]

Hello joergwirsig-

It's a bug. Not specifically a blossom bug though. A UI bug instead so the ticket was moved to this project. The problem now is it becomes a low prio bug since we are talking about support for older dialogs. However, you are saying that replacing that single line did fix the issue for you? I can create the PR if so.

Cheers
Rich

Comment by Jörg Wirsig [ 16/Nov/21 ]

I just thought about that issue again, and i guess the code should look like

for (ConfiguredFieldDefinition fieldDefinition : oldDefinition.getFields()) {
   if(oldDefinition.isI18n()){
      fieldDefinition.setI18n(oldDefinition.isI18n());
   }
   form = new ConfiguredFormDefinition();
   form.setName(fieldDefinition.getName());
   form.setProperties(Collections.singletonList(DefinitionConverter.FIELD.convert(fieldDefinition)));
   formList.add(form);
} 

I guess the purpose was to set all subfields to i18n if switchable is i18n. Deactivation if i18n on subfields if switchable is not i18n seems to be an unwanted side effect.
So my proposal is as stated above.

In the meanwhile we will add our own converter with the fix to DefinitionConverter.FIELD_DEFINITION_CONVERTERS.

Comment by Roman Kovařík [ 03/Dec/21 ]

Estimated: integrate and QA the existing PR.

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