From 5c0aba86ab6bc1c5a1a5b63c1c975f1f86029ef1 Mon Sep 17 00:00:00 2001 From: Akiko Takano Date: Tue, 11 Feb 2020 22:02:55 +0900 Subject: [PATCH 1/5] Refactoring: Not to use project setting tab but to use project menu. Related to #127 Update test code and css. --- app/controllers/banner_controller.rb | 17 ++++-- app/views/banner/show.html.erb | 47 ++++++++++++++ assets/stylesheets/banner.css | 2 +- config/routes.rb | 6 +- init.rb | 7 ++- lib/banners/projects_helper_patch.rb | 31 ---------- .../api_global_banner_controller_test.rb | 2 +- test/functional/banner_controller_test.rb | 3 +- test/functional/projects_controller_test.rb | 14 ++--- test/integration/layout_test.rb | 4 +- test/unit/projects_helper_patch_test.rb | 61 ------------------- 11 files changed, 78 insertions(+), 116 deletions(-) create mode 100644 app/views/banner/show.html.erb delete mode 100644 lib/banners/projects_helper_patch.rb delete mode 100644 test/unit/projects_helper_patch_test.rb diff --git a/app/controllers/banner_controller.rb b/app/controllers/banner_controller.rb index a6c1b3e..c12a41b 100644 --- a/app/controllers/banner_controller.rb +++ b/app/controllers/banner_controller.rb @@ -31,15 +31,24 @@ def project_banner_off render action: '_project_banner_off', layout: false end + def show + @banner = Banner.find_or_create(@project.id) + render layout: !request.xhr? + end + def edit return if params[:setting].nil? @banner = Banner.find_or_create(@project.id) @banner.safe_attributes = banner_params - @banner.save - flash[:notice] = l(:notice_successful_update) - redirect_to controller: 'projects', - action: 'settings', id: @project, tab: 'banner' + + if @banner.save + flash[:notice] = l(:notice_successful_update) + else + flash[:error] = @banner.errors.messages + end + redirect_to action: 'show' + nil end private diff --git a/app/views/banner/show.html.erb b/app/views/banner/show.html.erb new file mode 100644 index 0000000..1b54b0f --- /dev/null +++ b/app/views/banner/show.html.erb @@ -0,0 +1,47 @@ +<% banner = @banner %> + +<%= form_tag({ action: 'edit' }, id: 'banner') do %> + + +<%= submit_tag l(:button_save) %> + +<% end %> + + diff --git a/assets/stylesheets/banner.css b/assets/stylesheets/banner.css index 484467e..5d7bd56 100644 --- a/assets/stylesheets/banner.css +++ b/assets/stylesheets/banner.css @@ -1,4 +1,4 @@ -.banner { +div.banner { border: 1px solid; margin: 2px 0; padding: 2px 20px; diff --git a/config/routes.rb b/config/routes.rb index e6a3b76..0b0e7e2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -7,9 +7,9 @@ end resources :projects do - resources :banner do - patch 'edit', on: :member - put 'edit', on: :collection + resources :banner, only: %i[show] do + get '/', to: 'banner#show', on: :collection + post '/', to: 'banner#edit', on: :collection post 'project_banner_off', on: :collection get 'project_banner_off', on: :collection end diff --git a/init.rb b/init.rb index 3d2bb36..807f1e4 100644 --- a/init.rb +++ b/init.rb @@ -3,7 +3,6 @@ require 'redmine' require_relative 'app/models/global_banner' require 'banners/application_hooks' -require 'banners/projects_helper_patch' # NOTE: Keep error message for a while to support Redmine3.x users. def banner_version_message(original_message = nil) @@ -21,6 +20,10 @@ def banner_admin? GlobalBanner.banner_admin?(User.current) end +def project_menu_allowed? + proc { |p| User.current.allowed_to?({ controller: 'banner', action: 'show' }, p) } +end + Redmine::Plugin.register :redmine_banner do begin name 'Redmine Banner plugin' @@ -37,6 +40,8 @@ def banner_admin? menu :top_menu, :redmine_banner, { controller: 'global_banner', action: 'show', "id": nil }, caption: :banner, last: true, if: proc { banner_admin? } + menu :project_menu, :banner, { controller: 'banner', action: 'show', "id": nil }, + caption: :banner, param: :project_id, after: :settings, if: project_menu_allowed? project_module :banner do permission :manage_banner, { banner: %I[show edit project_banner_off] }, require: :member diff --git a/lib/banners/projects_helper_patch.rb b/lib/banners/projects_helper_patch.rb deleted file mode 100644 index 08ef2e9..0000000 --- a/lib/banners/projects_helper_patch.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -require 'projects_helper' - -module Banners - module ProjectsHelperPatch - extend ActiveSupport::Concern - - def project_settings_tabs - tabs = super - return tabs unless @project.module_enabled?(:banner) - - tabs.tap { |t| t << append_banner_tab }.compact - end - - def append_banner_tab - @banner = Banner.find_or_create(@project.id) - action = { name: 'banner', - controller: 'banner', - action: :show, - partial: 'banner/show', label: :banner } - return nil unless User.current.allowed_to?(action, @project) - - action - end - end -end - -unless ProjectsHelper.included_modules.include?(Banners::ProjectsHelperPatch) - ProjectsHelper.prepend(Banners::ProjectsHelperPatch) -end diff --git a/test/controller/api_global_banner_controller_test.rb b/test/controller/api_global_banner_controller_test.rb index 8d370b7..70cd5f7 100644 --- a/test/controller/api_global_banner_controller_test.rb +++ b/test/controller/api_global_banner_controller_test.rb @@ -20,7 +20,7 @@ def test_routing def test_get_global_banner_when_api_disable Setting.rest_api_enabled = '0' get '/banners/api/global_banner.json', headers: { 'X-Redmine-API-Key' => @user.api_key } - assert_response 403 + assert_response :forbidden end def test_get_global_banner_when_api_enable diff --git a/test/functional/banner_controller_test.rb b/test/functional/banner_controller_test.rb index 30c6195..0e04a30 100644 --- a/test/functional/banner_controller_test.rb +++ b/test/functional/banner_controller_test.rb @@ -90,8 +90,7 @@ def test_redirect_post setting: { enabled: '1', description: 'Edit test', display_part: 'all', style: 'alert' } } assert_response :redirect - assert_redirected_to controller: 'projects', - action: 'settings', id: @project, tab: 'banner' + assert_redirected_to controller: 'banner', action: 'show' end def test_return_success_when_banner_off_with_manage_permission diff --git a/test/functional/projects_controller_test.rb b/test/functional/projects_controller_test.rb index 735dc7c..74c0651 100644 --- a/test/functional/projects_controller_test.rb +++ b/test/functional/projects_controller_test.rb @@ -28,26 +28,20 @@ def setup @banner.save! end - def test_settings - get :settings, params: { id: 1 } - assert_response :success - assert_select 'a#tab-banner' - assert_select 'div#project_banner_area div.banner_info', false, - 'Banner should be displayed Overview only.' - end - # project 1 is enabled banner and type is info, display_part is overview only. def test_show_overview get :show, params: { id: 1 } assert_response :success - assert_select 'div#project_banner_area div.banner_info' + assert_select '#main-menu ul li a.banner' + assert_select 'div#project_banner_area div.banner_info', true, + 'Banner should be displayed Overview only.' end def test_show_all @banner.display_part = 'all' @banner.style = 'warn' @banner.save! - get :settings, params: { id: 1 } + get :show, params: { id: 1 } assert_response :success assert_select 'div#project_banner_area div.banner_warn' end diff --git a/test/integration/layout_test.rb b/test/integration/layout_test.rb index 16f4c85..2386c3e 100644 --- a/test/integration/layout_test.rb +++ b/test/integration/layout_test.rb @@ -36,8 +36,8 @@ def test_project_banner_visible_when_module_on get '/projects/ecookbook/issues' assert_select 'div#project_banner_area div.banner_info', 0 - put '/projects/ecookbook/banner/edit', - params: { setting: { enabled: '1', style: 'warn', display_part: 'all', banner_description: 'Test banner message.' }, project_id: 'ecookbook' } + post '/projects/ecookbook/banner', + params: { setting: { enabled: '1', style: 'warn', display_part: 'all', banner_description: 'Test banner message.' }, project_id: 'ecookbook' } assert_response :redirect get '/projects/ecookbook/issues' diff --git a/test/unit/projects_helper_patch_test.rb b/test/unit/projects_helper_patch_test.rb deleted file mode 100644 index b81de8c..0000000 --- a/test/unit/projects_helper_patch_test.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -require File.expand_path(File.dirname(__FILE__) + '/../test_helper') -require File.expand_path(File.dirname(__FILE__) + '/../../lib/banners/projects_helper_patch') -require 'minitest/autorun' - -class ProjectsHelperPatchTest < Redmine::HelperTest - include ApplicationHelper - include ProjectsHelper - - fixtures :projects, :users - - def setup - MockHelperInstance.prepend(Banners::ProjectsHelperPatch) - User.current = User.first - @project = Project.first - @mock_helper_instance = MockHelperInstance.new(@project) - end - - def test_patch_applied - assert_equal true, @mock_helper_instance.respond_to?(:project_settings_tabs) - assert_equal true, @mock_helper_instance.respond_to?(:append_banner_tab) - end - - def test_project_settings_tabs - tabs = @mock_helper_instance.project_settings_tabs - tab_names = tabs.map { |h| h[:name] } - assert_equal true, tabs.any? - assert_equal false, tab_names.include?('banner') - end - - def test_project_settings_tabs_with_banner_enabled - EnabledModule.create(name: 'banner', project_id: @project.id) - tabs = @mock_helper_instance.project_settings_tabs - tab_names = tabs.map { |h| h[:name] } - assert_equal true, tabs.any? - assert_equal true, tab_names.include?('banner') - end - - def test_append_banner_tab_called - mock = Minitest::Mock.new.expect(:call, true) - EnabledModule.create(name: 'banner', project_id: @project.id) - @mock_helper_instance.stub :append_banner_tab, mock do - @mock_helper_instance.project_settings_tabs - end - mock.verify - end -end - -class MockHelperInstance - include ProjectsHelper - attr_reader :project - def initialize(project) - @project = project - end - - # mock parameter - def params - { version_status: 'active', version_name: 'latest' } - end -end From a18aae517dbed704324f1b59e677e8828d1880cf Mon Sep 17 00:00:00 2001 From: Akiko Takano Date: Tue, 11 Feb 2020 23:13:04 +0900 Subject: [PATCH 2/5] Add message to implement #126. --- config/locales/en.yml | 4 ++++ config/locales/ja.yml | 5 ++++- script/swagger.yml | 9 +++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 51a7893..4b7e912 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -28,3 +28,7 @@ en: label_more_info: "Link to more information." banner_admin_group: "Banner Admin Group" description_for_banner_admin_group: "Specify a group that can manage the global banner without admin rights." + setting_banner_display_for: "Display for" + label_anonymous_only: "Anonymous users" + label_authenticated_only: "Authenticated users" + label_display_all: "All" diff --git a/config/locales/ja.yml b/config/locales/ja.yml index be4a6b8..8c988bc 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -28,4 +28,7 @@ ja: label_more_info: "詳細はこちら。" banner_admin_group: "バナー管理者グループ" description_for_banner_admin_group: "バナー管理者グループのユーザは、Redmine全体の管理者権限なしでもグローバルバナーを編集できるようになります。" - + setting_banner_display_for: "表示対象" + label_anonymous_only: "ログインしていないユーザのみ" + label_authenticated_only: "ログインユーザのみ" + label_display_all: "全てのユーザ" diff --git a/script/swagger.yml b/script/swagger.yml index 354c56b..e562ac2 100644 --- a/script/swagger.yml +++ b/script/swagger.yml @@ -119,6 +119,15 @@ definitions: banner_description: type: string example: "Message for Global Banner" + display_for: + type: "string" + example: "all" + enum: [all, anonymous, authenticated] + description: > + Display for: + * `all` - For all users + * `anonymous` - For anonymous users + * `authenticated` - For authenticated users display_part: type: "string" example: "both" From 0945293d32d0bf165623a1a402d5a9bd43f7467f Mon Sep 17 00:00:00 2001 From: Akiko Takano Date: Tue, 11 Feb 2020 23:18:28 +0900 Subject: [PATCH 3/5] Add option to select target users to show global banner message. --- app/models/global_banner.rb | 2 +- app/views/global_banner/show.html.erb | 13 +++++++++---- lib/banners/application_hooks.rb | 12 ++++++++++-- test/integration/layout_test.rb | 8 ++++---- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app/models/global_banner.rb b/app/models/global_banner.rb index a4cf84d..6dfb015 100644 --- a/app/models/global_banner.rb +++ b/app/models/global_banner.rb @@ -7,7 +7,7 @@ class GlobalBanner < Setting GLOBAL_BANNER_DEFAULT_SETTING = { enable: 'false', banner_description: 'exp. Information about upcoming Service Interruption.', - only_authenticated: nil, + display_for: 'all', display_only_login_page: nil, type: 'info', display_part: 'both', diff --git a/app/views/global_banner/show.html.erb b/app/views/global_banner/show.html.erb index abdffbd..8dbdd1e 100644 --- a/app/views/global_banner/show.html.erb +++ b/app/views/global_banner/show.html.erb @@ -12,9 +12,12 @@

- <%= content_tag(:label, l(:label_display_authenticated_user_only))%> - <%= hidden_field_tag('setting[only_authenticated]', false).html_safe %> - <%= check_box_tag 'setting[only_authenticated]', true, setting['only_authenticated'] == 'true' %> + <%= content_tag(:label, l(:setting_banner_display_for, default: 'Display for'))%> + <%= select_tag('setting[display_for]', + options_for_select([[l(:label_anonymous_only, default: 'Anonymous'),'anonymous'], + [l(:label_authenticated_only, default: 'Authenticated'),'authenticated'], + [l(:label_display_all, default: 'All'), 'all']], + { selected: setting['display_for'] })) %>

@@ -29,7 +32,9 @@ <%= content_tag(:label, l(:setting_banner_display_part))%> <%= select_tag('setting[display_part]', options_for_select([[l(:label_header_only),'header'], - [l(:label_footer_only),'footer'],[l(:label_both),'both']],setting['display_part'])) %> + [l(:label_footer_only),'footer'], + [l(:label_both),'both']], + setting['display_part'])) %> <%= hidden_field_tag('setting[display_only_login_page]', false).html_safe %> <%= check_box_tag 'setting[display_only_login_page]', true, setting['display_only_login_page'] == 'true' %> <%= l(:label_check_if_banner_show_only_login_page) %> diff --git a/lib/banners/application_hooks.rb b/lib/banners/application_hooks.rb index 2d0ca20..7de0045 100644 --- a/lib/banners/application_hooks.rb +++ b/lib/banners/application_hooks.rb @@ -91,9 +91,17 @@ def should_display_on_current_page?(context, setting) (context[:controller].action_name != 'login')) && (setting['display_only_login_page'] == 'true') - return false if !User.current.logged? && setting['only_authenticated'] == 'true' + return should_display_for?(setting) + end + + def should_display_for?(setting) + target = setting['display_for'] || 'all' + return true if target == 'all' + + return true if target == 'authenticated' && User.current.logged? + return true if target == 'anonymous' && User.current.anonymous? - true + false end end end diff --git a/test/integration/layout_test.rb b/test/integration/layout_test.rb index 2386c3e..38427cd 100644 --- a/test/integration/layout_test.rb +++ b/test/integration/layout_test.rb @@ -75,8 +75,7 @@ def test_display_only_for_login_page enable: 'true', type: 'warn', display_part: 'both', use_timer: 'false', banner_description: 'h1. Test data.', - display_only_login_page: 'true', - only_authenticated: 'true' + display_for: 'authenticated' } } # Session is cleared @@ -100,11 +99,12 @@ def test_display_more_link use_timer: 'false', banner_description: 'h1. Test data.', display_only_login_page: 'false', - only_authenticated: 'false', + display_for: 'authenticated', related_link: '' } } get '/' + assert_select 'div.banner_area', 1 assert_select 'div.banner_more_info', 0 # Update setting. @@ -114,7 +114,7 @@ def test_display_more_link use_timer: 'false', banner_description: 'h1. Test data.', display_only_login_page: 'false', - only_authenticated: 'false', + display_for: 'all', related_link: 'http://www.redmine.org/' } } From 7181526160b442b60ed2b331ed5f40fb57d8ef99 Mon Sep 17 00:00:00 2001 From: Akiko Takano Date: Tue, 11 Feb 2020 23:23:44 +0900 Subject: [PATCH 4/5] Remove redundant return. --- lib/banners/application_hooks.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/banners/application_hooks.rb b/lib/banners/application_hooks.rb index 7de0045..dcee623 100644 --- a/lib/banners/application_hooks.rb +++ b/lib/banners/application_hooks.rb @@ -91,7 +91,7 @@ def should_display_on_current_page?(context, setting) (context[:controller].action_name != 'login')) && (setting['display_only_login_page'] == 'true') - return should_display_for?(setting) + should_display_for?(setting) end def should_display_for?(setting) From 520abb177af50aca75f48faf22fdc2cc60f693f8 Mon Sep 17 00:00:00 2001 From: Akiko Takano Date: Tue, 11 Feb 2020 23:50:42 +0900 Subject: [PATCH 5/5] Update version to 0.3.1. --- README.md | 5 +++++ init.rb | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index a94df92..d8c804c 100644 --- a/README.md +++ b/README.md @@ -51,6 +51,11 @@ Please use ver **0.1.x** or ``v0.1.x-support-Redmine3`` branch in case using Red ## Changelog +### 0.3.1 + +* Feature: Enabled to switch who can see the global banner. (#126) +* Refactor: Change to use project menu to prevent the project setting tab's conflict. (#127) + ### 0.3.0 * Add feature: Give the ability to specific users to manage the site-wide banner. (GitHub: #86 / #113) diff --git a/init.rb b/init.rb index 807f1e4..73ff925 100644 --- a/init.rb +++ b/init.rb @@ -30,7 +30,7 @@ def project_menu_allowed? author 'Akiko Takano' author_url 'http://twitter.com/akiko_pusu' description 'Plugin to show site-wide message, such as maintenacne informations or notifications.' - version '0.3.0' + version '0.3.1' requires_redmine version_or_higher: '4.0' url 'https://github.com/akiko-pusu/redmine_banner'