From 6f34bd34c598fc8e0243f570693eceab6bccb64e Mon Sep 17 00:00:00 2001 From: Akiko Takano Date: Sat, 21 Dec 2019 15:27:44 +0900 Subject: [PATCH 1/4] Remove unused files. Add bullet. --- .circleci/config.yml | 20 -------------------- Gemfile.local | 4 ++++ 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 64ec212..e606814 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -2,9 +2,6 @@ # # Check https://circleci.com/docs/2.0/language-ruby/ for more details # -orbs: - aws-ecr: circleci/aws-ecr@6.2.0 - version: 2.1 general: @@ -96,23 +93,6 @@ workflows: branches: ignore: - /v0.1.x-support-Redmine3.*/ - - aws-ecr/build-and-push-image: - account-url: AWS_ECR_ACCOUNT_URL - aws-access-key-id: AWS_ACCESS_KEY_ID - aws-secret-access-key: AWS_SECRET_ACCESS_KEY - create-repo: true - dockerfile: Dockerfile - path: . - region: AWS_REGION - repo: redmine_banner - extra-build-args: '--build-arg COMMIT=$CIRCLE_SHA1 --build-arg=BRANCH=$CIRCLE_BRANCH' - requires: - - test - tag: '$CIRCLE_BUILD_NUM' - filters: - branches: - only: - - master - deploy_heroku: requires: - build diff --git a/Gemfile.local b/Gemfile.local index 8479342..b99c6ce 100644 --- a/Gemfile.local +++ b/Gemfile.local @@ -2,3 +2,7 @@ source 'https://rubygems.org' group :test do gem 'simplecov-rcov', :require => false end + +group :development do + gem 'bullet' +end From 940f77aa406388d710a0ac6eabb0b03c6ca1ab1a Mon Sep 17 00:00:00 2001 From: Akiko Takano Date: Tue, 31 Dec 2019 06:26:58 +0900 Subject: [PATCH 2/4] Enabled to edit global banner by non-admin. (#124) New feature: Enabled to edit global banner by non-admin. Code refactoring. --- .rubocop.yml | 1 + .rubocop_todo.yml | 4 +- README.md | 13 +- app/controllers/banner_controller.rb | 14 +- .../banners/api/global_banner_controller.rb | 56 ++++---- app/controllers/global_banner_controller.rb | 83 ++++++++++++ app/models/banner.rb | 15 ++- app/models/global_banner.rb | 103 +++++++++++++++ app/views/banner/_after_top_menu.html.erb | 12 -- app/views/banner/_body_bottom.html.erb | 58 +++++---- app/views/banner/_off.erb | 2 +- app/views/banner/_project_banner_off.erb | 2 +- .../banner/_project_body_bottom.html.erb | 10 +- app/views/banner/_show.html.erb | 42 +++--- app/views/global_banner/show.html.erb | 117 +++++++++++++++++ app/views/settings/_redmine_banner.html.erb | 97 -------------- assets/javascripts/banner.js | 40 ++---- assets/stylesheets/banner.css | 5 - config/locales/en.yml | 6 +- config/routes.rb | 5 + init.rb | 35 ++--- lib/banners/application_hooks.rb | 111 +++++++--------- lib/banners/banner_helper.rb | 41 ------ lib/banners/settings_controller_patch.rb | 93 -------------- ...b => api_global_banner_controller_test.rb} | 4 +- test/functional/banner_controller_test.rb | 16 ++- .../banner_setting_controller_test.rb | 120 ------------------ .../global_banner_controller_test.rb | 118 +++++++++++++++++ test/functional/projects_controller_test.rb | 13 +- test/integration/layout_test.rb | 33 +++-- test/test_helper.rb | 10 +- test/unit/banner_application_hooks_test.rb | 114 +++++------------ test/unit/banner_helper_test.rb | 26 ---- test/unit/banner_test.rb | 2 + test/unit/projects_helper_patch_test.rb | 2 + 35 files changed, 696 insertions(+), 727 deletions(-) create mode 100644 app/controllers/global_banner_controller.rb create mode 100644 app/models/global_banner.rb delete mode 100644 app/views/banner/_after_top_menu.html.erb create mode 100644 app/views/global_banner/show.html.erb delete mode 100644 app/views/settings/_redmine_banner.html.erb delete mode 100644 lib/banners/banner_helper.rb delete mode 100644 lib/banners/settings_controller_patch.rb rename test/controller/{global_banner_controller_test.rb => api_global_banner_controller_test.rb} (96%) delete mode 100644 test/functional/banner_setting_controller_test.rb create mode 100644 test/functional/global_banner_controller_test.rb delete mode 100644 test/unit/banner_helper_test.rb diff --git a/.rubocop.yml b/.rubocop.yml index 7cd3c92..bad06aa 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,4 +1,5 @@ inherit_from: .rubocop_todo.yml + AllCops: TargetRubyVersion: 2.3 diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 73ee013..698c103 100644 --- a/.rubocop_todo.yml +++ b/.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 - diff --git a/README.md b/README.md index 579228c..86c6fe9 100644 --- a/README.md +++ b/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 diff --git a/app/controllers/banner_controller.rb b/app/controllers/banner_controller.rb index b89d086..a6c1b3e 100644 --- a/app/controllers/banner_controller.rb +++ b/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 diff --git a/app/controllers/banners/api/global_banner_controller.rb b/app/controllers/banners/api/global_banner_controller.rb index c6bf9dc..4a403d4 100644 --- a/app/controllers/banners/api/global_banner_controller.rb +++ b/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 diff --git a/app/controllers/global_banner_controller.rb b/app/controllers/global_banner_controller.rb new file mode 100644 index 0000000..25fb820 --- /dev/null +++ b/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 diff --git a/app/models/banner.rb b/app/models/banner.rb index 72247eb..6e7f264 100644 --- a/app/models/banner.rb +++ b/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 diff --git a/app/models/global_banner.rb b/app/models/global_banner.rb new file mode 100644 index 0000000..a4cf84d --- /dev/null +++ b/app/models/global_banner.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +class GlobalBanner < Setting + # Default + GLOBAL_BANNER_ADMIN_GROUP = 'GlobalBanner_Admin' + + GLOBAL_BANNER_DEFAULT_SETTING = { + enable: 'false', + banner_description: 'exp. Information about upcoming Service Interruption.', + only_authenticated: nil, + display_only_login_page: nil, + type: 'info', + display_part: 'both', + use_timer: 'false', + start_ymd: nil, + start_hour: nil, + start_min: nil, + end_ymd: nil, + end_hour: nil, + end_min: nil, + related_link: nil, + banner_admin: nil + }.freeze + + # Class method + # + class << self + def find_or_default + super('plugin_redmine_banner') + end + + def banner_admin?(user) + return true if user.admin? + + instance = find_or_default + instance.banner_admin?(user) + end + end + + def admin_group + value[:admin_group] || GLOBAL_BANNER_ADMIN_GROUP + end + + def banner_admin?(user) + return true if user.admin? + + admin_group = value['banner_admin'] || GLOBAL_BANNER_ADMIN_GROUP + + banner_admin_group = Group.find_by(lastname: admin_group) + return false if banner_admin_group.blank? + + banner_admin_group.users.include?(user) + end + + def use_timer? + value['use_timer'] == 'true' + end + + def start_time + get_time( + value['start_ymd'], + value['start_hour'], + value['start_min'] + ) + end + + def end_time + get_time( + value['end_ymd'], + value['end_hour'], + value['end_min'] + ) + end + + def get_time(ymd, hour, minute) + begin + d = Date.strptime(ymd, '%Y-%m-%d') + rescue StandardError => e + logger&.warn("Passed value #{ymd} for Banner has wrong format. #{e.message}") + d = Date.current + end + Time.mktime(d.year, d.month, d.day, hour.to_i, minute.to_i) + end + + def valid_date_range? + return true unless use_timer? + + end_time > start_time + end + + def enable? + value['enable'] == 'true' + end + + def disable? + value['enable'] != 'true' + end + + def merge_value(new_value) + current_value = value + self.value = current_value.merge(new_value).stringify_keys + end +end diff --git a/app/views/banner/_after_top_menu.html.erb b/app/views/banner/_after_top_menu.html.erb deleted file mode 100644 index 44b7b98..0000000 --- a/app/views/banner/_after_top_menu.html.erb +++ /dev/null @@ -1,12 +0,0 @@ -<%= render 'banner/body_bottom' %> - - diff --git a/app/views/banner/_body_bottom.html.erb b/app/views/banner/_body_bottom.html.erb index 7f43c9f..2cf219c 100644 --- a/app/views/banner/_body_bottom.html.erb +++ b/app/views/banner/_body_bottom.html.erb @@ -1,32 +1,46 @@ -<% - # TODO: Display logic should be moved from view.... - s = session[:pref_banner_off] - t = Setting.find_by_name('plugin_redmine_banner') - if !t.blank? && !s.blank? && t.updated_on.to_i < s - return '' - end - - # TODO: This is workaround for SQLite3 - banner_description = Setting.plugin_redmine_banner['banner_description']; - banner_description.force_encoding("UTF-8") if banner_description.respond_to?(:force_encoding) --%> + +<% if setting['display_part'] == 'header' %> + +<% end %> + +<% if setting['display_part'] == 'both' %> + +<% end %> \ No newline at end of file diff --git a/app/views/banner/_off.erb b/app/views/banner/_off.erb index 15d0f3c..4f9e9cd 100644 --- a/app/views/banner/_off.erb +++ b/app/views/banner/_off.erb @@ -1,4 +1,4 @@ -var elements = document.getElementsByClassName('global_banner'); +let elements = document.getElementsByClassName('global_banner'); elements = Array.from(elements); elements.forEach(function(element) { element.classList.add('fadeout'); diff --git a/app/views/banner/_project_banner_off.erb b/app/views/banner/_project_banner_off.erb index e1d0994..9800ccf 100644 --- a/app/views/banner/_project_banner_off.erb +++ b/app/views/banner/_project_banner_off.erb @@ -1,4 +1,4 @@ -var elements = document.getElementsById('banner_content'); +let elements = document.getElementsById('banner_content'); element.classList.add('fadeout'); setTimeout(function(){ element.style.display = "none"; diff --git a/app/views/banner/_project_body_bottom.html.erb b/app/views/banner/_project_body_bottom.html.erb index 7ebd119..7498350 100644 --- a/app/views/banner/_project_body_bottom.html.erb +++ b/app/views/banner/_project_body_bottom.html.erb @@ -8,17 +8,17 @@ if authorize_for(:banner, :edit) %>