diff --git a/modules/costs/app/models/time_entry.rb b/modules/costs/app/models/time_entry.rb
index c4ea44097a14..962f12750a5f 100644
--- a/modules/costs/app/models/time_entry.rb
+++ b/modules/costs/app/models/time_entry.rb
@@ -36,6 +36,9 @@ class TimeEntry < ApplicationRecord
belongs_to :rate, -> { where(type: %w[HourlyRate DefaultHourlyRate]) }, class_name: "Rate"
belongs_to :logged_by, class_name: "User"
+ MIN_TIME = 0 # => 00:00
+ MAX_TIME = (60 * 24) - 1 # => 23:59
+
acts_as_customizable
acts_as_journalized
@@ -44,6 +47,23 @@ class TimeEntry < ApplicationRecord
validates_presence_of :hours, if: -> { !ongoing? }
validates_numericality_of :hours, allow_nil: true, message: :invalid
+ validates :start_time, :end_time,
+ presence: true,
+ if: -> { TimeEntry.must_track_start_and_end_time? }
+
+ validates :start_time,
+ numericality: { only_integer: true, greater_than_or_equal_to: MIN_TIME, less_than_or_equal_to: MAX_TIME },
+ allow_blank: true
+
+ validates :end_time,
+ numericality: {
+ only_integer: true,
+ greater_than: ->(te) { te.start_time.to_i },
+ less_than_or_equal_to: MAX_TIME
+ # TODO: nice error message
+ },
+ allow_blank: true
+
scope :on_work_packages, ->(work_packages) { where(work_package_id: work_packages) }
extend ::TimeEntries::TimeEntryScopes
@@ -101,11 +121,17 @@ def costs_visible_by?(usr)
(user_id == usr.id && usr.allowed_in_project?(:view_own_hourly_rate, project))
end
- def self.can_track_start_and_end_time?(_project: nil)
- OpenProject::FeatureDecisions.track_start_and_end_times_for_time_entries_active? &&
- Setting.allow_tracking_start_and_end_times
+ class << self
+ def can_track_start_and_end_time?(_project: nil)
+ OpenProject::FeatureDecisions.track_start_and_end_times_for_time_entries_active? &&
+ Setting.allow_tracking_start_and_end_times?
+ # TODO: Add project check when we have decided if we also want a project specific flag
+ end
- # TODO: Add project check when we have decided if we also want a project specific flag
+ def must_track_start_and_end_time?(_project: nil)
+ can_track_start_and_end_time? && Setting.enforce_tracking_start_and_end_times?
+ # TODO: Add project check when we have decided if we also want a project specific flag
+ end
end
private
diff --git a/modules/costs/app/views/costs_settings/show.html.erb b/modules/costs/app/views/costs_settings/show.html.erb
index a45cecca679d..1f4be12f6635 100644
--- a/modules/costs/app/views/costs_settings/show.html.erb
+++ b/modules/costs/app/views/costs_settings/show.html.erb
@@ -56,6 +56,10 @@ See COPYRIGHT and LICENSE files for more details.
<%= setting_check_box :allow_tracking_start_and_end_times, container_class: "-middle" %>
+
+
+ <%= setting_check_box :enforce_tracking_start_and_end_times, container_class: "-middle" %>
+
<%- end %>
diff --git a/modules/costs/config/locales/en.yml b/modules/costs/config/locales/en.yml
index d07e35aee47f..9fee7d5e047a 100644
--- a/modules/costs/config/locales/en.yml
+++ b/modules/costs/config/locales/en.yml
@@ -151,9 +151,10 @@ en:
project_module_costs: "Time and costs"
- setting_allow_tracking_start_and_end_times: "Time records with start and end time"
+ setting_allow_tracking_start_and_end_times: "Allow users to track start and end time on time records"
setting_costs_currency: "Currency"
setting_costs_currency_format: "Format of currency"
+ setting_enforce_tracking_start_and_end_times: "Force users to set start and end time on time records"
text_assign_time_and_cost_entries_to_project: "Assign reported hours and costs to the project"
text_destroy_cost_entries_question: "%{cost_entries} were reported on the work packages you are about to delete. What do you want to do ?"
diff --git a/modules/costs/lib/costs/engine.rb b/modules/costs/lib/costs/engine.rb
index 7963f2c70958..553062b13386 100644
--- a/modules/costs/lib/costs/engine.rb
+++ b/modules/costs/lib/costs/engine.rb
@@ -143,6 +143,7 @@ class Engine < ::Rails::Engine
::Settings::Definition.add "costs_currency", default: "EUR", format: :string
::Settings::Definition.add "costs_currency_format", default: "%n %u", format: :string
::Settings::Definition.add "allow_tracking_start_and_end_times", default: false, format: :boolean
+ ::Settings::Definition.add "enforce_tracking_start_and_end_times", default: false, format: :boolean
end
initializer "costs.feature_decisions" do
diff --git a/modules/costs/spec/models/time_entry_spec.rb b/modules/costs/spec/models/time_entry_spec.rb
index 7bb2348fe4b9..521288515a69 100644
--- a/modules/costs/spec/models/time_entry_spec.rb
+++ b/modules/costs/spec/models/time_entry_spec.rb
@@ -52,12 +52,15 @@
let!(:default_hourly_three) { create(:default_hourly_rate, valid_from: 4.days.ago, project:, user: user2) }
let!(:default_hourly_five) { create(:default_hourly_rate, valid_from: 6.days.ago, project:, user: user2) }
let(:hours) { 5.0 }
+ let(:start_time) { 10 * 60 } # 10:00
let(:time_entry) do
create(:time_entry,
project:,
work_package:,
spent_on: date,
hours:,
+ start_time: start_time,
+ end_time: start_time + (hours * 60).to_i,
user:,
rate: hourly_one,
comments: "lorem")
@@ -408,4 +411,131 @@ def ensure_membership(project, user, permissions)
end
end
end
+
+ describe "validations" do
+ describe "start_time" do
+ it "allows blank values" do
+ time_entry.start_time = nil
+ expect(time_entry).to be_valid
+ end
+
+ it "allows integer values between 0 and 1440" do
+ time_entry.start_time = (5 * 60) + 30
+ expect(time_entry).to be_valid
+ end
+
+ it "does not allow non integer values" do
+ time_entry.start_time = 1.5
+ expect(time_entry).not_to be_valid
+ end
+
+ it "does not allow negative values" do
+ time_entry.start_time = -42
+ expect(time_entry).not_to be_valid
+ end
+ end
+
+ describe "end_time" do
+ it "allows blank values" do
+ time_entry.end_time = nil
+ expect(time_entry).to be_valid
+ end
+
+ it "allows integer values between 0 and 1439" do
+ time_entry.end_time = 1337
+ expect(time_entry).to be_valid
+ end
+
+ it "does not allow values > 1439" do
+ time_entry.end_time = 1440
+ expect(time_entry).not_to be_valid
+ end
+
+ it "does not allow non integer values" do
+ time_entry.end_time = 1.5
+ expect(time_entry).not_to be_valid
+ end
+
+ it "does not allow negative values" do
+ time_entry.end_time = -42
+ expect(time_entry).not_to be_valid
+ end
+ end
+
+ describe "start_time and end_time" do
+ it "does not allow end times smaller than the start time" do
+ time_entry.start_time = 10 * 60
+ time_entry.end_time = 8 * 60
+
+ expect(time_entry).not_to be_valid
+ end
+
+ context "when enforcing start and end times" do
+ before do
+ allow(described_class).to receive(:must_track_start_and_end_time?).and_return(true)
+ end
+
+ it "validates that both values are present" do
+ time_entry.start_time = nil
+ time_entry.end_time = nil
+
+ expect(time_entry).not_to be_valid
+
+ time_entry.start_time = 10 * 60
+
+ expect(time_entry).not_to be_valid
+
+ time_entry.end_time = 12 * 60
+
+ expect(time_entry).to be_valid
+ end
+ end
+ end
+ end
+
+ describe ".must_track_start_and_end_time?" do
+ context "with the feature flag enabled", with_flag: { track_start_and_end_times_for_time_entries: true } do
+ context "with the allow setting enabled", with_settings: { allow_tracking_start_and_end_times: true } do
+ context "with the enforce setting enabled", with_settings: { enforce_tracking_start_and_end_times: true } do
+ it { expect(described_class).to be_must_track_start_and_end_time }
+ end
+
+ context "with the enforce setting disabled", with_settings: { enforce_tracking_start_and_end_times: false } do
+ it { expect(described_class).not_to be_must_track_start_and_end_time }
+ end
+ end
+
+ context "with the allow setting disabled", with_settings: { allow_tracking_start_and_end_times: false } do
+ context "with the enforce setting enabled", with_settings: { enforce_tracking_start_and_end_times: true } do
+ it { expect(described_class).not_to be_must_track_start_and_end_time }
+ end
+
+ context "with the enforce setting disabled", with_settings: { enforce_tracking_start_and_end_times: false } do
+ it { expect(described_class).not_to be_must_track_start_and_end_time }
+ end
+ end
+ end
+
+ context "with the feature flag disabled", with_flag: { track_start_and_end_times_for_time_entries: false } do
+ context "with the allow setting enabled", with_settings: { allow_tracking_start_and_end_times: true } do
+ context "with the enforce setting enabled", with_settings: { enforce_tracking_start_and_end_times: true } do
+ it { expect(described_class).not_to be_must_track_start_and_end_time }
+ end
+
+ context "with the enforce setting disabled", with_settings: { enforce_tracking_start_and_end_times: false } do
+ it { expect(described_class).not_to be_must_track_start_and_end_time }
+ end
+ end
+
+ context "with the allow setting disabled", with_settings: { allow_tracking_start_and_end_times: false } do
+ context "with the enforce setting enabled", with_settings: { enforce_tracking_start_and_end_times: true } do
+ it { expect(described_class).not_to be_must_track_start_and_end_time }
+ end
+
+ context "with the enforce setting disabled", with_settings: { enforce_tracking_start_and_end_times: false } do
+ it { expect(described_class).not_to be_must_track_start_and_end_time }
+ end
+ end
+ end
+ end
end