Allow people to revert back to the "Default Theme"

fixes: CNVS-29805 test plan: * apply a theme to an account * pick the "default theme" card * make no changes * the save button should disabled with a "you have not set any custom values..." message * you should be able to click the apply button * you should now be looking at the "default theme" * window.ENV.active_brand_config should be null Change-Id: I362d8fde96faf0ebceed12e4c0560532a6e4e5b7 Reviewed-on: https://gerrit.instructure.com/81765 Product-Review: Colleen Palmer <[email protected]> Tested-by: Jenkins QA-Review: Benjamin Christian Nelson <[email protected]> Reviewed-by: Simon Williams <[email protected]>
jiniaoxu · Jun 23, 2016 · fe91cf9 · fe91cf9
1 parent 4151b40
commit fe91cf9
Show file tree

Hide file tree

Showing 5 changed files with 107 additions and 17 deletions.
diff --git a/app/jsx/theme_editor/SaveThemeButton.jsx b/app/jsx/theme_editor/SaveThemeButton.jsx
@@ -53,12 +53,17 @@ define([
     },
 
     render() {
-      let disableMessage = null
+      let disable = false
+      let disableMessage
       if (this.props.userNeedsToPreviewFirst) {
+        disable = true
         disableMessage = I18n.t('You need to "Preview Changes" before saving')
       } else if (this.props.sharedBrandConfigBeingEdited &&
                  this.props.sharedBrandConfigBeingEdited.brand_config_md5 === this.props.brandConfigMd5) {
+        disable = true
         disableMessage = I18n.t('There are no unsaved changes')
+      } else if (!this.props.brandConfigMd5) {
+        disable = true
       }
 
       return (
@@ -70,7 +75,7 @@ define([
           <button
             type="button"
             className="Button Button--primary"
-            disabled={!!disableMessage}
+            disabled={disable}
             onClick={this.save}
           >
             {I18n.t('Save theme')}

diff --git a/app/jsx/theme_editor/ThemeEditor.jsx b/app/jsx/theme_editor/ThemeEditor.jsx
@@ -284,7 +284,7 @@ define([
       let tooltipForWhyApplyIsDisabled = null
       if (this.somethingHasChanged()) {
         tooltipForWhyApplyIsDisabled = I18n.t('You need to "Preview Changes" before you can apply this to your account')
-      } else if (!this.displayedMatchesSaved()) {
+      } else if (this.props.brandConfig.md5 && !this.displayedMatchesSaved()) {
         tooltipForWhyApplyIsDisabled = I18n.t('You need to "Save" before applying to this account')
       }
 

diff --git a/lib/brand_config_regenerator.rb b/lib/brand_config_regenerator.rb
@@ -7,11 +7,11 @@ class BrandConfigRegenerator
   def initialize(account, current_user, new_brand_config)
     @account = account
     @current_user = current_user
-    old_config_md5 = @account.brand_config.try(:md5)
+    old_config_md5 = @account.brand_config_md5
     @account.brand_config = new_brand_config
     @account.save!
     @new_configs = {}
-    @new_configs[old_config_md5] = new_brand_config if old_config_md5 && new_brand_config
+    @new_configs[old_config_md5] = new_brand_config if old_config_md5
     @progresses = []
     process
   end
@@ -21,19 +21,26 @@ def things_that_need_to_be_regenerated
       all_subaccounts = @account.sub_accounts_recursive(100000, nil)
       branded_subaccounts = all_subaccounts.select(&:brand_config)
       branded_subaccounts + SharedBrandConfig.where(account_id: all_subaccounts)
-    end
+    end.freeze
+  end
+
+  # Returns true if this brand config is not based on anything that needs to be regenerated.
+  # This should not be common but can happen in dev/test setups that got into an inconsistent state
+  def orphan?(brand_config)
+    things_that_need_to_be_regenerated.none? { |thing| thing.brand_config_md5 == brand_config.parent_md5 }
   end
 
   # If we haven't saved a new copy for a config's parent,
   # we don't know its new parent_md5 yet.
   def ready_to_process?(account_or_shared_brand_config)
     config = account_or_shared_brand_config.brand_config
-    !config.parent || @new_configs[config.parent_md5]
+    !config.parent || @new_configs.key?(config.parent_md5) || orphan?(config)
   end
 
   def regenerate(thing)
     config = thing.brand_config
-    new_parent_md5 = config.parent_md5 ? @new_configs[config.parent_md5].md5 : @account.brand_config_md5
+    return unless config
+    new_parent_md5 = config.parent_md5 && @new_configs[config.parent_md5].try(:md5) || @account.brand_config_md5
     new_config = config.clone_with_new_parent(new_parent_md5)
     new_config.save_unless_dup!
 
@@ -53,10 +60,11 @@ def regenerate(thing)
   end
 
   def process
-    while thing = things_that_need_to_be_regenerated.sample
+    things_left_to_process = things_that_need_to_be_regenerated.dup
+    while thing = things_left_to_process.sample
       next unless ready_to_process?(thing)
       regenerate(thing)
-      things_that_need_to_be_regenerated.delete(thing)
+      things_left_to_process.delete(thing)
     end
   end
 

diff --git a/spec/javascripts/jsx/theme_editor/SaveThemeButtonSpec.jsx b/spec/javascripts/jsx/theme_editor/SaveThemeButtonSpec.jsx
@@ -8,6 +8,8 @@ define([
 
   let elem, props
 
+  const isType = (type) => (child) => child.type === type
+
   module('SaveThemeButton Component', {
     setup () {
       elem = document.createElement('div')
@@ -80,10 +82,44 @@ define([
     })
     shallowRenderer = React.addons.TestUtils.createRenderer()
     shallowRenderer.render(<SaveThemeButton {...props} />)
-    modal = shallowRenderer.getRenderOutput().props.children.filter(child => {
-      return child.type == Modal
-    })[0]
+    modal = shallowRenderer.getRenderOutput().props.children.find(isType(Modal))
     ok(modal.props.isOpen, 'modal is open')
   })
+
+  test('disabling button', () => {
+    const shallowRenderer = React.addons.TestUtils.createRenderer()
+    shallowRenderer.render(<SaveThemeButton {...props} />)
+    const button = shallowRenderer.getRenderOutput().props.children.find(isType('button'))
+    equal(button.props.disabled, false, 'not disabled by default')
+  })
+
+  test('disabling button: disabled if userNeedsToPreviewFirst', () => {
+    const shallowRenderer = React.addons.TestUtils.createRenderer()
+    shallowRenderer.render(<SaveThemeButton {...props} userNeedsToPreviewFirst={true} />)
+    const button = shallowRenderer.getRenderOutput().props.children.find(isType('button'))
+    ok(button.props.disabled)
+  })
+
+  test('disabling button: disabled if there are no unsaved changes', () => {
+    const shallowRenderer = React.addons.TestUtils.createRenderer()
+    shallowRenderer.render(<SaveThemeButton
+      {...props}
+      brandConfigMd5={props.sharedBrandConfigBeingEdited.brand_config_md5}
+    />)
+    const button = shallowRenderer.getRenderOutput().props.children.find(isType('button'))
+    ok(button.props.disabled)
+  })
+
+  test('disabling button: disabled if everything is default', () => {
+    const shallowRenderer = React.addons.TestUtils.createRenderer()
+    shallowRenderer.render(<SaveThemeButton
+      {...props}
+      brandConfigMd5={null}
+    />)
+    const button = shallowRenderer.getRenderOutput().props.children.find(isType('button'))
+    ok(button.props.disabled)
+  })
+
+
 })
 
diff --git a/spec/lib/brand_config_regenerator_spec.rb b/spec/lib/brand_config_regenerator_spec.rb
@@ -2,6 +2,7 @@
 require 'delayed/testing'
 
 describe BrandConfigRegenerator do
+  let(:new_brand_config) { BrandConfig.for(variables: {"ic-brand-primary" => "green"}) }
   def setup_account_family_with_configs
     @parent_account = Account.default
     @parent_account.enable_feature!(:use_new_styles)
@@ -12,8 +13,7 @@ def setup_account_family_with_configs
       name: 'parent theme',
       brand_config_md5: @parent_config.md5
     )
-
-
+
     @child_account = Account.create!(parent_account: @parent_account, name: 'child')
     @child_account.brand_config = @child_config = BrandConfig.for(variables: {"ic-brand-global-nav-bgd" => "white"}, parent_md5: @parent_config.md5)
     @child_config.save!
@@ -43,7 +43,6 @@ def setup_account_family_with_configs
       brand_config_md5: second_config.md5
     )
 
-    new_brand_config = BrandConfig.for(variables: {"ic-brand-primary" => "green"})
     regenerator = BrandConfigRegenerator.new(@parent_account, user, new_brand_config)
 
     brandable_css_stub = BrandableCSS.stubs(:compile_brand!).times(5)
@@ -55,7 +54,7 @@ def setup_account_family_with_configs
     expect(@child_account.reload.brand_config.parent).to eq(new_brand_config)
     # make sure the shared brand configs in the child account are all based this new config
     expect(@child_shared_config.reload.brand_config.parent).to eq(new_brand_config)
-    # make sure the child'd active theme still is the same as it's SharedBrandConfig named 'child theme'
+    # make sure the child'd active theme still is the same as its SharedBrandConfig named 'child theme'
     expect(@child_shared_config.brand_config).to eq(@child_account.brand_config)
 
     # make sure the same for the grandchild account.
@@ -68,4 +67,46 @@ def setup_account_family_with_configs
     expect(@second_shared_config.reload.brand_config.parent).to eq(@child_account.brand_config)
   end
 
+  it "handles orphan themes that were not decendant of @parent_account" do
+    setup_account_family_with_configs
+
+    bogus_config = BrandConfig.for(variables: {"ic-brand-primary" => "brown"})
+    bogus_config.save!
+
+    @child_account.brand_config = child_config = BrandConfig.for(
+      variables: {"ic-brand-primary" => "brown"},
+      parent_md5: bogus_config.md5
+    )
+    child_config.save!
+    @child_account.save!
+
+    regenerator = BrandConfigRegenerator.new(@parent_account, user, new_brand_config)
+
+    brandable_css_stub = BrandableCSS.stubs(:compile_brand!).times(4)
+    Delayed::Testing.drain
+    expect(brandable_css_stub).to be_verified
+    expect(regenerator.progresses.count).to eq(4)
+
+    expect(@child_account.reload.brand_config.parent).to eq(new_brand_config)
+  end
+
+  it "handles reverting to default (nil) theme correctly" do
+    setup_account_family_with_configs
+
+    regenerator = BrandConfigRegenerator.new(@parent_account, user, nil)
+
+    brandable_css_stub = BrandableCSS.stubs(:compile_brand!).times(4)
+    Delayed::Testing.drain
+    expect(brandable_css_stub).to be_verified
+    expect(regenerator.progresses.count).to eq(4)
+
+    expect(@child_account.reload.brand_config.parent).to eq(nil)
+    expect(@child_shared_config.reload.brand_config.parent).to eq(nil)
+    expect(@child_shared_config.brand_config).to eq(@child_account.brand_config)
+
+    expect(@grand_child_account.reload.brand_config.parent).to eq(@child_account.brand_config)
+    expect(@grand_child_shared_config.reload.brand_config.parent).to eq(@child_account.brand_config)
+    expect(@grand_child_shared_config.brand_config).to eq(@grand_child_account.brand_config)
+  end
+
 end