Merge branch 'revert-fc1c1aaa' into 'master'

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

See merge request gitlab-org/gitlab!80893

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(
-      current_path: request.path,
-      user_access_level: current_user_access_level_for_project_or_group
-    ).select do |message|
+    BroadcastMessage.current_banner_messages(request.path).select do |message|
       cookies["hide_broadcast_message_#{message.id}"].blank?
     end
   end
 
   def current_broadcast_notification_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|
+    not_hidden_messages = BroadcastMessage.current_notification_messages(request.path).select do |message|
       cookies["hide_broadcast_message_#{message.id}"].blank?
     end
     not_hidden_messages.last
@@ -69,31 +61,4 @@ 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,21 +4,12 @@ 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
@@ -38,20 +29,20 @@ class BroadcastMessage < ApplicationRecord
   }
 
   class << self
-    def current_banner_messages(current_path: nil, user_access_level: nil)
-      fetch_messages BANNER_CACHE_KEY, current_path, user_access_level do
+    def current_banner_messages(current_path = nil)
+      fetch_messages BANNER_CACHE_KEY, current_path do
         current_and_future_messages.banner
       end
     end
 
-    def current_notification_messages(current_path: nil, user_access_level: nil)
-      fetch_messages NOTIFICATION_CACHE_KEY, current_path, user_access_level do
+    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, user_access_level: nil)
-      fetch_messages CACHE_KEY, current_path, user_access_level do
+    def current(current_path = nil)
+      fetch_messages CACHE_KEY, current_path do
         current_and_future_messages
       end
     end
@@ -72,7 +63,7 @@ class BroadcastMessage < ApplicationRecord
 
     private
 
-    def fetch_messages(cache_key, current_path, user_access_level)
+    def fetch_messages(cache_key, current_path)
       messages = cache.fetch(cache_key, as: BroadcastMessage, expires_in: cache_expires_in) do
         yield
       end
@@ -83,13 +74,7 @@ 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
 
-      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
+      now_or_future.select(&:now?).select { |message| message.matches_current_path(current_path) }
     end
   end
 
@@ -117,12 +102,6 @@ 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(current_path: project.full_path).select do |message|
+      scoped_messages = BroadcastMessage.current_banner_messages(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,14 +55,6 @@
       = 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
-  = _('Use banners and notifications to notify your users about scheduled maintenance, recent upgrades, and more.')
+  = _('Broadcast messages are displayed for every user and can be used to notify users about scheduled maintenance, recent upgrades and more.')
 
 = render 'form'
 
@@ -20,10 +19,8 @@
           %th= _('Preview')
           %th= _('Starts')
           %th= _('Ends')
-          - if targeted_broadcast_messages_enabled
-            %th= _('Target roles')
-          %th= _('Target Path')
-          %th= _('Type')
+          %th= _(' Target Path')
+          %th= _(' Type')
           %th  
       %tbody
         - @broadcast_messages.each do |message|
@@ -36,9 +33,6 @@
               = 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,8 +11364,7 @@ CREATE TABLE broadcast_messages (
     cached_markdown_version integer,
     target_path character varying(255),
     broadcast_type smallint DEFAULT 1 NOT NULL,
-    dismissable boolean,
-    target_access_levels integer[] DEFAULT '{}'::integer[] NOT NULL
+    dismissable boolean
 );
 
 CREATE SEQUENCE broadcast_messages_id_seq

locale/gitlab.pot

@@ -28,9 +28,15 @@ 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 ""
 
@@ -6130,6 +6136,9 @@ 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 ""
 
@@ -35808,9 +35817,6 @@ msgstr ""
 msgid "Target branch"
 msgstr ""
 
-msgid "Target roles"
-msgstr ""
-
 msgid "Target-Branch"
 msgstr ""
 
@@ -36299,9 +36305,6 @@ 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 ""
 
@@ -39547,9 +39550,6 @@ 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,12 +7,7 @@ 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',
-      target_access_levels: [Gitlab::Access::DEVELOPER]
-    )
+    create(:broadcast_message, :expired, message: 'Migration to new server')
     visit admin_broadcast_messages_path
   end
 
@@ -26,13 +21,10 @@ 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"])
@@ -43,14 +35,10 @@ 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'
@@ -59,15 +47,10 @@ 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,71 +3,6 @@
 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 }
 
@@ -89,27 +24,17 @@ 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 '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]) }
+  describe 'broadcast_message' do
+    let_it_be(:user) { create(:user) }
 
-      subject { helper.current_broadcast_banner_messages.first }
+    let(:current_broadcast_message) { BroadcastMessage.new(message: 'Current Message') }
 
-      include_examples 'returns role-targeted broadcast message when in project, group, or sub-group URL'
-    end
+    before do
+      allow(helper).to receive(:current_user).and_return(user)
     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,8 +23,6 @@ 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|
@@ -177,48 +175,12 @@ 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 do
-      -> (path = nil, user_access_level = nil) do
-        described_class.current(current_path: path, user_access_level: user_access_level)
-      end
-    end
+    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_behaves_like 'matches with user access level', :banner
 
     it 'returns both types' do
       banner_message = create(:broadcast_message, broadcast_type: :banner)
@@ -229,16 +191,11 @@ RSpec.describe BroadcastMessage do
   end
 
   describe '.current_banner_messages', :use_clean_rails_memory_store_caching do
-    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
+    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_behaves_like 'matches with user access level', :banner
 
     it 'only returns banners' do
       banner_message = create(:broadcast_message, broadcast_type: :banner)
@@ -249,16 +206,11 @@ RSpec.describe BroadcastMessage do
   end
 
   describe '.current_notification_messages', :use_clean_rails_memory_store_caching do
-    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
+    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_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