Merge branch 'role-targeted-broadcast' into 'master'

Role-targeted broadcast messages

See merge request gitlab-org/gitlab!77498

app/controllers/admin/broadcast_messages_controller.rb

@@ -65,6 +65,6 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController
       target_path
       broadcast_type
       dismissable
-    ))
+    ), target_access_levels: []).reverse_merge!(target_access_levels: [])
   end
 end

app/helpers/broadcast_messages_helper.rb

 # frozen_string_literal: true
 
 module BroadcastMessagesHelper
+  include Gitlab::Utils::StrongMemoize
+
   def current_broadcast_banner_messages
-    BroadcastMessage.current_banner_messages(request.path).select do |message|
+    BroadcastMessage.current_banner_messages(
+      current_path: request.path,
+      user_access_level: current_user_access_level_for_project_or_group
+    ).select do |message|
       cookies["hide_broadcast_message_#{message.id}"].blank?
     end
   end
 
   def current_broadcast_notification_message
-    not_hidden_messages = BroadcastMessage.current_notification_messages(request.path).select do |message|
+    not_hidden_messages = BroadcastMessage.current_notification_messages(
+      current_path: request.path,
+      user_access_level: current_user_access_level_for_project_or_group
+    ).select do |message|
       cookies["hide_broadcast_message_#{message.id}"].blank?
     end
     not_hidden_messages.last
@@ -61,4 +69,31 @@ module BroadcastMessagesHelper
   def broadcast_type_options
     BroadcastMessage.broadcast_types.keys.map { |w| [w.humanize, w] }
   end
+
+  def target_access_level_options
+    BroadcastMessage::ALLOWED_TARGET_ACCESS_LEVELS.map do |access_level|
+      [Gitlab::Access.human_access(access_level), access_level]
+    end
+  end
+
+  def target_access_levels_display(access_levels)
+    access_levels.map do |access_level|
+      Gitlab::Access.human_access(access_level)
+    end.join(', ')
+  end
+
+  private
+
+  def current_user_access_level_for_project_or_group
+    return if Feature.disabled?(:role_targeted_broadcast_messages, default_enabled: :yaml)
+    return unless current_user.present?
+
+    strong_memoize(:current_user_access_level_for_project_or_group) do
+      if controller.is_a? Projects::ApplicationController
+        @project&.team&.max_member_access(current_user.id)
+      elsif controller.is_a? Groups::ApplicationController
+        @group&.max_member_access_for_user(current_user)
+      end
+    end
+  end
 end

app/models/broadcast_message.rb

@@ -4,12 +4,21 @@ class BroadcastMessage < ApplicationRecord
   include CacheMarkdownField
   include Sortable
 
+  ALLOWED_TARGET_ACCESS_LEVELS = [
+    Gitlab::Access::GUEST,
+    Gitlab::Access::REPORTER,
+    Gitlab::Access::DEVELOPER,
+    Gitlab::Access::MAINTAINER,
+    Gitlab::Access::OWNER
+  ].freeze
+
   cache_markdown_field :message, pipeline: :broadcast_message, whitelisted: true
 
   validates :message,   presence: true
   validates :starts_at, presence: true
   validates :ends_at,   presence: true
   validates :broadcast_type, presence: true
+  validates :target_access_levels, inclusion: { in: ALLOWED_TARGET_ACCESS_LEVELS }
 
   validates :color, allow_blank: true, color: true
   validates :font,  allow_blank: true, color: true
@@ -29,20 +38,20 @@ class BroadcastMessage < ApplicationRecord
   }
 
   class << self
-    def current_banner_messages(current_path = nil)
-      fetch_messages BANNER_CACHE_KEY, current_path do
+    def current_banner_messages(current_path: nil, user_access_level: nil)
+      fetch_messages BANNER_CACHE_KEY, current_path, user_access_level do
         current_and_future_messages.banner
       end
     end
 
-    def current_notification_messages(current_path = nil)
-      fetch_messages NOTIFICATION_CACHE_KEY, current_path do
+    def current_notification_messages(current_path: nil, user_access_level: nil)
+      fetch_messages NOTIFICATION_CACHE_KEY, current_path, user_access_level do
         current_and_future_messages.notification
       end
     end
 
-    def current(current_path = nil)
-      fetch_messages CACHE_KEY, current_path do
+    def current(current_path: nil, user_access_level: nil)
+      fetch_messages CACHE_KEY, current_path, user_access_level do
         current_and_future_messages
       end
     end
@@ -63,7 +72,7 @@ class BroadcastMessage < ApplicationRecord
 
     private
 
-    def fetch_messages(cache_key, current_path)
+    def fetch_messages(cache_key, current_path, user_access_level)
       messages = cache.fetch(cache_key, as: BroadcastMessage, expires_in: cache_expires_in) do
         yield
       end
@@ -74,7 +83,13 @@ class BroadcastMessage < ApplicationRecord
       # displaying we'll refresh the cache so we don't need to keep filtering.
       cache.expire(cache_key) if now_or_future != messages
 
-      now_or_future.select(&:now?).select { |message| message.matches_current_path(current_path) }
+      messages = now_or_future.select(&:now?)
+      messages = messages.select do |message|
+        message.matches_current_user_access_level?(user_access_level)
+      end
+      messages.select do |message|
+        message.matches_current_path(current_path)
+      end
     end
   end
 
@@ -102,6 +117,12 @@ class BroadcastMessage < ApplicationRecord
     now? || future?
   end
 
+  def matches_current_user_access_level?(user_access_level)
+    return true if target_access_levels.empty?
+
+    target_access_levels.include? user_access_level
+  end
+
   def matches_current_path(current_path)
     return false if current_path.blank? && target_path.present?
     return true if current_path.blank? || target_path.blank?

app/services/post_receive_service.rb

@@ -86,7 +86,7 @@ class PostReceiveService
     banner = nil
 
     if project
-      scoped_messages = BroadcastMessage.current_banner_messages(project.full_path).select do |message|
+      scoped_messages = BroadcastMessage.current_banner_messages(current_path: project.full_path).select do |message|
         message.target_path.present? && message.matches_current_path(project.full_path)
       end
 

app/views/admin/broadcast_messages/_form.html.haml

@@ -55,6 +55,14 @@
       = f.check_box :dismissable
       = f.label :dismissable do
         = _('Allow users to dismiss the broadcast message')
+  - if Feature.enabled?(:role_targeted_broadcast_messages, default_enabled: :yaml)
+    .form-group.row
+      .col-sm-2.col-form-label
+        = f.label :target_access_levels, _('Target roles')
+      .col-sm-10
+        = f.select :target_access_levels, target_access_level_options, { include_hidden: false }, multiple: true, class: 'form-control'
+        .form-text.text-muted
+          = _('The broadcast message displays only to users in projects and groups who have these roles.')
   .form-group.row.js-toggle-colors-container.toggle-colors.hide
     .col-sm-2.col-form-label
       = f.label :font, _("Font Color")

app/views/admin/broadcast_messages/index.html.haml

 - breadcrumb_title _("Messages")
 - page_title _("Broadcast Messages")
+- targeted_broadcast_messages_enabled = Feature.enabled?(:role_targeted_broadcast_messages, default_enabled: :yaml)
 
 %h3.page-title
   = _('Broadcast Messages')
 %p.light
-  = _('Broadcast messages are displayed for every user and can be used to notify users about scheduled maintenance, recent upgrades and more.')
+  = _('Use banners and notifications to notify your users about scheduled maintenance, recent upgrades, and more.')
 
 = render 'form'
 
@@ -19,8 +20,10 @@
           %th= _('Preview')
           %th= _('Starts')
           %th= _('Ends')
-          %th= _(' Target Path')
-          %th= _(' Type')
+          - if targeted_broadcast_messages_enabled
+            %th= _('Target roles')
+          %th= _('Target Path')
+          %th= _('Type')
           %th  
       %tbody
         - @broadcast_messages.each do |message|
@@ -33,6 +36,9 @@
               = message.starts_at
             %td
               = message.ends_at
+            - if targeted_broadcast_messages_enabled
+              %td
+                = target_access_levels_display(message.target_access_levels)
             %td
               = message.target_path
             %td

config/feature_flags/development/role_targeted_broadcast_messages.yml

+---
+name: role_targeted_broadcast_messages
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77498
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/351736
+milestone: '14.8'
+type: development
+group: group::activation
+default_enabled: false

db/migrate/20220128081329_add_target_access_levels_to_broadcast_messages.rb

+# frozen_string_literal: true
+
+class AddTargetAccessLevelsToBroadcastMessages < Gitlab::Database::Migration[1.0]
+  def change
+    add_column :broadcast_messages, :target_access_levels, :integer, array: true, null: false, default: []
+  end
+end

db/schema_migrations/20220128081329

+6e273d5b92595ae6054b0665b4ff446fb2bed24ff1aab122537833dc8f4d9ab8
\ No newline at end of file

db/structure.sql

@@ -11364,7 +11364,8 @@ CREATE TABLE broadcast_messages (
     cached_markdown_version integer,
     target_path character varying(255),
     broadcast_type smallint DEFAULT 1 NOT NULL,
-    dismissable boolean
+    dismissable boolean,
+    target_access_levels integer[] DEFAULT '{}'::integer[] NOT NULL
 );
 
 CREATE SEQUENCE broadcast_messages_id_seq

locale/gitlab.pot

@@ -28,15 +28,9 @@ msgstr ""
 msgid " Please sign in."
 msgstr ""
 
-msgid " Target Path"
-msgstr ""
-
 msgid " Try to %{action} this file again."
 msgstr ""
 
-msgid " Type"
-msgstr ""
-
 msgid " You need to do this before %{grace_period_deadline}."
 msgstr ""
 
@@ -6136,9 +6130,6 @@ msgstr ""
 msgid "Broadcast Messages"
 msgstr ""
 
-msgid "Broadcast messages are displayed for every user and can be used to notify users about scheduled maintenance, recent upgrades and more."
-msgstr ""
-
 msgid "Browse Directory"
 msgstr ""
 
@@ -35702,6 +35693,9 @@ msgstr ""
 msgid "Target branch"
 msgstr ""
 
+msgid "Target roles"
+msgstr ""
+
 msgid "Target-Branch"
 msgstr ""
 
@@ -36190,6 +36184,9 @@ msgstr ""
 msgid "The branch or tag does not exist"
 msgstr ""
 
+msgid "The broadcast message displays only to users in projects and groups who have these roles."
+msgstr ""
+
 msgid "The character highlighter helps you keep the subject line to %{titleLength} characters and wrap the body at %{bodyLength} so they are readable in git."
 msgstr ""
 
@@ -39435,6 +39432,9 @@ msgstr ""
 msgid "Use authorized_keys file to authenticate SSH keys"
 msgstr ""
 
+msgid "Use banners and notifications to notify your users about scheduled maintenance, recent upgrades, and more."
+msgstr ""
+
 msgid "Use cURL"
 msgstr ""
 

spec/features/admin/admin_broadcast_messages_spec.rb

@@ -7,7 +7,12 @@ RSpec.describe 'Admin Broadcast Messages' do
     admin = create(:admin)
     sign_in(admin)
     gitlab_enable_admin_mode_sign_in(admin)
-    create(:broadcast_message, :expired, message: 'Migration to new server')
+    create(
+      :broadcast_message,
+      :expired,
+      message: 'Migration to new server',
+      target_access_levels: [Gitlab::Access::DEVELOPER]
+    )
     visit admin_broadcast_messages_path
   end
 
@@ -21,10 +26,13 @@ RSpec.describe 'Admin Broadcast Messages' do
     fill_in 'broadcast_message_target_path', with: '*/user_onboarded'
     fill_in 'broadcast_message_font', with: '#b94a48'
     select Date.today.next_year.year, from: 'broadcast_message_ends_at_1i'
+    select 'Guest', from: 'broadcast_message_target_access_levels'
+    select 'Owner', from: 'broadcast_message_target_access_levels'
     click_button 'Add broadcast message'
 
     expect(current_path).to eq admin_broadcast_messages_path
     expect(page).to have_content 'Application update from 4:00 CST to 5:00 CST'
+    expect(page).to have_content 'Guest, Owner'
     expect(page).to have_content '*/user_onboarded'
     expect(page).to have_selector 'strong', text: '4:00 CST to 5:00 CST'
     expect(page).to have_selector %(div[style="background-color: #f2dede; color: #b94a48"])
@@ -35,10 +43,14 @@ RSpec.describe 'Admin Broadcast Messages' do
     fill_in 'broadcast_message_target_path', with: '*/user_onboarded'
     select 'Notification', from: 'broadcast_message_broadcast_type'
     select Date.today.next_year.year, from: 'broadcast_message_ends_at_1i'
+    select 'Reporter', from: 'broadcast_message_target_access_levels'
+    select 'Developer', from: 'broadcast_message_target_access_levels'
+    select 'Maintainer', from: 'broadcast_message_target_access_levels'
     click_button 'Add broadcast message'
 
     expect(current_path).to eq admin_broadcast_messages_path
     expect(page).to have_content 'Application update from 4:00 CST to 5:00 CST'
+    expect(page).to have_content 'Reporter, Developer, Maintainer'
     expect(page).to have_content '*/user_onboarded'
     expect(page).to have_content 'Notification'
     expect(page).to have_selector 'strong', text: '4:00 CST to 5:00 CST'
@@ -47,10 +59,15 @@ RSpec.describe 'Admin Broadcast Messages' do
   it 'edit an existing broadcast message' do
     click_link 'Edit'
     fill_in 'broadcast_message_message', with: 'Application update RIGHT NOW'
+    select 'Reporter', from: 'broadcast_message_target_access_levels'
     click_button 'Update broadcast message'
 
     expect(current_path).to eq admin_broadcast_messages_path
     expect(page).to have_content 'Application update RIGHT NOW'
+
+    page.within('.table-responsive') do
+      expect(page).to have_content 'Reporter, Developer'
+    end
   end
 
   it 'remove an existing broadcast message' do

spec/helpers/broadcast_messages_helper_spec.rb

@@ -3,6 +3,71 @@
 require 'spec_helper'
 
 RSpec.describe BroadcastMessagesHelper do
+  include Gitlab::Routing.url_helpers
+
+  let_it_be(:user) { create(:user) }
+
+  before do
+    allow(helper).to receive(:current_user).and_return(user)
+  end
+
+  shared_examples 'returns role-targeted broadcast message when in project, group, or sub-group URL' do
+    let(:feature_flag_state) { true }
+
+    before do
+      stub_feature_flags(role_targeted_broadcast_messages: feature_flag_state)
+      allow(helper).to receive(:cookies) { {} }
+    end
+
+    context 'when in a project page' do
+      let_it_be(:project) { create(:project) }
+
+      before do
+        project.add_developer(user)
+
+        assign(:project, project)
+        allow(helper).to receive(:controller) { ProjectsController.new }
+      end
+
+      it { is_expected.to eq message }
+
+      context 'when feature flag is disabled' do
+        let(:feature_flag_state) { false }
+
+        it { is_expected.to be_nil }
+      end
+    end
+
+    context 'when in a group page' do
+      let_it_be(:group) { create(:group) }
+
+      before do
+        group.add_developer(user)
+
+        assign(:group, group)
+        allow(helper).to receive(:controller) { GroupsController.new }
+      end
+
+      it { is_expected.to eq message }
+
+      context 'when feature flag is disabled' do
+        let(:feature_flag_state) { false }
+
+        it { is_expected.to be_nil }
+      end
+    end
+
+    context 'when not in a project, group, or sub-group page' do
+      it { is_expected.to be_nil }
+
+      context 'when feature flag is disabled' do
+        let(:feature_flag_state) { false }
+
+        it { is_expected.to be_nil }
+      end
+    end
+  end
+
   describe 'current_broadcast_notification_message' do
     subject { helper.current_broadcast_notification_message }
 
@@ -24,17 +89,27 @@ RSpec.describe BroadcastMessagesHelper do
     context 'without broadcast notification messages' do
       it { is_expected.to be_nil }
     end
+
+    describe 'user access level targeted messages' do
+      let_it_be(:message) { create(:broadcast_message, broadcast_type: 'notification', starts_at: Time.now, target_access_levels: [Gitlab::Access::DEVELOPER]) }
+
+      include_examples 'returns role-targeted broadcast message when in project, group, or sub-group URL'
+    end
   end
 
-  describe 'broadcast_message' do
-    let_it_be(:user) { create(:user) }
+  describe 'current_broadcast_banner_messages' do
+    describe 'user access level targeted messages' do
+      let_it_be(:message) { create(:broadcast_message, broadcast_type: 'banner', starts_at: Time.now, target_access_levels: [Gitlab::Access::DEVELOPER]) }
 
-    let(:current_broadcast_message) { BroadcastMessage.new(message: 'Current Message') }
+      subject { helper.current_broadcast_banner_messages.first }
 
-    before do
-      allow(helper).to receive(:current_user).and_return(user)
+      include_examples 'returns role-targeted broadcast message when in project, group, or sub-group URL'
+    end
   end
 
+  describe 'broadcast_message' do
+    let(:current_broadcast_message) { BroadcastMessage.new(message: 'Current Message') }
+
     it 'returns nil when no current message' do
       expect(helper.broadcast_message(nil)).to be_nil
     end

spec/models/broadcast_message_spec.rb

@@ -23,6 +23,8 @@ RSpec.describe BroadcastMessage do
 
     it { is_expected.to allow_value(1).for(:broadcast_type) }
     it { is_expected.not_to allow_value(nil).for(:broadcast_type) }
+    it { is_expected.not_to allow_value(nil).for(:target_access_levels) }
+    it { is_expected.to validate_inclusion_of(:target_access_levels).in_array(described_class::ALLOWED_TARGET_ACCESS_LEVELS) }
   end
 
   shared_examples 'time constrainted' do |broadcast_type|
@@ -175,12 +177,48 @@ RSpec.describe BroadcastMessage do
     end
   end
 
+  shared_examples "matches with user access level" do |broadcast_type|
+    context 'when target_access_levels is empty' do
+      let_it_be(:message) { create(:broadcast_message, target_access_levels: [], broadcast_type: broadcast_type) }
+
+      it 'returns the message if user access level is not nil' do
+        expect(subject.call(nil, Gitlab::Access::MINIMAL_ACCESS)).to include(message)
+      end
+
+      it 'returns the message if user access level is nil' do
+        expect(subject.call(nil, nil)).to include(message)
+      end
+    end
+
+    context 'when target_access_levels is not empty' do
+      let_it_be(:target_access_levels) { [Gitlab::Access::GUEST] }
+      let_it_be(:message) { create(:broadcast_message, target_access_levels: target_access_levels, broadcast_type: broadcast_type) }
+
+      it "does not return the message if user access level is nil" do
+        expect(subject.call(nil, nil)).to be_empty
+      end
+
+      it "returns the message if user access level is in target_access_levels" do
+        expect(subject.call(nil, Gitlab::Access::GUEST)).to include(message)
+      end
+
+      it "does not return the message if user access level is not in target_access_levels" do
+        expect(subject.call(nil, Gitlab::Access::MINIMAL_ACCESS)).to be_empty
+      end
+    end
+  end
+
   describe '.current', :use_clean_rails_memory_store_caching do
-    subject { -> (path = nil) { described_class.current(path) } }
+    subject do
+      -> (path = nil, user_access_level = nil) do
+        described_class.current(current_path: path, user_access_level: user_access_level)
+      end
+    end
 
     it_behaves_like 'time constrainted', :banner
     it_behaves_like 'message cache', :banner
     it_behaves_like 'matches with current path', :banner
+    it_behaves_like 'matches with user access level', :banner
 
     it 'returns both types' do
       banner_message = create(:broadcast_message, broadcast_type: :banner)
@@ -191,11 +229,16 @@ RSpec.describe BroadcastMessage do
   end
 
   describe '.current_banner_messages', :use_clean_rails_memory_store_caching do
-    subject { -> (path = nil) { described_class.current_banner_messages(path) } }
+    subject do
+      -> (path = nil, user_access_level = nil) do
+        described_class.current_banner_messages(current_path: path, user_access_level: user_access_level)
+      end
+    end
 
     it_behaves_like 'time constrainted', :banner
     it_behaves_like 'message cache', :banner
     it_behaves_like 'matches with current path', :banner
+    it_behaves_like 'matches with user access level', :banner
 
     it 'only returns banners' do
       banner_message = create(:broadcast_message, broadcast_type: :banner)
@@ -206,11 +249,16 @@ RSpec.describe BroadcastMessage do
   end
 
   describe '.current_notification_messages', :use_clean_rails_memory_store_caching do
-    subject { -> (path = nil) { described_class.current_notification_messages(path) } }
+    subject do
+      -> (path = nil, user_access_level = nil) do
+        described_class.current_notification_messages(current_path: path, user_access_level: user_access_level)
+      end
+    end
 
     it_behaves_like 'time constrainted', :notification
     it_behaves_like 'message cache', :notification
     it_behaves_like 'matches with current path', :notification
+    it_behaves_like 'matches with user access level', :notification
 
     it 'only returns notifications' do
       notification_message = create(:broadcast_message, broadcast_type: :notification)

spec/views/admin/broadcast_messages/index.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'admin/broadcast_messages/index' do
+  describe 'Target roles select and table column' do
+    let(:feature_flag_state) { true }
+
+    let_it_be(:message) { create(:broadcast_message, broadcast_type: 'banner', target_access_levels: [Gitlab::Access::GUEST, Gitlab::Access::DEVELOPER]) }
+
+    before do
+      assign(:broadcast_messages, BroadcastMessage.page(1))
+      assign(:broadcast_message, BroadcastMessage.new)
+
+      stub_feature_flags(role_targeted_broadcast_messages: feature_flag_state)
+
+      render
+    end
+
+    it 'rendered' do
+      expect(rendered).to have_content('Target roles')
+      expect(rendered).to have_content('Owner')
+      expect(rendered).to have_content('Guest, Developer')
+    end
+
+    context 'when feature flag is off' do
+      let(:feature_flag_state) { false }
+
+      it 'is not rendered' do
+        expect(rendered).not_to have_content('Target roles')
+        expect(rendered).not_to have_content('Owner')
+        expect(rendered).not_to have_content('Guest, Developer')
+      end
+    end
+  end
+end