AO3-6042 Fix that emails ignored the locale preference feature flag

[Bilka2]

Issue

https://otwarchive.atlassian.net/browse/AO3-6042

Purpose

Check that the :set_locale_preference rollout is active before using the users locale preference for emails. If it is not active, fall back to the default locale.

Testing Instructions

See Jira.

Credit

Bilka (he/him)

[sarken]

A thought: would it make sense to override preferred_locale to work like this instead of adding a new email-specific method? I can't imagine we'd ever want to get and use the preferred locale if the feature flag was off.

[Bilka2]

I considered that, however the preference index uses it in dev and test even when the rollout is disabled:

otwarchive/app/views/preferences/index.html.erb

Lines 82 to 85 in da664ae

	<% if Rails.env.test? || Rails.env.development? || $rollout.active?(:set_locale_preference, @user) %>
	<dt><%= f.label :preferred_locale, ts('Your locale') %> <%= link_to_help 'locale-preferences' %></dt>
	<dd><%= f.select :preferred_locale, locale_options_for_select(@available_locales, "id"),
	default: @preference.preferred_locale %></dd>

It was an easy way to reproduce the bug here - select a locale there despite the rollout being deactivated and you'd get localised emails.

But with this bug fix, the preference doesn't affect anything else with the inactive rollout. So if wanted, I could remove the exception for dev/test on the preference index and override preferred_locale.

Followup question if that's wanted: The locale_for_emails method returns the locale iso. I assume I should switch back to returning the locale id when overriding preferred_locale so that the format is consistent with the schema? (and so that it can be easily used in the preference index)

[Bilka2]

I realized how silly it sounds to have a setting in test/dev that does nothing (if the rollout is deactivated). So went and changed it to an override and made the setting hidden unless the rollout is active.

[sarken]

Excellent, thank you! That was indeed a bit silly of us to have it set up that way.

This PR is fine as-is now, but we've got a few single-issue deploys we still need to get through before there's any chance of merging other things. Since we're talking about silly things we did in the past that could be improved upon, would you be interested in tweaking the preferred_locale code a bit more?

If so, we were thinking it would be an improvement to set the preferred_locale up as a Rails relation and to define a locale_iso method for returning the ISO code for the user's preferred locale. That would let us use the simpler user.preference.locale_iso in mailers instead of Locale.find(@user.preference.preferred_locale).iso.

b3e436c by @redsummernight is an earlier draft of what we had in mind, using the name i18n_locale instead of locale_iso.

No worries if you don't have time or just plain don't want to, though! We can always do it in another issue/pull request.

[Bilka2]

I'm up for that, I'll look into it!

[Bilka2]

Okay, I turned preferred_locale into a belongs_to. Two points that are different from the commit by redsummernight:

1. My commit has the override on the relation instead of making a new method. For reasons see the first 3 comments in this thread.
2. My commit does not add the inverse has_many on locale because
   (a) I don't think you'd ever want to get all preferences that have the locale set (current code doesn't).
   (b) With the rollout off, Locale.find_by(iso: "de-DE").preferences.first.locale != Locale.find_by(iso: "de-DE") due to the overridden method. So that'd be very confusing. This is easily avoided by making Locale.preferences not a thing, by leaving out the inverse has_many.