Fix admin localized date formatting

[tdgroot]

Description (*)

There were 2 issues going on that are fixed by this PR.

1. Incorrect initial value parsing
   When parsing the initial value, say 2020-03-17, the date was getting parse with the following format (for nl_NL): dd-MM-y. The value used was derived from the variable outputDateFormat. I changed the format by using the variable inputeDateFormat, which is yyyy-MM-dd.
2. Cyclic event chain
   Variable shiftedValue was initialized by the onValueChange listener. But when changing shiftedValue, the value variable is changed by the onShiftedValueChange listener. This would mess up the date value as well.
   A cyclic event chain like the following could occur:
   • Change shiftedValue to 2020-03-17.
   • Change value to 17-03-2020.
   • Change shiftedValue to 20-03-2017.

Fixed Issues (if relevant)

1. fixes Localized date field formatting in admin messing up actual date #27310: Localized date field formatting in admin messing up actual date

Manual testing scenarios (*)

1. Have 2 admin users. One on locale en_US, one on nl_NL.
2. Log in on nl_NL admin user.
3. Create a cart price rule, no coupon code needed, just a simple cart price rule. Make sure you set the from date (e.g. 17 march 2020). Save the cart price rule.
4. Check if the from date is correct.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @tdgroot. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[lbajsarowicz]

@tdgroot Great that you've approached the issue.
Could you cover the change with Functional tests (MFTF)?

[tdgroot]

@tdgroot Great that you've approached the issue.
Could you cover the change with Functional tests (MFTF)?

Will do!

[tdgroot]

@lbajsarowicz I added an MFTF test to cover this PR. Please let me know if it's any good 😄.

[lbajsarowicz]

It looks like you've already worked with MFTF.
Almost perfect 👏

We need to separate steps of preparing the test from actual testing:

Before

1. Generate date objects
2. Login as Main Admin
3. Create Dutch Admin
4. Logout from Main Admin

After

1. Login as Main Admin
2. Remove Dutch Account
3. Logout from Main Admin

Test

1. Login as Main Admin
2. Create price rule in en_US and make assertions
3. Logout from Main Admin
4. Login as Dutch
5. Create price rule in nl_nl and make assertions
6. Logout from dutch account

---

That would be awesome if you find the way to remove Price Rules in After section.

[tdgroot]

It looks like you've already worked with MFTF.

I joined the MFTF workshop with you last year in Florence at MageTestFest!

We need to separate steps of preparing the test from actual testing:

Will do!

That would be awesome if you find the way to remove Price Rules in After section.

I was thinking about that, but somehow forgot to add that. I'll add that in!

[tdgroot]

@lbajsarowicz I added the requested changes.

One thing I could not do was move the generateDate steps to before section. The variables would only be usable within the before section scope.

[lbajsarowicz]

You are right about data scope. That is fine
Let's wait for tests result and you have 👍

[tdgroot]

@magento-engcom-team what changes are requested?

[lbajsarowicz]

@tdgroot Could you fix static tests:

and update a branch with latest 2.4-develop

[tdgroot]

@ihor-sviziev @lbajsarowicz I see the following error:
select option "#page_tabs_main_section_content select[name='interface_locale']","nl_NL"

The nl_NL locale is not deployed in the Magento test system, so the nl_NL locale cannot be selected when creating an admin user.

How do we go about that? To me it seems very important to be able to test the application on other locales as well.

[ihor-sviziev]

@lbajsarowicz could you help with the issue in MFTF?

[tdgroot]

@ihor-sviziev I updated the branch to resolve merge conflicts. Could you let me know what the status is of this issue?

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

Hi @tdgroot,
Please review and fix failing tests

[barryvdh]

I can confirm this issue also occurs for the special_from/to dates on the catalog edit page. Example:

Special price from 2021-02-01 - 2021-03-01, shows in the English interface:

Change interface to dutch, dat now shows like this:

Without changing anything, just pressing the Save button, now changes the date from the product, also in English:

Bonus: if you open it again in NL it changes again!

So basically the admin is unusable on Dutch date format.

[barryvdh]

@lbajsarowicz @ihor-sviziev Could you help with the tests? We have to disable the NL locale in all our back-ends because it changes all dates upon saving, which is very confusing for our clients.

[barryvdh]

Is this fixed by #31047 or just partially?

[PascalBrouwers]

Confirmed that this PR works. Thank you tdgroot.
What I don't get is why this STILL isn't fixed since the release of Magento 2.0.
Magento maybe help with the tests if you're so adement on having this required. People stop contributing because of the extreme bad experience getting a PR merged.

[ihor-sviziev]

@Den4ik, maybe you could review and help with tests?

[Den4ik]

This is related to #33591
I'm trying implement correct ICU convert for all date UI components

[PascalBrouwers]

Looking at #31047. This one works better and is less code change.

[Den4ik]

But in fact it's masking of talk problem. Look at normalizeDate function. This method replacing year and day placeholders because correct converting ICU dates format to jq calendar and momentjs is not working

[joshuaswarren]

@tdgroot - thank you for the effort you put into this pull request! It's been out here a while - it's actually one of the oldest pull requests still open. In your opinion, is this still an issue? Pascal's comment from 2021 makes it seem like the merged pull request #31047 may also have addressed this problem. And Igor pointed out that there's failed tests that need to be addressed.

What do you think, is there a need to put the additional effort into this PR to get it merged, or is it out of date now and could be closed?

[tdgroot]

Hi @joshuaswarren,

Yeah it's been a while 😅. I'm not actively developing Magento projects anymore, so I haven't been in the in the loop on this one.

It could very well be the case that the problem has been solved, but I know there were multiple problems that required various fixes. It might be good to ask @barryvdh or @PascalBrouwers if this is still a problem.

Anyway, last time I checked, the tests were failing because I introduced something that the test suite didn't support: multiple admin locales. So the tests change the admin user locale to nl_NL, but that locale isn't available in the test runner, because it only seems to deploy en_US.

[engcom-Echo]

@tdgroot
Thank you for your contribution and collaboration.
The issue seems to fixed under this commit.

Hence Closing this PR.
If you are still facing the Issue please feel free to reopen the issue.