From 41ee5b7c86af8911ddba9f808079fcf2ba341e4f Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Tue, 1 Jun 2021 22:11:48 +0200 Subject: [PATCH] FIX: Don't store translated trust level names in anonymous cache (#13224) Refactors `TrustLevel` and moves translations from server to client Additional changes: * "staff" and "admin" wasn't translatable in site settings * it replaces a concatenated string with a translation * uses translation for trust levels in users_by_trust_level report * adds a DB migration to rename keys of translation overrides affected by this commit --- .../javascripts/discourse/app/models/site.js | 6 ++-- .../discourse/app/models/trust-level.js | 26 +++++++++++++---- .../discourse/tests/fixtures/site-fixtures.js | 29 +++++-------------- .../discourse/tests/helpers/site.js | 14 ++++----- .../concerns/reports/users_by_trust_level.rb | 2 +- app/models/site.rb | 2 +- app/models/trust_level_and_staff_setting.rb | 8 +++++ app/models/trust_level_setting.rb | 19 +++++++----- app/serializers/site_serializer.rb | 2 +- app/serializers/trust_level_serializer.rb | 7 ----- config/locales/client.en.yml | 11 ++++++- config/locales/server.en.yml | 14 ++------- ...1002145_rename_trust_level_translations.rb | 25 ++++++++++++++++ lib/trust_level.rb | 20 ++----------- spec/integrity/i18n_spec.rb | 6 ---- .../trust_level_and_staff_setting_spec.rb | 26 +++++++++++++++++ spec/models/trust_level_setting_spec.rb | 15 ++++++++++ 17 files changed, 144 insertions(+), 88 deletions(-) delete mode 100644 app/serializers/trust_level_serializer.rb create mode 100644 db/migrate/20210601002145_rename_trust_level_translations.rb create mode 100644 spec/models/trust_level_and_staff_setting_spec.rb create mode 100644 spec/models/trust_level_setting_spec.rb diff --git a/app/assets/javascripts/discourse/app/models/site.js b/app/assets/javascripts/discourse/app/models/site.js index fa59e1cc21..9fc38c92de 100644 --- a/app/assets/javascripts/discourse/app/models/site.js +++ b/app/assets/javascripts/discourse/app/models/site.js @@ -179,8 +179,10 @@ Site.reopenClass(Singleton, { } if (result.trust_levels) { - result.trustLevels = result.trust_levels.map((tl) => - TrustLevel.create(tl) + result.trustLevels = Object.entries(result.trust_levels).map( + ([key, id]) => { + return new TrustLevel(id, key); + } ); delete result.trust_levels; } diff --git a/app/assets/javascripts/discourse/app/models/trust-level.js b/app/assets/javascripts/discourse/app/models/trust-level.js index b793195506..2ee5136fda 100644 --- a/app/assets/javascripts/discourse/app/models/trust-level.js +++ b/app/assets/javascripts/discourse/app/models/trust-level.js @@ -1,6 +1,22 @@ -import RestModel from "discourse/models/rest"; -import { fmt } from "discourse/lib/computed"; +import { computed } from "@ember/object"; +import I18n from "I18n"; -export default RestModel.extend({ - detailedName: fmt("id", "name", "%@ - %@"), -}); +export default class TrustLevel { + constructor(id, key) { + this.id = id; + this._key = key; + } + + @computed + get name() { + return I18n.t(`trust_levels.names.${this._key}`); + } + + @computed + get detailedName() { + return I18n.t("trust_levels.detailed_name", { + level: this.id, + name: this.name, + }); + } +} diff --git a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js index c143a5241d..bdd982a7ec 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js @@ -12,6 +12,13 @@ export default { small_action: 3, whisper: 4, }, + trust_levels: { + newuser: 0, + basic: 1, + member: 2, + regular: 3, + leader: 4, + }, groups: [ { id: 0, name: "everyone" }, { id: 1, name: "admins" }, @@ -535,28 +542,6 @@ export default { is_custom_flag: true, }, ], - trust_levels: [ - { - id: 0, - name: "new user", - }, - { - id: 1, - name: "basic user", - }, - { - id: 2, - name: "member", - }, - { - id: 3, - name: "regular", - }, - { - id: 4, - name: "leader", - }, - ], archetypes: [ { id: "regular", diff --git a/app/assets/javascripts/discourse/tests/helpers/site.js b/app/assets/javascripts/discourse/tests/helpers/site.js index c6a68e93e0..7d38afa2de 100644 --- a/app/assets/javascripts/discourse/tests/helpers/site.js +++ b/app/assets/javascripts/discourse/tests/helpers/site.js @@ -16,6 +16,13 @@ PreloadStore.store("site", { moved_post: 10, }, post_types: { regular: 1, moderator_action: 2 }, + trust_levels: { + newuser: 0, + basic: 1, + member: 2, + regular: 3, + leader: 4, + }, groups: [ { id: 0, name: "everyone" }, { id: 1, name: "admins" }, @@ -349,12 +356,5 @@ PreloadStore.store("site", { is_custom_flag: true, }, ], - trust_levels: [ - { id: 0, name: "new user" }, - { id: 1, name: "basic user" }, - { id: 2, name: "member" }, - { id: 3, name: "regular" }, - { id: 4, name: "leader" }, - ], archetypes: [{ id: "regular", name: "Regular Topic", options: [] }], }); diff --git a/app/models/concerns/reports/users_by_trust_level.rb b/app/models/concerns/reports/users_by_trust_level.rb index a2b99e1639..95cda8068d 100644 --- a/app/models/concerns/reports/users_by_trust_level.rb +++ b/app/models/concerns/reports/users_by_trust_level.rb @@ -24,7 +24,7 @@ module Reports::UsersByTrustLevel ] User.real.group('trust_level').count.sort.each do |level, count| - key = TrustLevel.levels[level.to_i] + key = TrustLevel.name(level.to_i) url = Proc.new { |k| "/admin/users/list/#{k}" } report.data << { url: url.call(key), key: key, x: level.to_i, y: count } end diff --git a/app/models/site.rb b/app/models/site.rb index c7c2d7a1c1..f8c131ef1d 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -21,7 +21,7 @@ class Site end def trust_levels - TrustLevel.all + TrustLevel.levels end def user_fields diff --git a/app/models/trust_level_and_staff_setting.rb b/app/models/trust_level_and_staff_setting.rb index ecab020b93..a72f634667 100644 --- a/app/models/trust_level_and_staff_setting.rb +++ b/app/models/trust_level_and_staff_setting.rb @@ -18,4 +18,12 @@ class TrustLevelAndStaffSetting < TrustLevelSetting def self.special_groups ['staff', 'admin'] end + + def self.translation(value) + if special_group?(value) + I18n.t("trust_levels.#{value}") + else + super + end + end end diff --git a/app/models/trust_level_setting.rb b/app/models/trust_level_setting.rb index 68b7e2b1d9..fc48bf8a10 100644 --- a/app/models/trust_level_setting.rb +++ b/app/models/trust_level_setting.rb @@ -8,18 +8,23 @@ class TrustLevelSetting < EnumSiteSetting end def self.values - levels = TrustLevel.all - valid_values.map { |x| - { - name: x.is_a?(Integer) ? "#{x}: #{levels[x.to_i].name}" : x, - value: x - } - } + valid_values.map do |value| + { name: translation(value), value: value } + end end def self.valid_values TrustLevel.valid_range.to_a end + def self.translation(value) + I18n.t( + "js.trust_levels.detailed_name", + level: value, + name: TrustLevel.name(value) + ) + end + private_class_method :valid_values + private_class_method :translation end diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index 6fe1d7ca73..e1d0982ae1 100644 --- a/app/serializers/site_serializer.rb +++ b/app/serializers/site_serializer.rb @@ -6,6 +6,7 @@ class SiteSerializer < ApplicationSerializer :default_archetype, :notification_types, :post_types, + :trust_levels, :groups, :filters, :periods, @@ -32,7 +33,6 @@ class SiteSerializer < ApplicationSerializer ) has_many :categories, serializer: SiteCategorySerializer, embed: :objects - has_many :trust_levels, embed: :objects has_many :archetypes, embed: :objects, serializer: ArchetypeSerializer has_many :user_fields, embed: :objects, serializer: UserFieldSerializer has_many :auth_providers, embed: :objects, serializer: AuthProviderSerializer diff --git a/app/serializers/trust_level_serializer.rb b/app/serializers/trust_level_serializer.rb deleted file mode 100644 index c41495ff3e..0000000000 --- a/app/serializers/trust_level_serializer.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -class TrustLevelSerializer < ApplicationSerializer - - attributes :id, :name - -end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d9930d2b58..502f893e2d 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -702,7 +702,7 @@ en: smtp_instructions: "When you enable SMTP for the group, all outbound emails sent from the group's inbox will be sent via the SMTP settings specified here instead of the mail server configured for other emails sent by your forum." imap_title: "IMAP" imap_additional_settings: "Additional Settings" - imap_instructions: "When you enable IMAP for the group, emails are synced between the group inbox and the provided IMAP server and mailbox. SMTP must be enabled with valid and tested credentials before IMAP can be enabled. The email username and password used for SMTP will be used for IMAP. For more information see feature announcement on Discourse Meta." + imap_instructions: 'When you enable IMAP for the group, emails are synced between the group inbox and the provided IMAP server and mailbox. SMTP must be enabled with valid and tested credentials before IMAP can be enabled. The email username and password used for SMTP will be used for IMAP. For more information see feature announcement on Discourse Meta.' imap_alpha_warning: "Warning: This is an alpha-stage feature. Only Gmail is officially supported. Use at your own risk!" imap_settings_valid: "IMAP settings valid." smtp_disable_confirm: "If you disable SMTP, all SMTP and IMAP settings will be reset and the associated functionality will be disabled. Are you sure you want to continue?" @@ -3764,6 +3764,15 @@ en: custom: "Custom" set_schedule: "Set a notification schedule" + trust_levels: + names: + newuser: "new user" + basic: "basic user" + member: "member" + regular: "regular" + leader: "leader" + detailed_name: "%{level}: %{name}" + # This section is exported to the javascript for i18n in the admin section admin_js: type_to_filter: "type to filter..." diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index dac040c9f0..9d50e66152 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -717,16 +717,8 @@ en: topic_exists_no_oldest: "Can't delete this category because topic count is %{count}." uncategorized_description: "Topics that don't need a category, or don't fit into any other existing category." trust_levels: - newuser: - title: "new user" - basic: - title: "basic user" - member: - title: "member" - regular: - title: "regular" - leader: - title: "leader" + admin: "Admin" + staff: "Staff" change_failed_explanation: "You attempted to demote %{user_name} to '%{new_trust_level}'. However their trust level is already '%{current_trust_level}'. %{user_name} will remain at '%{current_trust_level}' - if you wish to demote user lock trust level first" post: @@ -994,7 +986,7 @@ en: imap_authentication_error: "There was an issue with the IMAP credentials provided, check the username and password and try again." imap_no_response_error: "An error occurred when communicating with the IMAP server. %{message}" smtp_authentication_error: "There was an issue with the SMTP credentials provided, check the username and password and try again." - authentication_error_gmail_app_password: "Application-specific password required. Learn more at this Google Help article" + authentication_error_gmail_app_password: 'Application-specific password required. Learn more at this Google Help article' smtp_server_busy_error: "The SMTP server is currently busy, try again later." smtp_unhandled_error: "There was an unhandled error when communicating with the SMTP server. %{message}" imap_unhandled_error: "There was an unhandled error when communicating with the IMAP server. %{message}" diff --git a/db/migrate/20210601002145_rename_trust_level_translations.rb b/db/migrate/20210601002145_rename_trust_level_translations.rb new file mode 100644 index 0000000000..b5cb17fd74 --- /dev/null +++ b/db/migrate/20210601002145_rename_trust_level_translations.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class RenameTrustLevelTranslations < ActiveRecord::Migration[6.1] + KEYS = %w[newuser basic member regular leader] + + def up + KEYS.each do |key| + execute <<~SQL + UPDATE translation_overrides + SET translation_key = 'js.trust_levels.names.#{key}' + WHERE translation_key = 'trust_levels.#{key}.title' + SQL + end + end + + def down + KEYS.each do |key| + execute <<~SQL + UPDATE translation_overrides + SET translation_key = 'trust_levels.#{key}.title' + WHERE translation_key = 'js.trust_levels.names.#{key}' + SQL + end + end +end diff --git a/lib/trust_level.rb b/lib/trust_level.rb index 20787bb418..8c87c9ac1e 100644 --- a/lib/trust_level.rb +++ b/lib/trust_level.rb @@ -4,8 +4,6 @@ class InvalidTrustLevel < StandardError; end class TrustLevel - attr_reader :id, :name - class << self def [](level) @@ -17,12 +15,6 @@ class TrustLevel @levels ||= Enum.new(:newuser, :basic, :member, :regular, :leader, start: 0) end - def all - levels.map do |name_key, id| - TrustLevel.new(name_key, id) - end - end - def valid?(level) valid_range === level end @@ -35,15 +27,9 @@ class TrustLevel (current_level || 0) >= level end - end - - def initialize(name_key, id) - @name = I18n.t("trust_levels.#{name_key}.title") - @id = id - end - - def serializable_hash - { id: @id, name: @name } + def name(level) + I18n.t("js.trust_levels.names.#{levels[level]}") + end end end diff --git a/spec/integrity/i18n_spec.rb b/spec/integrity/i18n_spec.rb index 2c7494274e..907524a39c 100644 --- a/spec/integrity/i18n_spec.rb +++ b/spec/integrity/i18n_spec.rb @@ -24,12 +24,6 @@ def is_yaml_compatible?(english, translated) end describe "i18n integrity checks" do - it 'has an i18n key for each Trust Levels' do - TrustLevel.all.each do |ts| - expect(ts.name).not_to match(/translation missing/) - end - end - it "has an i18n key for each Site Setting" do SiteSetting.all_settings.each do |s| next if s[:setting][/^test_/] diff --git a/spec/models/trust_level_and_staff_setting_spec.rb b/spec/models/trust_level_and_staff_setting_spec.rb new file mode 100644 index 0000000000..f538346d67 --- /dev/null +++ b/spec/models/trust_level_and_staff_setting_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe TrustLevelAndStaffSetting do + describe ".values" do + it "returns translated names" do + TranslationOverride.upsert!(I18n.locale, "js.trust_levels.names.newuser", "New Member") + TranslationOverride.upsert!(I18n.locale, "trust_levels.admin", "Hero") + + values = TrustLevelAndStaffSetting.values + + value = values.find { |v| v[:value] == 0 } + expect(value).to be_present + expect(value[:name]).to eq(I18n.t("js.trust_levels.detailed_name", level: 0, name: "New Member")) + + value = values.find { |v| v[:value] == "admin" } + expect(value).to be_present + expect(value[:name]).to eq("Hero") + + value = values.find { |v| v[:value] == "staff" } + expect(value).to be_present + expect(value[:name]).to eq(I18n.t("trust_levels.staff")) + end + end +end diff --git a/spec/models/trust_level_setting_spec.rb b/spec/models/trust_level_setting_spec.rb new file mode 100644 index 0000000000..01ea1adedd --- /dev/null +++ b/spec/models/trust_level_setting_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe TrustLevelSetting do + describe ".values" do + it "returns translated names" do + TranslationOverride.upsert!(I18n.locale, "js.trust_levels.names.newuser", "New Member") + + value = TrustLevelSetting.values.first + expect(value[:name]).to eq(I18n.t("js.trust_levels.detailed_name", level: 0, name: "New Member")) + expect(value[:value]).to eq(0) + end + end +end