[MGNLUI-3955] Switching authoring locale changes values of non-i18ned fields Created: 20/Jul/16  Updated: 11/Aug/17  Resolved: 07/Mar/17

Status: Closed
Project: Magnolia UI
Component/s: dialogs, forms
Affects Version/s: 5.4.4, 5.4.5, 5.4.6, 5.4.7
Fix Version/s: 5.4.12, 5.5.3

Type: Bug Priority: Major
Reporter: Vivian Steller Assignee: Ngoc Nguyenthanh
Resolution: Fixed Votes: 1
Labels: regression
Remaining Estimate: 0d
Time Spent: 9d 1.5h
Original Estimate: 1.5d

Issue Links:
Relates
relates to MGNLUI-3913 i18n: Checkboxes should stay checked ... Closed
causality
relation
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: Saigon 77, Saigon 78, Saigon 79, Saigon 80, Saigon 81, Saigon 82, Saigon 83, Saigon 84, Saigon 85, Saigon 86
Story Points: 8

 Description   

In a form dialog, switching locales also changes the values of non-i18ned fields, which is wrong.

To reproduce the issue on demo author:
1. Log in Demo author and open a tour for editing, e.g. https://demoauthor.magnolia-cms.com/.magnolia/admincentral#app:tours:detail;/magnolia-travels/Vietnam--Tradition-and-Today:edit
2. Switch authoring locale to "German"
3. Locate a non-i18ned field and change the value, e.g. Tour Operator = Something
4. Switch authoring locale to "English"
5. Edit the value of the same field again, e.g. Tour Operator = Lemonize
6. Again, switch authoring locale back to "German": the field's value is changes back to Something, which is wrong. Lemonize would be right.

Note that the last entered value is being stored when saving the form, which is correct.

It seems that the field's values are not properly synchronized for non-i18ned fields. I wasn't able to figure out in detail where the code breaks: I might be wrong but I think the underlying datasource property still contains the correct value after the info.magnolia.ui.vaadin.form.FormViewReduced.Listener#localeChanged event is fired. There must be some event occurring thereafter which changes the value incorrectly.



 Comments   
Comment by Ngoc Nguyenthanh [ 05/Jan/17 ]

Re:

  • The non-i18n field mentioned above just a simple text field with BasicTransformer
  • Templating-samples / dialog-showroom has the same behaviors:
    + Open the dialog -> Change the field -> Switch language -> New value will present in the current language due to FormPresenterImpl#setLocale buildFormTab from the itemDataSource
    + Open the dialog -> Switch language to German -> Switch back to English -> Change the field -> Switch to German -> new value will not present in the current form due to the form tab has built already.
  • Some UI test classes may related to but this case is not covered in any UI test: I18nUITest, CompositeFieldUITest, MultiFieldUITest

FormPresenterImpl#setLocale will be fired when switching language. When the field is change, changedProperties in itemDataSource will be updated. But seem like we don't have a mechanism to reflect the changed property (model) to the UI.

Comment by Ngoc Nguyenthanh [ 06/Jan/17 ]

A UI-Test to cover this case in SimpleFieldUITest

    @Test
    public void setNonI18nTextFieldValue() {
        // GIVEN
        goToDialogShowRoomAndOpenDialogComponent("ftl");
        openTabWithCaption("Edit controls");

        // WHEN
        // Set input values
        switchToLanguage("German");
        setFormTextFieldText("Text 1", "test-de");
        switchToLanguage("English");
        setFormTextFieldText("Text 1", "test-en");
        switchToLanguage("German");

        // THEN
        assertEquals("test-en", getFormTextField("Text 1").getAttribute("value"));
    }
Comment by Mikaël Geljić [ 09/Jan/17 ]

Thanks ngoc.nguyenthanh here's the recap from our discussion, two options:

A. Remove the null check in FormPresenterImpl#setLocale:155

  • effectively rebuilding the form tab afresh upon every locale change, without caching form tabs built previously for each locale.
  • Minimal impact.
  • We must carefully evaluate consequences; not so much performance-wise, but rather behavior-wise (nested complex-fields w/ or w/o i18n).

B. Reuse non-i18n fields throughout locales

  • effectively moving fields across different layouts / component trees
  • FormPresenter could be responsible for creating individual fields
  • and further maintaining its state, such as some locale-aware field-cache
  • kind of same direction as the bigger forms refactoring: move away from static-ish FormBuilder
  • Conceptually more sensible, but could turn ticket into yet another intermediate refactoring.

apchelintcev Any objection against option A, or any caveat you remember from the previous iteration? —I pasted some bits from the commit message below, in case that rings a bell.
How about option B, do you consider any intermediate refactoring realistic/worthwhile? For what it's worth, Sang already had this new "standalone" FormPresenter PoC, which we could theoretically build upon for locale (or even variant) switching.

* Extract and publish FormBuilder method for tab and field creation
    * Introduce FormPresenter component
    * encapsulates operations w/ FormBuilder
    * supports re-creation of the tab contents upon locale change
* Form UI components bug-fixing and improvements according to FormPresenter
    * introduction - prevent MagnoliaFormTab from unnecessary #content field mangling
Comment by Vivian Steller [ 07/Mar/17 ]

Congratulations, folks! Thanks for fixing this

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