Translate order getCreatedAtFormatted() to store locale #10491

JeroenVanLeusden · 2017-08-11T08:03:26Z

Description

Updated version of #10390, added new interface method en deprecated the current one.

Add getDefaultStoreLocale() to allow fetching scoped values. Use this in getCreatedAtFormatted() so created_at date of order will be translated in emails to locale being used in that store view.

Fixed Issues (if relevant)

None that I could find.

Manual testing scenarios

Set default locale of your store to any language but English.
Create order and send confirmation email.

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 on Travis CI are green)

orlangur · 2017-08-11T08:12:51Z

You cannot add new methods into existing @api interfaces as it will break every existing interface implementation. Please check http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/#introduction-of-a-method-to-a-class-or-interface

JeroenVanLeusden · 2017-08-11T12:10:44Z

@orlangur creating a new implementation and use that in \Magento\Framework\Locale\Resolver would be the way to go? If so, I will update the PR when I have the time.

orlangur · 2017-08-14T15:59:47Z

I think \Magento\Framework\Locale\ResolverInterface should not be touched (changed/deprecated) for the sake of the fix even if we didn't have BC constraints.

Any idea why \Magento\Framework\Locale\Resolver and \Magento\Backend\Model\Locale\Resolver does not cover this case with order model properly? From implementation it looks like correct locale should be already there.

So, I wouldn't recommend to introduce any StoreAwareLocaleResolverInterface unless really necessary.

JeroenVanLeusden · 2017-08-14T17:54:59Z

@orlangur After some more debugging I found the real issue.

\Magento\Framework\Locale\Resolver::__construct() will set the locale using $this->setLocale($locale); where $locale is null since no argument is set in the di.xml.

Then in the Magento\Backend\Model\Locale\Resolver::setLocale() will get $local as null. In there it will load the session locale which is also null and the user locale which is en_US (Magento backend locale). So there it will set the locale to en_US although our store locale is nl_NL. The changes I've made aren't neccesary because the getDefaultLocale() will fetch the correct value. So this issue is only relevant is backend and frontend locales don't match.

In my opinion the best solution would be to emulate the store locale in \Magento\Sales\Model\OrdergetCreatedAtFormatted() and change it back after the date is formatted. If you agree on this one I will implement this.

orlangur · 2017-08-14T18:02:30Z

Do we need to really emulate locale or it's enough to pass proper locale in

-            null,
 +            $this->localeResolver->getDefaultStoreLocale($this->getStore()),

?

JeroenVanLeusden · 2017-08-14T18:14:39Z

@orlangur Yes, if you have the proper locale you can insert it there. Using $this->localeResolver->getDefaultLocale() there will solve this if I don't miss anything?

orlangur · 2017-08-14T18:40:50Z

Not sure, please try it.

Please go this way, if getDefaultLocale will not work do something like $locale = $this->scopeConfig->getValue($this->getDefaultLocalePath(), $this->scopeType, $store); in place (just a fix in order model, no need in additional interfaces/classes).

Please force push into the same branch with a single commit when it is ready (i.e. works & builds are green).

JeroenVanLeusden · 2017-08-15T09:36:18Z

@orlangur Tested and works. Tests are green, only one job exceeded the maximum time limit for jobs, and has been terminated.

orlangur · 2017-08-15T09:48:05Z

app/code/Magento/Sales/Model/Order.php

@@ -214,6 +216,11 @@ class Order extends AbstractModel implements EntityInterface, OrderInterface
    protected $_currencyFactory;

    /**
+     * @var \Magento\Eav\Model\Config
+     */
+    protected $_eavConfig;


Thanks for noticing this 👍

No need to make it private I believe, not sure it will even pass the tests this way.

@orlangur It was dynamically declared (read: public), making it private may break things in 3rd party code?

I would give it a try and if no core tests are broken leave it private.

I believe if any extension relied on this property as public it's totally weird, inheritance from core classes is not recommended and property is not marked as @api.

orlangur · 2017-08-15T09:48:38Z

app/code/Magento/Sales/Model/Order.php

@@ -269,6 +276,11 @@ class Order extends AbstractModel implements EntityInterface, OrderInterface
    protected $timezone;

    /**
+     * @var ResolverInterface
+     */
+    protected $localeResolver;


Please make it private (this is a rule for any new class properties/methods).

orlangur · 2017-08-15T09:51:33Z

app/code/Magento/Sales/Model/Order.php

@@ -1831,7 +1847,7 @@ public function getCreatedAtFormatted($format)
            new \DateTime($this->getCreatedAt()),
            $format,
            $format,
-            null,
+            $this->localeResolver->getDefaultLocale(),


Seems like this public method can be easily covered with unit test, however, as there is no logic and we just call method of another class with a bunch of arguments I do not insist.

It indeed calls public methods, making a test would be duplicate I guess?

Nope, mock this dependency, the only thing to be asserted that we are passing concrete locale and not null.

Will do that later today and notify you when ready!

Thanks for keeping me updated, waiting for a ping.

orlangur · 2017-08-15T09:55:06Z

@JeroenVanLeusden cool job! Please do an amend commit + force push (only protected -> private for one field is a necessary field, other is at your discretion) and I will proceed with merging.

JeroenVanLeusden · 2017-08-15T16:11:11Z

@orlangur Want to test if the dependency is not null like

$localeResolver = $this->getMockBuilder(\Magento\Framework\Locale\ResolverInterface::class)
    ->disableOriginalConstructor()
    ->getMock();
$this->assertNotNull($localeResolver);

Or if the getDefaultLocale() returns a string?

orlangur · 2017-08-15T16:54:07Z

@JeroenVanLeusden NotNull is bad practice, some good-looking locale string like 'nl_NL' please.

JeroenVanLeusden · 2017-08-15T19:25:40Z

@orlangur Still getting used to Magento's tests please bare with me :)

$localeResolver = $this->getMockBuilder(\Magento\Framework\Locale\ResolverInterface::class)
    ->disableOriginalConstructor()
    ->getMock();

$localeResolver->expects($this->once())
     ->method('getDefaultLocale')
    ->willReturn('en_US');

Taken this looking from other tests but this doesn't work, method isn't being called. Also would like to test with some different languages then just en_US, what would be the way to approach this?

orlangur · 2017-08-15T19:50:31Z

@JeroenVanLeusden just make sure that you passed localeResolver into object under test.

As I mentioned, better use some non-default locale like 'nl_NL'. No need to test multiple cases I believe.

There is no need to assert that getDefaultLocale method was called, such checks are fragile, just check that correct locale is passed into called method of other mock dependency (timeWhatever->someMethod(..., locale instead of null, ...).

orlangur · 2017-08-15T22:24:55Z

@JeroenVanLeusden feel free to push your test even if it does not work yet, I'll do necessary changes, push and then you'll squash all changes into single commit again.

JeroenVanLeusden · 2017-08-16T07:52:37Z

@orlangur Pushed my changes so far. getDefaultLocale() returns null so that will need some work. Waiting for you ping to squash all changes :)

JeroenVanLeusden · 2017-08-18T15:19:53Z

@orlangur Any progress with the tests? Would love to see this merged.

orlangur · 2017-08-18T15:33:46Z

@JeroenVanLeusden, yeah, I'll have some time to work on this today, stay tuned.

orlangur · 2017-08-18T21:29:55Z

So, here is an example of appropriate test: orlangur@9e58ec2

Main idea behind unit tests is that we call only public methods of object under test and all other classes should be replaced with mocks (so, for example, setDefaultLocale of localeResolver does not work as it does nothing).

Please rebase against develop branch, adopt test to PHPUnit 6 to minimize merging efforts (getMock -> createMock, maybe something else) and then amend all changes into your first commit.

JeroenVanLeusden · 2017-08-19T07:43:21Z

@orlangur Getting an Error: Call to undefined method getMock() on almost all calls. Any idea where this might be coming from?

orlangur · 2017-08-19T15:22:55Z

@JeroenVanLeusden, I mentioned this, it is called now createMock. More details in https://thephp.cc/news/2017/02/migrating-to-phpunit-6

…always backend language

JeroenVanLeusden · 2017-08-20T10:28:39Z

@orlangur My bad, all good now!

…#10491

[EngCom] Public Pull Requests - MAGETWO-71769: Add missing translations in Customer module #10604 - MAGETWO-71761: Resolve fatal error in repository generator #10601 - MAGETWO-71603: Translate order getCreatedAtFormatted() to store locale #10491

orlangur self-assigned this Aug 11, 2017

orlangur added develop Progress: needs update labels Aug 11, 2017

orlangur added this to the August 2017 milestone Aug 14, 2017

orlangur requested changes Aug 15, 2017

View reviewed changes

magento-team added the develop label Aug 15, 2017

JeroenVanLeusden closed this Aug 19, 2017

JeroenVanLeusden reopened this Aug 19, 2017

Translate order getCreatedAtFormatted() to default locale instead of …

3b263af

…always backend language

orlangur added Progress: accept and removed Progress: needs update labels Aug 20, 2017

magento-team merged commit 3b263af into magento:develop Aug 22, 2017

magento-team pushed a commit that referenced this pull request Aug 22, 2017

MAGETWO-71603: Translate order getCreatedAtFormatted() to store locale …

65564bf

…#10491

JeroenVanLeusden mentioned this pull request Oct 13, 2017

[Backport 2.2] Translate order getCreatedAtFormatted() to store locale #11422

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate order getCreatedAtFormatted() to store locale #10491

Translate order getCreatedAtFormatted() to store locale #10491

JeroenVanLeusden commented Aug 11, 2017 •

edited by orlangur

Loading

orlangur commented Aug 11, 2017

JeroenVanLeusden commented Aug 11, 2017

orlangur commented Aug 14, 2017

JeroenVanLeusden commented Aug 14, 2017

orlangur commented Aug 14, 2017

JeroenVanLeusden commented Aug 14, 2017 •

edited

Loading

orlangur commented Aug 14, 2017

JeroenVanLeusden commented Aug 15, 2017

orlangur Aug 15, 2017

orlangur Aug 15, 2017

JeroenVanLeusden Aug 15, 2017 •

edited

Loading

orlangur Aug 15, 2017

orlangur Aug 15, 2017

orlangur Aug 15, 2017

JeroenVanLeusden Aug 15, 2017

orlangur Aug 15, 2017

JeroenVanLeusden Aug 15, 2017

orlangur Aug 15, 2017

orlangur commented Aug 15, 2017

JeroenVanLeusden commented Aug 15, 2017

orlangur commented Aug 15, 2017

JeroenVanLeusden commented Aug 15, 2017

orlangur commented Aug 15, 2017

orlangur commented Aug 15, 2017

JeroenVanLeusden commented Aug 16, 2017

JeroenVanLeusden commented Aug 18, 2017

orlangur commented Aug 18, 2017

orlangur commented Aug 18, 2017

JeroenVanLeusden commented Aug 19, 2017

orlangur commented Aug 19, 2017

JeroenVanLeusden commented Aug 20, 2017

Translate order getCreatedAtFormatted() to store locale #10491

Translate order getCreatedAtFormatted() to store locale #10491

Conversation

JeroenVanLeusden commented Aug 11, 2017 • edited by orlangur Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

orlangur commented Aug 11, 2017

JeroenVanLeusden commented Aug 11, 2017

orlangur commented Aug 14, 2017

JeroenVanLeusden commented Aug 14, 2017

orlangur commented Aug 14, 2017

JeroenVanLeusden commented Aug 14, 2017 • edited Loading

orlangur commented Aug 14, 2017

JeroenVanLeusden commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeroenVanLeusden Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orlangur commented Aug 15, 2017

JeroenVanLeusden commented Aug 15, 2017

orlangur commented Aug 15, 2017

JeroenVanLeusden commented Aug 15, 2017

orlangur commented Aug 15, 2017

orlangur commented Aug 15, 2017

JeroenVanLeusden commented Aug 16, 2017

JeroenVanLeusden commented Aug 18, 2017

orlangur commented Aug 18, 2017

orlangur commented Aug 18, 2017

JeroenVanLeusden commented Aug 19, 2017

orlangur commented Aug 19, 2017

JeroenVanLeusden commented Aug 20, 2017

JeroenVanLeusden commented Aug 11, 2017 •

edited by orlangur

Loading

JeroenVanLeusden commented Aug 14, 2017 •

edited

Loading

JeroenVanLeusden Aug 15, 2017 •

edited

Loading