From 51a6dc6674756ed48ee9fc09dbc53520e98040fa Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Wed, 13 Nov 2024 13:30:44 +0800
Subject: [PATCH] project setting to deny time logging on closed issues

---
 app/controllers/context_menus_controller.rb   |  2 +-
 app/models/issue.rb                           |  5 ++
 app/models/project.rb                         |  3 +-
 app/models/time_entry.rb                      |  3 +
 app/views/issues/_action_menu.html.erb        |  2 +-
 app/views/issues/_edit.html.erb               |  2 +-
 app/views/projects/settings/_issues.html.erb  |  8 +++
 config/locales/de.yml                         |  3 +
 config/locales/en.yml                         |  3 +
 ...melog_on_closed_issues_flag_to_projects.rb |  5 ++
 test/functional/issues_controller_test.rb     | 61 +++++++++++++++++++
 test/unit/time_entry_test.rb                  | 11 ++++
 12 files changed, 104 insertions(+), 4 deletions(-)
 create mode 100644 db/migrate/20241113033816_add_timelog_on_closed_issues_flag_to_projects.rb

diff --git a/app/controllers/context_menus_controller.rb b/app/controllers/context_menus_controller.rb
index 4c7305873f..1e37f623be 100644
--- a/app/controllers/context_menus_controller.rb
+++ b/app/controllers/context_menus_controller.rb
@@ -33,7 +33,7 @@ class ContextMenusController < ApplicationController
 
     @can = {
       :edit => @issues.all?(&:attributes_editable?),
-      :log_time => (@project && User.current.allowed_to?(:log_time, @project)),
+      :log_time => @issue&.time_loggable?,
       :copy => User.current.allowed_to?(:copy_issues, @projects) && Issue.allowed_target_projects.any?,
       :add_watchers => User.current.allowed_to?(:add_issue_watchers, @projects),
       :delete => @issues.all?(&:deletable?),
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 980bd56f0c..f85b31e4ef 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -188,6 +188,11 @@ class Issue < ApplicationRecord
     end
   end
 
+  # Returns true if user or current user is allowed to log time on the issue
+  def time_loggable?(user=User.current)
+    user.allowed_to?(:log_time, project) && (project.timelog_on_closed_issues? || !closed?)
+  end
+
   # Returns true if user or current user is allowed to edit or add notes to the issue
   def editable?(user=User.current)
     attributes_editable?(user) || notes_addable?(user)
diff --git a/app/models/project.rb b/app/models/project.rb
index c438be16d4..e972e0f38f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -834,7 +834,8 @@ class Project < ApplicationRecord
     'parent_id',
     'default_version_id',
     'default_issue_query_id',
-    'default_assigned_to_id')
+    'default_assigned_to_id',
+    'timelog_on_closed_issues')
 
   safe_attributes(
     'is_public',
diff --git a/app/models/time_entry.rb b/app/models/time_entry.rb
index abbbe44412..c82768ff32 100644
--- a/app/models/time_entry.rb
+++ b/app/models/time_entry.rb
@@ -182,6 +182,9 @@ class TimeEntry < ApplicationRecord
     if spent_on && spent_on_changed? && user
       errors.add :base, I18n.t(:error_spent_on_future_date) if !Setting.timelog_accept_future_dates? && (spent_on > user.today)
     end
+    if project && !project.timelog_on_closed_issues? && issue&.closed? && issue&.was_closed?
+      errors.add :base, I18n.t(:error_timelog_on_closed_issue)
+    end
   end
 
   def hours=(h)
diff --git a/app/views/issues/_action_menu.html.erb b/app/views/issues/_action_menu.html.erb
index 6009df89e4..8b5d3021d1 100644
--- a/app/views/issues/_action_menu.html.erb
+++ b/app/views/issues/_action_menu.html.erb
@@ -3,7 +3,7 @@
             :onclick => 'showAndScrollTo("update", "issue_notes"); return false;',
             :class => 'icon icon-edit ', :accesskey => accesskey(:edit) if @issue.editable? %>
 <%= link_to sprite_icon('time-add', l(:button_log_time)), new_issue_time_entry_path(@issue),
-            :class => 'icon icon-time-add ' if User.current.allowed_to?(:log_time, @project) %>
+            :class => 'icon icon-time-add ' if @issue.time_loggable? %>
 <%= watcher_link(@issue, User.current) %>
 <%= link_to sprite_icon('copy', l(:button_copy)), project_copy_issue_path(@project, @issue),
             :class => 'icon icon-copy ' if User.current.allowed_to?(:copy_issues, @project) && Issue.allowed_target_projects.any? %>
diff --git a/app/views/issues/_edit.html.erb b/app/views/issues/_edit.html.erb
index f00b65750e..5692eb80bd 100644
--- a/app/views/issues/_edit.html.erb
+++ b/app/views/issues/_edit.html.erb
@@ -9,7 +9,7 @@
         </div>
         </fieldset>
     <% end %>
-    <% if User.current.allowed_to?(:log_time, @issue.project) %>
+    <% if @issue.time_loggable? %>
         <fieldset class="tabular" id="log_time"><legend><%= l(:button_log_time) %></legend>
         <%= labelled_fields_for :time_entry, @time_entry do |time_entry| %>
         <div class="splitcontent">
diff --git a/app/views/projects/settings/_issues.html.erb b/app/views/projects/settings/_issues.html.erb
index c54c56375c..3ba01fc7d7 100644
--- a/app/views/projects/settings/_issues.html.erb
+++ b/app/views/projects/settings/_issues.html.erb
@@ -48,5 +48,13 @@
   <% end %>
   </div>
 
+  <% if @project.module_enabled?('time_tracking') %>
+  <div class="box tabular">
+    <p>
+      <%= f.check_box :timelog_on_closed_issues %><em class="info"><%=l(:text_timelog_on_closed_issues)%></em>
+    </p>
+  </div>
+  <% end %>
+
   <p><%= submit_tag l(:button_save) %></p>
 <% end %>
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 527afa2d1c..07f938c463 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -1462,3 +1462,6 @@ de:
     zero: "%{filename}"
     one: "%{filename} and 1 file"
     other: "%{filename} and %{count} files"
+  field_timelog_on_closed_issues: Zeiterfassung an geschlossenen Aufgaben
+  text_timelog_on_closed_issues: Deaktivieren um die Zeiterfassung an geschlossenen Aufgaben zu verhindern.
+  error_timelog_on_closed_issue: Zeiterfassung an geschlossenen Aufgaben ist nicht möglich.
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 9ac73ff449..d3e65ac110 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1424,3 +1424,6 @@ en:
   text_user_destroy_confirmation: "Are you sure you want to delete this user and remove all references to them? This cannot be undone. Often, locking a user instead of deleting them is the better solution. To confirm, please enter their login (%{login}) below."
   text_project_destroy_enter_identifier: "To confirm, please enter the project's identifier (%{identifier}) below."
   field_name_or_email_or_login: Name, email or login
+  field_timelog_on_closed_issues: Time tracking on closed issues
+  text_timelog_on_closed_issues: Uncheck to prevent users from logging time on closed issues.
+  error_timelog_on_closed_issue: Cannot log time on a closed issue
diff --git a/db/migrate/20241113033816_add_timelog_on_closed_issues_flag_to_projects.rb b/db/migrate/20241113033816_add_timelog_on_closed_issues_flag_to_projects.rb
new file mode 100644
index 0000000000..fa6a673b55
--- /dev/null
+++ b/db/migrate/20241113033816_add_timelog_on_closed_issues_flag_to_projects.rb
@@ -0,0 +1,5 @@
+class AddTimelogOnClosedIssuesFlagToProjects < ActiveRecord::Migration[5.2]
+  def change
+    add_column :projects, :timelog_on_closed_issues, :bool, default: true, null: false
+  end
+end
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index 3b7521d407..a2e9095b05 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -5912,6 +5912,15 @@ class IssuesControllerTest < Redmine::ControllerTest
     assert_select 'input[name=?]', 'time_entry[hours]', 0
   end
 
+  def test_get_edit_should_not_display_the_time_entry_form_on_closed_issue
+    @request.session[:user_id] = 2
+    issue = Issue.find(1)
+    issue.project.update_attribute(:timelog_on_closed_issues, false)
+    issue.update :status => IssueStatus.find(5)
+    get(:edit, :params => {:id => 1})
+    assert_select 'input[name=?]', 'time_entry[hours]', 0
+  end
+
   def test_get_edit_with_params
     @request.session[:user_id] = 2
     get(
@@ -6365,6 +6374,58 @@ class IssuesControllerTest < Redmine::ControllerTest
     assert mail.subject.include?("(#{IssueStatus.find(2).name})")
   end
 
+  def test_update_should_accept_time_entry_when_closing_issue
+    issue = Issue.find(1)
+    issue.project.update_attribute(:timelog_on_closed_issues, false)
+    assert_equal 1, issue.status_id
+    @request.session[:user_id] = 2
+
+    assert_difference('TimeEntry.count', 1) do
+      put(
+        :update,
+        :params => {
+          :id => 1,
+          :issue => {
+            :status_id => 5,
+          },
+          :time_entry => {
+            :hours => '2',
+            :comments => '',
+            :activity_id => TimeEntryActivity.first
+          }
+        }
+      )
+    end
+
+    assert_redirected_to :action => 'show', :id => '1'
+    issue.reload
+    assert issue.closed?
+  end
+
+  def test_update_should_not_accept_time_entry_on_closed_issue
+    issue = Issue.find(1)
+    issue.project.update_attribute(:timelog_on_closed_issues, false)
+    issue.update :status => IssueStatus.find(5)
+    @request.session[:user_id] = 2
+
+    assert_no_difference('TimeEntry.count') do
+      put(
+        :update,
+        :params => {
+          :id => 1,
+          :issue => {
+          },
+          :time_entry => {
+            :hours => '2',
+            :comments => '',
+            :activity_id => TimeEntryActivity.first
+          }
+        }
+      )
+    end
+    assert_response :success
+  end
+
   def test_put_update_with_note_only
     notes = 'Note added by IssuesControllerTest#test_update_with_note_only'
 
diff --git a/test/unit/time_entry_test.rb b/test/unit/time_entry_test.rb
index 358408c995..8624d09c07 100644
--- a/test/unit/time_entry_test.rb
+++ b/test/unit/time_entry_test.rb
@@ -185,6 +185,17 @@ class TimeEntryTest < ActiveSupport::TestCase
     end
   end
 
+  def test_should_not_accept_closed_issue
+    project = Project.find(1)
+    project.update_attribute(:timelog_on_closed_issues, false)
+    entry = TimeEntry.generate project: project
+    issue = project.issues.to_a.detect(&:closed?)
+    entry.issue = issue
+    assert !entry.save
+    assert entry.errors[:base].present?
+    assert_equal 'Cannot log time on a closed issue', entry.errors[:base].first
+  end
+
   def test_should_require_spent_on
     with_settings :timelog_accept_future_dates => '0' do
       entry = TimeEntry.find(1)
-- 
2.47.1

