Hydrate translatable

[c960657]

Follow-up to #23879, based on comment from @ClearlyClaire:

StatusCacheHydrator should hydrate translatable based on current user and locale.

[ClearlyClaire]

Hi! Thank you for your contribution, but I believe this is still incorrect!

Swapping the example case around by setting the user's language to es and the post's to en highlights the failure:

diff --git a/spec/lib/status_cache_hydrator_spec.rb b/spec/lib/status_cache_hydrator_spec.rb
index d1a1847309..da19f591c1 100644
--- a/spec/lib/status_cache_hydrator_spec.rb
+++ b/spec/lib/status_cache_hydrator_spec.rb
@@ -4,7 +4,7 @@ require 'rails_helper'
 
 describe StatusCacheHydrator do
   let(:status)  { Fabricate(:status) }
-  let(:account) { Fabricate(:account) }
+  let(:account) { Fabricate(:user, locale: 'es').account }
 
   describe '#hydrate' do
     let(:compare_to_hash) { InlineRenderer.render(status, account, :status) }
@@ -30,7 +30,7 @@ describe StatusCacheHydrator do
 
       context 'when handling a translatable status' do
         let(:poll) { Fabricate(:poll, account: account) }
-        let(:status) { Fabricate(:status, poll: poll, account: account, language: 'es') }
+        let(:status) { Fabricate(:status, poll: poll, account: account, language: 'en') }
 
         before do
           service = instance_double(TranslationService::DeepL, supported?: true)

This is due to I18n.locale not being set to the user's locale in StatusCacheHydrator call sites, which makes sense as it did not previously return locale-specific values.

More generally, there is also the issue that the client can override the locale per API request using the lang parameter.

[c960657]

Thank you for your review. I am still new to Mastodon, so I appreciate your insights.

I have now wrapped the calls from StatusCacheHydrator and InlineRenderer to Status#translatable? in I18n.with_locale. Is this sufficient?

More generally, there is also the issue that the client can override the locale per API request using the lang parameter.

When the status is serialised via an API request, translatable? uses the overridden locale (?lang=xx) instead of current_user.locale, so I assume this works as intended?

I considered whether the target locale should be passed as an argument for translatable?, but I noticed that other models access I18n.locale directly, so I went with that.

[ClearlyClaire]

This is better, but I'm afraid this could negate some of the performance benefits of using StatusCacheHydrator in the first place (though maybe I'm wrong and it does not change anything, I'll try to find some time to investigate this).

What I'm more worried about though are the inconsistencies with how locale selection is done in other situations. Indeed, until your recent change, the streaming API was locale-agnostic, and the locale only mattered on the REST API endpoints. The locale selection for the REST API is handled in app/controllers/concerns/localized.rb as following:

• use the lang query parameter if defined
• otherwise, use the user-set locale if valid (you're missing the check to see if it's valid, but that can quickly be fixed)
• otherwise, use the Accept-Language request header
• otherwise, use the DEFAULT_LANGUAGE environment variable

The lang query parameter and the Accept-Language request header are unavailable in the streaming-related codepaths (one could get them in the streaming server, but it would be a significant architectural change and would mean you could have multiple channels for a single user, since the languages could be different).

I'm not sure what the best way to proceed would be.

Maybe a good compromise would be to come back on the initial change, and instead output the list of supported input languages, and the list of supported output languages. Though translation services might also work on language couples rather than such lists…

[c960657]

Adding ?lang=de in the browser changes the UI language to German, but AFAICT it does not affect API requests made from the React app, so pressing Translate will tranlate the status into the user language, not German. I don't know if that is intended, but I guess that is a separate issue.

Maybe a good compromise would be to come back on the initial change, and instead output the list of supported input languages, and the list of supported output languages. Though translation services might also work on language couples rather than such lists…

I have opened a separate PR with an alternative implementation along these lines. This is a better fit for the current caching and streaming logic, and it only moves a little bit of the logic back into React.

[c960657]

Closing in favour of #24037.