Enabled to edit global banner by non-admin. (#124)

New feature: Enabled to edit global banner by non-admin.
Code refactoring.

.rubocop.yml

@@ -1,4 +1,5 @@
 inherit_from: .rubocop_todo.yml
+
 AllCops:
   TargetRubyVersion: 2.3
 

.rubocop_todo.yml

@@ -12,6 +12,7 @@ Metrics/BlockLength:
     - '**/*.rake'
     - 'spec/**/*.rb'
     - init.rb
+    - config/routes.rb
 
 # Offense count: 3
 Metrics/AbcSize:
@@ -23,7 +24,7 @@ Metrics/ClassLength:
   Max: 200
 
 # "Line is too long"を無効
-Metrics/LineLength:
+Layout/LineLength:
   Enabled: false
 
 # Offense count: 1
@@ -46,4 +47,3 @@ Documentation:
 EndOfLine:
   Enabled: false
 
-

README.md

@@ -160,16 +160,17 @@ Please try:
 
 ```bash
 # Admin password is 'redmine_banner_commit_sha'
-$ https://github.com/akiko-pusu/redmine_banner
-$ docker-compose up web -d
+% git clone https://github.com/akiko-pusu/redmine_banner
+% cd redmine_banner
+% docker-compose up web -d
 
 # or
 #
 # Admin password is 'redmine_banner_{COMMIT}'
-$ docker build --build-arg=COMMIT=$(git rev-parse --short HEAD) \
+% docker build --build-arg=COMMIT=$(git rev-parse --short HEAD) \
   --build-arg=BRANCH=$(git name-rev --name-only HEAD) -t akiko/redmine_banner:latest .
 
-$ docker run -p 3000:3000 akiko/redmine_banner:latest
+% docker run -p 3000:3000 akiko/redmine_banner:latest
 ```
 
 ### Run test
@@ -181,8 +182,8 @@ Please see wercker.yml for more details.
 % cp plugins/redmine_banner/Gemfile.local plugins/redmine_banner/Gemfile
 % bundle install --with test
 % export RAILS_ENV=test
-% bundle exec ruby -I"lib:test" -I plugins/redmine_banner/test plugins/ \
-  redmine_banner/test/controller/global_banner_controller_test.rb
+% bundle exec ruby -I"lib:test" -I plugins/redmine_banner/test \
+  plugins/redmine_banner/test/functional/banner_controller_test.rb
 ```
 
 or

app/controllers/banner_controller.rb

@@ -1,13 +1,15 @@
+# frozen_string_literal: true
+
 class BannerController < ApplicationController
   #
   # NOTE: Authorized user can turn off banner while their in session. (Changed from version 0.0.9)
   #       If Administrator hope to disable site wide banner, please go to settings page and uncheck
   #       eabned checkbox.
   before_action :require_login, only: [:off]
-  before_action :find_user, :find_project, :authorize, except: [:preview, :off]
+  before_action :find_user, :find_project, :authorize, except: %i[preview off]
 
   def preview
-    @text = params[:settings][:banner_description]
+    @text = params[:setting][:banner_description]
     render partial: 'common/preview'
   end
 
@@ -17,8 +19,8 @@ def preview
   def off
     session[:pref_banner_off] = Time.now.to_i
     render action: '_off', layout: false
-  rescue => e
-    logger.warn("Message for the log file / When off banner #{e.message}")
+  rescue StandardError => e
+    logger&.warn("Message for the log file / When off banner #{e.message}")
     render text: ''
   end
 
@@ -30,7 +32,7 @@ def project_banner_off
   end
 
   def edit
-    return if params[:settings].nil?
+    return if params[:setting].nil?
 
     @banner = Banner.find_or_create(@project.id)
     @banner.safe_attributes = banner_params
@@ -53,6 +55,6 @@ def find_project
   end
 
   def banner_params
-    params.require(:settings).permit('banner_description', 'style', 'start_date', 'end_date', 'enabled', 'use_timer', 'display_part')
+    params.require(:setting).permit('banner_description', 'style', 'start_date', 'end_date', 'enabled', 'use_timer', 'display_part')
   end
 end

app/controllers/banners/api/global_banner_controller.rb

@@ -3,9 +3,6 @@
 module Banners
   module Api
     class GlobalBannerController < ApplicationController
-      # TODO: This group should be customize.
-      GLOBAL_BANNER_ADMIN_GROUP = 'GlobalBanner_Admin'
-
       before_action :require_login, :require_banner_admin
       accept_api_auth :show, :register_banner
 
@@ -20,10 +17,11 @@ def register_banner
           response_bad_request(e.message) && (return)
         end
 
-        retval = Setting.send('plugin_redmine_banner=', global_banner_params.stringify_keys)
+        global_banner = GlobalBanner.find_or_default
+        global_banner.merge_value(global_banner_params.stringify_keys)
 
-        if retval
-          render status: 200, json: { status: 'OK', message: 'updating the global banner' }
+        if global_banner.save
+          render status: :ok, json: { status: 'OK', message: 'updating the global banner' }
         else
           response_bad_request("Can't save data to settings table.")
         end
@@ -33,35 +31,41 @@ def register_banner
 
       private
 
-      # TODO: Private methods should be refactoring.
-      # TODO: Validation is required
       def build_params
-        valid_params(params)
+        keys = GlobalBanner::GLOBAL_BANNER_DEFAULT_SETTING.stringify_keys.keys
+
+        unless User.current.admin?
+          keys.delete('banner_admin')
+        end
+
+        params.require(:global_banner).permit(keys)
       end
 
       def global_banner_json
-        { global_banner: Setting['plugin_redmine_banner'] }
+        { global_banner: GlobalBanner.find_or_default.value }
       end
 
       # 400 Bad Request
       def response_bad_request(error_message = nil)
         json_data = { status: 400, message: 'Bad Request' }
-        json_data.merge!(reason: error_message) if error_message.present?
-        logger.warn("Global Banner Update failed. Caused: #{json_data}")
-        render status: 400, json: json_data
+        json_data[:reason] = error_message if error_message.present?
+        logger&.warn("Global Banner Update failed. Caused: #{json_data}")
+
+        render status: :bad_request, json: json_data
       end
 
       # 401 Unauthorized
       def response_unauthorized
-        render status: 401, json: { status: 401, message: 'Unauthorized' }
+        render status: :unauthorized, json: { status: 401, message: 'Unauthorized' }
       end
 
       # 500 Internal Server Error
       def response_internal_server_error(error_message = nil)
         json_data = { status: 500, message: 'Internal Server Error' }
-        json_data.merge!(reason: error_message) if error_message.present?
-        logger.warn("Global Banner Update failed. Caused: #{json_data}")
-        render status: 500, json: json_data
+        json_data[:reason] = error_message if error_message.present?
+        logger&.warn("Global Banner Update failed. Caused: #{json_data}")
+
+        render status: :internal_server_error, json: json_data
       end
 
       def require_banner_admin
@@ -71,23 +75,7 @@ def require_banner_admin
       end
 
       def banner_admin?(user)
-        banner_admin_group = Group.find_by_lastname(GLOBAL_BANNER_ADMIN_GROUP)
-        return false if banner_admin_group.blank?
-
-        banner_admin_group.users.include?(user)
-      end
-
-      def valid_params(params)
-        params.require(:global_banner).permit(
-          :banner_description,
-          :display_part,
-          :enable,
-          :end_hour, :end_min, :end_ymd,
-          :related_link,
-          :start_hour, :start_min, :start_ymd,
-          :type,
-          :use_timer
-        )
+        GlobalBanner.banner_admin?(user)
       end
     end
   end

app/controllers/global_banner_controller.rb

@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+class GlobalBannerController < ApplicationController
+  before_action :require_login, :require_banner_admin
+
+  def show
+    global_banner = GlobalBanner.find_or_default
+    render layout: !request.xhr?, locals: setting_values(global_banner)
+  end
+
+  def update
+    global_banner = GlobalBanner.find_or_default
+
+    begin
+      global_banner_params = build_params
+    rescue ActionController::ParameterMissing, ActionController::UnpermittedParameters => e
+      response_bad_request(e.message) && (return)
+    end
+
+    global_banner.merge_value(global_banner_params.stringify_keys)
+
+    unless global_banner.valid_date_range? && global_banner.save
+      flash[:error] = l(:error_banner_date_range)
+      redirect_to action: 'show'
+      return
+    end
+
+    flash[:notice] = l(:notice_successful_update)
+    redirect_to action: 'show'
+    nil
+  end
+
+  private
+
+  def require_banner_admin
+    return if User.current.admin? || GlobalBanner.banner_admin?(User.current)
+
+    render_403
+    false
+  end
+
+  def build_params
+    keys = GlobalBanner::GLOBAL_BANNER_DEFAULT_SETTING.stringify_keys.keys
+
+    unless User.current.admin?
+      keys.delete('banner_admin')
+    end
+
+    params.require(:setting).permit(keys)
+  end
+
+  def setting_values(instance)
+    setting = instance.value
+
+    current_time = Time.zone.now
+    use_timer = instance.use_timer?
+
+    begin
+      # date range check
+      start_datetime = instance.start_time
+      end_datetime = instance.end_time
+    rescue ArgumentError
+      # Ref. https://github.com/akiko-pusu/redmine_banner/issues/11
+      start_datetime = current_time
+      end_datetime = current_time
+      use_timer = false
+    end
+
+    banner_description = setting[:banner_description]
+
+    if banner_description.respond_to?(:force_encoding)
+      setting[:banner_description] = banner_description.force_encoding('UTF-8')
+    end
+
+    {
+      setting: setting.stringify_keys,
+      start_datetime: start_datetime,
+      end_datetime: end_datetime,
+      use_timer: use_timer,
+      banner_updated_on: instance&.updated_on&.localtime
+    }
+  end
+end

app/models/banner.rb

@@ -1,17 +1,19 @@
+# frozen_string_literal: true
+
 class Banner < ActiveRecord::Base
   include Redmine::SafeAttributes
   belongs_to :project
 
-  validates_uniqueness_of :project_id
+  validates :project_id, uniqueness: true
   validates :project_id, presence: true
-  validates_inclusion_of :display_part, in: %w[all new_issue overview overview_and_issues]
-  validates_inclusion_of :style, in: %w[info warn alert normal nodata]
+  validates :display_part, inclusion: { in: %w[all new_issue overview overview_and_issues] }
+  validates :style, inclusion: { in: %w[info warn alert normal nodata] }
   # project should be stable.
   safe_attributes 'banner_description', 'style', 'start_date', 'end_date', 'enabled', 'use_timer', 'display_part'
 
   def self.find_or_create(project_id)
-    banner = Banner.where(['project_id = ?', project_id]).first
-    unless banner.present?
+    banner = Banner.find_by(project_id: project_id)
+    if banner.blank?
       banner = Banner.new
       banner.project_id = project_id
       banner.enabled = false
@@ -25,7 +27,8 @@ def self.find_or_create(project_id)
   end
 
   def enable_banner?
-    return true if enabled == true && !banner_description.blank?
+    return true if enabled == true && banner_description.present?
+
     false
   end
 end