Merge branch 'nicolasdular/add-broadcast-type' into 'master'

Add type to broadcast messages

See merge request gitlab-org/gitlab!21038

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?
-    now_or_future = messages.select(&:now_or_future?)
-    # 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
-  def self.current_and_future_messages
-    where('ends_at > :now', now: Time.zone.now).order_id_asc
-  end
-  def self.cache
-    Gitlab::JsonCache.new(cache_key_with_version: false)
-  end
-  def self.cache_expires_in
-    2.weeks
+    def current_notification_messages(current_path = nil)
+      fetch_messages NOTIFICATION_CACHE_KEY, current_path do
+        current_and_future_messages.notification
+      end
+    end
+    def current(current_path = nil)
+      fetch_messages CACHE_KEY, current_path do
+        current_and_future_messages
+      end
+    end
+    def current_and_future_messages
+      where('ends_at > :now', now: Time.current).order_id_asc
+    end
+    def cache
+      Gitlab::JsonCache.new(cache_key_with_version: false)
+    end
+    def cache_expires_in
+      2.weeks
+    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?)
+      # 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)
-      expect(described_class.current).to include(message)
+      message = create(:broadcast_message, broadcast_type: broadcast_type)
+      expect(subject.call).to include(message)
     end
     it 'returns multiple messages if time match' do
-      message1 = create(:broadcast_message)
-      message2 = create(:broadcast_message)
-      expect(described_class.current).to contain_exactly(message1, message2)
+      message1 = create(:broadcast_message, broadcast_type: broadcast_type)
+      message2 = create(:broadcast_message, broadcast_type: broadcast_type)
+      expect(subject.call).to contain_exactly(message1, message2)
     end
     it 'returns empty list if time not come' do
-      create(:broadcast_message, :future)
-      expect(described_class.current).to be_empty
+      create(:broadcast_message, :future, broadcast_type: broadcast_type)
+      expect(subject.call).to be_empty
     end
     it 'returns empty list if time has passed' do
-      create(:broadcast_message, :expired)
-      expect(described_class.current).to be_empty
+      create(:broadcast_message, :expired, broadcast_type: broadcast_type)
+      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)
-      expect { described_class.current }.not_to change { described_class.count }
+      create(:broadcast_message, broadcast_type: broadcast_type)
+      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")
-      expect(described_class.current('/users/onboarding_completed')).to include(message)
+      message = create(:broadcast_message, target_path: "*/onboarding_completed", broadcast_type: broadcast_type)
+      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")
-      expect(described_class.current('/users/name/issues').length).to eq(1)
+      create(:broadcast_message, target_path: "/users/*/issues", broadcast_type: broadcast_type)
+      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: "")
-      expect(described_class.current('/users/name/issues').length).to eq(1)
+      create(:broadcast_message, target_path: "", broadcast_type: broadcast_type)
+      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)
-      expect(described_class.current('/users/name/issues').length).to eq(1)
+      create(:broadcast_message, target_path: nil, broadcast_type: broadcast_type)
+      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")
-      expect(described_class.current('/welcome').length).to eq(0)
+      create(:broadcast_message, target_path: "/onboarding_completed", broadcast_type: broadcast_type)
+      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")
-      expect(described_class.current('/group/groupname/issues').length).to eq(0)
+      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(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