diff --git a/app/controllers/admin/broadcast_messages_controller.rb b/app/controllers/admin/broadcast_messages_controller.rb index 4660b0bfbb0920eb2b9edbe35ee004bcc0f5bc75..ef843a84e6ccbbb023406056be0424c566c2abbc 100644 --- a/app/controllers/admin/broadcast_messages_controller.rb +++ b/app/controllers/admin/broadcast_messages_controller.rb @@ -65,6 +65,6 @@ def broadcast_message_params target_path broadcast_type dismissable - )) + ), target_access_levels: []).reverse_merge!(target_access_levels: []) end end diff --git a/app/helpers/broadcast_messages_helper.rb b/app/helpers/broadcast_messages_helper.rb index 881e11b10ea42b95d206406dd5663aa7b3bf330b..95e68c7e3cfc74ed03a4bebce83f45ac629f5ca6 100644 --- a/app/helpers/broadcast_messages_helper.rb +++ b/app/helpers/broadcast_messages_helper.rb @@ -1,14 +1,22 @@ # 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 @@ def render_broadcast_message(broadcast_message) 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 diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index 1ee5c0818404776c8057af2993a5dc5dc7873066..90fde5f83853b172801d93a4f4cfd844d127b0ed 100644 --- a/app/models/broadcast_message.rb +++ b/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 @@ def cache_expires_in 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 @@ def fetch_messages(cache_key, current_path) # 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 @@ def now_or_future? 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? diff --git a/app/services/post_receive_service.rb b/app/services/post_receive_service.rb index f5638b0aa401266a187e3eeeb370b24f7cca91c5..15c978e6763ada2fa73685f8fd0da7c45c7ceb5c 100644 --- a/app/services/post_receive_service.rb +++ b/app/services/post_receive_service.rb @@ -86,7 +86,7 @@ def broadcast_message 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 diff --git a/app/views/admin/broadcast_messages/_form.html.haml b/app/views/admin/broadcast_messages/_form.html.haml index b68c22b6942bc057353c67e2675a1d590a0fc757..d81ebb8a2bbbe9ab64c878897e2d1eb3cc6d6bc7 100644 --- a/app/views/admin/broadcast_messages/_form.html.haml +++ b/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") diff --git a/app/views/admin/broadcast_messages/index.html.haml b/app/views/admin/broadcast_messages/index.html.haml index 3f07bea78405c55a1038bd3cc460f274b5d662db..54c2a9d525015f0bd226a067b05c6b0de95f002e 100644 --- a/app/views/admin/broadcast_messages/index.html.haml +++ b/app/views/admin/broadcast_messages/index.html.haml @@ -1,10 +1,11 @@ - 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 diff --git a/config/feature_flags/development/role_targeted_broadcast_messages.yml b/config/feature_flags/development/role_targeted_broadcast_messages.yml new file mode 100644 index 0000000000000000000000000000000000000000..723cab1abbb35b8a95e7f7568829f468ef95a4b3 --- /dev/null +++ b/config/feature_flags/development/role_targeted_broadcast_messages.yml @@ -0,0 +1,8 @@ +--- +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 diff --git a/db/migrate/20220128081329_add_target_access_levels_to_broadcast_messages.rb b/db/migrate/20220128081329_add_target_access_levels_to_broadcast_messages.rb new file mode 100644 index 0000000000000000000000000000000000000000..5958895ede8687408486e92085fb030dde7182e3 --- /dev/null +++ b/db/migrate/20220128081329_add_target_access_levels_to_broadcast_messages.rb @@ -0,0 +1,7 @@ +# 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 diff --git a/db/schema_migrations/20220128081329 b/db/schema_migrations/20220128081329 new file mode 100644 index 0000000000000000000000000000000000000000..765b4c3a5192820ec9c48540c0de0e498fb561fd --- /dev/null +++ b/db/schema_migrations/20220128081329 @@ -0,0 +1 @@ +6e273d5b92595ae6054b0665b4ff446fb2bed24ff1aab122537833dc8f4d9ab8 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 34866dcff4cb31b60c5ba18139c4c4ac5fd23b51..c158165b44dde142b9464e7240a7bd50f16351eb 100644 --- a/db/structure.sql +++ b/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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8769f0fcede5bea0353b3326aa7b91a5a7acc8e8..76254c89a79d188f8d7ea9004347f35e7319f5fd 100644 --- a/locale/gitlab.pot +++ b/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 "" @@ -6118,9 +6112,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 "" @@ -35666,6 +35657,9 @@ msgstr "" msgid "Target branch" msgstr "" +msgid "Target roles" +msgstr "" + msgid "Target-Branch" msgstr "" @@ -36154,6 +36148,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 "" @@ -39399,6 +39396,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 "" diff --git a/spec/features/admin/admin_broadcast_messages_spec.rb b/spec/features/admin/admin_broadcast_messages_spec.rb index 476dd4469bc6331bc8b5f041175cf628419152f0..aaa1f08c84fd6aa42bbe4fdb708554b93c55e043 100644 --- a/spec/features/admin/admin_broadcast_messages_spec.rb +++ b/spec/features/admin/admin_broadcast_messages_spec.rb @@ -7,7 +7,12 @@ 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 @@ 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 @@ 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 @@ 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 diff --git a/spec/helpers/broadcast_messages_helper_spec.rb b/spec/helpers/broadcast_messages_helper_spec.rb index 3e8cbdf89a07adafd43624dd7122729fb487a460..e721a3fdc9573c0e79340de2fb7cb55a0427ed03 100644 --- a/spec/helpers/broadcast_messages_helper_spec.rb +++ b/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,16 +89,26 @@ 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 diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb index d981189c6f133d0179b6f718cce09b8d685aa99b..3a072cfe2ecb8514fea1fbf2bf51571aa61ba021 100644 --- a/spec/models/broadcast_message_spec.rb +++ b/spec/models/broadcast_message_spec.rb @@ -23,6 +23,8 @@ 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 @@ 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 @@ 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 @@ 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) diff --git a/spec/views/admin/broadcast_messages/index.html.haml_spec.rb b/spec/views/admin/broadcast_messages/index.html.haml_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..e1dc76428df6fd7af881b42257a67408994d2e10 --- /dev/null +++ b/spec/views/admin/broadcast_messages/index.html.haml_spec.rb @@ -0,0 +1,36 @@ +# 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