Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/components/release_list_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions app/controllers/trains_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -244,15 +244,15 @@ 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)
return if config_params[:release_schedule_enabled].blank?

{
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
Expand Down
6 changes: 3 additions & 3 deletions app/helpers/trains_helper.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down
28 changes: 19 additions & 9 deletions app/models/train.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -116,7 +116,7 @@
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
Expand Down Expand Up @@ -146,7 +146,7 @@
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
Expand Down Expand Up @@ -213,13 +213,13 @@
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
Expand All @@ -230,11 +230,22 @@
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')}")

Check failure on line 248 in app/models/train.rb

View workflow job for this annotation

GitHub Actions / lint

Style/StringLiteralsInInterpolation: Prefer double-quoted strings inside interpolations.
end

def diff_since_last_release?
Expand Down Expand Up @@ -586,9 +597,8 @@
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
Expand Down
9 changes: 4 additions & 5 deletions app/views/trains/_release_schedule_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@
<% component.with_child do %>
<div class="grid gap-y-4 gap-x-2 md:grid-cols-3">
<div class="col-span-1">
<%= 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"}} %>
</div>
<div class="col-span-2">
<%= form.label_only :repeat_duration, "Every" %>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Expand Down
54 changes: 54 additions & 0 deletions db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb
Original file line number Diff line number Diff line change
@@ -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
Comment on lines +10 to +21
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Down-migration parsing bug for kickoff_time + minor migration cleanup

Two points here:

  1. Down path: malformed parse string (bug)
    In the down direction you build the datetime with:

    kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")

    For a time column, train.kickoff_time will typically stringify with its own date and zone, so the resulting string is ambiguous (e.g., "2025-11-19 2000-01-01 11:00:00 UTC"), and tz.parse may misinterpret or fail. You already do the right thing in Train#next_run_at by formatting only the time portion.

    I’d strongly recommend mirroring that here:

  •      kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")
    
  •      kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time.strftime("%H:%M:%S")}")
    
    
    This makes the down migration deterministic and matches the semantics used in the model.
    
    
  1. Optional: avoid N+1 on app lookups
    Both up and down use Train.find_each and then hit train.app.timezone, which will issue an extra query per train. If you expect a large trains table, you can cheaply remove this N+1:

  •    Train.reset_column_information
    
  •    Train.find_each do |train|
    
  •    Train.reset_column_information
    
  •    Train.includes(:app).find_each do |train|
    
    
    (Same pattern in the down block.)
    
    This keeps behavior identical but scales better on larger datasets.
    
    
    
    
    
    
    

Also applies to: 34-52


🏁 Script executed:

#!/bin/bash
# Retrieve the full migration file to inspect down block
find . -name "20251118165553_change_kickoff_at_to_kickoff_time.rb" -type f -exec cat {} \;

Length of output: 1764


🏁 Script executed:

#!/bin/bash
# Find Train model to check next_run_at method
fd -e rb -path "*/models/*" -name "*train*" -type f | head -20

Length of output: 168


🏁 Script executed:

#!/bin/bash
# Search for next_run_at method in Train model
rg -n "def next_run_at" --type=rb -A 5

Length of output: 26


🏁 Script executed:

#!/bin/bash
# Check how time columns are typically used in migrations/models
rg -n "\.strftime\(" --type=rb -C 2 | grep -A 2 -B 2 "time"

Length of output: 26


🏁 Script executed:

#!/bin/bash
# Find Train model files
find . -name "*train*" -type f | grep -E "models?/" | head -10

Length of output: 49


🏁 Script executed:

#!/bin/bash
# Search for next_run_at method without type flag
rg -n "def next_run_at" -A 5

Length of output: 649


🏁 Script executed:

#!/bin/bash
# Search for how time columns are used in codebase
rg -n "kickoff_time" -C 3

Length of output: 20885


🏁 Script executed:

#!/bin/bash
# Look for strftime usage with time values to understand pattern
rg "strftime.*%H:%M:%S" -C 2

Length of output: 8379


Down-migration parsing bug for kickoff_time + minor migration cleanup

Two points here:

  1. Down path: malformed parse string (bug)
    In the down direction you build the datetime with:

    kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")

    For a time column, train.kickoff_time when interpolated lacks explicit formatting. The codebase consistently uses strftime("%H:%M:%S") when parsing—as seen in Train#next_run_at (line 243) and throughout specs. Update to match this pattern:

  •      kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")
    
  •      kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time.strftime("%H:%M:%S")}")
    
    
    This ensures the down migration is deterministic and consistent with the model logic.
    
    
  1. Optional: avoid N+1 on app lookups
    Both up and down use Train.find_each and then hit train.app.timezone, which issues an extra query per train. Add eager loading:

  •    Train.reset_column_information
    
  •    Train.find_each do |train|
    
  •    Train.reset_column_information
    
  •    Train.includes(:app).find_each do |train|
    
    
    (Same pattern in the down block.)
    
    
🤖 Prompt for AI Agents
In db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb around lines
10 to 21, the down migration builds a datetime using an unformatted kickoff_time
string which can parse inconsistently; change the interpolation to use the same
formatting pattern used elsewhere (strftime("%H:%M:%S")) when reconstructing the
time so parsing is deterministic, and to avoid N+1 queries preload apps for both
up and down (use a query that eager-loads the associated app before iterating,
e.g., include the app association) so train.app.timezone does not hit the DB per
record.


# 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
2 changes: 1 addition & 1 deletion lib/tasks/anonymize.rake
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions spec/components/scheduled_train_component_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion spec/factories/trains.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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") }

Check failure on line 52 in spec/factories/trains.rb

View workflow job for this annotation

GitHub Actions / lint

Rails/DurationArithmetic: Do not add or subtract duration.
repeat_duration { 1.day }
end

Expand Down
19 changes: 14 additions & 5 deletions spec/jobs/schedule_train_releases_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)

Expand Down
Loading
Loading