Merge branch 'rs-broadcasts' into 'master'

Allow broadcast messages to be edited

Closes #3046 

See merge request !2268

CHANGELOG

@@ -43,6 +43,7 @@ v 8.4.0 (unreleased)
   - API: Add support for deleting a tag via the API (Robert Schilling)
   - Allow subsequent validations in CI Linter
   - Fix Encoding::CompatibilityError bug when markdown content has some complex URL (Jason Lee)
+  - Allow broadcast messages to be edited
 v 8.3.4
   - Use gitlab-workhorse 0.5.4 (fixes API routing bug)

app/assets/javascripts/admin.js.coffee

@@ -10,19 +10,19 @@ class @Admin
     $('body').on 'click', '.js-toggle-colors-link', (e) ->
       e.preventDefault()
-      $('.js-toggle-colors-link').hide()
-      $('.js-toggle-colors-container').show()
+      $('.js-toggle-colors-container').toggle()
     $('input#broadcast_message_color').on 'input', ->
-      previewColor = $('input#broadcast_message_color').val()
+      previewColor = $(@).val()
       $('div.broadcast-message-preview').css('background-color', previewColor)
     $('input#broadcast_message_font').on 'input', ->
-      previewColor = $('input#broadcast_message_font').val()
+      previewColor = $(@).val()
       $('div.broadcast-message-preview').css('color', previewColor)
     $('textarea#broadcast_message_message').on 'input', ->
-      previewMessage = $('textarea#broadcast_message_message').val()
+      previewMessage = $(@).val()
+      previewMessage = "Your message here" if previewMessage.trim() == ''
       $('div.broadcast-message-preview span').text(previewMessage)
     $('.log-tabs a').click (e) ->

app/assets/stylesheets/framework/forms.scss

@@ -78,6 +78,10 @@ label {
   padding: 8px $gl-padding;
 }
+.form-control-inline {
+  display: inline;
+}
+
 .wiki-content {
   margin-top: 35px;
 }

app/controllers/admin/broadcast_messages_controller.rb

 class Admin::BroadcastMessagesController < Admin::ApplicationController
-  before_action :broadcast_messages
+  before_action :finder, only: [:edit, :update, :destroy]
   def index
-    @broadcast_message = BroadcastMessage.new
+    @broadcast_messages = BroadcastMessage.reorder("starts_at ASC").page(params[:page])
+    @broadcast_message  = BroadcastMessage.new
+  end
+
+  def edit
   end
   def create
@@ -15,8 +19,16 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController
     end
   end
+  def update
+    if @broadcast_message.update(broadcast_message_params)
+      redirect_to admin_broadcast_messages_path, notice: 'Broadcast Message was successfully updated.'
+    else
+      render :edit
+    end
+  end
+
   def destroy
-    BroadcastMessage.find(params[:id]).destroy
+    @broadcast_message.destroy
     respond_to do |format|
       format.html { redirect_back_or_default(default: { action: 'index' }) }
@@ -26,14 +38,17 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController
   protected
-  def broadcast_messages
-    @broadcast_messages ||= BroadcastMessage.order("starts_at DESC").page(params[:page])
+  def finder
+    @broadcast_message = BroadcastMessage.find(params[:id])
   end
   def broadcast_message_params
-    params.require(:broadcast_message).permit(
-      :alert_type, :color, :ends_at, :font,
-      :message, :starts_at
-    )
+    params.require(:broadcast_message).permit(%i(
+      color
+      ends_at
+      font
+      message
+      starts_at
+    ))
   end
 end

app/helpers/application_helper.rb

@@ -181,10 +181,6 @@ module ApplicationHelper
     end
   end
-  def broadcast_message
-    BroadcastMessage.current
-  end
-
   # Render a `time` element with Javascript-based relative date and tooltip
   #
   # time       - Time object

app/helpers/broadcast_messages_helper.rb

 module BroadcastMessagesHelper
-  def broadcast_styling(broadcast_message)
-    styling = ''
+  def broadcast_message(message = BroadcastMessage.current)
+    return unless message.present?
+
+    content_tag :div, class: 'broadcast-message', style: broadcast_message_style(message) do
+      icon('bullhorn') << ' ' << message.message
+    end
+  end
+
+  def broadcast_message_style(broadcast_message)
+    style = ''
     if broadcast_message.color.present?
-      styling << "background-color: #{broadcast_message.color}"
-      styling << '; ' if broadcast_message.font.present?
+      style << "background-color: #{broadcast_message.color}"
+      style << '; ' if broadcast_message.font.present?
     end
     if broadcast_message.font.present?
-      styling << "color: #{broadcast_message.font}"
+      style << "color: #{broadcast_message.font}"
     end
-    styling
+    style
+  end
+
+  def broadcast_message_status(broadcast_message)
+    if broadcast_message.active?
+      'Active'
+    elsif broadcast_message.ended?
+      'Expired'
+    else
+      'Pending'
+    end
   end
 end

app/models/broadcast_message.rb

@@ -6,7 +6,6 @@
 #  message    :text             not null
 #  starts_at  :datetime
 #  ends_at    :datetime
-#  alert_type :integer
 #  created_at :datetime
 #  updated_at :datetime
 #  color      :string(255)
@@ -23,7 +22,22 @@ class BroadcastMessage < ActiveRecord::Base
   validates :color, allow_blank: true, color: true
   validates :font,  allow_blank: true, color: true
+  default_value_for :color, '#E75E40'
+  default_value_for :font,  '#FFFFFF'
+
   def self.current
-    where("ends_at > :now AND starts_at < :now", now: Time.zone.now).last
+    where("ends_at > :now AND starts_at <= :now", now: Time.zone.now).last
+  end
+
+  def active?
+    started? && !ended?
+  end
+
+  def started?
+    Time.zone.now >= starts_at
+  end
+
+  def ended?
+    ends_at < Time.zone.now
   end
 end

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

+.broadcast-message-preview{ style: broadcast_message_style(@broadcast_message) }
+  = icon('bullhorn')
+  %span= @broadcast_message.message || "Your message here"
+
+= form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal js-requires-input'} do |f|
+  -if @broadcast_message.errors.any?
+    .alert.alert-danger
+      - @broadcast_message.errors.full_messages.each do |msg|
+        %p= msg
+  .form-group
+    = f.label :message, class: 'control-label'
+    .col-sm-10
+      = f.text_area :message, class: "form-control js-quick-submit", rows: 2, required: true
+  .form-group.js-toggle-colors-container
+    .col-sm-10.col-sm-offset-2
+      = link_to 'Customize colors', '#', class: 'js-toggle-colors-link'
+  .form-group.js-toggle-colors-container.hide
+    = f.label :color, "Background Color", class: 'control-label'
+    .col-sm-10
+      = f.color_field :color, class: "form-control"
+  .form-group.js-toggle-colors-container.hide
+    = f.label :font, "Font Color", class: 'control-label'
+    .col-sm-10
+      = f.color_field :font, class: "form-control"
+  .form-group
+    = f.label :starts_at, class: 'control-label'
+    .col-sm-10.datetime-controls
+      = f.datetime_select :starts_at, {}, class: 'form-control form-control-inline'
+  .form-group
+    = f.label :ends_at, class: 'control-label'
+    .col-sm-10.datetime-controls
+      = f.datetime_select :ends_at, {}, class: 'form-control form-control-inline'
+  .form-actions
+    - if @broadcast_message.persisted?
+      = f.submit "Update broadcast message", class: "btn btn-create"
+    - else
+      = f.submit "Add broadcast message", class: "btn btn-create"

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

+- page_title "Broadcast Messages"
+
+= render 'form'

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

 - page_title "Broadcast Messages"
+
 %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.
-.broadcast-message-preview
-  %i.fa.fa-bullhorn
-  %span Your message here
-
-= form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal'} do |f|
-  -if @broadcast_message.errors.any?
-    .alert.alert-danger
-      - @broadcast_message.errors.full_messages.each do |msg|
-        %p= msg
-  .form-group
-    = f.label :message, class: 'control-label'
-    .col-sm-10
-      = f.text_area :message, class: "form-control", rows: 2, required: true
-      %div
-        = link_to '#', class: 'js-toggle-colors-link' do
-          Customize colors
-  .form-group.js-toggle-colors-container.hide
-    = f.label :color, "Background Color", class: 'control-label'
-    .col-sm-10
-      = f.color_field :color, value: "#eb9532", class: "form-control"
-  .form-group.js-toggle-colors-container.hide
-    = f.label :font, "Font Color", class: 'control-label'
-    .col-sm-10
-      = f.color_field :font, value: "#FFFFFF", class: "form-control"
-  .form-group
-    = f.label :starts_at, class: 'control-label'
-    .col-sm-10.datetime-controls
-      = f.datetime_select :starts_at
-  .form-group
-    = f.label :ends_at, class: 'control-label'
-    .col-sm-10.datetime-controls
-      = f.datetime_select :ends_at
-  .form-actions
-    = f.submit "Add broadcast message", class: "btn btn-create"
--if @broadcast_messages.any?
-  %ul.bordered-list.broadcast-messages
-    - @broadcast_messages.each do |broadcast_message|
-      %li
-        .pull-right
-          - if broadcast_message.starts_at
-            %strong
-              #{broadcast_message.starts_at.to_s(:short)}
-            \...
-          - if broadcast_message.ends_at
-            %strong
-              #{broadcast_message.ends_at.to_s(:short)}
-           
-          = link_to [:admin, broadcast_message], method: :delete, remote: true, class: 'remove-row btn btn-xs' do
-            %i.fa.fa-times.cred
-        .message= broadcast_message.message
+  Broadcast messages are displayed for every user and can be used to notify
+  users about scheduled maintenance, recent upgrades and more.
+= render 'form'
+%br.clearfix
+-if @broadcast_messages.any?
+  %table.table
+    %thead
+      %tr
+        %th Status
+        %th Preview
+        %th Starts
+        %th Ends
+        %th  
+    %tbody
+      - @broadcast_messages.each do |message|
+        %tr
+          %td
+            = broadcast_message_status(message)
+          %td
+            = broadcast_message(message)
+          %td
+            = message.starts_at
+          %td
+            = message.ends_at
+          %td
+            = link_to icon('pencil-square-o'), edit_admin_broadcast_message_path(message), title: 'Edit', class: 'btn btn-xs'
+            = link_to icon('times'), admin_broadcast_message_path(message), method: :delete, remote: true, title: 'Remove', class: 'js-remove-tr btn btn-xs btn-danger'
   = paginate @broadcast_messages

app/views/layouts/_broadcast.html.haml

-- if broadcast_message.present?
-  .broadcast-message{ style: broadcast_styling(broadcast_message) }
-    %i.fa.fa-bullhorn
-    = broadcast_message.message
+= broadcast_message

config/routes.rb

@@ -219,7 +219,7 @@ Rails.application.routes.draw do
       get :test
     end
-    resources :broadcast_messages, only: [:index, :create, :destroy]
+    resources :broadcast_messages, only: [:index, :edit, :create, :update, :destroy]
     resource :logs, only: [:show]
     resource :background_jobs, controller: 'background_jobs', only: [:show]

db/migrate/20151231202530_remove_alert_type_from_broadcast_messages.rb

+class RemoveAlertTypeFromBroadcastMessages < ActiveRecord::Migration
+  def change
+    remove_column :broadcast_messages, :alert_type, :integer
+  end
+end

db/schema.rb

@@ -82,7 +82,6 @@ ActiveRecord::Schema.define(version: 20160113111034) do
     t.text     "message",    null: false
     t.datetime "starts_at"
     t.datetime "ends_at"
-    t.integer  "alert_type"
     t.datetime "created_at"
     t.datetime "updated_at"
     t.string   "color"

features/admin/broadcast_messages.feature

@@ -2,16 +2,11 @@
 Feature: Admin Broadcast Messages
   Background:
     Given I sign in as an admin
-    And application already has admin messages
+    And application already has a broadcast message
     And I visit admin messages page
   Scenario: See broadcast messages list
-    Then I should be all broadcast messages
-
-  Scenario: Create a broadcast message
-    When submit form with new broadcast message
-    Then I should be redirected to admin messages page
-    And I should see newly created broadcast message
+    Then I should see all broadcast messages
   Scenario: Create a customized broadcast message
     When submit form with new customized broadcast message
@@ -19,3 +14,14 @@ Feature: Admin Broadcast Messages
     And I should see newly created broadcast message
     Then I visit dashboard page
     And I should see a customized broadcast message
+
+  Scenario: Edit an existing broadcast message
+    When I edit an existing broadcast message
+    And I change the broadcast message text
+    Then I should be redirected to admin messages page
+    And I should see the updated broadcast message
+
+  Scenario: Remove an existing broadcast message
+    When I remove an existing broadcast message
+    Then I should be redirected to admin messages page
+    And I should not see the removed broadcast message

features/steps/admin/broadcast_messages.rb

 class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps
   include SharedAuthentication
   include SharedPaths
-  include SharedAdmin
-  step 'application already has admin messages' do
-    FactoryGirl.create(:broadcast_message, message: "Migration to new server")
+  step 'application already has a broadcast message' do
+    FactoryGirl.create(:broadcast_message, :expired, message: "Migration to new server")
   end
-  step 'I should be all broadcast messages' do
+  step 'I should see all broadcast messages' do
     expect(page).to have_content "Migration to new server"
   end
-  step 'submit form with new broadcast message' do
-    fill_in 'broadcast_message_message', with: 'Application update from 4:00 CST to 5:00 CST'
-    select '2018', from: "broadcast_message_ends_at_1i"
-    click_button "Add broadcast message"
-  end
-
   step 'I should be redirected to admin messages page' do
     expect(current_path).to eq admin_broadcast_messages_path
   end
@@ -27,10 +20,9 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps
   step 'submit form with new customized broadcast message' do
     fill_in 'broadcast_message_message', with: 'Application update from 4:00 CST to 5:00 CST'
-    click_link "Customize colors"
     fill_in 'broadcast_message_color', with: '#f2dede'
     fill_in 'broadcast_message_font', with: '#b94a48'
-    select '2018', from: "broadcast_message_ends_at_1i"
+    select Date.today.next_year.year, from: "broadcast_message_ends_at_1i"
     click_button "Add broadcast message"
   end
@@ -38,4 +30,25 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps
     expect(page).to have_content 'Application update from 4:00 CST to 5:00 CST'
     expect(page).to have_selector %(div[style="background-color: #f2dede; color: #b94a48"])
   end
+
+  step 'I edit an existing broadcast message' do
+    click_link 'Edit'
+  end
+
+  step 'I change the broadcast message text' do
+    fill_in 'broadcast_message_message', with: 'Application update RIGHT NOW'
+    click_button 'Update broadcast message'
+  end
+
+  step 'I should see the updated broadcast message' do
+    expect(page).to have_content "Application update RIGHT NOW"
+  end
+
+  step 'I remove an existing broadcast message' do
+    click_link 'Remove'
+  end
+
+  step 'I should not see the removed broadcast message' do
+    expect(page).not_to have_content 'Migration to new server'
+  end
 end

spec/factories/broadcast_messages.rb

@@ -6,7 +6,6 @@
 #  message    :text             not null
 #  starts_at  :datetime
 #  ends_at    :datetime
-#  alert_type :integer
 #  created_at :datetime
 #  updated_at :datetime
 #  color      :string(255)
@@ -18,10 +17,17 @@
 FactoryGirl.define do
   factory :broadcast_message do
     message "MyText"
-    starts_at "2013-11-12 13:43:25"
-    ends_at "2013-11-12 13:43:25"
-    alert_type 1
-    color "#555555"
-    font "#BBBBBB"
+    starts_at Date.today
+    ends_at Date.tomorrow
+
+    trait :expired do
+      starts_at 5.days.ago
+      ends_at 3.days.ago
+    end
+
+    trait :future do
+      starts_at 5.days.from_now
+      ends_at 6.days.from_now
+    end
   end
 end

spec/helpers/broadcast_messages_helper_spec.rb

 require 'spec_helper'
 describe BroadcastMessagesHelper do
-  describe 'broadcast_styling' do
-    let(:broadcast_message) { double(color: '', font: '') }
-    context "default style" do
-      it "should have no style" do
-        expect(broadcast_styling(broadcast_message)).to eq ''
-      end
+  describe 'broadcast_message' do
+    it 'returns nil when no current message' do
+      expect(helper.broadcast_message(nil)).to be_nil
+    end
+
+    it 'includes the current message' do
+      current = double(message: 'Current Message')
+
+      allow(helper).to receive(:broadcast_message_style).and_return(nil)
+
+      expect(helper.broadcast_message(current)).to include 'Current Message'
+    end
+
+    it 'includes custom style' do
+      current = double(message: 'Current Message')
+
+      allow(helper).to receive(:broadcast_message_style).and_return('foo')
+
+      expect(helper.broadcast_message(current)).to include 'style="foo"'
+    end
+  end
+
+  describe 'broadcast_message_style' do
+    it 'defaults to no style' do
+      broadcast_message = spy
+
+      expect(helper.broadcast_message_style(broadcast_message)).to eq ''
+    end
+
+    it 'allows custom style' do
+      broadcast_message = double(color: '#f2dede', font: '#b94a48')
+
+      expect(helper.broadcast_message_style(broadcast_message)).
+        to match('background-color: #f2dede; color: #b94a48')
+    end
+  end
+
+  describe 'broadcast_message_status' do
+    it 'returns Active' do
+      message = build(:broadcast_message)
+
+      expect(helper.broadcast_message_status(message)).to eq 'Active'
+    end
+
+    it 'returns Expired' do
+      message = build(:broadcast_message, :expired)
+      expect(helper.broadcast_message_status(message)).to eq 'Expired'
     end
-    context "customized style" do
-      let(:broadcast_message) { double(color: "#f2dede", font: '#b94a48') }
-      it "should have a customized style" do
-        expect(broadcast_styling(broadcast_message)).
-          to match('background-color: #f2dede; color: #b94a48')
-      end
+    it 'returns Pending' do
+      message = build(:broadcast_message, :future)
+      expect(helper.broadcast_message_status(message)).to eq 'Pending'
     end
   end
 end

spec/models/broadcast_message_spec.rb

@@ -6,7 +6,6 @@
 #  message    :text             not null
 #  starts_at  :datetime
 #  ends_at    :datetime
-#  alert_type :integer
 #  created_at :datetime
 #  updated_at :datetime
 #  color      :string(255)
@@ -16,6 +15,8 @@
 require 'spec_helper'
 describe BroadcastMessage, models: true do
+  include ActiveSupport::Testing::TimeHelpers
+
   subject { create(:broadcast_message) }
   it { is_expected.to be_valid }
@@ -35,20 +36,79 @@ describe BroadcastMessage, models: true do
     it { is_expected.not_to allow_value('000').for(:font) }
   end
-  describe :current do
+  describe '.current' do
     it "should return last message if time match" do
-      broadcast_message = create(:broadcast_message, starts_at: Time.now.yesterday, ends_at: Time.now.tomorrow)
-      expect(BroadcastMessage.current).to eq(broadcast_message)
+      message = create(:broadcast_message)
+
+      expect(BroadcastMessage.current).to eq message
     end
     it "should return nil if time not come" do
-      create(:broadcast_message, starts_at: Time.now.tomorrow, ends_at: Time.now + 2.days)
+      create(:broadcast_message, :future)
+
       expect(BroadcastMessage.current).to be_nil
     end
     it "should return nil if time has passed" do
-      create(:broadcast_message, starts_at: Time.now - 2.days, ends_at: Time.now.yesterday)
+      create(:broadcast_message, :expired)
+
       expect(BroadcastMessage.current).to be_nil
     end
   end
+
+  describe '#active?' do
+    it 'is truthy when started and not ended' do
+      message = build(:broadcast_message)
+
+      expect(message).to be_active
+    end
+
+    it 'is falsey when ended' do
+      message = build(:broadcast_message, :expired)
+
+      expect(message).not_to be_active
+    end
+
+    it 'is falsey when not started' do
+      message = build(:broadcast_message, :future)
+
+      expect(message).not_to be_active
+    end
+  end
+
+  describe '#started?' do
+    it 'is truthy when starts_at has passed' do
+      message = build(:broadcast_message)
+
+      travel_to(3.days.from_now) do
+        expect(message).to be_started
+      end
+    end
+
+    it 'is falsey when starts_at is in the future' do
+      message = build(:broadcast_message)
+
+      travel_to(3.days.ago) do
+        expect(message).not_to be_started
+      end
+    end
+  end
+
+  describe '#ended?' do
+    it 'is truthy when ends_at has passed' do
+      message = build(:broadcast_message)
+
+      travel_to(3.days.from_now) do
+        expect(message).to be_ended
+      end
+    end
+
+    it 'is falsey when ends_at is in the future' do
+      message = build(:broadcast_message)
+
+      travel_to(3.days.ago) do
+        expect(message).not_to be_ended
+      end
+    end
+  end
 end