Merge branch 'appearances-caching-and-schema' into 'master'

Cache Appearance instances in Redis

Closes #36066 and #31698

See merge request !13433
# Conflicts:
#	db/schema.rb

7 files changed, 97 insertions, 5 deletions

diff --git a/app/controllers/admin/appearances_controller.rb b/app/controllers/admin/appearances_controller.rb
index 4b0ec54b3f4..92df1c8dff0 100644
--- a/app/controllers/admin/appearances_controller.rb
+++ b/app/controllers/admin/appearances_controller.rb
@@ -45,7 +45,7 @@ class Admin::AppearancesController < Admin::ApplicationController
 
   # Use callbacks to share common setup or constraints between actions.
   def set_appearance
-    @appearance = Appearance.last || Appearance.new
+    @appearance = Appearance.current || Appearance.new
   end
 
   # Only allow a trusted parameter "white list" through.
diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb
index 16136d02530..cdf5fa5d4b7 100644
--- a/app/helpers/appearances_helper.rb
+++ b/app/helpers/appearances_helper.rb
@@ -20,7 +20,7 @@ module AppearancesHelper
   end
 
   def brand_item
-    @appearance ||= Appearance.first
+    @appearance ||= Appearance.current
   end
 
   def brand_header_logo
diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index f9c48482be7..ff15689ecac 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -8,7 +8,27 @@ class Appearance < ActiveRecord::Base
   validates :logo,        file_size: { maximum: 1.megabyte }
   validates :header_logo, file_size: { maximum: 1.megabyte }
 
+  validate :single_appearance_row, on: :create
+
   mount_uploader :logo,         AttachmentUploader
   mount_uploader :header_logo,  AttachmentUploader
   has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+
+  CACHE_KEY = 'current_appearance'.freeze
+
+  after_commit :flush_redis_cache
+
+  def self.current
+    Rails.cache.fetch(CACHE_KEY) { first }
+  end
+
+  def flush_redis_cache
+    Rails.cache.delete(CACHE_KEY)
+  end
+
+  def single_appearance_row
+    if self.class.any?
+      errors.add(:single_appearance_row, 'Only 1 appearances row can exist')
+    end
+  end
 end
diff --git a/changelogs/unreleased/appearances-caching-and-schema.yml b/changelogs/unreleased/appearances-caching-and-schema.yml
new file mode 100644
index 00000000000..5743f6e0f2d
--- /dev/null
+++ b/changelogs/unreleased/appearances-caching-and-schema.yml
@@ -0,0 +1,4 @@
+---
+title: Cache Appearance instances in Redis
+merge_request:
+author:
diff --git a/db/migrate/20170809142252_cleanup_appearances_schema.rb b/db/migrate/20170809142252_cleanup_appearances_schema.rb
new file mode 100644
index 00000000000..90d12925ba2
--- /dev/null
+++ b/db/migrate/20170809142252_cleanup_appearances_schema.rb
@@ -0,0 +1,33 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class CleanupAppearancesSchema < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  NOT_NULL_COLUMNS = %i[title description description_html created_at updated_at]
+
+  TIME_COLUMNS = %i[created_at updated_at]
+
+  def up
+    NOT_NULL_COLUMNS.each do |column|
+      change_column_null :appearances, column, false
+    end
+
+    TIME_COLUMNS.each do |column|
+      change_column :appearances, column, :datetime_with_timezone
+    end
+  end
+
+  def down
+    NOT_NULL_COLUMNS.each do |column|
+      change_column_null :appearances, column, true
+    end
+
+    TIME_COLUMNS.each do |column|
+      change_column :appearances, column, :datetime # rubocop: disable Migration/Datetime
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index f7cb68059e5..afab9cd857f 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -28,13 +28,13 @@ ActiveRecord::Schema.define(version: 20170815060945) do
   end
 
   create_table "appearances", force: :cascade do |t|
-    t.string "title"
-    t.text "description"
+    t.string "title", null: false
+    t.text "description", null: false
     t.string "header_logo"
     t.string "logo"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.text "description_html"
+    t.text "description_html", null: false
     t.integer "cached_markdown_version"
   end
 
diff --git a/spec/models/appearance_spec.rb b/spec/models/appearance_spec.rb
index 7cd3a84d592..b5d5d58697b 100644
--- a/spec/models/appearance_spec.rb
+++ b/spec/models/appearance_spec.rb
@@ -9,4 +9,39 @@ RSpec.describe Appearance do
   it { is_expected.to validate_presence_of(:description) }
 
   it { is_expected.to have_many(:uploads).dependent(:destroy) }
+
+  describe '.current', :use_clean_rails_memory_store_caching do
+    let!(:appearance) { create(:appearance) }
+
+    it 'returns the current appearance row' do
+      expect(described_class.current).to eq(appearance)
+    end
+
+    it 'caches the result' do
+      expect(described_class).to receive(:first).once
+
+      2.times { described_class.current }
+    end
+  end
+
+  describe '#flush_redis_cache' do
+    it 'flushes the cache in Redis' do
+      appearance = create(:appearance)
+
+      expect(Rails.cache).to receive(:delete).with(described_class::CACHE_KEY)
+
+      appearance.flush_redis_cache
+    end
+  end
+
+  describe '#single_appearance_row' do
+    it 'adds an error when more than 1 row exists' do
+      create(:appearance)
+
+      new_row = build(:appearance)
+      new_row.save
+
+      expect(new_row.valid?).to eq(false)
+    end
+  end
 end