Merge pull request #17249 from opf/implementation/59467-add-global-an…

…d-project-settings-to-enforce-input-of-start-and-end-times

Implementation/59467 add global settings to enforce input of start and end times

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

modules/costs/app/views/costs_settings/show.html.erb

@@ -56,6 +56,10 @@ See COPYRIGHT and LICENSE files for more details.
           <div class="form--field">
             <%= setting_check_box :allow_tracking_start_and_end_times, container_class: "-middle" %>
           </div>
+
+          <div class="form--field">
+            <%= setting_check_box :enforce_tracking_start_and_end_times, container_class: "-middle" %>
+          </div>
         <%- end %>
       </section>
 

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 ?"

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

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