Merge branch 'feat/broadcast-message-dimsiss' into 'master'

Add user dismiss option to broadcast messages

Closes #16999

See merge request gitlab-org/gitlab!20665

app/assets/javascripts/broadcast_notification.js

@@ -6,16 +6,14 @@ const handleOnDismiss = ({ currentTarget }) => {
     dataset: { id },
   } = currentTarget;
 
-  Cookies.set(`hide_broadcast_notification_message_${id}`, true);
+  Cookies.set(`hide_broadcast_message_${id}`, true);
 
   const notification = document.querySelector(`.js-broadcast-notification-${id}`);
   notification.parentNode.removeChild(notification);
 };
 
 export default () => {
-  const dismissButton = document.querySelector('.js-dismiss-current-broadcast-notification');
-
-  if (dismissButton) {
-    dismissButton.addEventListener('click', handleOnDismiss);
-  }
+  document
+    .querySelectorAll('.js-dismiss-current-broadcast-notification')
+    .forEach(dismissButton => dismissButton.addEventListener('click', handleOnDismiss));
 };

app/assets/javascripts/pages/admin/broadcast_messages/broadcast_message.js

@@ -25,9 +25,11 @@ export default () => {
 
   $broadcastMessageType.on('change', () => {
     const $broadcastMessageColorFormGroup = $('.js-broadcast-message-background-color-form-group');
+    const $broadcastMessageDismissableFormGroup = $('.js-broadcast-message-dismissable-form-group');
     const $broadcastNotificationMessagePreview = $('.js-broadcast-notification-message-preview');
 
     $broadcastMessageColorFormGroup.toggleClass('hidden');
+    $broadcastMessageDismissableFormGroup.toggleClass('hidden');
     $broadcastBannerMessagePreview.toggleClass('hidden');
     $broadcastNotificationMessagePreview.toggleClass('hidden');
   });

app/assets/stylesheets/framework/broadcast_messages.scss

@@ -17,6 +17,10 @@
   @extend .broadcast-message;
   @extend .alert-warning;
   text-align: center;
+
+  .broadcast-message-dismiss {
+    color: inherit;
+  }
 }
 
 .broadcast-notification-message {
@@ -36,6 +40,11 @@
   &.preview {
     position: static;
   }
+
+  .broadcast-message-dismiss {
+    height: 100%;
+    color: $gray-800;
+  }
 }
 
 .toggle-colors {

app/controllers/admin/broadcast_messages_controller.rb

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

app/helpers/broadcast_messages_helper.rb

@@ -2,12 +2,14 @@
 
 module BroadcastMessagesHelper
   def current_broadcast_banner_messages
-    BroadcastMessage.current_banner_messages(request.path)
+    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(request.path).select do |message|
-      cookies["hide_broadcast_notification_message_#{message.id}"].blank?
+      cookies["hide_broadcast_message_#{message.id}"].blank?
     end
     not_hidden_messages.last
   end

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

@@ -46,6 +46,13 @@
 
       = render_suggested_colors
 
+  .form-group.row.js-broadcast-message-dismissable-form-group{ class: ('hidden' unless @broadcast_message.banner? ) }
+    .col-sm-2.col-form-label.pt-0
+      = f.label :starts_at, _("Dismissable")
+    .col-sm-10
+      = f.check_box :dismissable
+      = f.label :dismissable do
+        = _('Allow users to dismiss the broadcast message')
   .form-group.row.js-toggle-colors-container.toggle-colors.hide
     .col-sm-2.col-form-label
       = f.label :font, "Font Color"

app/views/shared/_broadcast_message.html.haml

 %div{ class: "broadcast-#{message.broadcast_type}-message #{opts[:preview] && 'preview'} js-broadcast-notification-#{message.id} d-flex",
   style: broadcast_message_style(message), dir: 'auto' }
-  %div
+  .flex-grow-1.text-right.pr-2
     = sprite_icon('bullhorn', size: 16, css_class: 'vertical-align-text-top')
+  %div{ class: !fluid_layout && 'container-limited' }
     = render_broadcast_message(message)
-  - if message.notification? && opts[:preview].blank?
-    %button.js-dismiss-current-broadcast-notification.btn.btn-link.text-dark.pl-2.pr-2{ 'aria-label' => _('Close'), :type => 'button', data: { id: message.id } }
-      %i.fa.fa-times
+  .flex-grow-1.text-right{ style: 'flex-basis: 0' }
+    - if (message.notification? || message.dismissable?) && opts[:preview].blank?
+      %button.broadcast-message-dismiss.js-dismiss-current-broadcast-notification.btn.btn-link.pl-2.pr-2{ 'aria-label' => _('Close'), :type => 'button', data: { id: message.id } }
+        %i.fa.fa-times

changelogs/unreleased/feat-broadcast-message-dimsiss.yml

+---
+title: Add user dismiss option to broadcast messages
+merge_request: 20665
+author: Fabio Huser
+type: added

db/migrate/20191123081456_add_dismissable_to_broadcast_messages.rb

+# frozen_string_literal: true
+
+class AddDismissableToBroadcastMessages < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_column :broadcast_messages, :dismissable, :boolean
+  end
+end

db/schema.rb

@@ -573,6 +573,7 @@ ActiveRecord::Schema.define(version: 2020_03_03_074328) do
     t.integer "cached_markdown_version"
     t.string "target_path", limit: 255
     t.integer "broadcast_type", limit: 2, default: 1, null: false
+    t.boolean "dismissable"
     t.index ["ends_at", "broadcast_type", "id"], name: "index_broadcast_message_on_ends_at_and_broadcast_type_and_id"
   end
 

doc/api/broadcast_messages.md

@@ -36,7 +36,8 @@ Example response:
         "id":1,
         "active": false,
         "target_path": "*/welcome",
-        "broadcast_type": "banner"
+        "broadcast_type": "banner",
+        "dismissable": false
     }
 ]
 ```
@@ -73,7 +74,8 @@ Example response:
     "id":1,
     "active":false,
     "target_path": "*/welcome",
-    "broadcast_type": "banner"
+    "broadcast_type": "banner",
+    "dismissable": false
 }
 ```
 
@@ -87,15 +89,16 @@ POST /broadcast_messages
 
 Parameters:
 
-| Attribute   | Type     | Required | Description                                           |
-|:------------|:---------|:---------|:------------------------------------------------------|
-| `message`   | string   | yes      | Message to display.                                   |
-| `starts_at` | datetime | no       | Starting time (defaults to current time).             |
-| `ends_at`   | datetime | no       | Ending time (defaults to one hour from current time). |
-| `color`     | string   | no       | Background color hex code.                            |
-| `font`      | string   | no       | Foreground color hex code.                            |
-| `target_path`| string  | no       | Target path of the broadcast message.                 |
-| `broadcast_type`| string | no     | Appearance type (defaults to banner)                  |
+| Attribute       | Type     | Required | Description                                           |
+|:----------------|:---------|:---------|:------------------------------------------------------|
+| `message`       | string   | yes      | Message to display.                                   |
+| `starts_at`     | datetime | no       | Starting time (defaults to current time).             |
+| `ends_at`       | datetime | no       | Ending time (defaults to one hour from current time). |
+| `color`         | string   | no       | Background color hex code.                            |
+| `font`          | string   | no       | Foreground color hex code.                            |
+| `target_path`   | string   | no       | Target path of the broadcast message.                 |
+| `broadcast_type`| string   | no       | Appearance type (defaults to banner)                  |
+| `dismissable`   | boolean  | no       | Can the user dismiss the message?                     |
 
 Example request:
 
@@ -116,6 +119,7 @@ Example response:
     "active": true,
     "target_path": "*/welcome",
     "broadcast_type": "notification",
+    "dismissable": false
 }
 ```
 
@@ -129,16 +133,17 @@ PUT /broadcast_messages/:id
 
 Parameters:
 
-| Attribute   | Type     | Required | Description                        |
-|:------------|:---------|:---------|:-----------------------------------|
-| `id`        | integer  | yes      | ID of broadcast message to update. |
-| `message`   | string   | no       | Message to display.                |
-| `starts_at` | datetime | no       | Starting time.                     |
-| `ends_at`   | datetime | no       | Ending time.                       |
-| `color`     | string   | no       | Background color hex code.         |
-| `font`      | string   | no       | Foreground color hex code.         |
-| `target_path`| string  | no       | Target path of the broadcast message. |
-| `broadcast_type`| string | no     | Appearance type (defaults to banner) |
+| Attribute       | Type     | Required | Description                           |
+|:----------------|:---------|:---------|:--------------------------------------|
+| `id`            | integer  | yes      | ID of broadcast message to update.    |
+| `message`       | string   | no       | Message to display.                   |
+| `starts_at`     | datetime | no       | Starting time.                        |
+| `ends_at`       | datetime | no       | Ending time.                          |
+| `color`         | string   | no       | Background color hex code.            |
+| `font`          | string   | no       | Foreground color hex code.            |
+| `target_path`   | string   | no       | Target path of the broadcast message. |
+| `broadcast_type`| string   | no       | Appearance type (defaults to banner)  |
+| `dismissable`   | boolean  | no       | Can the user dismiss the message?     |
 
 Example request:
 
@@ -159,6 +164,7 @@ Example response:
     "active": true,
     "target_path": "*/welcome",
     "broadcast_type": "notification",
+    "dismissable": false
 }
 ```
 

doc/user/admin_area/broadcast_messages.md

@@ -28,7 +28,7 @@ To add a broadcast message:
 
 NOTE: **Note:**
 Once a broadcast message has expired, it is no longer displayed in the UI but is still listed in the
-list of broadcast messages.
+list of broadcast messages. User can also dismiss a broadcast message if the option **Dismissable** is set.
 
 ## Editing a broadcast message
 

lib/api/broadcast_messages.rb

@@ -36,6 +36,7 @@ module API
         optional :font, type: String, desc: 'Foreground color'
         optional :target_path, type: String, desc: 'Target path'
         optional :broadcast_type, type: String, values: BroadcastMessage.broadcast_types.keys, desc: 'Broadcast type. Defaults to banner', default: -> { 'banner' }
+        optional :dismissable, type: Boolean, desc: 'Is dismissable'
       end
       post do
         authenticated_as_admin!
@@ -75,6 +76,7 @@ module API
         optional :font, type: String, desc: 'Foreground color'
         optional :target_path, type: String, desc: 'Target path'
         optional :broadcast_type, type: String, values: BroadcastMessage.broadcast_types.keys, desc: 'Broadcast Type'
+        optional :dismissable, type: Boolean, desc: 'Is dismissable'
       end
       put ':id' do
         authenticated_as_admin!

lib/api/entities/broadcast_message.rb

@@ -3,7 +3,7 @@
 module API
   module Entities
     class BroadcastMessage < Grape::Entity
-      expose :id, :message, :starts_at, :ends_at, :color, :font, :target_path, :broadcast_type
+      expose :id, :message, :starts_at, :ends_at, :color, :font, :target_path, :broadcast_type, :dismissable
       expose :active?, as: :active
     end
   end

locale/gitlab.pot

@@ -1666,6 +1666,9 @@ msgstr ""
 msgid "Allow this secondary node to replicate content on Object Storage"
 msgstr ""
 
+msgid "Allow users to dismiss the broadcast message"
+msgstr ""
+
 msgid "Allow users to register any application to use GitLab as an OAuth provider"
 msgstr ""
 
@@ -6910,6 +6913,9 @@ msgstr ""
 msgid "Dismiss trial promotion"
 msgstr ""
 
+msgid "Dismissable"
+msgstr ""
+
 msgid "Dismissed"
 msgstr ""
 

spec/features/broadcast_messages_spec.rb

@@ -3,28 +3,58 @@
 require 'spec_helper'
 
 describe 'Broadcast Messages' do
-  let!(:broadcast_message) { create(:broadcast_message, broadcast_type: 'notification', message: 'SampleMessage') }
+  shared_examples 'a Broadcast Messages' do
+    it 'shows broadcast message' do
+      visit root_path
 
-  it 'shows broadcast message' do
-    visit root_path
+      expect(page).to have_content 'SampleMessage'
+    end
+  end
+
+  shared_examples 'a dismissable Broadcast Messages' do
+    it 'hides broadcast message after dismiss', :js do
+      visit root_path
+
+      find('.js-dismiss-current-broadcast-notification').click
+
+      expect(page).not_to have_content 'SampleMessage'
+    end
+
+    it 'broadcast message is still hidden after refresh', :js do
+      visit root_path
+
+      find('.js-dismiss-current-broadcast-notification').click
+      visit root_path
+
+      expect(page).not_to have_content 'SampleMessage'
+    end
+  end
+
+  describe 'banner type' do
+    let!(:broadcast_message) { create(:broadcast_message, message: 'SampleMessage') }
+
+    it_behaves_like 'a Broadcast Messages'
+
+    it 'shows broadcast message' do
+      visit root_path
 
-    expect(page).to have_content 'SampleMessage'
+      expect(page).not_to have_selector('.js-dismiss-current-broadcast-notification')
+    end
   end
 
-  it 'hides broadcast message after dismiss', :js do
-    visit root_path
+  describe 'dismissable banner type' do
+    let!(:broadcast_message) { create(:broadcast_message, dismissable: true, message: 'SampleMessage') }
 
-    find('.js-dismiss-current-broadcast-notification').click
+    it_behaves_like 'a Broadcast Messages'
 
-    expect(page).not_to have_content 'SampleMessage'
+    it_behaves_like 'a dismissable Broadcast Messages'
   end
 
-  it 'broadcast message is still hidden after refresh', :js do
-    visit root_path
+  describe 'notification type' do
+    let!(:broadcast_message) { create(:broadcast_message, broadcast_type: 'notification', message: 'SampleMessage') }
 
-    find('.js-dismiss-current-broadcast-notification').click
-    visit root_path
+    it_behaves_like 'a Broadcast Messages'
 
-    expect(page).not_to have_content 'SampleMessage'
+    it_behaves_like 'a dismissable Broadcast Messages'
   end
 end

spec/helpers/broadcast_messages_helper_spec.rb

@@ -14,7 +14,7 @@ describe BroadcastMessagesHelper do
 
       context 'when last broadcast message is hidden' do
         before do
-          helper.request.cookies["hide_broadcast_notification_message_#{broadcast_message_2.id}"] = 'true'
+          helper.request.cookies["hide_broadcast_message_#{broadcast_message_2.id}"] = 'true'
         end
 
         it { is_expected.to eq broadcast_message_1 }
@@ -29,6 +29,10 @@ describe BroadcastMessagesHelper do
   describe 'broadcast_message' do
     let(:current_broadcast_message) { BroadcastMessage.new(message: 'Current Message') }
 
+    before do
+      allow(helper).to receive(:current_user).and_return(create(:user))
+    end
+
     it 'returns nil when no current message' do
       expect(helper.broadcast_message(nil)).to be_nil
     end

spec/requests/api/broadcast_messages_spec.rb

@@ -17,7 +17,7 @@ describe API::BroadcastMessages do
       expect(response).to include_pagination_headers
       expect(json_response).to be_kind_of(Array)
       expect(json_response.first.keys)
-        .to match_array(%w(id message starts_at ends_at color font active target_path broadcast_type))
+        .to match_array(%w(id message starts_at ends_at color font active target_path broadcast_type dismissable))
     end
   end
 
@@ -28,7 +28,7 @@ describe API::BroadcastMessages do
       expect(response).to have_gitlab_http_status(:ok)
       expect(json_response['id']).to eq message.id
       expect(json_response.keys)
-        .to match_array(%w(id message starts_at ends_at color font active target_path broadcast_type))
+        .to match_array(%w(id message starts_at ends_at color font active target_path broadcast_type dismissable))
     end
   end
 
@@ -111,6 +111,15 @@ describe API::BroadcastMessages do
 
         expect(response).to have_gitlab_http_status(:bad_request)
       end
+
+      it 'accepts an active dismissable value ' do
+        attrs = { message: 'new message', dismissable: true }
+
+        post api('/broadcast_messages', admin), params: attrs
+
+        expect(response).to have_gitlab_http_status(:created)
+        expect(json_response['dismissable']).to eq true
+      end
     end
   end
 
@@ -187,6 +196,15 @@ describe API::BroadcastMessages do
 
         expect(response).to have_gitlab_http_status(:bad_request)
       end
+
+      it 'accepts a new dismissable value ' do
+        attrs = { message: 'new message', dismissable: true }
+
+        put api("/broadcast_messages/#{message.id}", admin), params: attrs
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['dismissable']).to eq true
+      end
     end
   end