Merge pull request #17272 from opf/add-timezone-to-time-entry

Store timezones for TimeEntries and finalize how we determine end time

db/migrate/20241125104347_add_timezone_identifier_to_time_entry.rb

@@ -0,0 +1,13 @@
+class AddTimezoneIdentifierToTimeEntry < ActiveRecord::Migration[7.1]
+  def change
+    change_table :time_entries, bulk: true do |t|
+      t.string :time_zone, null: true
+      t.remove :end_time, type: :integer
+    end
+
+    change_table :time_entry_journals, bulk: true do |t|
+      t.string :time_zone, null: true
+      t.remove :end_time, type: :integer
+    end
+  end
+end

modules/costs/app/models/time_entry.rb

@@ -47,23 +47,14 @@ class TimeEntry < ApplicationRecord
   validates_presence_of :hours, if: -> { !ongoing? }
   validates_numericality_of :hours, allow_nil: true, message: :invalid
 
-  validates :start_time, :end_time,
+  validates :start_time, :hours,
             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
@@ -121,6 +112,21 @@ def costs_visible_by?(usr)
       (user_id == usr.id && usr.allowed_in_project?(:view_own_hourly_rate, project))
   end
 
+  def start_timestamp
+    return nil if start_time.blank?
+    return nil if time_zone.blank?
+
+    time_zone_object.local(spent_on.year, spent_on.month, spent_on.day, start_time / 60, start_time % 60)
+  end
+
+  def end_timestamp
+    return nil if start_time.blank?
+    return nil if time_zone.blank?
+    return nil if hours.blank?
+
+    start_timestamp + hours.hours
+  end
+
   class << self
     def can_track_start_and_end_time?(_project: nil)
       OpenProject::FeatureDecisions.track_start_and_end_times_for_time_entries_active? &&
@@ -139,4 +145,8 @@ def must_track_start_and_end_time?(_project: nil)
   def cost_attribute
     hours
   end
+
+  def time_zone_object
+    ActiveSupport::TimeZone[time_zone]
+  end
 end

modules/costs/spec/models/time_entry_spec.rb

@@ -60,8 +60,8 @@
            spent_on: date,
            hours:,
            start_time: start_time,
-           end_time: start_time + (hours * 60).to_i,
            user:,
+           time_zone: user.time_zone,
            rate: hourly_one,
            comments: "lorem")
   end
@@ -435,64 +435,66 @@ def ensure_membership(project, user, permissions)
       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
+      context "when enforcing 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 "#start_timestamp" do
+    it "returns nil if start_time is nil" do
+      time_entry.start_time = nil
+      expect(time_entry.start_timestamp).to be_nil
+    end
+
+    it "returns nil if timezone is nil" do
+      time_entry.time_zone = nil
+      expect(time_entry.start_timestamp).to be_nil
+    end
+
+    it "generates a proper timestamp from the stored information" do
+      time_entry.start_time = 14 * 60
+      time_entry.spent_on = Date.new(2024, 12, 24)
+      time_entry.time_zone = "America/Los_Angeles"
+
+      expect(time_entry.start_timestamp.iso8601).to eq("2024-12-24T14:00:00-08:00")
+    end
+  end
+
+  describe "#end_timestamp" do
+    it "returns nil if start_time is nil" do
+      time_entry.start_time = nil
+      expect(time_entry.end_timestamp).to be_nil
+    end
+
+    it "returns nil if timezone is nil" do
+      time_entry.time_zone = nil
+      expect(time_entry.end_timestamp).to be_nil
+    end
+
+    it "generates a proper timestamp from the stored information" do
+      time_entry.start_time = 8 * 60
+      time_entry.hours = 2.5
+      time_entry.spent_on = Date.new(2024, 12, 24)
+      time_entry.time_zone = "America/Los_Angeles"
+
+      expect(time_entry.end_timestamp.iso8601).to eq("2024-12-24T10:30:00-08:00")
+    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