diff --git a/app/components/release_list_component.rb b/app/components/release_list_component.rb index a3d076449..ee0d9f3db 100644 --- a/app/components/release_list_component.rb +++ b/app/components/release_list_component.rb @@ -151,7 +151,7 @@ def no_release_empty_state else { title: "Upcoming release", - text: "Your first scheduled release will automatically kick-off at #{train.kickoff_at.to_fs(:short)}. You can also manually run a new release by clicking the prepare button." + text: "Your first scheduled release will automatically kick-off at #{train.next_run_at.in_time_zone(train.app.timezone).to_fs(:short)}. You can also manually run a new release by clicking the prepare button." } end else diff --git a/app/controllers/trains_controller.rb b/app/controllers/trains_controller.rb index 2a14839f9..abbe2fbc8 100644 --- a/app/controllers/trains_controller.rb +++ b/app/controllers/trains_controller.rb @@ -107,7 +107,7 @@ def train_params :versioning_strategy, :release_backmerge_branch, :release_branch, - :kickoff_at, + :kickoff_time, :repeat_duration_value, :repeat_duration_unit, :notification_channel, @@ -166,7 +166,7 @@ def train_update_params :build_queue_size, :build_queue_wait_time_unit, :build_queue_wait_time_value, - :kickoff_at, + :kickoff_time, :repeat_duration_value, :repeat_duration_unit, :release_schedule_enabled, @@ -244,7 +244,7 @@ def build_queue_config(config_params) end def release_schedule_config_params - [:release_schedule_enabled, :kickoff_at, :repeat_duration_value, :repeat_duration_unit, :stop_automatic_releases_on_failure] + [:release_schedule_enabled, :kickoff_time, :repeat_duration_value, :repeat_duration_unit, :stop_automatic_releases_on_failure] end def release_schedule_config(config_params) @@ -252,7 +252,7 @@ def release_schedule_config(config_params) { repeat_duration: parsed_duration(config_params[:repeat_duration_value], config_params[:repeat_duration_unit]), - kickoff_at: config_params[:kickoff_at]&.time_in_utc, + kickoff_time: config_params[:kickoff_time], stop_automatic_releases_on_failure: config_params[:stop_automatic_releases_on_failure] } end diff --git a/app/helpers/trains_helper.rb b/app/helpers/trains_helper.rb index 3750c4a4b..5e0adb183 100644 --- a/app/helpers/trains_helper.rb +++ b/app/helpers/trains_helper.rb @@ -1,10 +1,10 @@ module TrainsHelper def release_schedule(train) if train.automatic? - date = time_format(train.kickoff_at, with_year: true, with_time: false) duration = train.repeat_duration.inspect - time = train.kickoff_at.strftime("%I:%M%p (%Z)") - "Kickoff at #{date} – runs every #{duration} at #{time}" + time = train.kickoff_time.strftime("%I:%M%p") + timezone = train.app.timezone + "Runs every #{duration} at #{time} (#{timezone})" else "No release schedule" end diff --git a/app/models/train.rb b/app/models/train.rb index affb9b382..f2612b2c4 100644 --- a/app/models/train.rb +++ b/app/models/train.rb @@ -16,7 +16,7 @@ # description :string # enable_changelog_linking_in_notifications :boolean default(FALSE) # freeze_version :boolean default(FALSE) -# kickoff_at :datetime +# kickoff_time :time # name :string not null # notification_channel :jsonb # notifications_release_specific_channel_enabled :boolean default(FALSE) @@ -116,7 +116,7 @@ class Train < ApplicationRecord validates :release_branch, presence: true, if: -> { branching_strategy == "parallel_working" } validate :version_compatibility, on: :create validate :ready?, on: :create - validate :valid_schedule, if: -> { kickoff_at_changed? || repeat_duration_changed? } + validate :valid_schedule, if: -> { kickoff_time_changed? || repeat_duration_changed? } validate :build_queue_config validate :backmerge_config validate :working_branch_presence, on: :create @@ -146,7 +146,7 @@ class Train < ApplicationRecord before_update :create_default_notification_settings, if: -> do notification_channel_changed? || notifications_release_specific_channel_enabled_changed? end - after_update :schedule_release!, if: -> { kickoff_at.present? && kickoff_at_previously_was.blank? } + after_update :schedule_release!, if: -> { kickoff_time.present? && kickoff_time_previously_was.blank? } def disable_copy_approvals self.copy_approvals = false @@ -213,13 +213,13 @@ def previously_finished_release end def automatic? - kickoff_at.present? && repeat_duration.present? + kickoff_time.present? && repeat_duration.present? end def next_run_at return unless automatic? - base_time = last_run_at + base_time = last_run_at || kickoff_datetime.utc now = Time.current return base_time if now < base_time @@ -230,11 +230,22 @@ def next_run_at end def runnable? + return false unless next_run_at + return true if last_run_at.nil? # First run is always runnable next_run_at > last_run_at end def last_run_at - scheduled_releases.last&.scheduled_at || kickoff_at + scheduled_releases.last&.scheduled_at + end + + # Converts kickoff_time to a datetime in the app's timezone for the next occurrence + def kickoff_datetime(reference_time = Time.current) + tz = app.timezone + ref_in_tz = reference_time.in_time_zone(tz) + + # Combine kickoff_time with reference date in app's timezone + tz.parse("#{ref_in_tz.to_date} #{kickoff_time.strftime('%H:%M:%S')}") end def diff_since_last_release? @@ -586,9 +597,8 @@ def ensure_deletable end def valid_schedule - if kickoff_at.present? || repeat_duration.present? - errors.add(:repeat_duration, "invalid schedule, provide both kickoff and period for repeat") unless kickoff_at.present? && repeat_duration.present? - errors.add(:kickoff_at, "the schedule kickoff should be in the future") if kickoff_at && kickoff_at <= Time.current + if kickoff_time.present? || repeat_duration.present? + errors.add(:repeat_duration, "invalid schedule, provide both kickoff time and period for repeat") unless kickoff_time.present? && repeat_duration.present? errors.add(:repeat_duration, "the repeat duration should be more than 1 day") if repeat_duration && repeat_duration < 1.day end end diff --git a/app/views/trains/_release_schedule_form.html.erb b/app/views/trains/_release_schedule_form.html.erb index ee12aa239..2b981060b 100644 --- a/app/views/trains/_release_schedule_form.html.erb +++ b/app/views/trains/_release_schedule_form.html.erb @@ -11,11 +11,10 @@ <% component.with_child do %>
- <%= form.labeled_datetime_field :kickoff_at, - "Kickoff at", - {min: Date.current, - data: {action: "domain--release-schedule-help#change", - domain__release_schedule_help_target: "kickoffDate"}} %> + <%= form.labeled_time_field :kickoff_time, + "Kickoff time", + {data: {action: "domain--release-schedule-help#change", + domain__release_schedule_help_target: "kickoffTime"}} %>
<%= form.label_only :repeat_duration, "Every" %> diff --git a/db/data/20240305124235_connect_scheduled_releases_with_release.rb b/db/data/20240305124235_connect_scheduled_releases_with_release.rb index fda6cfa60..161f035a8 100644 --- a/db/data/20240305124235_connect_scheduled_releases_with_release.rb +++ b/db/data/20240305124235_connect_scheduled_releases_with_release.rb @@ -3,7 +3,7 @@ def up return ActiveRecord::Base.transaction do # scan all automatic trains - Train.where.not(kickoff_at: nil).where.not(repeat_duration: nil).each do |train| + Train.where.not(kickoff_time: nil).where.not(repeat_duration: nil).each do |train| # check redundantly to be sure next unless train.automatic? diff --git a/db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb b/db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb new file mode 100644 index 000000000..1a527c231 --- /dev/null +++ b/db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +class ChangeKickoffAtToKickoffTime < ActiveRecord::Migration[7.0] + def up + # Step 1: Add new kickoff_time column (time without timezone) + add_column :trains, :kickoff_time, :time + + # Step 2: Migrate existing data + # Convert kickoff_at (UTC datetime) to time in app's timezone + reversible do |dir| + dir.up do + Train.reset_column_information + Train.find_each do |train| + next if train.kickoff_at.blank? + + # Convert kickoff_at (UTC) to app's timezone and extract time component + time_in_app_tz = train.kickoff_at.in_time_zone(train.app.timezone) + train.update_column(:kickoff_time, time_in_app_tz.strftime("%H:%M:%S")) + end + end + end + + # Step 3: Remove old column + safety_assured { remove_column :trains, :kickoff_at } + end + + def down + # Add back the datetime column + add_column :trains, :kickoff_at, :datetime + + # Attempt to reverse the migration + # Note: We can't perfectly reverse since we lose the date component + # We'll set it to the next occurrence from now + reversible do |dir| + dir.down do + Train.reset_column_information + Train.find_each do |train| + next if train.kickoff_time.blank? + + # Set to next occurrence from now in app's timezone + tz = train.app.timezone + now = Time.current.in_time_zone(tz) + kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}") + kickoff_datetime = kickoff_today > now ? kickoff_today : kickoff_today + 1.day + + train.update_column(:kickoff_at, kickoff_datetime.utc) + end + end + end + + # Remove the time column + safety_assured { remove_column :trains, :kickoff_time } + end +end diff --git a/lib/tasks/anonymize.rake b/lib/tasks/anonymize.rake index 98707eb14..e1b804049 100644 --- a/lib/tasks/anonymize.rake +++ b/lib/tasks/anonymize.rake @@ -49,7 +49,7 @@ namespace :anonymize do whitelist "name", "slug", "description", "status", "branching_strategy", "version_seeded_with", "version_current", "repeat_duration", "build_queue_wait_time", "build_queue_size", "backmerge_strategy", "manual_release", "tag_store_releases_with_platform_names", "tag_store_releases", "compact_build_notes", "tag_end_of_release", "build_queue_enabled", - "kickoff_at", "versioning_strategy", "send_build_notes", "notifications_release_specific_channel_enabled", + "kickoff_time", "versioning_strategy", "send_build_notes", "notifications_release_specific_channel_enabled", "tag_end_of_release_prefix", "tag_end_of_release_suffix", "version_bump_strategy", "enable_changelog_linking_in_notifications", "release_branch_pattern", "webhooks_enabled" whitelist_timestamps diff --git a/spec/components/scheduled_train_component_spec.rb b/spec/components/scheduled_train_component_spec.rb index 0d0fbe9d1..5928e11cb 100644 --- a/spec/components/scheduled_train_component_spec.rb +++ b/spec/components/scheduled_train_component_spec.rb @@ -41,7 +41,7 @@ version_seeded_with: "2025.01.01", versioning_strategy: :calver, repeat_duration: 1.day, - kickoff_at: creation_time + 1.hour) + kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S")) create(:scheduled_release, train:, scheduled_at: scheduled_time) end @@ -66,7 +66,7 @@ version_seeded_with: "2025.01.01", versioning_strategy: :calver, repeat_duration:, - kickoff_at: creation_time + 1.hour) + kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S")) create(:scheduled_release, train:, scheduled_at: scheduled_time) end @@ -93,7 +93,7 @@ version_seeded_with: "2025.01.01", versioning_strategy: :calver, repeat_duration: 1.day, - kickoff_at: creation_time + 1.hour) + kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S")) scheduled_release = create(:scheduled_release, train:, scheduled_at: scheduled_time) create(:release, :on_track, train:, original_release_version: "2025.02.01", scheduled_release:) end @@ -118,7 +118,7 @@ version_seeded_with: "2025.01.01", versioning_strategy: :calver, repeat_duration:, - kickoff_at: creation_time + 1.hour) + kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S")) scheduled_release = create(:scheduled_release, train:, scheduled_at: scheduled_time) create(:release, :on_track, train:, original_release_version: "2025.02.01", scheduled_release:) end @@ -171,7 +171,7 @@ version_seeded_with: "2025.01.01", versioning_strategy: :calver, repeat_duration: 1.day, - kickoff_at: creation_time + 1.hour) + kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S")) create(:scheduled_release, train:, scheduled_at: scheduled_time) end @@ -196,7 +196,7 @@ version_seeded_with: "2025.01.01", versioning_strategy: :calver, repeat_duration:, - kickoff_at: creation_time + 1.hour) + kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S")) create(:scheduled_release, train:, scheduled_at: scheduled_time) end @@ -223,7 +223,7 @@ version_seeded_with: "2025.01.01", versioning_strategy: :calver, repeat_duration: 1.day, - kickoff_at: creation_time + 1.hour) + kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S")) scheduled_release = create(:scheduled_release, train:, scheduled_at: scheduled_time) create(:release, :on_track, train:, original_release_version: "2025.02.01", scheduled_release:) end @@ -248,7 +248,7 @@ version_seeded_with: "2025.01.01", versioning_strategy: :calver, repeat_duration:, - kickoff_at: creation_time + 1.hour) + kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S")) scheduled_release = create(:scheduled_release, train:, scheduled_at: scheduled_time) create(:release, :on_track, train:, original_release_version: "2025.02.01", scheduled_release:) end diff --git a/spec/factories/trains.rb b/spec/factories/trains.rb index 4e6c40c4d..0e95ea8ac 100644 --- a/spec/factories/trains.rb +++ b/spec/factories/trains.rb @@ -49,7 +49,7 @@ trait :with_schedule do branching_strategy { "almost_trunk" } - kickoff_at { 2.hours.from_now } + kickoff_time { (Time.current + 2.hours).strftime("%H:%M:%S") } repeat_duration { 1.day } end diff --git a/spec/jobs/schedule_train_releases_job_spec.rb b/spec/jobs/schedule_train_releases_job_spec.rb index 545554941..41263bf47 100644 --- a/spec/jobs/schedule_train_releases_job_spec.rb +++ b/spec/jobs/schedule_train_releases_job_spec.rb @@ -4,12 +4,16 @@ describe ScheduleTrainReleasesJob do it "schedules release for active trains" do train = create(:train, :active, :with_schedule) - train.scheduled_releases.create!(scheduled_at: train.kickoff_at) + tz = train.app.timezone - travel_to train.kickoff_at + 1.hour do + # Create a scheduled release in the past + first_scheduled = 2.hours.ago.in_time_zone(tz).change(hour: 14, min: 0, sec: 0).utc + train.scheduled_releases.create!(scheduled_at: first_scheduled) + + travel_to first_scheduled + 1.hour do described_class.new.perform expect(train.reload.scheduled_releases.count).to eq(2) - expect(train.reload.scheduled_releases.last.scheduled_at).to eq(train.kickoff_at + train.repeat_duration) + expect(train.reload.scheduled_releases.last.scheduled_at).to eq(first_scheduled + train.repeat_duration) end end @@ -23,8 +27,13 @@ end it "does not schedule release for active trains with nothing to schedule" do - train = create(:train, :with_almost_trunk, :active, kickoff_at: Time.current + 2.hours, repeat_duration: 5.days) - train.scheduled_releases.create!(scheduled_at: train.kickoff_at) + future_time = (Time.current + 2.hours).strftime("%H:%M:%S") + train = create(:train, :with_almost_trunk, :active, kickoff_time: future_time, repeat_duration: 5.days) + tz = train.app.timezone + + # Create a scheduled release in the future + future_scheduled = Time.current.in_time_zone(tz).change(hour: Time.current.hour + 2).utc + train.scheduled_releases.create!(scheduled_at: future_scheduled) expect(train.scheduled_releases.count).to eq(1) diff --git a/spec/models/train_spec.rb b/spec/models/train_spec.rb index 3b7ddb19f..3b09668d2 100644 --- a/spec/models/train_spec.rb +++ b/spec/models/train_spec.rb @@ -102,51 +102,73 @@ train.activate! expect(train.reload.scheduled_releases.count).to be(1) - expect(train.reload.scheduled_releases.first.scheduled_at).to eq(train.kickoff_at) + scheduled_at = train.reload.scheduled_releases.first.scheduled_at + expect(scheduled_at.in_time_zone(train.app.timezone).strftime("%H:%M:%S")).to eq(train.kickoff_time.strftime("%H:%M:%S")) end end describe "#next_run_at" do - it "returns kickoff time if no releases have been scheduled yet and kickoff is in the future" do + it "returns next occurrence of kickoff time if no releases have been scheduled yet" do train = create(:train, :with_schedule, :active) + next_run = train.next_run_at - expect(train.next_run_at).to eq(train.kickoff_at) + expect(next_run).to be > Time.current + expect(next_run.in_time_zone(train.app.timezone).strftime("%H:%M:%S")).to eq(train.kickoff_time.strftime("%H:%M:%S")) end - it "returns kickoff + repeat duration time if no releases have been scheduled yet and kickoff is in the past" do - train = create(:train, :with_schedule, :active) + it "returns next occurrence with repeat duration if kickoff time today has passed" do + # Set kickoff_time to 10 AM + train = create(:train, :active, branching_strategy: "almost_trunk", kickoff_time: "10:00:00", repeat_duration: 1.day) + + # Travel to 3 PM same day - should return next occurrence (tomorrow at 10 AM) + tz = train.app.timezone + today_3pm = Time.current.in_time_zone(tz).change(hour: 15, min: 0, sec: 0) - travel_to train.kickoff_at + 1.hour do - expect(train.next_run_at).to eq(train.kickoff_at + train.repeat_duration) + travel_to today_3pm do + next_run = train.next_run_at + expect(next_run).to be > Time.current + expect(next_run.in_time_zone(tz).strftime("%H:%M:%S")).to eq("10:00:00") end end it "returns next available schedule time if there is a scheduled release" do train = create(:train, :with_schedule, :active) - train.scheduled_releases.create!(scheduled_at: train.kickoff_at) + tz = train.app.timezone - travel_to train.kickoff_at + 1.hour do - expect(train.next_run_at).to eq(train.kickoff_at + train.repeat_duration) - end + # Create a scheduled release for a specific time + first_scheduled = Time.current.in_time_zone(tz).change(hour: 14, min: 0, sec: 0).utc + train.scheduled_releases.create!(scheduled_at: first_scheduled) + + # Next run should be first_scheduled + repeat_duration + expect(train.next_run_at).to eq(first_scheduled + train.repeat_duration) end it "returns next available schedule time if there are many scheduled releases" do train = create(:train, :with_schedule, :active) - train.scheduled_releases.create!(scheduled_at: train.kickoff_at) - train.scheduled_releases.create!(scheduled_at: train.kickoff_at + train.repeat_duration) + tz = train.app.timezone - travel_to train.kickoff_at + 1.day + 1.hour do - expect(train.next_run_at).to eq(train.kickoff_at + train.repeat_duration * 2) - end + first_scheduled = Time.current.in_time_zone(tz).change(hour: 14, min: 0, sec: 0).utc + train.scheduled_releases.create!(scheduled_at: first_scheduled) + train.scheduled_releases.create!(scheduled_at: first_scheduled + train.repeat_duration) + + # Next run should be last scheduled + repeat_duration + expect(train.next_run_at).to eq(first_scheduled + train.repeat_duration * 2) end it "returns next available schedule time if there are scheduled releases and more than repeat duration has passed since last scheduled release" do train = create(:train, :with_schedule, :active) - train.scheduled_releases.create!(scheduled_at: train.kickoff_at) + tz = train.app.timezone - travel_to train.kickoff_at + 2.days + 1.hour do - expect(train.next_run_at).to eq(train.kickoff_at + train.repeat_duration * 3) - end + # Create a scheduled release in the past + past_scheduled = 3.days.ago.in_time_zone(tz).change(hour: 14, min: 0, sec: 0).utc + train.scheduled_releases.create!(scheduled_at: past_scheduled) + + next_run = train.next_run_at + + # Should be in the future + expect(next_run).to be > Time.current + # Should be calculated by adding repeat_duration multiples to past_scheduled + expect(next_run).to be > past_scheduled end end @@ -175,10 +197,12 @@ train = create(:train, :with_almost_trunk, :active) expect(train.scheduled_releases.count).to be(0) - train.update!(kickoff_at: 2.days.from_now, repeat_duration: 2.days) + future_time = (Time.current + 2.hours).strftime("%H:%M:%S") + train.update!(kickoff_time: future_time, repeat_duration: 2.days) expect(train.reload.scheduled_releases.count).to be(1) - expect(train.reload.scheduled_releases.first.scheduled_at).to eq(train.kickoff_at) + scheduled_at = train.reload.scheduled_releases.first.scheduled_at + expect(scheduled_at.in_time_zone(train.app.timezone).strftime("%H:%M:%S")).to eq(train.kickoff_time.strftime("%H:%M:%S")) end end