From 9c6cb5856fcfd5ddd0b7bb85797f102a6e017d7b Mon Sep 17 00:00:00 2001
From: Marius BALTEANU <marius.balteanu@zitec.com>
Date: Mon, 1 Jul 2019 06:16:44 +0000
Subject: [PATCH] Per role visibility settings for spent time custom fields

---
 app/models/time_entry.rb                           |   7 ++
 app/models/time_entry_custom_field.rb              |  10 +-
 app/views/custom_fields/_form.html.erb             | 106 ++++++++++-----------
 .../_visibility_by_role_selector.html.erb          |  20 ++++
 app/views/timelog/_form.html.erb                   |   2 +-
 app/views/timelog/index.api.rsb                    |   2 +-
 test/functional/custom_fields_controller_test.rb   |  25 +++++
 .../timelog_custom_fields_visibility_test.rb       |  85 +++++++++++------
 test/functional/timelog_report_test.rb             |   2 +-
 9 files changed, 168 insertions(+), 91 deletions(-)
 create mode 100644 app/views/custom_fields/_visibility_by_role_selector.html.erb

diff --git a/app/models/time_entry.rb b/app/models/time_entry.rb
index 9701622..d2661d0 100644
--- a/app/models/time_entry.rb
+++ b/app/models/time_entry.rb
@@ -189,6 +189,13 @@ class TimeEntry < ActiveRecord::Base
     editable_custom_field_values(user).map(&:custom_field).uniq
   end
 
+  def visible_custom_field_values(user = nil)
+    user ||= User.current
+    custom_field_values.select do |value|
+      value.custom_field.visible_by?(project, user)
+    end
+  end
+
   def assignable_users
     users = []
     if project
diff --git a/app/models/time_entry_custom_field.rb b/app/models/time_entry_custom_field.rb
index 9ef820e..601e1b9 100644
--- a/app/models/time_entry_custom_field.rb
+++ b/app/models/time_entry_custom_field.rb
@@ -21,5 +21,13 @@ class TimeEntryCustomField < CustomField
   def type_name
     :label_spent_time
   end
-end
 
+  def visible_by?(project, user=User.current)
+    super || (roles & user.roles_for_project(project)).present?
+  end
+
+  def validate_custom_field
+    super
+    errors.add(:base, l(:label_role_plural) + ' ' + l('activerecord.errors.messages.blank')) unless visible? || roles.present?
+  end
+end
diff --git a/app/views/custom_fields/_form.html.erb b/app/views/custom_fields/_form.html.erb
index cac457c..e428a27 100644
--- a/app/views/custom_fields/_form.html.erb
+++ b/app/views/custom_fields/_form.html.erb
@@ -31,83 +31,75 @@
 </div>
 
 <div class="splitcontentright">
-<div class="box tabular">
 <% case @custom_field.class.name
 when "IssueCustomField" %>
-    <p><%= f.check_box :is_required %></p>
-    <% if @custom_field.format.is_filter_supported %>
-    <p><%= f.check_box :is_filter %></p>
-    <% end %>
-    <% if @custom_field.format.searchable_supported %>
-    <p><%= f.check_box :searchable %></p>
-    <% end %>
+    <div class="box tabular">
+      <p><%= f.check_box :is_required %></p>
+      <% if @custom_field.format.is_filter_supported %>
+      <p><%= f.check_box :is_filter %></p>
+      <% end %>
+      <% if @custom_field.format.searchable_supported %>
+      <p><%= f.check_box :searchable %></p>
+      <% end %>
+    </div>
+    <%= render :partial => 'visibility_by_role_selector' %>
 
 <% when "UserCustomField" %>
-    <p><%= f.check_box :is_required %></p>
-    <p><%= f.check_box :visible %></p>
-    <p><%= f.check_box :editable %></p>
-    <% if @custom_field.format.is_filter_supported %>
-    <p><%= f.check_box :is_filter %></p>
-    <% end %>
+    <div class="box tabular">
+      <p><%= f.check_box :is_required %></p>
+      <p><%= f.check_box :visible %></p>
+      <p><%= f.check_box :editable %></p>
+      <% if @custom_field.format.is_filter_supported %>
+      <p><%= f.check_box :is_filter %></p>
+      <% end %>
+    </div>
 
 <% when "ProjectCustomField" %>
-    <p><%= f.check_box :is_required %></p>
-    <p><%= f.check_box :visible %></p>
-    <% if @custom_field.format.searchable_supported %>
-    <p><%= f.check_box :searchable %></p>
-    <% end %>
-    <% if @custom_field.format.is_filter_supported %>
-    <p><%= f.check_box :is_filter %></p>
-    <% end %>
+    <div class="box tabular">
+      <p><%= f.check_box :is_required %></p>
+      <p><%= f.check_box :visible %></p>
+      <% if @custom_field.format.searchable_supported %>
+      <p><%= f.check_box :searchable %></p>
+      <% end %>
+      <% if @custom_field.format.is_filter_supported %>
+      <p><%= f.check_box :is_filter %></p>
+      <% end %>
+    </div>
 
 <% when "VersionCustomField" %>
-    <p><%= f.check_box :is_required %></p>
-    <% if @custom_field.format.is_filter_supported %>
-    <p><%= f.check_box :is_filter %></p>
-    <% end %>
+    <div class="box tabular">
+      <p><%= f.check_box :is_required %></p>
+      <% if @custom_field.format.is_filter_supported %>
+      <p><%= f.check_box :is_filter %></p>
+      <% end %>
+    </div>
 
 <% when "GroupCustomField" %>
-    <p><%= f.check_box :is_required %></p>
-    <% if @custom_field.format.is_filter_supported %>
-    <p><%= f.check_box :is_filter %></p>
-    <% end %>
+    <div class="box tabular">
+      <p><%= f.check_box :is_required %></p>
+      <% if @custom_field.format.is_filter_supported %>
+      <p><%= f.check_box :is_filter %></p>
+      <% end %>
+    </div>
 
 <% when "TimeEntryCustomField" %>
-    <p><%= f.check_box :is_required %></p>
-    <% if @custom_field.format.is_filter_supported %>
-    <p><%= f.check_box :is_filter %></p>
-    <% end %>
+    <div class="box tabular">
+      <p><%= f.check_box :is_required %></p>
+      <% if @custom_field.format.is_filter_supported %>
+      <p><%= f.check_box :is_filter %></p>
+      <% end %>
+    </div>
+    <%= render :partial => 'visibility_by_role_selector' %>
 
 <% else %>
+  <div class="box tabular">
     <p><%= f.check_box :is_required %></p>
-
+  </div>
 <% end %>
 <%= call_hook(:"view_custom_fields_form_#{@custom_field.type.to_s.underscore}", :custom_field => @custom_field, :form => f) %>
-</div>
 
 <% if @custom_field.is_a?(IssueCustomField) %>
 
-  <fieldset class="box tabular"><legend><%= l(:field_visible) %></legend>
-    <label class="block">
-      <%= radio_button_tag 'custom_field[visible]', 1, @custom_field.visible?, :id => 'custom_field_visible_on',
-            :data => {:disables => '.custom_field_role input'} %>
-      <%= l(:label_visibility_public) %>
-    </label>
-    <label class="block">
-      <%= radio_button_tag 'custom_field[visible]', 0, !@custom_field.visible?, :id => 'custom_field_visible_off',
-            :data => {:enables => '.custom_field_role input'} %>
-      <%= l(:label_visibility_roles) %>:
-    </label>
-    <% role_ids = @custom_field.role_ids %>
-    <% Role.givable.sorted.each do |role| %>
-      <label class="block custom_field_role" style="padding-left:2em;">
-        <%= check_box_tag 'custom_field[role_ids][]', role.id, role_ids.include?(role.id), :id => nil %>
-        <%= role.name %>
-      </label>
-    <% end %>
-    <%= hidden_field_tag 'custom_field[role_ids][]', '' %>
-  </fieldset>
-
   <fieldset class="box" id="custom_field_tracker_ids"><legend><%= toggle_checkboxes_link("#custom_field_tracker_ids input[type=checkbox]") %><%=l(:label_tracker_plural)%></legend>
   <% tracker_ids = @custom_field.tracker_ids %>
   <% Tracker.sorted.each do |tracker| %>
diff --git a/app/views/custom_fields/_visibility_by_role_selector.html.erb b/app/views/custom_fields/_visibility_by_role_selector.html.erb
new file mode 100644
index 0000000..552bc79
--- /dev/null
+++ b/app/views/custom_fields/_visibility_by_role_selector.html.erb
@@ -0,0 +1,20 @@
+<fieldset class="box tabular"><legend><%= l(:field_visible) %></legend>
+  <label class="block">
+    <%= radio_button_tag 'custom_field[visible]', 1, @custom_field.visible?, :id => 'custom_field_visible_on',
+          :data => {:disables => '.custom_field_role input'} %>
+    <%= l(:label_visibility_public) %>
+  </label>
+  <label class="block">
+    <%= radio_button_tag 'custom_field[visible]', 0, !@custom_field.visible?, :id => 'custom_field_visible_off',
+          :data => {:enables => '.custom_field_role input'} %>
+    <%= l(:label_visibility_roles) %>:
+  </label>
+  <% role_ids = @custom_field.role_ids %>
+  <% Role.givable.sorted.each do |role| %>
+    <label class="block custom_field_role" style="padding-left:2em;">
+      <%= check_box_tag 'custom_field[role_ids][]', role.id, role_ids.include?(role.id), :id => nil %>
+      <%= role.name %>
+    </label>
+  <% end %>
+  <%= hidden_field_tag 'custom_field[role_ids][]', '' %>
+</fieldset>
diff --git a/app/views/timelog/_form.html.erb b/app/views/timelog/_form.html.erb
index 691da9e..bfeb673 100644
--- a/app/views/timelog/_form.html.erb
+++ b/app/views/timelog/_form.html.erb
@@ -23,7 +23,7 @@
   <p><%= f.hours_field :hours, :size => 6, :required => true %></p>
   <p><%= f.text_field :comments, :size => 100, :maxlength => 1024, :required => Setting.timelog_required_fields.include?('comments') %></p>
   <p><%= f.select :activity_id, activity_collection_for_select_options(@time_entry), :required => true %></p>
-  <% @time_entry.custom_field_values.each do |value| %>
+  <% @time_entry.editable_custom_field_values.each do |value| %>
     <p><%= custom_field_tag_with_label :time_entry, value %></p>
   <% end %>
   <%= call_hook(:view_timelog_edit_form_bottom, { :time_entry => @time_entry, :form => f }) %>
diff --git a/app/views/timelog/index.api.rsb b/app/views/timelog/index.api.rsb
index d281e89..9767148 100644
--- a/app/views/timelog/index.api.rsb
+++ b/app/views/timelog/index.api.rsb
@@ -12,7 +12,7 @@ api.array :time_entries, api_meta(:total_count => @entry_count, :offset => @offs
       api.created_on time_entry.created_on
       api.updated_on time_entry.updated_on
 
-      render_api_custom_values time_entry.custom_field_values, api
+      render_api_custom_values time_entry.visible_custom_field_values, api
     end
   end
 end
diff --git a/test/functional/custom_fields_controller_test.rb b/test/functional/custom_fields_controller_test.rb
index 9745e23..e59d408 100644
--- a/test/functional/custom_fields_controller_test.rb
+++ b/test/functional/custom_fields_controller_test.rb
@@ -95,12 +95,37 @@ class CustomFieldsControllerTest < Redmine::ControllerTest
         assert_select 'option[value=user]', :text => 'User'
         assert_select 'option[value=version]', :text => 'Version'
       end
+
+      # Visibility
+      assert_select 'input[type=radio][name=?]', 'custom_field[visible]', 2
+      assert_select 'input[type=checkbox][name=?]', 'custom_field[role_ids][]', 3
+
       assert_select 'input[type=checkbox][name=?]', 'custom_field[project_ids][]', Project.count
       assert_select 'input[type=hidden][name=?]', 'custom_field[project_ids][]', 1
       assert_select 'input[type=hidden][name=type][value=IssueCustomField]'
     end
   end
 
+  def test_new_time_entry_custom_field
+    get :new, :params => {
+        :type => 'TimeEntryCustomField'
+      }
+    assert_response :success
+
+    assert_select 'form#custom_field_form' do
+      assert_select 'select#custom_field_field_format[name=?]', 'custom_field[field_format]' do
+        assert_select 'option[value=user]', :text => 'User'
+        assert_select 'option[value=version]', :text => 'Version'
+      end
+
+      # Visibility
+      assert_select 'input[type=radio][name=?]', 'custom_field[visible]', 2
+      assert_select 'input[type=checkbox][name=?]', 'custom_field[role_ids][]', 3
+
+      assert_select 'input[type=hidden][name=type][value=TimeEntryCustomField]'
+    end
+  end
+
   def test_new_time_entry_custom_field_should_not_show_trackers_and_projects
     get :new, :params => {
         :type => 'TimeEntryCustomField'
diff --git a/test/functional/timelog_custom_fields_visibility_test.rb b/test/functional/timelog_custom_fields_visibility_test.rb
index 4304e2d..d43f3a3 100644
--- a/test/functional/timelog_custom_fields_visibility_test.rb
+++ b/test/functional/timelog_custom_fields_visibility_test.rb
@@ -34,37 +34,9 @@ class TimelogCustomFieldsVisibilityTest < Redmine::ControllerTest
            :workflows,
            :custom_fields, :custom_values, :custom_fields_trackers
 
-  def setup
-    field_attributes = {:field_format => 'string', :is_for_all => true, :is_filter => true, :trackers => Tracker.all}
-    @fields = []
-    @fields << (@field1 = IssueCustomField.create!(field_attributes.merge(:name => 'Field 1', :visible => true)))
-    @fields << (@field2 = IssueCustomField.create!(field_attributes.merge(:name => 'Field 2', :visible => false, :role_ids => [1, 2])))
-    @fields << (@field3 = IssueCustomField.create!(field_attributes.merge(:name => 'Field 3', :visible => false, :role_ids => [1, 3])))
-    @issue = Issue.generate!(
-      :author_id => 1,
-      :project_id => 1,
-      :tracker_id => 1,
-      :custom_field_values => {@field1.id => 'Value0', @field2.id => 'Value1', @field3.id => 'Value2'}
-    )
-    TimeEntry.generate!(:issue => @issue)
-
-    @user_with_role_on_other_project = User.generate!
-    User.add_to_project(@user_with_role_on_other_project, Project.find(2), Role.find(3))
-
-    @users_to_test = {
-      User.find(1) => [@field1, @field2, @field3],
-      User.find(3) => [@field1, @field2],
-      @user_with_role_on_other_project => [@field1], # should see field1 only on Project 1
-      User.generate! => [@field1],
-      User.anonymous => [@field1]
-    }
-
-    Member.where(:project_id => 1).each do |member|
-      member.destroy unless @users_to_test.keys.include?(member.principal)
-    end
-  end
-
   def test_index_should_show_visible_custom_fields_only
+    prepare_test_data
+
     @users_to_test.each do |user, fields|
       @request.session[:user_id] = user.id
       get :index, :params => {
@@ -83,6 +55,8 @@ class TimelogCustomFieldsVisibilityTest < Redmine::ControllerTest
   end
 
   def test_index_as_csv_should_show_visible_custom_fields_only
+    prepare_test_data
+
     @users_to_test.each do |user, fields|
       @request.session[:user_id] = user.id
       get :index, :params => {
@@ -102,6 +76,8 @@ class TimelogCustomFieldsVisibilityTest < Redmine::ControllerTest
   end
 
   def test_index_with_partial_custom_field_visibility_should_show_visible_custom_fields_only
+    prepare_test_data
+
     Issue.delete_all
     TimeEntry.delete_all
     CustomValue.delete_all
@@ -131,4 +107,53 @@ class TimelogCustomFieldsVisibilityTest < Redmine::ControllerTest
     assert_select 'td', :text => "ValueC"
     assert_select 'td', :text => "ValueB", :count => 0
   end
+
+  def test_edit_should_not_show_custom_fields_not_visible_for_user
+    time_entry_cf = TimeEntryCustomField.find(10)
+    time_entry_cf.visible = false
+    time_entry_cf.role_ids = [2]
+    time_entry_cf.save!
+
+    @request.session[:user_id] = 2
+
+    get :edit, :params => {
+      :id => 3,
+      :project_id => 1
+    }
+
+    assert_response :success
+    assert_select 'select#time_entry_custom_field_values_10', 0
+  end
+
+  private
+
+  def prepare_test_data
+    field_attributes = {:field_format => 'string', :is_for_all => true, :is_filter => true, :trackers => Tracker.all}
+    @fields = []
+    @fields << (@field1 = IssueCustomField.create!(field_attributes.merge(:name => 'Field 1', :visible => true)))
+    @fields << (@field2 = IssueCustomField.create!(field_attributes.merge(:name => 'Field 2', :visible => false, :role_ids => [1, 2])))
+    @fields << (@field3 = IssueCustomField.create!(field_attributes.merge(:name => 'Field 3', :visible => false, :role_ids => [1, 3])))
+    @issue = Issue.generate!(
+      :author_id => 1,
+      :project_id => 1,
+      :tracker_id => 1,
+      :custom_field_values => {@field1.id => 'Value0', @field2.id => 'Value1', @field3.id => 'Value2'}
+    )
+    TimeEntry.generate!(:issue => @issue)
+
+    @user_with_role_on_other_project = User.generate!
+    User.add_to_project(@user_with_role_on_other_project, Project.find(2), Role.find(3))
+
+    @users_to_test = {
+      User.find(1) => [@field1, @field2, @field3],
+      User.find(3) => [@field1, @field2],
+      @user_with_role_on_other_project => [@field1], # should see field1 only on Project 1
+      User.generate! => [@field1],
+      User.anonymous => [@field1]
+    }
+
+    Member.where(:project_id => 1).each do |member|
+      member.destroy unless @users_to_test.keys.include?(member.principal)
+    end
+  end
 end
diff --git a/test/functional/timelog_report_test.rb b/test/functional/timelog_report_test.rb
index 0043230..a643522 100644
--- a/test/functional/timelog_report_test.rb
+++ b/test/functional/timelog_report_test.rb
@@ -138,7 +138,7 @@ class TimelogReportTest < Redmine::ControllerTest
 
   def test_hidden_custom_fields_should_not_be_proposed
     TimeEntryCustomField.create!(name: 'shown', field_format: 'list', possible_values: ['value1', 'value2'], visible: true)
-    TimeEntryCustomField.create!(name: 'Hidden', field_format: 'list', possible_values: ['value1', 'value2'], visible: false)
+    TimeEntryCustomField.create!(name: 'Hidden', field_format: 'list', possible_values: ['value1', 'value2'], visible: false, role_ids: [3])
 
     get :report, :params => {:project_id => 1}
     assert_response :success
-- 
2.1.4