From 83375645d2cb32d2291927357e6bf3a70442e2f5 Mon Sep 17 00:00:00 2001 From: victor Date: Thu, 14 Jul 2022 23:28:09 +0200 Subject: [PATCH] Apply patch https://www.redmine.org/issues/3088 --- app/controllers/projects_controller.rb | 2 + app/models/issue.rb | 4 +- app/models/issue_query.rb | 44 +++++++++++-------- app/views/issues/show.api.rsb | 6 ++- app/views/issues/show.html.erb | 2 +- app/views/projects/show.html.erb | 23 +++++----- app/views/versions/show.html.erb | 17 ++++--- config/locales/en.yml | 1 + ...w_estimated_hours_to_all_existing_roles.rb | 9 ++++ lib/redmine/default_data/loader.rb | 4 ++ lib/redmine/export/pdf/issues_pdf_helper.rb | 3 +- lib/redmine/preparation.rb | 1 + test/fixtures/roles.yml | 5 +++ test/functional/issues_controller_test.rb | 9 ++++ test/functional/projects_controller_test.rb | 23 ++++++++++ test/functional/versions_controller_test.rb | 17 ++++++- test/integration/api_test/issues_test.rb | 39 ++++++++++++++++ test/unit/mail_handler_test.rb | 9 ++++ 18 files changed, 176 insertions(+), 42 deletions(-) create mode 100644 db/migrate/20220714211705_add_view_estimated_hours_to_all_existing_roles.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 695ca1dec20..0e28470b07c 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -189,6 +189,8 @@ def show if User.current.allowed_to_view_all_time_entries?(@project) @total_hours = TimeEntry.visible.where(cond).sum(:hours).to_f + end + if User.current.allowed_to?(:view_estimated_hours, @project) @total_estimated_hours = Issue.visible.where(cond).sum(:estimated_hours).to_f end diff --git a/app/models/issue.rb b/app/models/issue.rb index 84907a475f5..26ecf1a6e5e 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -490,7 +490,6 @@ def estimated_hours=(h) 'start_date', 'due_date', 'done_ratio', - 'estimated_hours', 'custom_field_values', 'custom_fields', 'lock_version', @@ -520,6 +519,9 @@ def estimated_hours=(h) 'deleted_attachment_ids', :if => lambda {|issue, user| issue.attachments_deletable?(user)}) + safe_attributes 'estimated_hours', + :if => lambda {|issue, user| user.allowed_to?(:view_estimated_hours, issue.project)} + def safe_attribute_names(user=nil) names = super names -= disabled_core_fields diff --git a/app/models/issue_query.rb b/app/models/issue_query.rb index 45ffcb57ce3..28dfb5f8757 100644 --- a/app/models/issue_query.rb +++ b/app/models/issue_query.rb @@ -47,19 +47,6 @@ class IssueQuery < Query :groupable => true), QueryColumn.new(:start_date, :sortable => "#{Issue.table_name}.start_date", :groupable => true), QueryColumn.new(:due_date, :sortable => "#{Issue.table_name}.due_date", :groupable => true), - QueryColumn.new(:estimated_hours, :sortable => "#{Issue.table_name}.estimated_hours", - :totalable => true), - QueryColumn.new( - :total_estimated_hours, - :sortable => - lambda do - "COALESCE((SELECT SUM(estimated_hours) FROM #{Issue.table_name} subtasks" \ - " WHERE #{Issue.visible_condition(User.current).gsub(/\bissues\b/, 'subtasks')}" \ - " AND subtasks.root_id = #{Issue.table_name}.root_id" \ - " AND subtasks.lft >= #{Issue.table_name}.lft" \ - " AND subtasks.rgt <= #{Issue.table_name}.rgt), 0)" - end, - :default_order => 'desc'), QueryColumn.new(:done_ratio, :sortable => "#{Issue.table_name}.done_ratio", :groupable => true), TimestampQueryColumn.new(:created_on, :sortable => "#{Issue.table_name}.created_on", :default_order => 'desc', :groupable => true), @@ -201,10 +188,9 @@ def initialize_available_filters add_available_filter "closed_on", :type => :date_past add_available_filter "start_date", :type => :date add_available_filter "due_date", :type => :date - add_available_filter "estimated_hours", :type => :float - if User.current.allowed_to?(:view_time_entries, project, :global => true) - add_available_filter "spent_time", :type => :float, :label => :label_spent_time + if User.current.allowed_to?(:view_estimated_hours, nil, :global => true) + add_available_filter "estimated_hours", :type => :float end add_available_filter "done_ratio", :type => :integer @@ -279,9 +265,28 @@ def available_columns @available_columns = self.class.available_columns.dup @available_columns += issue_custom_fields.visible.collect {|cf| QueryCustomFieldColumn.new(cf)} + if User.current.allowed_to?(:view_estimated_hours, project, :global => true) + # insert the columns after due_date or at the end + index = @available_columns.rindex {|column| column.name == :due_date} + index = (index ? index + 1 : -1) + + @available_columns.insert index, QueryColumn.new(:estimated_hours, + :sortable => "#{Issue.table_name}.estimated_hours", + :totalable => true + ) + index = (index.negative? ? index : index + 1) + @available_columns.insert index, QueryColumn.new(:total_estimated_hours, + :sortable => -> { + "COALESCE((SELECT SUM(estimated_hours) FROM #{Issue.table_name} subtasks" + + " WHERE #{Issue.visible_condition(User.current).gsub(/\bissues\b/, 'subtasks')} AND subtasks.root_id = #{Issue.table_name}.root_id AND subtasks.lft >= #{Issue.table_name}.lft AND subtasks.rgt <= #{Issue.table_name}.rgt), 0)" + }, + :default_order => 'desc' + ) + end + if User.current.allowed_to?(:view_time_entries, project, :global => true) - # insert the columns after total_estimated_hours or at the end - index = @available_columns.find_index {|column| column.name == :total_estimated_hours} + # insert the columns after total_estimated_hours or the columns after due_date or at the end + index = @available_columns.rindex {|column| column.name == :total_estimated_hours || column.name == :due_date } index = (index ? index + 1 : -1) subselect = "SELECT SUM(hours) FROM #{TimeEntry.table_name}" + @@ -303,8 +308,9 @@ def available_columns " WHERE (#{TimeEntry.visible_condition(User.current)})" + " AND subtasks.root_id = #{Issue.table_name}.root_id AND subtasks.lft >= #{Issue.table_name}.lft AND subtasks.rgt <= #{Issue.table_name}.rgt" + index = (index.negative? ? index : index + 1) @available_columns.insert( - index + 1, + index, QueryColumn.new(:total_spent_hours, :sortable => "COALESCE((#{subselect}), 0)", :default_order => 'desc', diff --git a/app/views/issues/show.api.rsb b/app/views/issues/show.api.rsb index 6f23ca4b474..0ca0de6c051 100644 --- a/app/views/issues/show.api.rsb +++ b/app/views/issues/show.api.rsb @@ -16,8 +16,10 @@ api.issue do api.due_date @issue.due_date api.done_ratio @issue.done_ratio api.is_private @issue.is_private - api.estimated_hours @issue.estimated_hours - api.total_estimated_hours @issue.total_estimated_hours + if User.current.allowed_to?(:view_estimated_hours, @project) + api.estimated_hours @issue.estimated_hours + api.total_estimated_hours @issue.total_estimated_hours + end if User.current.allowed_to?(:view_time_entries, @project) api.spent_hours(@issue.spent_hours) api.total_spent_hours(@issue.total_spent_hours) diff --git a/app/views/issues/show.html.erb b/app/views/issues/show.html.erb index c7cd5689c70..acb97aec8f7 100644 --- a/app/views/issues/show.html.erb +++ b/app/views/issues/show.html.erb @@ -68,7 +68,7 @@ unless @issue.disabled_core_fields.include?('done_ratio') rows.right l(:field_done_ratio), progress_bar(@issue.done_ratio, :legend => "#{@issue.done_ratio}%"), :class => 'progress' end - unless @issue.disabled_core_fields.include?('estimated_hours') + if User.current.allowed_to?(:view_estimated_hours, @project) && !@issue.disabled_core_fields.include?('estimated_hours') rows.right l(:field_estimated_hours), issue_estimated_hours_details(@issue), :class => 'estimated-hours' end if User.current.allowed_to?(:view_time_entries, @project) && @issue.total_spent_hours > 0 diff --git a/app/views/projects/show.html.erb b/app/views/projects/show.html.erb index a1035927fa8..d7c45488c92 100644 --- a/app/views/projects/show.html.erb +++ b/app/views/projects/show.html.erb @@ -97,24 +97,27 @@ <% end %> - <% if User.current.allowed_to?(:view_time_entries, @project) %> + <% allowed_to_view_time_entries, allowed_to_view_estimated_hours = User.current.allowed_to?(:view_time_entries, @project), User.current.allowed_to?(:view_estimated_hours, @project) %> + <% if allowed_to_view_time_entries || allowed_to_view_estimated_hours %>

<%= l(:label_time_tracking) %>

-

- <% if User.current.allowed_to?(:log_time, @project) %> - <%= link_to l(:button_log_time), new_project_time_entry_path(@project) %> | + <% if allowed_to_view_time_entries %> +

+ <% if User.current.allowed_to?(:log_time, @project) %> + <%= link_to l(:button_log_time), new_project_time_entry_path(@project) %> | + <% end %> + <%= link_to(l(:label_details), project_time_entries_path(@project)) %> | + <%= link_to(l(:label_report), report_project_time_entries_path(@project)) %> +

<% end %> - <%= link_to(l(:label_details), project_time_entries_path(@project)) %> | - <%= link_to(l(:label_report), report_project_time_entries_path(@project)) %> -

<% end %> <%= call_hook(:view_projects_show_left, :project => @project) %> diff --git a/app/views/versions/show.html.erb b/app/views/versions/show.html.erb index 0527eae9cc3..17a4b25fbbb 100644 --- a/app/views/versions/show.html.erb +++ b/app/views/versions/show.html.erb @@ -14,15 +14,18 @@ <%= render(:partial => "wiki/content", :locals => {:content => @version.wiki_page.content}) if @version.wiki_page %>
-<% if @version.visible_fixed_issues.estimated_hours > 0 || User.current.allowed_to?(:view_time_entries, @project) %> +<% allowed_to_view_all_time_entries, allowed_to_view_estimated_hours = User.current.allowed_to_view_all_time_entries?(@project), User.current.allowed_to?(:view_estimated_hours, @project) %> +<% if (@version.visible_fixed_issues.estimated_hours > 0 && allowed_to_view_estimated_hours) || allowed_to_view_all_time_entries %>
<%= l(:label_time_tracking) %> - - - - -<% if User.current.allowed_to_view_all_time_entries?(@project) %> +<% if allowed_to_view_estimated_hours %> + + + + +<% end %> +<% if allowed_to_view_all_time_entries %>
<%= l(:field_estimated_hours) %><%= link_to html_hours(l_hours(@version.visible_fixed_issues.estimated_hours)), - project_issues_path(@version.project, :set_filter => 1, :status_id => '*', :fixed_version_id => @version.id, :c => [:tracker, :status, :subject, :estimated_hours], :t => [:estimated_hours]) %>
<%= l(:field_estimated_hours) %><%= link_to html_hours(l_hours(@version.visible_fixed_issues.estimated_hours)), + project_issues_path(@version.project, :set_filter => 1, :status_id => '*', :fixed_version_id => @version.id, :c => [:tracker, :status, :subject, :estimated_hours], :t => [:estimated_hours]) %>
<%= l(:label_spent_time) %> <%= link_to html_hours(l_hours(@version.spent_hours)), diff --git a/config/locales/en.yml b/config/locales/en.yml index bfaf7f80419..5ab6b47f845 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -547,6 +547,7 @@ en: permission_delete_issue_watchers: Delete watchers permission_log_time: Log spent time permission_view_time_entries: View spent time + permission_view_estimated_hours: View estimated time permission_edit_time_entries: Edit time logs permission_edit_own_time_entries: Edit own time logs permission_view_news: View news diff --git a/db/migrate/20220714211705_add_view_estimated_hours_to_all_existing_roles.rb b/db/migrate/20220714211705_add_view_estimated_hours_to_all_existing_roles.rb new file mode 100644 index 00000000000..ed2ad47dee7 --- /dev/null +++ b/db/migrate/20220714211705_add_view_estimated_hours_to_all_existing_roles.rb @@ -0,0 +1,9 @@ +class AddViewEstimatedHoursToAllExistingRoles < ActiveRecord::Migration[6.1] + def up + Role.all.each { |role| role.add_permission! :view_estimated_hours } + end + + def down + # nothing to revert + end +end diff --git a/lib/redmine/default_data/loader.rb b/lib/redmine/default_data/loader.rb index e86f1e87d76..baf7556e150 100644 --- a/lib/redmine/default_data/loader.rb +++ b/lib/redmine/default_data/loader.rb @@ -71,6 +71,7 @@ def load(lang=nil, options={}) :view_calendar, :log_time, :view_time_entries, + :view_estimated_hours, :view_news, :comment_news, :view_documents, @@ -102,6 +103,7 @@ def load(lang=nil, options={}) :view_calendar, :log_time, :view_time_entries, + :view_estimated_hours, :view_news, :comment_news, :view_documents, @@ -122,6 +124,7 @@ def load(lang=nil, options={}) :view_gantt, :view_calendar, :view_time_entries, + :view_estimated_hours, :view_news, :comment_news, :view_documents, @@ -137,6 +140,7 @@ def load(lang=nil, options={}) :view_gantt, :view_calendar, :view_time_entries, + :view_estimated_hours, :view_news, :view_documents, :view_wiki_pages, diff --git a/lib/redmine/export/pdf/issues_pdf_helper.rb b/lib/redmine/export/pdf/issues_pdf_helper.rb index 2f6e12dea67..b61089b7f6f 100644 --- a/lib/redmine/export/pdf/issues_pdf_helper.rb +++ b/lib/redmine/export/pdf/issues_pdf_helper.rb @@ -57,7 +57,8 @@ def issue_to_pdf(issue, assoc={}) right << [l(:field_start_date), format_date(issue.start_date)] unless issue.disabled_core_fields.include?('start_date') right << [l(:field_due_date), format_date(issue.due_date)] unless issue.disabled_core_fields.include?('due_date') right << [l(:field_done_ratio), "#{issue.done_ratio}%"] unless issue.disabled_core_fields.include?('done_ratio') - right << [l(:field_estimated_hours), l_hours(issue.estimated_hours)] unless issue.disabled_core_fields.include?('estimated_hours') + right << [l(:field_estimated_hours), l_hours(issue.estimated_hours)] if \ + User.current.allowed_to?(:view_estimated_hours, issue.project) && !issue.disabled_core_fields.include?('estimated_hours') right << [l(:label_spent_time), l_hours(issue.total_spent_hours)] if User.current.allowed_to?(:view_time_entries, issue.project) rows = left.size > right.size ? left.size : right.size diff --git a/lib/redmine/preparation.rb b/lib/redmine/preparation.rb index 79f4f82eec4..817bcd95a92 100644 --- a/lib/redmine/preparation.rb +++ b/lib/redmine/preparation.rb @@ -84,6 +84,7 @@ def self.prepare map.project_module :time_tracking do |map| map.permission :view_time_entries, {:timelog => [:index, :report, :show]}, :read => true + map.permission :view_estimated_hours, {}, :read => true map.permission :log_time, {:timelog => [:new, :create]}, :require => :loggedin map.permission :edit_time_entries, {:timelog => [:edit, :update, :destroy, :bulk_edit, :bulk_update]}, diff --git a/test/fixtures/roles.yml b/test/fixtures/roles.yml index 076d347ff73..9a8eeba0056 100644 --- a/test/fixtures/roles.yml +++ b/test/fixtures/roles.yml @@ -35,6 +35,7 @@ roles_001: - :view_calendar - :log_time - :view_time_entries + - :view_estimated_hours - :edit_time_entries - :delete_time_entries - :import_time_entries @@ -102,6 +103,7 @@ roles_002: - :view_calendar - :log_time - :view_time_entries + - :view_estimated_hours - :edit_own_time_entries - :view_news - :manage_news @@ -151,6 +153,7 @@ roles_003: - :view_calendar - :log_time - :view_time_entries + - :view_estimated_hours - :view_news - :manage_news - :comment_news @@ -191,6 +194,7 @@ roles_004: - :view_calendar - :log_time - :view_time_entries + - :view_estimated_hours - :view_news - :comment_news - :view_documents @@ -218,6 +222,7 @@ roles_005: - :view_gantt - :view_calendar - :view_time_entries + - :view_estimated_hours - :view_news - :view_documents - :view_wiki_pages diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index 60d1ff63828..1baad179756 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -1581,6 +1581,15 @@ def test_index_with_total_estimated_hours_column assert_select 'table.issues td.total_estimated_hours' end + def test_index_should_not_show_estimated_hours_column_without_permission + Role.anonymous.remove_permission! :view_estimated_hours + get :index, :params => { + :set_filter => 1, + :c => %w(subject estimated_hours) + } + assert_select 'td.estimated_hours', 0 + end + def test_index_should_not_show_spent_hours_column_without_permission Role.anonymous.remove_permission! :view_time_entries get( diff --git a/test/functional/projects_controller_test.rb b/test/functional/projects_controller_test.rb index 07f590365d1..2c9a126c224 100644 --- a/test/functional/projects_controller_test.rb +++ b/test/functional/projects_controller_test.rb @@ -804,6 +804,29 @@ def test_show_should_spent_and_estimated_time end end + def test_show_by_non_admin_user_with_view_estimated_hours_permission_should_show_estimated_time + @request.session[:user_id] = 2 # manager + get :show, :params => { + :id => 'ecookbook' + } + + assert_select 'div.spent_time.box>ul' do + assert_select '>li', :text => 'Estimated time: 203.50 hours' + end + end + + def test_show_by_non_admin_user_without_view_estimated_hours_permission_should_not_show_estimated_time + Role.find_by_name('Manager').remove_permission! :view_estimated_hours + @request.session[:user_id] = 2 # manager + get :show, :params => { + :id => 'ecookbook' + } + + assert_select 'div.spent_time.box>ul' do + assert_select '>li', :text => 'Estimated time: 203.50 hours', :count => 0 + end + end + def test_settings @request.session[:user_id] = 2 # manager get(:settings, :params => {:id => 1}) diff --git a/test/functional/versions_controller_test.rb b/test/functional/versions_controller_test.rb index fdd767560bd..f05fca92eca 100644 --- a/test/functional/versions_controller_test.rb +++ b/test/functional/versions_controller_test.rb @@ -165,7 +165,22 @@ def test_show_issue_calculations_should_take_into_account_only_visible_issues assert_select 'a', :text => '1 open' end - assert_select '.time-tracking td.total-hours a:first-child', :text => '2:00 hours' + assert_select '.time-tracking tr:first-child' do + assert_select 'th', :text => 'Estimated time' + assert_select 'td.total-hours a', :text => '2.00 hours' + end + + Role.non_member.remove_permission! :view_estimated_hours + + get :show, :params => {:id => 4} + assert_response :success + + assert_select 'p.progress-info' do + assert_select 'a', :text => '1 issue' + assert_select 'a', :text => '1 open' + end + + assert_select '.time-tracking th', :text => 'Estimated time', :count => 0 end def test_show_should_link_to_spent_time_on_version diff --git a/test/integration/api_test/issues_test.rb b/test/integration/api_test/issues_test.rb index 29b3390d21c..6347f3f5786 100644 --- a/test/integration/api_test/issues_test.rb +++ b/test/integration/api_test/issues_test.rb @@ -489,6 +489,26 @@ def test_show_should_include_issue_attributes end end + test "GET /issues/:id.xml should contains total_spent_hours, and should not contains estimated_hours and total_estimated_hours when permission does not exists" do + parent = Issue.find(3) + parent.update_columns :estimated_hours => 2.0 + child = Issue.generate!(:parent_issue_id => parent.id, :estimated_hours => 3.0) + TimeEntry.create!(:project => child.project, :issue => child, :user => child.author, :spent_on => child.author.today, + :hours => '2.5', :comments => '', :activity_id => TimeEntryActivity.first.id) + # remove permission! + Role.anonymous.remove_permission! :view_estimated_hours + + get '/issues/3.xml' + + assert_equal 'application/xml', response.content_type + assert_select 'issue' do + assert_select 'estimated_hours', false + assert_select 'total_estimated_hours', false + assert_select 'spent_hours', parent.spent_hours.to_s + assert_select 'total_spent_hours', (parent.spent_hours.to_f + 2.5).to_s + end + end + test "GET /issues/:id.xml should contains visible spent_hours only" do user = User.find_by_login('jsmith') Role.find(1).update(:time_entries_visibility => 'own') @@ -537,6 +557,25 @@ def test_show_should_include_issue_attributes assert_nil json['issue']['total_spent_hours'] end + test "GET /issues/:id.json should contains total_spent_hours, and should not contains estimated_hours and total_estimated_hours when permission does not exists" do + parent = Issue.find(3) + parent.update_columns :estimated_hours => 2.0 + child = Issue.generate!(:parent_issue_id => parent.id, :estimated_hours => 3.0) + TimeEntry.create!(:project => child.project, :issue => child, :user => child.author, :spent_on => child.author.today, + :hours => '2.5', :comments => '', :activity_id => TimeEntryActivity.first.id) + # remove permission! + Role.anonymous.remove_permission! :view_estimated_hours + + get '/issues/3.json' + + assert_equal 'application/json', response.content_type + json = ActiveSupport::JSON.decode(response.body) + assert_nil json['issue']['estimated_hours'] + assert_nil json['issue']['total_estimated_hours'] + assert_equal parent.spent_hours, json['issue']['spent_hours'] + assert_equal (parent.spent_hours.to_f + 2.5), json['issue']['total_spent_hours'] + end + test "GET /issues/:id.json should contains visible spent_hours only" do user = User.find_by_login('jsmith') Role.find(1).update(:time_entries_visibility => 'own') diff --git a/test/unit/mail_handler_test.rb b/test/unit/mail_handler_test.rb index b36259c14cf..a1b520c1860 100644 --- a/test/unit/mail_handler_test.rb +++ b/test/unit/mail_handler_test.rb @@ -43,6 +43,11 @@ def teardown end def test_add_issue_with_specific_overrides + project = Project.find_by_name('OnlineStore') + project.enabled_module_names += [:time_tracking] + project.save! + + issue = submit_email( 'ticket_on_given_project.eml', @@ -74,6 +79,10 @@ def test_add_issue_with_specific_overrides end def test_add_issue_with_all_overrides + project = Project.find_by_name('OnlineStore') + project.enabled_module_names += [:time_tracking] + project.save! + issue = submit_email('ticket_on_given_project.eml', :allow_override => 'all') assert issue.is_a?(Issue) assert !issue.new_record?