FEATURE: Warn about outdated translation overrides in admin dashboard (#22384)

This PR adds a feature to help admins stay up-to-date with their translations. We already have protections preventing admins from problems when they update their overrides. This change adds some protection in the other direction (where translations change in core due to an upgrade) by creating a notice for admins when defaults have changed.

Terms:

- In the case where Discourse core changes the default translation, the translation override is considered "outdated".
- In the case above where interpolation keys were changed from the ones the override is using, it is considered "invalid".
- If none of the above applies, the override is considered "up to date".

How does it work?

There are a few pieces that makes this work:

- When an admin creates or updates a translation override, we store the original translation at the time of write. (This is used to detect changes later on.)
- There is a background job that runs once every day and checks for outdated and invalid overrides, and marks them as such.
- When there are any outdated or invalid overrides, a notice is shown in admin dashboard with a link to the text customization page.

Known limitations

The link from the dashboard links to the default locale text customization page. Given there might be invalid overrides in multiple languages, I'm not sure what we could do here. Consideration for future improvement.

app/assets/javascripts/admin/addon/controllers/admin-site-text-index.js

@@ -8,10 +8,11 @@ export default class AdminSiteTextIndexController extends Controller {
   searching = false;
   siteTexts = null;
   preferred = false;
-  queryParams = ["q", "overridden", "locale"];
+  queryParams = ["q", "overridden", "outdated", "locale"];
   locale = null;
   q = null;
   overridden = false;
+  outdated = false;
 
   init() {
     super.init(...arguments);
@@ -21,7 +22,10 @@ export default class AdminSiteTextIndexController extends Controller {
 
   _performSearch() {
     this.store
-      .find("site-text", this.getProperties("q", "overridden", "locale"))
+      .find(
+        "site-text",
+        this.getProperties("q", "overridden", "outdated", "locale")
+      )
       .then((results) => {
         this.set("siteTexts", results);
       })
@@ -58,6 +62,13 @@ export default class AdminSiteTextIndexController extends Controller {
     discourseDebounce(this, this._performSearch, 400);
   }
 
+  @action
+  toggleOutdated() {
+    this.toggleProperty("outdated");
+    this.set("searching", true);
+    discourseDebounce(this, this._performSearch, 400);
+  }
+
   @action
   search() {
     const q = this.q;

app/assets/javascripts/admin/addon/routes/admin-site-text-index.js

@@ -6,13 +6,14 @@ export default class AdminSiteTextIndexRoute extends Route {
   queryParams = {
     q: { replace: true },
     overridden: { replace: true },
+    outdated: { replace: true },
     locale: { replace: true },
   };
 
   model(params) {
     return this.store.find(
       "site-text",
-      getProperties(params, "q", "overridden", "locale")
+      getProperties(params, "q", "overridden", "outdated", "locale")
     );
   }
 

app/assets/javascripts/admin/addon/templates/site-text-index.hbs

@@ -34,12 +34,23 @@
 
     <label>
       <Input
+        id="toggle-overridden"
         @type="checkbox"
         @checked={{this.overridden}}
         {{on "click" (action "toggleOverridden")}}
       />
       {{i18n "admin.site_text.show_overriden"}}
     </label>
+
+    <label>
+      <Input
+        id="toggle-outdated"
+        @type="checkbox"
+        @checked={{this.outdated}}
+        {{on "click" this.toggleOutdated}}
+      />
+      {{i18n "admin.site_text.show_outdated"}}
+    </label>
   </p>
 </div>
 

app/assets/javascripts/discourse/tests/acceptance/admin-site-text-test.js

@@ -24,7 +24,7 @@ acceptance("Admin - Site Texts", function (needs) {
     assert.ok(exists(".site-text.overridden"));
 
     // Only show overridden
-    await click(".search-area .filter-options input");
+    await click(".search-area .filter-options #toggle-overridden");
     assert.strictEqual(
       currentURL(),
       "/admin/customize/site_texts?overridden=true&q=Test"
@@ -32,6 +32,14 @@ acceptance("Admin - Site Texts", function (needs) {
 
     assert.ok(!exists(".site-text:not(.overridden)"));
     assert.ok(exists(".site-text.overridden"));
+    await click(".search-area .filter-options #toggle-overridden");
+
+    // Only show outdated
+    await click(".search-area .filter-options #toggle-outdated");
+    assert.strictEqual(
+      currentURL(),
+      "/admin/customize/site_texts?outdated=true&q=Test"
+    );
   });
 
   test("edit and revert a site text by key", async function (assert) {

app/controllers/admin/site_texts_controller.rb

@@ -20,17 +20,18 @@ class Admin::SiteTextsController < Admin::AdminController
 
   def index
     overridden = params[:overridden] == "true"
+    outdated = params[:outdated] == "true"
     extras = {}
 
     query = params[:q] || ""
 
     locale = fetch_locale(params[:locale])
 
-    if query.blank? && !overridden
+    if query.blank? && !overridden && !outdated
       extras[:recommended] = true
       results = self.class.preferred_keys.map { |k| record_for(key: k, locale: locale) }
     else
-      results = find_translations(query, overridden, locale)
+      results = find_translations(query, overridden, outdated, locale)
 
       if results.any?
         extras[:regex] = I18n::Backend::DiscourseI18n.create_search_regexp(query, as_string: true)
@@ -188,10 +189,18 @@ class Admin::SiteTextsController < Admin::AdminController
     raise Discourse::NotFound
   end
 
-  def find_translations(query, overridden, locale)
+  def find_translations(query, overridden, outdated, locale)
     translations = Hash.new { |hash, key| hash[key] = {} }
     search_results = I18n.with_locale(locale) { I18n.search(query, only_overridden: overridden) }
 
+    if outdated
+      outdated_keys =
+        TranslationOverride.where(status: %i[outdated invalid_interpolation_keys]).pluck(
+          :translation_key,
+        )
+      search_results.select! { |k, _| outdated_keys.include?(k) }
+    end
+
     search_results.each do |key, value|
       if PLURALIZED_REGEX.match(key)
         translations[$1][$2] = value

app/jobs/scheduled/check_translation_overrides.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+module Jobs
+  class CheckTranslationOverrides < ::Jobs::Scheduled
+    every 1.day
+
+    def execute(args)
+      invalid_ids = []
+      outdated_ids = []
+
+      TranslationOverride.find_each do |override|
+        if override.invalid_interpolation_keys.present?
+          invalid_ids << override.id
+        elsif override.original_translation_updated?
+          outdated_ids << override.id
+        end
+      end
+
+      TranslationOverride.where(id: outdated_ids).update_all(status: "outdated")
+      TranslationOverride.where(id: invalid_ids).update_all(status: "invalid_interpolation_keys")
+    end
+  end
+end

app/models/admin_dashboard_data.rb

@@ -206,7 +206,8 @@ class AdminDashboardData
                       :out_of_date_themes,
                       :unreachable_themes,
                       :watched_words_check,
-                      :google_analytics_version_check
+                      :google_analytics_version_check,
+                      :translation_overrides_check
 
     register_default_scheduled_problem_checks
 
@@ -360,6 +361,12 @@ class AdminDashboardData
     end
   end
 
+  def translation_overrides_check
+    if TranslationOverride.exists?(status: %i[outdated invalid_interpolation_keys])
+      I18n.t("dashboard.outdated_translations_warning", base_path: Discourse.base_path)
+    end
+  end
+
   def image_magick_check
     if SiteSetting.create_thumbnails && !system("command -v convert >/dev/null;")
       I18n.t("dashboard.image_magick_warning")

app/models/translation_override.rb

@@ -45,14 +45,18 @@ class TranslationOverride < ActiveRecord::Base
 
   validate :check_interpolation_keys
 
+  enum :status, %i[up_to_date outdated invalid_interpolation_keys]
+
   def self.upsert!(locale, key, value)
     params = { locale: locale, translation_key: key }
 
     translation_override = find_or_initialize_by(params)
     sanitized_value =
       translation_override.sanitize_field(value, additional_attributes: ["data-auto-route"])
+    original_translation =
+      I18n.overrides_disabled { I18n.t(transform_pluralized_key(key), locale: :en) }
 
-    data = { value: sanitized_value }
+    data = { value: sanitized_value, original_translation: original_translation }
     if key.end_with?("_MF")
       _, filename = JsLocaleHelper.find_message_format_locale([locale], fallback_to_english: false)
       data[:compiled_js] = JsLocaleHelper.compile_message_format(filename, locale, sanitized_value)
@@ -125,39 +129,48 @@ class TranslationOverride < ActiveRecord::Base
   private_class_method :i18n_changed
   private_class_method :expire_cache
 
-  private
+  def original_translation_updated?
+    return false if original_translation.blank?
 
-  def check_interpolation_keys
+    transformed_key = self.class.transform_pluralized_key(translation_key)
+
+    original_translation != I18n.overrides_disabled { I18n.t(transformed_key, locale: :en) }
+  end
+
+  def invalid_interpolation_keys
     transformed_key = self.class.transform_pluralized_key(translation_key)
 
     original_text = I18n.overrides_disabled { I18n.t(transformed_key, locale: :en) }
 
-    if original_text
-      original_interpolation_keys = I18nInterpolationKeysFinder.find(original_text)
-      new_interpolation_keys = I18nInterpolationKeysFinder.find(value)
-      custom_interpolation_keys = []
+    return [] if original_text.blank?
 
-      ALLOWED_CUSTOM_INTERPOLATION_KEYS.select do |keys, value|
-        custom_interpolation_keys = value if keys.any? { |key| transformed_key.start_with?(key) }
-      end
+    original_interpolation_keys = I18nInterpolationKeysFinder.find(original_text)
+    new_interpolation_keys = I18nInterpolationKeysFinder.find(value)
+    custom_interpolation_keys = []
 
-      invalid_keys =
-        (original_interpolation_keys | new_interpolation_keys) - original_interpolation_keys -
-          custom_interpolation_keys
-
-      if invalid_keys.present?
-        self.errors.add(
-          :base,
-          I18n.t(
-            "activerecord.errors.models.translation_overrides.attributes.value.invalid_interpolation_keys",
-            keys: invalid_keys.join(I18n.t("word_connector.comma")),
-            count: invalid_keys.size,
-          ),
-        )
-
-        false
-      end
+    ALLOWED_CUSTOM_INTERPOLATION_KEYS.select do |keys, value|
+      custom_interpolation_keys = value if keys.any? { |key| transformed_key.start_with?(key) }
     end
+
+    (original_interpolation_keys | new_interpolation_keys) - original_interpolation_keys -
+      custom_interpolation_keys
+  end
+
+  private
+
+  def check_interpolation_keys
+    invalid_keys = invalid_interpolation_keys
+
+    return if invalid_keys.blank?
+
+    self.errors.add(
+      :base,
+      I18n.t(
+        "activerecord.errors.models.translation_overrides.attributes.value.invalid_interpolation_keys",
+        keys: invalid_keys.join(I18n.t("word_connector.comma")),
+        count: invalid_keys.size,
+      ),
+    )
   end
 end
 
@@ -165,13 +178,15 @@ end
 #
 # Table name: translation_overrides
 #
-#  id              :integer          not null, primary key
-#  locale          :string           not null
-#  translation_key :string           not null
-#  value           :string           not null
-#  created_at      :datetime         not null
-#  updated_at      :datetime         not null
-#  compiled_js     :text
+#  id                   :integer          not null, primary key
+#  locale               :string           not null
+#  translation_key      :string           not null
+#  value                :string           not null
+#  created_at           :datetime         not null
+#  updated_at           :datetime         not null
+#  compiled_js          :text
+#  original_translation :text
+#  status               :integer          default("up_to_date"), not null
 #
 # Indexes
 #

config/locales/client.en.yml

@@ -6068,6 +6068,7 @@ en:
         go_back: "Back to Search"
         recommended: "We recommend customizing the following text to suit your needs:"
         show_overriden: "Only show overridden"
+        show_outdated: "Only show outdated/invalid"
         locale: "Language:"
         more_than_50_results: "There are more than 50 results. Please refine your search."
         interpolation_keys: "Available interpolation keys:"

config/locales/server.en.yml

@@ -1478,6 +1478,7 @@ en:
     image_magick_warning: 'The server is configured to create thumbnails of large images, but ImageMagick is not installed. Install ImageMagick using your favorite package manager or <a href="https://www.imagemagick.org/script/download.php" target="_blank">download the latest release</a>.'
     failing_emails_warning: 'There are %{num_failed_jobs} email jobs that failed. Check your app.yml and ensure that the mail server settings are correct. <a href="%{base_path}/sidekiq/retries" target="_blank">See the failed jobs in Sidekiq</a>.'
     subfolder_ends_in_slash: "Your subfolder setup is incorrect; the DISCOURSE_RELATIVE_URL_ROOT ends in a slash."
+    outdated_translations_warning: "Some of your translation overrides are out of date. Please check your <a href='%{base_path}/admin/customize/site_texts?outdated=true'>text customizations</a>."
     email_polling_errored_recently:
       one: "Email polling has generated an error in the past 24 hours. Look at <a href='%{base_path}/logs' target='_blank'>the logs</a> for more details."
       other: "Email polling has generated %{count} errors in the past 24 hours. Look at <a href='%{base_path}/logs' target='_blank'>the logs</a> for more details."

db/migrate/20230703035052_add_status_to_translation_overrides.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class AddStatusToTranslationOverrides < ActiveRecord::Migration[7.0]
+  def change
+    add_column :translation_overrides, :original_translation, :text
+    add_column :translation_overrides, :status, :integer, null: false, default: 0
+  end
+end

spec/fabricators/translation_override_fabricator.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+Fabricator(:translation_override) do
+  translation_key "title"
+  value "Discourse"
+  locale "en"
+end

spec/jobs/check_translation_overrides_spec.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+RSpec.describe Jobs::CheckTranslationOverrides do
+  fab!(:up_to_date_translation) { Fabricate(:translation_override, translation_key: "title") }
+  fab!(:outdated_translation) do
+    Fabricate(:translation_override, translation_key: "posts", original_translation: "outdated")
+  end
+  fab!(:invalid_translation) { Fabricate(:translation_override, translation_key: "topics") }
+
+  it "marks translations with invalid interpolation keys" do
+    invalid_translation.update_attribute("value", "Invalid %{foo}")
+
+    expect { described_class.new.execute({}) }.to change { invalid_translation.reload.status }.from(
+      "up_to_date",
+    ).to("invalid_interpolation_keys")
+  end
+
+  it "marks translations that are outdated" do
+    expect { described_class.new.execute({}) }.to change {
+      outdated_translation.reload.status
+    }.from("up_to_date").to("outdated")
+  end
+
+  it "does not mark up to date translations" do
+    expect { described_class.new.execute({}) }.not_to change {
+      up_to_date_translation.reload.status
+    }
+  end
+end

spec/models/admin_dashboard_data_spec.rb

@@ -345,4 +345,26 @@ RSpec.describe AdminDashboardData do
       expect(dashboard_data.out_of_date_themes).to eq(nil)
     end
   end
+
+  describe "#translation_overrides_check" do
+    subject(:dashboard_data) { described_class.new }
+
+    context "when there are outdated translations" do
+      before { Fabricate(:translation_override, translation_key: "foo.bar", status: "outdated") }
+
+      it "outputs the correct message" do
+        expect(dashboard_data.translation_overrides_check).to eq(
+          I18n.t("dashboard.outdated_translations_warning", base_path: Discourse.base_path),
+        )
+      end
+    end
+
+    context "when there are no outdated translations" do
+      before { Fabricate(:translation_override, status: "up_to_date") }
+
+      it "outputs nothing" do
+        expect(dashboard_data.translation_overrides_check).to eq(nil)
+      end
+    end
+  end
 end

spec/models/translation_override_spec.rb

@@ -283,4 +283,43 @@ RSpec.describe TranslationOverride do
       end
     end
   end
+
+  describe "#original_translation_updated?" do
+    context "when the translation is up to date" do
+      fab!(:translation) { Fabricate(:translation_override, translation_key: "title") }
+
+      it { expect(translation.original_translation_updated?).to eq(false) }
+    end
+
+    context "when the translation is outdated" do
+      fab!(:translation) do
+        Fabricate(:translation_override, translation_key: "title", original_translation: "outdated")
+      end
+
+      it { expect(translation.original_translation_updated?).to eq(true) }
+    end
+
+    context "when we can't tell because the translation is too old" do
+      fab!(:translation) do
+        Fabricate(:translation_override, translation_key: "title", original_translation: nil)
+      end
+
+      it { expect(translation.original_translation_updated?).to eq(false) }
+    end
+  end
+
+  describe "invalid_interpolation_keys" do
+    fab!(:translation) do
+      Fabricate(
+        :translation_override,
+        translation_key: "system_messages.welcome_user.subject_template",
+      )
+    end
+
+    it "picks out invalid keys and ignores known and custom keys" do
+      translation.update_attribute("value", "Hello, %{name}! Welcome to %{site_name}. %{foo}")
+
+      expect(translation.invalid_interpolation_keys).to contain_exactly("foo")
+    end
+  end
 end