search

RefugeRestrooms / refugerestrooms

REFUGE restrooms indexes and maps safe restroom locations for trans, intersex, and gender nonconforming individuals.
http://www.refugerestrooms.org
GNU Affero General Public License v3.0
888 stars 262 forks source link

Enhancement/i18n thread safe #647

Closed brunoocasali closed 3 years ago

brunoocasali commented 3 years ago

Context

This code makes any request free of locale leaking problems.

According to the rails guides: https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests Using I18n.locale is not a recommended way to set current locale for a request, using the with_locale is the best option!

Btw, searching more about the http_accept_language I've found this https://github.com/iain/http_accept_language/issues/68

Summary of Changes

Checklist

brunoocasali commented 3 years ago

This PR is related to https://github.com/RefugeRestrooms/refugerestrooms/pull/641 :)

DeeDeeG commented 3 years ago

cc @mi-wood again, or @tkwidmer. This seems like a good PR, making sure our internationalization (I18n) is ready for multi-threaded Puma. It even adds tests!

I'm personally not very good with Ruby, so I don't think my review would be super meaningful here. If anyone good with Ruby can give this a second pair of eyes, that would be great. Or at least give it the thumbs up so we can approve it, so the author gets credit for this PR toward their Hacktoberfest "5 good pull requests" goals.

brunoocasali commented 3 years ago

@DeeDeeG @tkwidmer rebased with latest version of develop branch, can I do something to help this get merged? :)

DeeDeeG commented 3 years ago

@brunoocasali thank you for rebasing!

I have simply been backed up with the number of pull requests we got this month, plus other life things.

(I'm trying to work through the remaining pull requests. The fewer of them that are left, the easier reviewing each one becomes, due to not having to worry which one will have merge conflicts, or which one will cause the next one to be harder to test.)

I am going to ensure that all the helpful Pull Requests we have received are either "Approved" or merged by the end of the month. I believe all of your Pull Requests are looking good, and we will be approving and eventually merging them. Thanks for sticking with this as I work through the backlog.

DeeDeeG commented 3 years ago

I do recall the documentation for I18n saying this is the correct way. If there are no issues in manual testing, then I am inclined to merge this.

DeeDeeG commented 3 years ago

Okay, this works.

I tested by going to about:preferences in Firefox, updating the "preferred language for displaying pages", then visiting links/refreshing the page at https://refuge-deedeeg-test.herokuapp.com. My "preferred language" was always respected accurately.

(As I understand it, this will only really make a difference if/when we enable multiple workers for Puma some time in the future. Note to other maintainers/readers: see #641 for discussion about multiple workers in Puma. May or may not be merged soon, depending on how review/discussion goes.)

I then compared page load times between:

I didn't notice a significant difference; either one would be faster or slower seemingly at random. So this pull request does not appear to have any adverse impact on performance.

I'll watch this change on the staging instance, after merging, and hopefully it will continue to behave well as intended in the coming days. In my opinion, this is ready to merge.

brunoocasali commented 3 years ago

I didn't notice a significant difference; either one would be faster or slower seemingly at random. So this pull request does not appear to have any adverse impact on performance

Yeah, this change will not improve the performance or impact in a negative way either. This just makes sure that there is no way someone will request a page in "pt-BR" and get the answer in "fr" because someone requested nanosecods before :)