Merge branch '31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd' into 'master'

Fix lazy error handling of cron parser

Closes #31274

See merge request !10948

5 files changed, 150 insertions, 11 deletions

diff --git a/changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml b/changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml
new file mode 100644
index 00000000000..b0c33ab3fa4
--- /dev/null
+++ b/changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml
@@ -0,0 +1,4 @@
+---
+title: Fix error on CI/CD Settings page related to invalid pipeline trigger
+merge_request: 10948
+author: dosuken123
diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb
index a3cc350ef22..dad8c3cdf5b 100644
--- a/lib/gitlab/ci/cron_parser.rb
+++ b/lib/gitlab/ci/cron_parser.rb
@@ -6,7 +6,7 @@ module Gitlab
 
       def initialize(cron, cron_timezone = 'UTC')
         @cron = cron
-        @cron_timezone = cron_timezone
+        @cron_timezone = ActiveSupport::TimeZone.find_tzinfo(cron_timezone).name
       end
 
       def next_time_from(time)
@@ -24,8 +24,23 @@ module Gitlab
 
       private
 
+      # NOTE:
+      # cron_timezone can only accept timezones listed in TZInfo::Timezone.
+      # Aliases of Timezones from ActiveSupport::TimeZone are NOT accepted,
+      # because Rufus::Scheduler only supports TZInfo::Timezone.
+      #
+      # For example, those codes have the same effect.
+      # Time.zone = 'Pacific Time (US & Canada)' (ActiveSupport::TimeZone)
+      # Time.zone = 'America/Los_Angeles' (TZInfo::Timezone)
+      #
+      # However, try_parse_cron only accepts the latter format.
+      # try_parse_cron('* * * * *', 'Pacific Time (US & Canada)') -> Doesn't work
+      # try_parse_cron('* * * * *', 'America/Los_Angeles') -> Works
+      # If you want to know more, please take a look
+      # https://github.com/rails/rails/blob/master/activesupport/lib/active_support/values/time_zone.rb
       def try_parse_cron(cron, cron_timezone)
-        Rufus::Scheduler.parse("#{cron} #{cron_timezone}")
+        cron_line = Rufus::Scheduler.parse("#{cron} #{cron_timezone}")
+        cron_line if cron_line.is_a?(Rufus::Scheduler::CronLine)
       rescue
         # noop
       end
diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb
index 81fa2de1cc3..783f330221c 100644
--- a/spec/features/triggers_spec.rb
+++ b/spec/features/triggers_spec.rb
@@ -104,6 +104,24 @@ feature 'Triggers', feature: true, js: true do
 
           expect(page).to have_content 'The form contains the following errors'
         end
+
+        context 'when GitLab time_zone is ActiveSupport::TimeZone format' do
+          before do
+            allow(Time).to receive(:zone)
+              .and_return(ActiveSupport::TimeZone['Eastern Time (US & Canada)'])
+          end
+
+          scenario 'do fill form with valid data and save' do
+            find('#trigger_trigger_schedule_attributes_active').click
+            fill_in 'trigger_trigger_schedule_attributes_cron', with: '1 * * * *'
+            fill_in 'trigger_trigger_schedule_attributes_cron_timezone', with: 'UTC'
+            fill_in 'trigger_trigger_schedule_attributes_ref', with: 'master'
+            click_button 'Save trigger'
+
+            expect(page.find('.flash-notice'))
+              .to have_content 'Trigger was successfully updated.'
+          end
+        end
       end
 
       context 'disabling schedule' do
diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb
index 0864bc7258d..809fda11879 100644
--- a/spec/lib/gitlab/ci/cron_parser_spec.rb
+++ b/spec/lib/gitlab/ci/cron_parser_spec.rb
@@ -60,14 +60,60 @@ describe Gitlab::Ci::CronParser do
         end
       end
 
-      context 'when cron_timezone is US/Pacific' do
-        let(:cron) { '0 0 * * *' }
-        let(:cron_timezone) { 'US/Pacific' }
+      context 'when cron_timezone is TZInfo format' do
+        before do
+          allow(Time).to receive(:zone)
+            .and_return(ActiveSupport::TimeZone['UTC'])
+        end
 
-        it_behaves_like "returns time in the future"
+        let(:hour_in_utc) do
+          ActiveSupport::TimeZone[cron_timezone]
+            .now.change(hour: 0).in_time_zone('UTC').hour
+        end
+
+        context 'when cron_timezone is US/Pacific' do
+          let(:cron) { '* 0 * * *' }
+          let(:cron_timezone) { 'US/Pacific' }
+
+          it_behaves_like "returns time in the future"
+
+          it 'converts time in server time zone' do
+            expect(subject.hour).to eq(hour_in_utc)
+          end
+        end
+      end
+
+      context 'when cron_timezone is ActiveSupport::TimeZone format' do
+        before do
+          allow(Time).to receive(:zone)
+            .and_return(ActiveSupport::TimeZone['UTC'])
+        end
+
+        let(:hour_in_utc) do
+          ActiveSupport::TimeZone[cron_timezone]
+            .now.change(hour: 0).in_time_zone('UTC').hour
+        end
+
+        context 'when cron_timezone is Berlin' do
+          let(:cron) { '* 0 * * *' }
+          let(:cron_timezone) { 'Berlin' }
+
+          it_behaves_like "returns time in the future"
+
+          it 'converts time in server time zone' do
+            expect(subject.hour).to eq(hour_in_utc)
+          end
+        end
 
-        it 'converts time in server time zone' do
-          expect(subject.hour).to eq((Time.zone.now.in_time_zone(cron_timezone).utc_offset / 60 / 60).abs)
+        context 'when cron_timezone is Eastern Time (US & Canada)' do
+          let(:cron) { '* 0 * * *' }
+          let(:cron_timezone) { 'Eastern Time (US & Canada)' }
+
+          it_behaves_like "returns time in the future"
+
+          it 'converts time in server time zone' do
+            expect(subject.hour).to eq(hour_in_utc)
+          end
         end
       end
     end
@@ -76,9 +122,21 @@ describe Gitlab::Ci::CronParser do
       let(:cron) { 'invalid_cron' }
       let(:cron_timezone) { 'invalid_cron_timezone' }
 
-      it 'returns nil' do
-        is_expected.to be_nil
-      end 
+      it { is_expected.to be_nil }
+    end
+
+    context 'when cron syntax is quoted' do
+      let(:cron) { "'0 * * * *'" }
+      let(:cron_timezone) { 'UTC' }
+
+      it { expect(subject).to be_nil }
+    end
+
+    context 'when cron syntax is rufus-scheduler syntax' do
+      let(:cron) { 'every 3h' }
+      let(:cron_timezone) { 'UTC' }
+
+      it { expect(subject).to be_nil }
     end
   end
 
@@ -96,6 +154,12 @@ describe Gitlab::Ci::CronParser do
 
       it { is_expected.to eq(false) }
     end
+
+    context 'when cron syntax is quoted' do
+      let(:cron) { "'0 * * * *'" }
+
+      it { is_expected.to eq(false) }
+    end
   end
 
   describe '#cron_timezone_valid?' do
@@ -112,5 +176,11 @@ describe Gitlab::Ci::CronParser do
 
       it { is_expected.to eq(false) }
     end
+
+    context 'when cron_timezone is ActiveSupport::TimeZone format' do
+      let(:cron_timezone) { 'Eastern Time (US & Canada)' }
+
+      it { is_expected.to eq(true) }
+    end
   end
 end
diff --git a/spec/models/ci/trigger_schedule_spec.rb b/spec/models/ci/trigger_schedule_spec.rb
index 75d21541cee..92447564d7c 100644
--- a/spec/models/ci/trigger_schedule_spec.rb
+++ b/spec/models/ci/trigger_schedule_spec.rb
@@ -73,4 +73,36 @@ describe Ci::TriggerSchedule, models: true do
       end
     end
   end
+
+  describe '#real_next_run' do
+    subject do
+      Ci::TriggerSchedule.last.real_next_run(worker_cron: worker_cron,
+                                             worker_time_zone: worker_time_zone)
+    end
+
+    context 'when GitLab time_zone is UTC' do
+      before do
+        allow(Time).to receive(:zone)
+          .and_return(ActiveSupport::TimeZone[worker_time_zone])
+      end
+
+      let(:worker_time_zone) { 'UTC' }
+
+      context 'when cron_timezone is Eastern Time (US & Canada)' do
+        before do
+          create(:ci_trigger_schedule, :nightly,
+                  cron_timezone: 'Eastern Time (US & Canada)')
+        end
+
+        let(:worker_cron) { '0 1 2 3 *' }
+
+        it 'returns the next time worker executes' do
+          expect(subject.min).to eq(0)
+          expect(subject.hour).to eq(1)
+          expect(subject.day).to eq(2)
+          expect(subject.month).to eq(3)
+        end
+      end
+    end
+  end
 end