Merge branch...

Merge branch '217803-follow-up-from-resolve-distribute-daily-cron-schedules-out-over-the-hour' into 'master'

Follow-up from: Distribute daily cron schedules out over the hour"

See merge request gitlab-org/gitlab!36471

app/assets/javascripts/pages/projects/pipeline_schedules/shared/components/interval_pattern_input.vue

 <script>
-import { GlSprintf, GlLink } from '@gitlab/ui';
+import { GlFormRadio, GlFormRadioGroup, GlLink, GlSprintf } from '@gitlab/ui';
 import { s__, sprintf } from '~/locale';
 import { getWeekdayNames } from '~/lib/utils/datetime_utility';
 
+const KEY_EVERY_DAY = 'everyDay';
+const KEY_EVERY_WEEK = 'everyWeek';
+const KEY_EVERY_MONTH = 'everyMonth';
+const KEY_CUSTOM = 'custom';
+
 export default {
   components: {
-    GlSprintf,
+    GlFormRadio,
+    GlFormRadioGroup,
     GlLink,
+    GlSprintf,
   },
   props: {
     initialCronInterval: {
@@ -22,6 +29,7 @@ export default {
       randomWeekDayIndex: this.generateRandomWeekDayIndex(),
       randomDay: this.generateRandomDay(),
       inputNameAttribute: 'schedule[cron]',
+      radioValue: this.initialCronInterval ? KEY_CUSTOM : KEY_EVERY_DAY,
       cronInterval: this.initialCronInterval,
       cronSyntaxUrl: 'https://en.wikipedia.org/wiki/Cron',
     };
@@ -29,14 +37,11 @@ export default {
   computed: {
     cronIntervalPresets() {
       return {
-        everyDay: `0 ${this.randomHour} * * *`,
-        everyWeek: `0 ${this.randomHour} * * ${this.randomWeekDayIndex}`,
-        everyMonth: `0 ${this.randomHour} ${this.randomDay} * *`,
+        [KEY_EVERY_DAY]: `0 ${this.randomHour} * * *`,
+        [KEY_EVERY_WEEK]: `0 ${this.randomHour} * * ${this.randomWeekDayIndex}`,
+        [KEY_EVERY_MONTH]: `0 ${this.randomHour} ${this.randomDay} * *`,
       };
     },
-    intervalIsPreset() {
-      return Object.values(this.cronIntervalPresets).includes(this.cronInterval);
-    },
     formattedTime() {
       if (this.randomHour > 12) {
         return `${this.randomHour - 12}:00pm`;
@@ -45,23 +50,35 @@ export default {
       }
       return `${this.randomHour}:00am`;
     },
-    weekday() {
-      return getWeekdayNames()[this.randomWeekDayIndex];
-    },
-    everyDayText() {
-      return sprintf(s__(`Every day (at %{time})`), { time: this.formattedTime });
+    radioOptions() {
+      return [
+        {
+          value: KEY_EVERY_DAY,
+          text: sprintf(s__(`Every day (at %{time})`), { time: this.formattedTime }),
         },
-    everyWeekText() {
-      return sprintf(s__('Every week (%{weekday} at %{time})'), {
+        {
+          value: KEY_EVERY_WEEK,
+          text: sprintf(s__('Every week (%{weekday} at %{time})'), {
             weekday: this.weekday,
             time: this.formattedTime,
-      });
+          }),
         },
-    everyMonthText() {
-      return sprintf(s__('Every month (Day %{day} at %{time})'), {
+        {
+          value: KEY_EVERY_MONTH,
+          text: sprintf(s__('Every month (Day %{day} at %{time})'), {
             day: this.randomDay,
             time: this.formattedTime,
-      });
+          }),
+        },
+        {
+          value: KEY_CUSTOM,
+          text: s__('PipelineScheduleIntervalPattern|Custom (%{linkStart}Cron syntax%{linkEnd})'),
+          link: this.cronSyntaxUrl,
+        },
+      ];
+    },
+    weekday() {
+      return getWeekdayNames()[this.randomWeekDayIndex];
     },
   },
   watch: {
@@ -72,38 +89,18 @@ export default {
         gl.pipelineScheduleFieldErrors.updateFormValidityState();
       });
     },
-  },
-  // If at the mounting stage the default is still an empty string, we
-  // know we are not editing an existing field so we update it so
-  // that the default is the first radio option
-  mounted() {
-    if (this.cronInterval === '') {
-      this.cronInterval = this.cronIntervalPresets.everyDay;
+    radioValue: {
+      immediate: true,
+      handler(val) {
+        if (val !== KEY_CUSTOM) {
+          this.cronInterval = this.cronIntervalPresets[val];
         }
       },
-  methods: {
-    setCustomInput(e) {
-      if (!this.isEditingCustom) {
-        this.isEditingCustom = true;
-        this.$refs.customInput.click();
-        // Because we need to manually trigger the click on the radio btn,
-        // it will add a space to update the v-model. If the user is typing
-        // and the space is added, it will feel very unituitive so we reset
-        // the value to the original
-        this.cronInterval = e.target.value;
-      }
-      if (this.intervalIsPreset) {
-        this.isEditingCustom = false;
-      }
     },
-    toggleCustomInput(shouldEnable) {
-      this.isEditingCustom = shouldEnable;
-
-      if (shouldEnable) {
-        // We need to change the value so other radios don't remain selected
-        // because the model (cronInterval) hasn't changed. The server trims it.
-        this.cronInterval = `${this.cronInterval} `;
-      }
+  },
+  methods: {
+    onCustomInput() {
+      this.radioValue = KEY_CUSTOM;
     },
     generateRandomHour() {
       return Math.floor(Math.random() * 23);
@@ -119,79 +116,24 @@ export default {
 </script>
 
 <template>
-  <div class="interval-pattern-form-group">
-    <div class="cron-preset-radio-input">
-      <input
-        id="every-day"
-        v-model="cronInterval"
-        :name="inputNameAttribute"
-        :value="cronIntervalPresets.everyDay"
-        class="label-bold"
-        type="radio"
-        @click="toggleCustomInput(false)"
-      />
-
-      <label class="label-bold" for="every-day">
-        {{ everyDayText }}
-      </label>
-    </div>
-
-    <div class="cron-preset-radio-input">
-      <input
-        id="every-week"
-        v-model="cronInterval"
-        :name="inputNameAttribute"
-        :value="cronIntervalPresets.everyWeek"
-        class="label-bold"
-        type="radio"
-        @click="toggleCustomInput(false)"
-      />
-
-      <label class="label-bold" for="every-week">
-        {{ everyWeekText }}
-      </label>
-    </div>
-
-    <div class="cron-preset-radio-input">
-      <input
-        id="every-month"
-        v-model="cronInterval"
-        :name="inputNameAttribute"
-        :value="cronIntervalPresets.everyMonth"
-        class="label-bold"
-        type="radio"
-        @click="toggleCustomInput(false)"
-      />
-
-      <label class="label-bold" for="every-month">
-        {{ everyMonthText }}
-      </label>
-    </div>
-
-    <div class="cron-preset-radio-input">
-      <input
-        id="custom"
-        ref="customInput"
-        v-model="cronInterval"
-        :name="inputNameAttribute"
-        :value="cronInterval"
-        class="label-bold"
-        type="radio"
-        @click="toggleCustomInput(true)"
-      />
-
-      <label for="custom"> {{ s__('PipelineSheduleIntervalPattern|Custom') }} </label>
-
-      <gl-sprintf :message="__('(%{linkStart}Cron syntax%{linkEnd})')">
+  <div>
+    <gl-form-radio-group v-model="radioValue" :name="inputNameAttribute">
+      <gl-form-radio
+        v-for="option in radioOptions"
+        :key="option.value"
+        :value="option.value"
+        :data-testid="option.value"
+      >
+        <gl-sprintf v-if="option.link" :message="option.text">
           <template #link="{content}">
-          <gl-link :href="cronSyntaxUrl" target="_blank" class="gl-font-sm">
+            <gl-link :href="option.link" target="_blank" class="gl-font-sm">
               {{ content }}
             </gl-link>
           </template>
         </gl-sprintf>
-    </div>
-
-    <div class="cron-interval-input-wrapper">
+        <template v-else>{{ option.text }}</template>
+      </gl-form-radio>
+    </gl-form-radio-group>
     <input
       id="schedule_cron"
       v-model="cronInterval"
@@ -200,8 +142,7 @@ export default {
       class="form-control inline cron-interval-input"
       type="text"
       required="true"
-        @input="setCustomInput"
+      @input="onCustomInput"
     />
   </div>
-  </div>
 </template>

changelogs/unreleased/217803-follow-up-from-resolve-distribute-daily-cron-schedules-out-over-th.yml

+---
+title: Fix UI quirks with pipeline schedule cron options
+merge_request: 36471
+author:
+type: changed

locale/gitlab.pot

@@ -757,9 +757,6 @@ msgid_plural "(%d closed)"
 msgstr[0] ""
 msgstr[1] ""
 
-msgid "(%{linkStart}Cron syntax%{linkEnd})"
-msgstr ""
-
 msgid "(%{mrCount} merged)"
 msgstr ""
 
@@ -16902,6 +16899,9 @@ msgstr ""
 msgid "PipelineCharts|Total:"
 msgstr ""
 
+msgid "PipelineScheduleIntervalPattern|Custom (%{linkStart}Cron syntax%{linkEnd})"
+msgstr ""
+
 msgid "PipelineSchedules|Activated"
 msgstr ""
 
@@ -16932,9 +16932,6 @@ msgstr ""
 msgid "PipelineSchedules|Variables"
 msgstr ""
 
-msgid "PipelineSheduleIntervalPattern|Custom"
-msgstr ""
-
 msgid "PipelineStatusTooltip|Pipeline: %{ciStatus}"
 msgstr ""
 

spec/frontend/pages/projects/pipeline_schedules/shared/components/interval_pattern_input_spec.js

-import { shallowMount } from '@vue/test-utils';
+import { mount } from '@vue/test-utils';
+import { trimText } from 'helpers/text_helper';
 import IntervalPatternInput from '~/pages/projects/pipeline_schedules/shared/components/interval_pattern_input.vue';
 
 describe('Interval Pattern Input Component', () => {
@@ -14,15 +15,22 @@ describe('Interval Pattern Input Component', () => {
     everyWeek: `0 ${mockHour} * * ${mockWeekDayIndex}`,
     everyMonth: `0 ${mockHour} ${mockDay} * *`,
   };
-
-  const findEveryDayRadio = () => wrapper.find('#every-day');
-  const findEveryWeekRadio = () => wrapper.find('#every-week');
-  const findEveryMonthRadio = () => wrapper.find('#every-month');
-  const findCustomRadio = () => wrapper.find('#custom');
+  const customKey = 'custom';
+  const everyDayKey = 'everyDay';
+  const cronIntervalNotInPreset = `0 12 * * *`;
+
+  const findEveryDayRadio = () => wrapper.find(`[data-testid=${everyDayKey}]`);
+  const findEveryWeekRadio = () => wrapper.find('[data-testid="everyWeek"]');
+  const findEveryMonthRadio = () => wrapper.find('[data-testid="everyMonth"]');
+  const findCustomRadio = () => wrapper.find(`[data-testid="${customKey}"]`);
   const findCustomInput = () => wrapper.find('#schedule_cron');
-  const selectEveryDayRadio = () => findEveryDayRadio().setChecked();
-  const selectEveryWeekRadio = () => findEveryWeekRadio().setChecked();
-  const selectEveryMonthRadio = () => findEveryMonthRadio().setChecked();
+  const findAllLabels = () => wrapper.findAll('label');
+  const findSelectedRadio = () =>
+    wrapper.findAll('input[type="radio"]').wrappers.find(x => x.element.checked);
+  const findSelectedRadioKey = () => findSelectedRadio()?.attributes('data-testid');
+  const selectEveryDayRadio = () => findEveryDayRadio().trigger('click');
+  const selectEveryWeekRadio = () => findEveryWeekRadio().trigger('click');
+  const selectEveryMonthRadio = () => findEveryMonthRadio().trigger('click');
   const selectCustomRadio = () => findCustomRadio().trigger('click');
 
   const createWrapper = (props = {}, data = {}) => {
@@ -30,7 +38,7 @@ describe('Interval Pattern Input Component', () => {
       throw new Error('A wrapper already exists');
     }
 
-    wrapper = shallowMount(IntervalPatternInput, {
+    wrapper = mount(IntervalPatternInput, {
       propsData: { ...props },
       data() {
         return {
@@ -63,8 +71,8 @@ describe('Interval Pattern Input Component', () => {
       createWrapper();
     });
 
-    it('to a non empty string when no initial value is not passed', () => {
-      expect(findCustomInput()).not.toBe('');
+    it('defaults to every day value when no `initialCronInterval` is passed', () => {
+      expect(findCustomInput().element.value).toBe(cronIntervalPresets.everyDay);
     });
   });
 
@@ -85,22 +93,22 @@ describe('Interval Pattern Input Component', () => {
       createWrapper();
     });
 
-    it('when a default option is selected', () => {
+    it('when a default option is selected', async () => {
       selectEveryDayRadio();
 
-      return wrapper.vm.$nextTick().then(() => {
+      await wrapper.vm.$nextTick();
+
       expect(findCustomInput().attributes('disabled')).toBeUndefined();
     });
-    });
 
-    it('when the custom option is selected', () => {
+    it('when the custom option is selected', async () => {
       selectCustomRadio();
 
-      return wrapper.vm.$nextTick().then(() => {
+      await wrapper.vm.$nextTick();
+
       expect(findCustomInput().attributes('disabled')).toBeUndefined();
     });
   });
-  });
 
   describe('formattedTime computed property', () => {
     it.each`
@@ -115,40 +123,83 @@ describe('Interval Pattern Input Component', () => {
     });
   });
 
+  describe('Time strings', () => {
+    beforeEach(() => {
+      createWrapper();
+    });
+
+    it('renders each label for radio options properly', () => {
+      const labels = findAllLabels().wrappers.map(el => trimText(el.text()));
+
+      expect(labels).toEqual([
+        'Every day (at 4:00am)',
+        'Every week (Monday at 4:00am)',
+        'Every month (Day 1 at 4:00am)',
+        'Custom ( Cron syntax )',
+      ]);
+    });
+  });
+
   describe('User Actions with radio buttons', () => {
+    describe('Default option', () => {
+      beforeEach(() => {
+        createWrapper();
+      });
+
+      it('when everyday is selected, update value', async () => {
+        selectEveryWeekRadio();
+        await wrapper.vm.$nextTick();
+        expect(findCustomInput().element.value).toBe(cronIntervalPresets.everyWeek);
+
+        selectEveryDayRadio();
+        await wrapper.vm.$nextTick();
+        expect(findCustomInput().element.value).toBe(cronIntervalPresets.everyDay);
+      });
+    });
+
+    describe('Other options', () => {
       it.each`
         desc                                                 | initialCronInterval               | act                      | expectedValue
-      ${'when everyday is selected, update value'}     | ${'1 2 3 4 5'}                    | ${selectEveryDayRadio}   | ${cronIntervalPresets.everyDay}
         ${'when everyweek is selected, update value'}        | ${'1 2 3 4 5'}                    | ${selectEveryWeekRadio}  | ${cronIntervalPresets.everyWeek}
         ${'when everymonth is selected, update value'}       | ${'1 2 3 4 5'}                    | ${selectEveryMonthRadio} | ${cronIntervalPresets.everyMonth}
-      ${'when custom is selected, add space to value'} | ${cronIntervalPresets.everyMonth} | ${selectCustomRadio}     | ${`${cronIntervalPresets.everyMonth} `}
-    `('$desc', ({ initialCronInterval, act, expectedValue }) => {
+        ${'when custom is selected, value remains the same'} | ${cronIntervalPresets.everyMonth} | ${selectCustomRadio}     | ${cronIntervalPresets.everyMonth}
+      `('$desc', async ({ initialCronInterval, act, expectedValue }) => {
         createWrapper({ initialCronInterval });
 
         act();
 
-      return wrapper.vm.$nextTick().then(() => {
+        await wrapper.vm.$nextTick();
+
         expect(findCustomInput().element.value).toBe(expectedValue);
       });
     });
   });
+
   describe('User actions with input field for Cron syntax', () => {
     beforeEach(() => {
       createWrapper();
     });
 
-    it('when editing the cron input it selects the custom radio button', () => {
+    it('when editing the cron input it selects the custom radio button', async () => {
       const newValue = '0 * * * *';
 
+      expect(findSelectedRadioKey()).toBe(everyDayKey);
+
       findCustomInput().setValue(newValue);
 
-      expect(wrapper.vm.cronInterval).toBe(newValue);
+      await wrapper.vm.$nextTick;
+
+      expect(findSelectedRadioKey()).toBe(customKey);
+    });
   });
 
-    it('when value of input is one of the defaults, it selects the corresponding radio button', () => {
-      findCustomInput().setValue(cronIntervalPresets.everyWeek);
+  describe('Edit form field', () => {
+    beforeEach(() => {
+      createWrapper({ initialCronInterval: cronIntervalNotInPreset });
+    });
 
-      expect(wrapper.vm.cronInterval).toBe(cronIntervalPresets.everyWeek);
+    it('loads with the custom option being selected', () => {
+      expect(findSelectedRadioKey()).toBe(customKey);
     });
   });
 });