Add broadcast_type to broadcast messages

Adds two different types of broadcast messages. Banner type is the
current design and will be kept shown on the top of the site, where
notification will be implemented in a separate frontend MR.

app/controllers/admin/broadcast_messages_controller.rb

@@ -61,6 +61,7 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController
       message
       starts_at
       target_path
+      broadcast_type
     ))
   end
 end

app/models/broadcast_message.rb

@@ -9,6 +9,7 @@ class BroadcastMessage < ApplicationRecord
   validates :message,   presence: true
   validates :starts_at, presence: true
   validates :ends_at,   presence: true
+  validates :broadcast_type, presence: true
 
   validates :color, allow_blank: true, color: true
   validates :font,  allow_blank: true, color: true
@@ -17,35 +18,62 @@ class BroadcastMessage < ApplicationRecord
   default_value_for :font,  '#FFFFFF'
 
   CACHE_KEY = 'broadcast_message_current_json'
+  BANNER_CACHE_KEY = 'broadcast_message_current_banner_json'
+  NOTIFICATION_CACHE_KEY = 'broadcast_message_current_notification_json'
 
   after_commit :flush_redis_cache
 
-  def self.current(current_path = nil)
-    messages = cache.fetch(CACHE_KEY, as: BroadcastMessage, expires_in: cache_expires_in) do
-      current_and_future_messages
+  enum broadcast_type: {
+    banner: 1,
+    notification: 2
+  }
+
+  class << self
+    def current_banner_messages(current_path = nil)
+      fetch_messages BANNER_CACHE_KEY, current_path do
+        current_and_future_messages.banner
+      end
     end
 
-    return [] unless messages&.present?
+    def current_notification_messages(current_path = nil)
+      fetch_messages NOTIFICATION_CACHE_KEY, current_path do
+        current_and_future_messages.notification
+      end
+    end
 
-    now_or_future = messages.select(&:now_or_future?)
+    def current(current_path = nil)
+      fetch_messages CACHE_KEY, current_path do
+        current_and_future_messages
+      end
+    end
 
-    # If there are cached entries but none are to be displayed we'll purge the
-    # cache so we don't keep running this code all the time.
-    cache.expire(CACHE_KEY) if now_or_future.empty?
+    def current_and_future_messages
+      where('ends_at > :now', now: Time.current).order_id_asc
+    end
 
-    now_or_future.select(&:now?).select { |message| message.matches_current_path(current_path) }
-  end
+    def cache
+      Gitlab::JsonCache.new(cache_key_with_version: false)
+    end
 
-  def self.current_and_future_messages
-    where('ends_at > :now', now: Time.zone.now).order_id_asc
-  end
+    def cache_expires_in
+      2.weeks
+    end
 
-  def self.cache
-    Gitlab::JsonCache.new(cache_key_with_version: false)
-  end
+    private
+
+    def fetch_messages(cache_key, current_path)
+      messages = cache.fetch(cache_key, as: BroadcastMessage, expires_in: cache_expires_in) do
+        yield
+      end
+
+      now_or_future = messages.select(&:now_or_future?)
 
-  def self.cache_expires_in
-    2.weeks
+      # If there are cached entries but none are to be displayed we'll purge the
+      # cache so we don't keep running this code all the time.
+      cache.expire(cache_key) if now_or_future.empty?
+
+      now_or_future.select(&:now?).select { |message| message.matches_current_path(current_path) }
+    end
   end
 
   def active?
@@ -53,19 +81,19 @@ class BroadcastMessage < ApplicationRecord
   end
 
   def started?
-    Time.zone.now >= starts_at
+    Time.current >= starts_at
   end
 
   def ended?
-    ends_at < Time.zone.now
+    ends_at < Time.current
   end
 
   def now?
-    (starts_at..ends_at).cover?(Time.zone.now)
+    (starts_at..ends_at).cover?(Time.current)
   end
 
   def future?
-    starts_at > Time.zone.now
+    starts_at > Time.current
   end
 
   def now_or_future?
@@ -79,7 +107,9 @@ class BroadcastMessage < ApplicationRecord
   end
 
   def flush_redis_cache
-    self.class.cache.expire(CACHE_KEY)
+    [CACHE_KEY, BANNER_CACHE_KEY, NOTIFICATION_CACHE_KEY].each do |key|
+      self.class.cache.expire(key)
+    end
   end
 end
 

changelogs/unreleased/nicolasdular-add-broadcast-type.yml

+---
+title: Add type to broadcast messages
+merge_request: 21038
+author:
+type: added

db/migrate/20191129134844_add_broadcast_type_to_broadcast_message.rb

+# frozen_string_literal: true
+
+class AddBroadcastTypeToBroadcastMessage < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+  BROADCAST_MESSAGE_BANNER_TYPE = 1
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:broadcast_messages, :broadcast_type, :smallint, default: BROADCAST_MESSAGE_BANNER_TYPE)
+  end
+
+  def down
+    remove_column(:broadcast_messages, :broadcast_type)
+  end
+end

db/schema.rb

@@ -575,6 +575,7 @@ ActiveRecord::Schema.define(version: 2019_12_06_122926) do
     t.text "message_html", null: false
     t.integer "cached_markdown_version"
     t.string "target_path", limit: 255
+    t.integer "broadcast_type", limit: 2, default: 1, null: false
     t.index ["starts_at", "ends_at", "id"], name: "index_broadcast_messages_on_starts_at_and_ends_at_and_id"
   end
 

spec/models/broadcast_message_spec.rb

@@ -20,65 +20,71 @@ describe BroadcastMessage do
     it { is_expected.to allow_value(triplet).for(:font) }
     it { is_expected.to allow_value(hex).for(:font) }
     it { is_expected.not_to allow_value('000').for(:font) }
+
+    it { is_expected.to allow_value(1).for(:broadcast_type) }
+    it { is_expected.not_to allow_value(nil).for(:broadcast_type) }
   end
 
-  describe '.current', :use_clean_rails_memory_store_caching do
+  shared_examples 'time constrainted' do |broadcast_type|
     it 'returns message if time match' do
-      message = create(:broadcast_message)
+      message = create(:broadcast_message, broadcast_type: broadcast_type)
 
-      expect(described_class.current).to include(message)
+      expect(subject.call).to include(message)
     end
 
     it 'returns multiple messages if time match' do
-      message1 = create(:broadcast_message)
-      message2 = create(:broadcast_message)
+      message1 = create(:broadcast_message, broadcast_type: broadcast_type)
+      message2 = create(:broadcast_message, broadcast_type: broadcast_type)
 
-      expect(described_class.current).to contain_exactly(message1, message2)
+      expect(subject.call).to contain_exactly(message1, message2)
     end
 
     it 'returns empty list if time not come' do
-      create(:broadcast_message, :future)
+      create(:broadcast_message, :future, broadcast_type: broadcast_type)
 
-      expect(described_class.current).to be_empty
+      expect(subject.call).to be_empty
     end
 
     it 'returns empty list if time has passed' do
-      create(:broadcast_message, :expired)
+      create(:broadcast_message, :expired, broadcast_type: broadcast_type)
 
-      expect(described_class.current).to be_empty
+      expect(subject.call).to be_empty
     end
+  end
 
+  shared_examples 'message cache' do |broadcast_type|
     it 'caches the output of the query for two weeks' do
-      create(:broadcast_message)
+      create(:broadcast_message, broadcast_type: broadcast_type)
 
       expect(described_class).to receive(:current_and_future_messages).and_call_original.twice
 
-      described_class.current
+      subject.call
 
       Timecop.travel(3.weeks) do
-        described_class.current
+        subject.call
       end
     end
 
     it 'does not create new records' do
-      create(:broadcast_message)
+      create(:broadcast_message, broadcast_type: broadcast_type)
 
-      expect { described_class.current }.not_to change { described_class.count }
+      expect { subject.call }.not_to change { described_class.count }
     end
 
     it 'includes messages that need to be displayed in the future' do
-      create(:broadcast_message)
+      create(:broadcast_message, broadcast_type: broadcast_type)
 
       future = create(
         :broadcast_message,
         starts_at: Time.now + 10.minutes,
-        ends_at: Time.now + 20.minutes
+        ends_at: Time.now + 20.minutes,
+        broadcast_type: broadcast_type
       )
 
-      expect(described_class.current.length).to eq(1)
+      expect(subject.call.length).to eq(1)
 
       Timecop.travel(future.starts_at) do
-        expect(described_class.current.length).to eq(2)
+        expect(subject.call.length).to eq(2)
       end
     end
 
@@ -86,43 +92,90 @@ describe BroadcastMessage do
       create(:broadcast_message, :future)
 
       expect(Rails.cache).not_to receive(:delete).with(described_class::CACHE_KEY)
-      expect(described_class.current.length).to eq(0)
+      expect(subject.call.length).to eq(0)
     end
+  end
 
+  shared_examples "matches with current path" do |broadcast_type|
     it 'returns message if it matches the target path' do
-      message = create(:broadcast_message, target_path: "*/onboarding_completed")
+      message = create(:broadcast_message, target_path: "*/onboarding_completed", broadcast_type: broadcast_type)
 
-      expect(described_class.current('/users/onboarding_completed')).to include(message)
+      expect(subject.call('/users/onboarding_completed')).to include(message)
     end
 
     it 'returns message if part of the target path matches' do
-      create(:broadcast_message, target_path: "/users/*/issues")
+      create(:broadcast_message, target_path: "/users/*/issues", broadcast_type: broadcast_type)
 
-      expect(described_class.current('/users/name/issues').length).to eq(1)
+      expect(subject.call('/users/name/issues').length).to eq(1)
     end
 
     it 'returns the message for empty target path' do
-      create(:broadcast_message, target_path: "")
+      create(:broadcast_message, target_path: "", broadcast_type: broadcast_type)
 
-      expect(described_class.current('/users/name/issues').length).to eq(1)
+      expect(subject.call('/users/name/issues').length).to eq(1)
     end
 
     it 'returns the message if target path is nil' do
-      create(:broadcast_message, target_path: nil)
+      create(:broadcast_message, target_path: nil, broadcast_type: broadcast_type)
 
-      expect(described_class.current('/users/name/issues').length).to eq(1)
+      expect(subject.call('/users/name/issues').length).to eq(1)
     end
 
     it 'does not return message if target path does not match' do
-      create(:broadcast_message, target_path: "/onboarding_completed")
+      create(:broadcast_message, target_path: "/onboarding_completed", broadcast_type: broadcast_type)
 
-      expect(described_class.current('/welcome').length).to eq(0)
+      expect(subject.call('/welcome').length).to eq(0)
     end
 
     it 'does not return message if target path does not match when using wildcard' do
-      create(:broadcast_message, target_path: "/users/*/issues")
+      create(:broadcast_message, target_path: "/users/*/issues", broadcast_type: broadcast_type)
+
+      expect(subject.call('/group/groupname/issues').length).to eq(0)
+    end
+  end
+
+  describe '.current', :use_clean_rails_memory_store_caching do
+    subject { -> (path = nil) { described_class.current(path) } }
+
+    it_behaves_like 'time constrainted', :banner
+    it_behaves_like 'message cache', :banner
+    it_behaves_like 'matches with current path', :banner
+
+    it 'returns both types' do
+      banner_message = create(:broadcast_message, broadcast_type: :banner)
+      notification_message = create(:broadcast_message, broadcast_type: :notification)
+
+      expect(subject.call).to contain_exactly(banner_message, notification_message)
+    end
+  end
+
+  describe '.current_banner_messages', :use_clean_rails_memory_store_caching do
+    subject { -> (path = nil) { described_class.current_banner_messages(path) } }
+
+    it_behaves_like 'time constrainted', :banner
+    it_behaves_like 'message cache', :banner
+    it_behaves_like 'matches with current path', :banner
+
+    it 'only returns banners' do
+      banner_message = create(:broadcast_message, broadcast_type: :banner)
+      create(:broadcast_message, broadcast_type: :notification)
+
+      expect(subject.call).to contain_exactly(banner_message)
+    end
+  end
+
+  describe '.current_notification_messages', :use_clean_rails_memory_store_caching do
+    subject { -> (path = nil) { described_class.current_notification_messages(path) } }
+
+    it_behaves_like 'time constrainted', :notification
+    it_behaves_like 'message cache', :notification
+    it_behaves_like 'matches with current path', :notification
+
+    it 'only returns notifications' do
+      notification_message = create(:broadcast_message, broadcast_type: :notification)
+      create(:broadcast_message, broadcast_type: :banner)
 
-      expect(described_class.current('/group/groupname/issues').length).to eq(0)
+      expect(subject.call).to contain_exactly(notification_message)
     end
   end
 
@@ -193,6 +246,8 @@ describe BroadcastMessage do
       message = create(:broadcast_message)
 
       expect(Rails.cache).to receive(:delete).with(described_class::CACHE_KEY)
+      expect(Rails.cache).to receive(:delete).with(described_class::BANNER_CACHE_KEY)
+      expect(Rails.cache).to receive(:delete).with(described_class::NOTIFICATION_CACHE_KEY)
 
       message.flush_redis_cache
     end