FEATURE: allow disabling theme components (#7812)

This allows you to temporarily disable components without having to remove them from a theme. 

This feature is very handy when doing quick fix engineering.

app/assets/javascripts/admin/components/themes-list-item.js.es6

@@ -2,6 +2,8 @@ import {
   default as computed,
   observes
 } from "ember-addons/ember-computed-decorators";
+import { iconHTML } from "discourse-common/lib/icon-library";
+import { escape } from "pretty-text/sanitizer";
 
 const MAX_COMPONENTS = 4;
 
@@ -64,7 +66,10 @@ export default Ember.Component.extend({
     children = this.childrenExpanded
       ? children
       : children.slice(0, MAX_COMPONENTS);
-    return children.map(t => t.get("name"));
+    return children.map(t => {
+      const name = escape(t.name);
+      return t.enabled ? name : `${iconHTML("ban")} ${name}`;
+    });
   },
 
   @computed("children")

app/assets/javascripts/admin/controllers/admin-customize-themes-show.js.es6

@@ -301,6 +301,20 @@ export default Ember.Controller.extend({
       } else {
         this.commitSwitchType();
       }
+    },
+
+    enableComponent() {
+      this.model.set("enabled", true);
+      this.model
+        .saveChanges("enabled")
+        .catch(() => this.model.set("enabled", false));
+    },
+
+    disableComponent() {
+      this.model.set("enabled", false);
+      this.model
+        .saveChanges("enabled")
+        .catch(() => this.model.set("enabled", true));
     }
   }
 });

app/assets/javascripts/admin/templates/components/themes-list-item.hbs

@@ -17,6 +17,9 @@
         {{#if theme.isBroken}}
           {{d-icon "exclamation-circle" class="broken-indicator" title="admin.customize.theme.broken_theme_tooltip"}}
         {{/if}}
+        {{#unless theme.enabled}}
+          {{d-icon "ban" class="light-grey-icon" title="admin.customize.theme.disabled_component_tooltip"}}
+        {{/unless}}
       {{else}}
         {{d-icon "caret-right"}}
       {{/unless}}
@@ -25,7 +28,7 @@
 
   {{#if displayComponents}}
     <div class="components-list">
-      <span class="components">{{childrenString}}</span>
+      <span class="components">{{{childrenString}}}</span>
 
       {{#if displayHasMore}}
         <span {{action "toggleChildrenExpanded"}} class="others-count">

app/assets/javascripts/admin/templates/customize-themes-show.hbs

@@ -16,7 +16,7 @@
     </div>
   {{/each}}
 
-  {{#unless model.enabled}}
+  {{#unless model.supported}}
     <div class="alert alert-error">
       {{i18n "admin.customize.theme.required_version.error"}}
       {{#if model.remote_theme.minimum_discourse_version}}
@@ -28,6 +28,26 @@
     </div>
   {{/unless}}
 
+  {{#unless model.enabled}}
+    <div class="alert alert-error">
+      {{#if model.disabled_by}}
+        {{i18n "admin.customize.theme.disabled_by"}}
+        {{#user-link user=model.disabled_by}}
+          {{avatar model.disabled_by imageSize="tiny"}}
+          {{model.disabled_by.username}}
+        {{/user-link}}
+        {{format-date model.disabled_at leaveAgo="true"}}
+      {{else}}
+        {{i18n "admin.customize.theme.disabled"}}
+      {{/if}}
+      {{d-button
+        class='btn-default'
+        action=(action "enableComponent")
+        icon="check"
+        label="admin.customize.theme.enable"}}
+    </div>
+  {{/unless}}
+
   {{#unless model.component}}
     <div class="control-unit">
       {{inline-edit-checkbox action=(action "applyDefault") labelKey="admin.customize.theme.is_default" checked=model.default}}
@@ -204,7 +224,13 @@
       {{#if model.childThemes.length}}
         <ul class='removable-list'>
           {{#each model.childThemes as |child|}}
-            <li>{{#link-to 'adminCustomizeThemes.show' child replace=true class='col'}}{{child.name}}{{/link-to}} {{d-button action=(action "removeChildTheme") actionParam=child class="btn-default cancel-edit col" icon="times"}}</li>
+            <li class={{unless child.enabled "disabled-child"}}>
+              {{#link-to 'adminCustomizeThemes.show' child replace=true class='col child-link'}}
+                {{child.name}}
+              {{/link-to}}
+
+              {{d-button action=(action "removeChildTheme") actionParam=child class="btn-default cancel-edit col" icon="times"}}
+            </li>
           {{/each}}
         </ul>
       {{/if}}
@@ -221,5 +247,22 @@
   <a class="btn btn-default export" target="_blank" href={{downloadUrl}}>{{d-icon "download"}} {{i18n 'admin.export_json.button_text'}}</a>
 
   {{d-button action=(action "switchType") label="admin.customize.theme.convert" icon=convertIcon class="btn-default btn-normal" title=convertTooltip}}
+
+  {{#if model.component}}
+    {{#if model.enabled}}
+      {{d-button
+        class='btn-default'
+        action=(action "disableComponent")
+        icon="ban"
+        label="admin.customize.theme.disable"}}
+    {{else}}
+      {{d-button
+        class='btn-default'
+        action=(action "enableComponent")
+        icon="check"
+        label="admin.customize.theme.enable"}}
+    {{/if}}
+  {{/if}}
+
   {{d-button action=(action "destroy") label="admin.customize.delete" icon="trash-alt" class="btn-danger"}}
 </div>

app/assets/stylesheets/common/admin/customize.scss

@@ -513,8 +513,19 @@
     list-style: none;
     margin-left: 0;
     li {
+      &.disabled-child {
+        .child-link {
+          color: $primary-medium;
+          &:hover {
+            text-decoration: underline;
+          }
+        }
+      }
+      .btn {
+        margin-left: 5px;
+      }
       display: table-row;
-      .col:first-child {
+      .col.child-link {
         padding-right: 10px;
         padding-bottom: 10px;
         min-width: 80px;

app/controllers/admin/themes_controller.rb

@@ -156,8 +156,10 @@ class Admin::ThemesController < Admin::AdminController
     raise Discourse::InvalidParameters.new(:id) unless @theme
 
     original_json = ThemeSerializer.new(@theme, root: false).to_json
+    disables_component = [false, "false"].include?(theme_params[:enabled])
+    enables_component = [true, "true"].include?(theme_params[:enabled])
 
-    [:name, :color_scheme_id, :user_selectable].each do |field|
+    [:name, :color_scheme_id, :user_selectable, :enabled].each do |field|
       if theme_params.key?(field)
         @theme.public_send("#{field}=", theme_params[field])
       end
@@ -203,7 +205,13 @@ class Admin::ThemesController < Admin::AdminController
         update_default_theme
 
         @theme.reload
-        log_theme_change(original_json, @theme)
+
+        if (!disables_component && !enables_component) || theme_params.keys.size > 1
+          log_theme_change(original_json, @theme)
+        end
+        log_theme_component_disabled if disables_component
+        log_theme_component_enabled if enables_component
+
         format.json { render json: @theme, status: :ok }
       else
         format.json do
@@ -304,6 +312,7 @@ class Admin::ThemesController < Admin::AdminController
           :default,
           :user_selectable,
           :component,
+          :enabled,
           settings: {},
           translations: {},
           theme_fields: [:name, :target, :value, :upload_id, :type_id],
@@ -350,6 +359,14 @@ class Admin::ThemesController < Admin::AdminController
     StaffActionLogger.new(current_user).log_theme_setting_change(setting_name, previous_value, new_value, @theme)
   end
 
+  def log_theme_component_disabled
+    StaffActionLogger.new(current_user).log_theme_component_disabled(@theme)
+  end
+
+  def log_theme_component_enabled
+    StaffActionLogger.new(current_user).log_theme_component_enabled(@theme)
+  end
+
   def handle_switch
     param = theme_params[:component]
     if param.to_s == "false" && @theme.component?

app/models/theme.rb

@@ -156,8 +156,10 @@ class Theme < ActiveRecord::Base
 
       all_ids = [parent, *components]
 
-      disabled_ids = Theme.where(id: all_ids).includes(:remote_theme)
-        .reject(&:enabled?).pluck(:id)
+      disabled_ids = Theme.where(id: all_ids)
+        .includes(:remote_theme)
+        .select { |t| !t.supported? || !t.enabled? }
+        .pluck(:id)
 
       all_ids - disabled_ids
     end
@@ -177,7 +179,7 @@ class Theme < ActiveRecord::Base
     SiteSetting.default_theme_id == id
   end
 
-  def enabled?
+  def supported?
     if minimum_version = remote_theme&.minimum_discourse_version
       return false unless Discourse.has_needed_version?(Discourse::VERSION::STRING, minimum_version)
     end
@@ -216,6 +218,7 @@ class Theme < ActiveRecord::Base
     return unless component
 
     Theme.transaction do
+      self.enabled = true
       self.component = false
       ChildTheme.where("child_theme_id = ?", id).destroy_all
       self.save!
@@ -489,6 +492,22 @@ class Theme < ActiveRecord::Base
 
     end
   end
+
+  def disabled_by
+    find_disable_action_log&.acting_user
+  end
+
+  def disabled_at
+    find_disable_action_log&.created_at
+  end
+
+  private
+
+  def find_disable_action_log
+    if component? && !enabled?
+      @disable_log ||= UserHistory.where(context: id.to_s, action: UserHistory.actions[:disable_theme_component]).order("created_at DESC").first
+    end
+  end
 end
 
 # == Schema Information
@@ -506,6 +525,7 @@ end
 #  color_scheme_id  :integer
 #  remote_theme_id  :integer
 #  component        :boolean          default(FALSE), not null
+#  enabled          :boolean          default(TRUE), not null
 #
 # Indexes
 #

app/models/user_history.rb

@@ -95,7 +95,9 @@ class UserHistory < ActiveRecord::Base
       embeddable_host_update: 74,
       embeddable_host_destroy: 75,
       web_hook_deactivate: 76,
-      change_theme_setting: 77
+      change_theme_setting: 77,
+      disable_theme_component: 78,
+      enable_theme_component: 79
     )
   end
 

app/serializers/theme_serializer.rb

@@ -63,9 +63,11 @@ class RemoteThemeSerializer < ApplicationSerializer
 end
 
 class ThemeSerializer < BasicThemeSerializer
-  attributes :color_scheme, :color_scheme_id, :user_selectable, :remote_theme_id, :settings, :errors, :enabled?, :description
+  attributes :color_scheme, :color_scheme_id, :user_selectable, :remote_theme_id,
+             :settings, :errors, :supported?, :description, :enabled?, :disabled_at
 
   has_one :user, serializer: UserNameSerializer, embed: :object
+  has_one :disabled_by, serializer: UserNameSerializer, embed: :object
 
   has_many :theme_fields, serializer: ThemeFieldSerializer, embed: :objects
   has_many :child_themes, serializer: BasicThemeSerializer, embed: :objects
@@ -108,4 +110,12 @@ class ThemeSerializer < BasicThemeSerializer
   def description
     object.internal_translations.find  { |t| t.key == "theme_metadata.description" } &.value
   end
+
+  def include_disabled_at?
+    object.component? && !object.enabled?
+  end
+
+  def include_disabled_by?
+    include_disabled_at?
+  end
 end

app/services/staff_action_logger.rb

@@ -206,6 +206,22 @@ class StaffActionLogger
     ))
   end
 
+  def log_theme_component_disabled(component)
+    UserHistory.create!(params.merge(
+      action: UserHistory.actions[:disable_theme_component],
+      subject: component.name,
+      context: component.id
+    ))
+  end
+
+  def log_theme_component_enabled(component)
+    UserHistory.create!(params.merge(
+      action: UserHistory.actions[:enable_theme_component],
+      subject: component.name,
+      context: component.id
+    ))
+  end
+
   def log_theme_setting_change(setting_name, previous_value, new_value, theme, opts = {})
     raise Discourse::InvalidParameters.new(:theme) unless theme
     raise Discourse::InvalidParameters.new(:setting_name) unless theme.included_settings.has_key?(setting_name)

config/locales/client.en.yml

@@ -3481,6 +3481,7 @@ en:
           inactive_themes: "Inactive themes:"
           inactive_components: "Unused components:"
           broken_theme_tooltip: "This theme has errors in its CSS, HTML or YAML"
+          disabled_component_tooltip: "This component has been disabled"
           default_theme_tooltip: "This theme is the site's default theme"
           updates_available_tooltip: "Updates are available for this theme"
           and_x_more: "and {{count}} more."
@@ -3521,6 +3522,10 @@ en:
           version: "Version:"
           authors: "Authored by:"
           source_url: "Source"
+          enable: "Enable"
+          disable: "Disable"
+          disabled: "This component has been disabled."
+          disabled_by: "This component has been disabled by"
           required_version:
             error: "This theme has been automatically disabled because it is not compatible with this version of Discourse."
             minimum: "Requires Discourse version {{version}} or above."

db/migrate/20190630165003_add_enabled_to_themes.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddEnabledToThemes < ActiveRecord::Migration[5.2]
+  def change
+    add_column :themes, :enabled, :boolean, null: false, default: true
+  end
+end

spec/models/theme_spec.rb

@@ -60,9 +60,9 @@ describe Theme do
   end
 
   it "can automatically disable for mismatching version" do
-    expect(theme.enabled?).to eq(true)
+    expect(theme.supported?).to eq(true)
     theme.create_remote_theme!(remote_url: "", minimum_discourse_version: "99.99.99")
-    expect(theme.enabled?).to eq(false)
+    expect(theme.supported?).to eq(false)
 
     expect(Theme.transform_ids([theme.id])).to be_empty
   end
@@ -72,6 +72,13 @@ describe Theme do
     expect(Theme.transform_ids([nil])).to eq([nil])
   end
 
+  it '#transform_ids filters out disabled components' do
+    theme.add_child_theme!(child)
+    expect(Theme.transform_ids([theme.id], extend: true)).to eq([theme.id, child.id])
+    child.update!(enabled: false)
+    expect(Theme.transform_ids([theme.id], extend: true)).to eq([theme.id])
+  end
+
   it "doesn't allow multi-level theme components" do
     grandchild = Fabricate(:theme, user: user)
     grandparent = Fabricate(:theme, user: user)

spec/requests/admin/themes_controller_spec.rb

@@ -322,6 +322,65 @@ describe Admin::ThemesController do
       expect(theme.theme_translation_overrides.count).to eq(0)
     end
 
+    it 'can disable component' do
+      child = Fabricate(:theme, component: true)
+
+      put "/admin/themes/#{child.id}.json", params: {
+        theme: {
+          enabled: false
+        }
+      }
+      expect(response.status).to eq(200)
+      json = JSON.parse(response.body)
+      expect(json["theme"]["enabled"]).to eq(false)
+      expect(UserHistory.where(
+        context: child.id.to_s,
+        action: UserHistory.actions[:disable_theme_component]
+      ).size).to eq(1)
+      expect(json["theme"]["disabled_by"]["id"]).to eq(admin.id)
+    end
+
+    it "enabling/disabling a component creates the correct staff action log" do
+      child = Fabricate(:theme, component: true)
+      UserHistory.destroy_all
+
+      put "/admin/themes/#{child.id}.json", params: {
+        theme: {
+          enabled: false
+        }
+      }
+      expect(response.status).to eq(200)
+
+      expect(UserHistory.where(
+        context: child.id.to_s,
+        action: UserHistory.actions[:disable_theme_component]
+      ).size).to eq(1)
+      expect(UserHistory.where(
+        context: child.id.to_s,
+        action: UserHistory.actions[:enable_theme_component]
+      ).size).to eq(0)
+
+      put "/admin/themes/#{child.id}.json", params: {
+        theme: {
+          enabled: true
+        }
+      }
+      expect(response.status).to eq(200)
+      json = JSON.parse(response.body)
+
+      expect(UserHistory.where(
+        context: child.id.to_s,
+        action: UserHistory.actions[:disable_theme_component]
+      ).size).to eq(1)
+      expect(UserHistory.where(
+        context: child.id.to_s,
+        action: UserHistory.actions[:enable_theme_component]
+      ).size).to eq(1)
+
+      expect(json["theme"]["disabled_by"]).to eq(nil)
+      expect(json["theme"]["enabled"]).to eq(true)
+    end
+
     it 'handles import errors on update' do
       theme.create_remote_theme!(remote_url: "https://example.com/repository")
 

test/javascripts/admin/components/themes-list-item-test.js.es6

@@ -71,11 +71,14 @@ componentTest("with children", {
     assert.deepEqual(
       find(".components")
         .text()
-        .trim(),
+        .trim()
+        .split(",")
+        .map(n => n.trim())
+        .join(","),
       childrenList
         .splice(0, 4)
         .map(theme => theme.get("name"))
-        .join(", "),
+        .join(","),
       "lists the first 4 children"
     );
     assert.deepEqual(