REFACTOR: Rename site settings to make them less confusing

approved

#1

REFACTOR: Rename site settings to make them less confusing

diff --git a/app/assets/javascripts/admin/services/admin-tools.js.es6 b/app/assets/javascripts/admin/services/admin-tools.js.es6
index ba6bd98..4a055b0 100644
--- a/app/assets/javascripts/admin/services/admin-tools.js.es6
+++ b/app/assets/javascripts/admin/services/admin-tools.js.es6
@@ -88,7 +88,7 @@ export default Ember.Service.extend({
 
   _deleteSpammer(adminUser) {
     // Try loading the email if the site supports it
-    let tryEmail = this.siteSettings.show_email_on_profile
+    let tryEmail = this.siteSettings.moderators_view_emails
       ? adminUser.checkEmail()
       : Ember.RSVP.resolve();
 
diff --git a/app/assets/javascripts/discourse/mixins/can-check-emails.js.es6 b/app/assets/javascripts/discourse/mixins/can-check-emails.js.es6
index b9d1142..a7eb59f 100644
--- a/app/assets/javascripts/discourse/mixins/can-check-emails.js.es6
+++ b/app/assets/javascripts/discourse/mixins/can-check-emails.js.es6
@@ -2,7 +2,7 @@ import { propertyEqual, setting } from "discourse/lib/computed";
 
 export default Ember.Mixin.create({
   isCurrentUser: propertyEqual("model.id", "currentUser.id"),
-  showEmailOnProfile: setting("show_email_on_profile"),
+  showEmailOnProfile: setting("moderators_view_emails"),
   canStaffCheckEmails: Ember.computed.and(
     "showEmailOnProfile",
     "currentUser.staff"
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 7c40397..224d94f 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -1386,7 +1386,7 @@ en:
     ga_universal_auto_link_domains: "Enable Google Universal Analytics (analytics.js) cross-domain tracking. Outgoing links to these domains will have the client id added to them. See <a href='https://support.google.com/analytics/answer/1034342?hl=en' target='_blank'>Google's Cross-Domain Tracking guide.</a>"
     gtm_container_id: "Google Tag Manager container id. eg: GTM-ABCDEF. <br/>Note: third-party scripts loaded by GTM may need to be whitelisted in 'content security policy script src'."
     enable_escaped_fragments: "Fall back to Google's Ajax-Crawling API if no webcrawler is detected. See <a href='https://developers.google.com/webmasters/ajax-crawling/docs/learn-more' target='_blank'>https://developers.google.com/webmasters/ajax-crawling/docs/learn-more</a>"
-    allow_moderators_to_create_categories: "Allow moderators to create new categories"
+    moderators_create_categories: "Allow moderators to create new categories"
     cors_origins: "Allowed origins for cross-origin requests (CORS). Each origin must include http:// or https://. The DISCOURSE_ENABLE_CORS env variable must be set to true to enable CORS."
     use_admin_ip_whitelist: "Admins can only log in if they are at an IP address defined in the Screened IPs list (Admin > Logs > Screened Ips)."
     blacklist_ip_blocks: "A list of private IP blocks that should never be crawled by Discourse"
@@ -1418,7 +1418,7 @@ en:
     topics_per_period_in_top_page: "Number of top topics shown on the expanded 'Show More' top topics."
     redirect_users_to_top_page: "Automatically redirect new and long absent users to the top page."
     top_page_default_timeframe: "Default timeframe for the top view page."
-    show_email_on_profile: "Allow moderators to view user emails"
+    moderators_view_emails: "Allow moderators to view user emails"
     prioritize_username_in_ux: "Show username first on user page, user card and posts (when disabled name is shown first)"
     enable_rich_text_paste: "Enable automatic HTML to Markdown conversion when pasting text into the composer. (Experimental)"
 
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 24e69ec..9155d12 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -490,9 +490,6 @@ users:
     default: 15
     min: 1
   redirect_users_to_top_page: true
-  show_email_on_profile:
-    client: true
-    default: false
   prioritize_username_in_ux:
     client: true
     default: true
@@ -1228,7 +1225,10 @@ security:
     regex: "^(Lax|Strict|Disabled)$"
   enable_escaped_fragments: true
   allow_index_in_robots_txt: true
-  allow_moderators_to_create_categories: false
+  moderators_create_categories: false
+  moderators_view_emails:
+    client: true
+    default: false
   non_crawler_user_agents:
     hidden: true
     default: "trident|webkit|gecko|chrome|safari|msie|opera|goanna"
diff --git a/db/migrate/20190313205652_rename_moderator_site_settings.rb b/db/migrate/20190313205652_rename_moderator_site_settings.rb
new file mode 100644
index 0000000..ff0747c
--- /dev/null
+++ b/db/migrate/20190313205652_rename_moderator_site_settings.rb
@@ -0,0 +1,11 @@
+class RenameModeratorSiteSettings < ActiveRecord::Migration[5.2]
+  def up
+    execute "UPDATE site_settings SET name = 'moderators_view_emails' WHERE name = 'show_email_on_profile'"
+    execute "UPDATE site_settings SET name = 'moderators_create_categories' WHERE name = 'allow_moderators_to_create_categories'"
+  end
+
+  def down
+    execute "UPDATE site_settings SET name = 'show_email_on_profile' WHERE name = 'moderators_view_emails'"
+    execute "UPDATE site_settings SET name = 'allow_moderators_to_create_categories' WHERE name = 'moderators_create_categories'"
+  end
+end
diff --git a/lib/guardian/category_guardian.rb b/lib/guardian/category_guardian.rb
index 9257210..116105f 100644
--- a/lib/guardian/category_guardian.rb
+++ b/lib/guardian/category_guardian.rb
@@ -5,7 +5,7 @@ module CategoryGuardian
   def can_create_category?(parent = nil)
     is_admin? ||
     (
-      SiteSetting.allow_moderators_to_create_categories &&
+      SiteSetting.moderators_create_categories &&
       is_moderator?
     )
   end
@@ -14,7 +14,7 @@ module CategoryGuardian
   def can_edit_category?(category)
     is_admin? ||
     (
-      SiteSetting.allow_moderators_to_create_categories &&
+      SiteSetting.moderators_create_categories &&
       is_moderator? &&
       can_see_category?(category)
     )
diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb
index 6751159..4bfbc09 100644
--- a/lib/guardian/user_guardian.rb
+++ b/lib/guardian/user_guardian.rb
@@ -67,7 +67,7 @@ module UserGuardian
   end
 
   def can_check_emails?(user)
-    is_admin? || (is_staff? && SiteSetting.show_email_on_profile)
+    is_admin? || (is_staff? && SiteSetting.moderators_view_emails)
   end
 
   def restrict_user_fields?(user)
diff --git a/lib/site_settings/deprecated_settings.rb b/lib/site_settings/deprecated_settings.rb
index c5f5392..377e540 100644
--- a/lib/site_settings/deprecated_settings.rb
+++ b/lib/site_settings/deprecated_settings.rb
@@ -11,7 +11,9 @@ module SiteSettings::DeprecatedSettings
     ['apple_touch_icon_url', 'apple_touch_icon', false, '2.3'],
     ['default_opengraph_image_url', 'opengraph_image', false, '2.3'],
     ['twitter_summary_large_image_url', 'twitter_summary_large_image', false, '2.3'],
-    ['push_notifications_icon_url', 'push_notifications_icon', false, '2.3']
+    ['push_notifications_icon_url', 'push_notifications_icon', false, '2.3'],
+    ['show_email_on_profile', 'moderators_view_emails', true, '2.4'],
+    ['allow_moderators_to_create_categories', 'moderators_create_categories', true, '2.4']
   ]
 
   def setup_deprecated_methods
diff --git a/spec/serializers/admin_user_list_serializer_spec.rb b/spec/serializers/admin_user_list_serializer_spec.rb
index 7f50c37..9a21b68 100644
--- a/spec/serializers/admin_user_list_serializer_spec.rb
+++ b/spec/serializers/admin_user_list_serializer_spec.rb
@@ -41,16 +41,16 @@ describe AdminUserListSerializer do
       expect(json[:secondary_emails]).to eq(nil)
     end
 
-    it "doesn't return emails for a moderator request when show_email_on_profile is disabled" do
-      SiteSetting.show_email_on_profile = false
+    it "doesn't return emails for a moderator request when moderators_view_emails is disabled" do
+      SiteSetting.moderators_view_emails = false
       fabricate_secondary_emails_for(user)

[... diff too long, it was truncated ...]

GitHub sha: c34a6ba6


#2

This commit has been mentioned on Discourse Meta. There might be relevant details there:


Approved #3

#4

I’m wondering if the migration should be a post migration instead. If the rename is done before the web server is redeployed, it will not be able to pick up the right site settings.


Follow Up #5

#7

That doesn’t make sense to me. Oh, you mean the old code that is still running in the time between the migration and the redeploy of servers?

Wouldn’t a post migration have a similar effect when the new code doesn’t find the renamed setting in the database until the post migration has run? :thinking:

I guess the correct way would be to INSERT in the regular migration and DELETE in the post migration instead of doing a simple UPDATE. Not sure if it is worth the effort though…


#8

Yup.

O you’re right, it will have the same effect.

Hmm actually this is tricky. Even if the insert is done in the regular migration, the site setting can be changed while the app servers are deployed. As a result, the new site setting may not reflect the latest value that the user has configured.

Yea probably not for this setting. I guess when we actually have to rename a site setting with a larger impact we can think about it then. Sorry for the confusion.


Approved #9