add locale parameter to links & forms which should include it

[mrbrdo]

May need more of these in other places, but this fixed the most important stuff for me.

[nciemniak]

Hey @mrbrdo, can you give some context on the PR? Specifically, what was going wrong/not working that your PR fixes? Would be useful for reviewing and testing on my end.

Also, in general, thanks for the contributions :) I'm one of the devs that will be helping to maintain the spree code, it's great to have members of the community involved in the process 😎

[mrbrdo]

@nciemniak well in general the localization functionality had a lot of problems for me. Many times while clicking around the site, the selected locale will be lost (as it is only saved in the URL itself).
To test it, you should create a non-default locale (for example German or French etc), and keep English as default.
Then you can try changing locale to German and test individually the cases:

• register new user - locale will change back to english, email will possibly be in english (to notice this you need translations for chosen language)
• search for product - locale will change back
• a lot of other stuff as well

In general a lot of issues and probably the worst part of the legacy frontend. But these are at least the most important ones.
I made a gem https://github.com/mrbrdo/better_spree_localization to further fix issues with localization, but the ones in this commit were necessary to do in the frontend gem itself.

I'm definitely glad that you guys are processing PRs. I put a lot of work into fixing the issues I had for my own store and I appreciate it a lot that changes are being merged back upstream, so I don't have to maintain a fork with 100 changes.

[rafalcymerys]

@mrbrdo we tried this on a few machines (running with Spree 4.5 beta), but we couldn't reproduce it - the redirects always persisted locale. What you described in the PR makes sense though - given that a better internationalization and content translations are part of the roadmap we should:

• store user's last locale in the database - so that we can trigger asynchronous email in the correct locale
• store session locale in a cookie and redirect to it, if accessing a session in a different locale

Would that make sense from your perspective?

[mrbrdo]

That's curious that you didn't have that issue. I did test that with 4.3.x but I don't remember that you did any changes to this functionality. If I have some time I may test again without these fixes, but even if then not this week (black friday and all).

There are 2 problems or things to keep in mind with suggested approach:

• if the user is not logged in, should still work correctly, including emails (including even the registration email)
• because of SEO the URLs should probably include the locale

I think the current approach is not completely flawed in this sense, the only issue is that there are places where the locale is left out. And that it's quite easy to forget about it. I'm not sure if saving the locale in db (or even cookies) for the user itself is necessarily the best way to go around it, although maybe it could be.

The async email locale now works correctly for me, I think I fixed that with https://github.com/mrbrdo/better_spree_localization/blob/main/lib/better_spree_localization/core_ext/spree/order_decorator.rb and https://github.com/mrbrdo/better_spree_localization/blob/main/lib/better_spree_localization/railtie.rb#L66 or something else from that gem. With the changes in this PR + that gem I made I don't really observe any more issues with locale changing, so I think it's quite possible to fix it without changing the general approach. The gem is kinda a dirty hack though because it also monkeypatches the url helpers to add the locale under certain conditions - for a proper fix the url generation should be fixed in a better way.

[wjwitek]

@mrbrdo have you added scope for locale in spree_auth_devise or somewhere else? I was trying to replicate your issue, but it only appeared after that, and also what you suggested fixed it.

[mrbrdo]

@wjwitek I don't understand what you mean, can you provide a specific example of how to add the scope, and I will check/try that?
I don't remember adding anything like that, but you never know. Also, it could have been different with Spree 4.3.1 (when I observed most of the issues fixed by these PRs). In case everything can be fixed in such a simple way, I will be very happy.

[wjwitek]

@mrbrdo Ok, to provide more context: I was trying to unify locale in url, since in login/registration etc. it was as param, not part of url itself, becouse everything to do with account is handled by spree/spree_auth_devise. So in spree_auth_devise's routes.rb I added scope like here: spree/spree_auth_devise#570 and that's when I started getting problems with lost locale as you've described.
And as I was earlier trying to recreate your issue, I got curious if this scope may have been the difference and reason why I was not loosing locale before.
Sorry if something is not clear, I'm quite new to spree and still learning a lot of things :)

[mrbrdo]

@wjwitek ok now I understand. I am using the main branch of spree/spree_auth_devise, so no changes to the scope. My signup URL looks like this: /signup?locale=sl-SI
Not sure if your case is directly related to mine.

[nciemniak]

• register new user - locale will change back to english, email will possibly be in english (to notice this you need translations for chosen language)
• search for product - locale will change back

@mrbrdo If you can reproduce these issues you had described on a clean copy of the spree app, let us know. For now we will hold off on merging your changes since the locale works as expected in login and search.

[mrbrdo]

@nciemniak ok, I will test it. We are talking about legacy frontend right?

[nciemniak]

@mrbrdo yes, legacy frontend, correct 👍

[mrbrdo]

@nciemniak I went to test with a spree_starter v4.5 fresh app and it seems these issues are indeed fixed and my commit seems not necessary. I have to test further with my app, if something comes up, I will let you know. I'm pretty sure these issues existed in 4.3.1 though, must have been fixed since. Good thing you are going through PRs faster now so this kind of situation will come up less in the future :)
The one issue that exists still is #41 but I think we need a new fix for that.
Cheers