Add a way to keep locale querystring when click on search

[brunoocasali]

CC: @DeeDeeG I've pointed this PR to your branch because I've no access to create commits there 😝

[DeeDeeG]

Thanks for this fix!

I find that it's mostly working. But there is one test failure. One of the tests uses the search bar and ends up on a page with an invalid locale.

I believe this is because no locale is set during most of the tests, and the hidden_field_tag added here always includes a locale= query string into the search results URL. So in the default situation, with no specific locale set in the app anywhere, the query string ?locale= (with a blank value) is included to the search results URL, and the app acts as if a locale has been set, even though the locale's name is zero-length/null.

In the end, a locale with a zero-length/null name is effectively an invalid locale, considering the way we have things coded right now.

To avoid this, I think it's best to not include the hidden_field_tag (the one that sets the locale query string for the search bar results page) if the locale parameter is unset.

I have a commit to solve this, let me know if this fix looks good to you, or if you have any issues with this fix: DeeDeeG@dff451d

I can probably push that commit to your branch, or you can commit a fix yourself or merge this commit, but I didn't want to do so without asking first.

Best Regards,

- DeeDeeG

[brunoocasali]

@DeeDeeG you can delete this branch if you want!

I don't ran the tests before (my bad - sorry!), actually I was looking for the Travis CI result, but it stucks on some kind of build, and then I wait for your response ;)

I think your fix is pretty nice, and we can use it, an alternative to it is just add the default language to that hidden field:

= hidden_field_tag :locale, params[:locale] || I18n.default_locale, is less verbose than the first option but equally effective (all the tests passes here).

[DeeDeeG]

I will think about which solution to use tomorrow as it is late in my time zone. But then this will be ready to go.

With that, all the hard-coded links are gone. The locale switcher should be ready once this fix lands on the locale switcher branch. 🎉

[DeeDeeG]

Hi again.

After testing, I've decided it's best to not set a locale in the search bar, if a locale hasn't been set yet.

The problem with falling back to I18n.default_locale (en), is that it doesn't match the logic from here:

refugerestrooms/app/controllers/application_controller.rb

Lines 14 to 15 in 5cff0cb

	locale = http_accept_language.language_region_compatible_from(I18n.available_locales)
	locale ||= I18n.default_locale

(Visitors to the site may have a locale auto-determined to be something other than our default: English. In which case the search bar should not send them to the English site.)

And writing out all that locale-auto-setting logic here would be code duplication and would be rather verbose. Either we can refactor all that out into a helper (maybe? I've never refactored Ruby/Rails code in that way so I don't know if it makes sense to try it here...) or simply have the search bar not set a locale at all if it's not already set.

The "don't set a locale" option makes the most sense to me personally. Though I admit the helper option might be okay. I like simple code, too, and "no helper" is simpler.

---

Describing the problem practically: A user could visit the site at https://refugerestrooms.org (no ?locale= query string) and have their ideal locale automatically determined to be, let's say fr. Then they click the search bar, which sends them to the English site. They would have expected to stay on fr.

---

I can amend this Pull Request to the solution I linked to before (DeeDeeG@dff451d) or if you have another idea I'd be open to discuss it as always.

Best Regards,

- DeeDeeG

[brunoocasali]

@DeeDeeG after many interactions in this code I think this is the best of it! If you have other use cases please tell me ;)

[DeeDeeG]

More code to review here, I'm thinking it over. Looks like a working design, no bugs, great. 👍

On my mind:

• Increased size/complexity of code, is it worth it?
  • Thank you for adding tests!
  • Nice abstractions here, and this provides good "ergonomics" as a developer to reuse this code, but I feel it may be good to keep locale picking logic simpler/shorter. I'm not immediately able to think of other places to use this helper/service.
  • I just yesterday noticed I still need to test if the "submit a new restroom" form preserves locale or not. If it doesn't, then that needs to be fixed. In which case, that form would probably be a good place for this kind of helper, re-using this code.
• Performance. Not sure how to benchmark this, so I probably can't properly see if this performs differently... But I'm planning to test manually just to see if there is any obvious, large change.

This one is more than I can get to in one day, but hopefully soon.

[brunoocasali]

@DeeDeeG the ideia behind the LocaleService is just to provide a way not just for reuse but mainly for testability.
If I keep all the logic inside of the application_controller, then I should deal with rails complexities, and of course the possibilities of reuse will be lower.

There are something I can do to help you to send this whole feature to production? (btw I joined the slack channel, we could talk there)

[tkwidmer]

This looks pretty straightforward to me.