[UI] Also cloning recurring run schedule, fixes #3761 (#3840)

* [UI] Also cloning recurring run schedule

* Fix unit test for trigger and utils

* Add and fix unit tests for Trigger

* Add NewRun page unit tests

* Fix unit tests

* Fix jest test timezone

frontend/global-setup.js

@@ -0,0 +1,6 @@
+export default () => {
+  // This let unit tests run in UTC timezone consistently, despite developers'
+  // dev machine's timezone.
+  // Reference: https://stackoverflow.com/a/56482581
+  process.env.TZ = 'UTC';
+};

frontend/package.json

@@ -110,6 +110,7 @@
       "!src/index.tsx",
       "!src/CSSReset.tsx"
     ],
+    "globalSetup": "./global-setup.js",
     "snapshotSerializers": [
       "./src/__serializers__/mock-function",
       "snapshot-diff/serializer.js",

frontend/src/components/Trigger.test.tsx

@@ -30,20 +30,31 @@ const PERIODIC_DEFAULT = {
 };
 const CRON_DEFAULT = { cron: '0 * * * * ?', end_time: undefined, start_time: undefined };
 
+beforeAll(() => {
+  process.env.TZ = 'UTC';
+});
+
 describe('Trigger', () => {
   // tslint:disable-next-line:variable-name
   const RealDate = Date;
 
   function mockDate(isoDate: any): void {
     (global as any).Date = class extends RealDate {
-      constructor() {
+      constructor(...args: any[]) {
         super();
-        return new RealDate(isoDate);
+        if (args.length === 0) {
+          // Use mocked date when calling new Date()
+          return new RealDate(isoDate);
+        } else {
+          // Otherwise, use real Date constructor
+          return new (RealDate as any)(...args);
+        }
       }
     };
   }
-  const testDate = new Date(2018, 11, 21, 7, 53);
+  const now = new Date(2018, 11, 21, 7, 53);
-  mockDate(testDate);
+  mockDate(now);
+  const oneWeekLater = new Date(2018, 11, 28, 7, 53);
 
   it('renders periodic schedule controls for initial render', () => {
     const tree = shallow(<Trigger />);
@@ -113,7 +124,7 @@ describe('Trigger', () => {
       expect(spy).toHaveBeenLastCalledWith({
         ...PARAMS_DEFAULT,
         trigger: {
-          periodic_schedule: { ...PERIODIC_DEFAULT, start_time: testDate },
+          periodic_schedule: { ...PERIODIC_DEFAULT, start_time: now },
         },
       });
     });
@@ -128,7 +139,7 @@ describe('Trigger', () => {
         target: { type: 'checkbox', checked: true },
       });
       (tree.instance() as Trigger).handleChange('startDate')({ target: { value: '2018-11-23' } });
-      (tree.instance() as Trigger).handleChange('endTime')({ target: { value: '08:35' } });
+      (tree.instance() as Trigger).handleChange('startTime')({ target: { value: '08:35' } });
       expect(spy).toHaveBeenLastCalledWith({
         ...PARAMS_DEFAULT,
         trigger: {
@@ -193,7 +204,7 @@ describe('Trigger', () => {
       expect(spy).toHaveBeenLastCalledWith({
         ...PARAMS_DEFAULT,
         trigger: {
-          periodic_schedule: { ...PERIODIC_DEFAULT, end_time: testDate, start_time: testDate },
+          periodic_schedule: { ...PERIODIC_DEFAULT, end_time: oneWeekLater, start_time: now },
         },
       });
     });
@@ -292,6 +303,38 @@ describe('Trigger', () => {
         },
       });
     });
+
+    it('inits with cloned initial props', () => {
+      const spy = jest.fn();
+      const startTime = new Date('2020-01-01T23:53:00.000Z');
+      shallow(
+        <Trigger
+          onChange={spy}
+          initialProps={{
+            maxConcurrentRuns: '3',
+            catchup: false,
+            trigger: {
+              periodic_schedule: {
+                interval_second: '' + 60 * 60 * 3, // 3 hours
+                start_time: startTime.toISOString() as any,
+              },
+            },
+          }}
+        />,
+      );
+      expect(spy).toHaveBeenCalledTimes(1);
+      expect(spy).toHaveBeenLastCalledWith({
+        catchup: false,
+        maxConcurrentRuns: '3',
+        trigger: {
+          periodic_schedule: {
+            end_time: undefined,
+            interval_second: '10800',
+            start_time: startTime,
+          },
+        },
+      });
+    });
   });
 
   describe('cron', () => {
@@ -318,7 +361,7 @@ describe('Trigger', () => {
       expect(spy).toHaveBeenLastCalledWith({
         ...PARAMS_DEFAULT,
         trigger: {
-          cron_schedule: { ...CRON_DEFAULT, start_time: testDate },
+          cron_schedule: { ...CRON_DEFAULT, start_time: new Date('2018-03-23T07:53:00.000Z') },
         },
       });
     });
@@ -336,7 +379,7 @@ describe('Trigger', () => {
       expect(spy).toHaveBeenLastCalledWith({
         ...PARAMS_DEFAULT,
         trigger: {
-          cron_schedule: { ...CRON_DEFAULT, end_time: testDate, cron: '0 0 0 * * ?' },
+          cron_schedule: { ...CRON_DEFAULT, end_time: oneWeekLater, cron: '0 0 0 * * ?' },
         },
       });
     });
@@ -384,5 +427,39 @@ describe('Trigger', () => {
         },
       });
     });
+
+    it('inits with cloned initial props', () => {
+      const spy = jest.fn();
+      const startTime = new Date('2020-01-01T00:00:00.000Z');
+      const endTime = new Date('2020-01-02T01:02:00.000Z');
+      shallow(
+        <Trigger
+          onChange={spy}
+          initialProps={{
+            maxConcurrentRuns: '4',
+            catchup: true,
+            trigger: {
+              cron_schedule: {
+                cron: '0 0 0 ? * 1,5,6',
+                start_time: startTime.toISOString() as any,
+                end_time: endTime.toISOString() as any,
+              },
+            },
+          }}
+        />,
+      );
+      expect(spy).toHaveBeenCalledTimes(1);
+      expect(spy).toHaveBeenLastCalledWith({
+        catchup: true,
+        maxConcurrentRuns: '4',
+        trigger: {
+          cron_schedule: {
+            cron: '0 0 0 ? * 1,5,6',
+            start_time: startTime,
+            end_time: endTime,
+          },
+        },
+      });
+    });
   });
 });

frontend/src/components/Trigger.tsx

@@ -33,9 +33,19 @@ import {
   pickersToDate,
   triggers,
   TriggerType,
+  parseTrigger,
+  ParsedTrigger,
 } from '../lib/TriggerUtils';
+import { logger } from 'src/lib/Utils';
+
+type TriggerInitialProps = {
+  maxConcurrentRuns?: string;
+  catchup?: boolean;
+  trigger?: ApiTrigger;
+};
 
 interface TriggerProps {
+  initialProps?: TriggerInitialProps;
   onChange?: (config: {
     trigger?: ApiTrigger;
     maxConcurrentRuns?: string;
@@ -67,33 +77,48 @@ const css = stylesheet({
 });
 
 export default class Trigger extends React.Component<TriggerProps, TriggerState> {
-  public state = (() => {
+  public state: TriggerState = (() => {
-    const now = new Date();
+    const { maxConcurrentRuns, catchup, trigger } =
-    const inAWeek = new Date(
+      this.props.initialProps || ({} as TriggerInitialProps);
-      now.getFullYear(),
+    let parsedTrigger: Partial<ParsedTrigger> = {};
-      now.getMonth(),
+    try {
-      now.getDate() + 7,
+      if (trigger) {
-      now.getHours(),
+        parsedTrigger = parseTrigger(trigger);
-      now.getMinutes(),
+      }
-    );
+    } catch (err) {
-    const [startDate, startTime] = dateToPickerFormat(now);
+      logger.warn('Failed to parse original trigger: ', trigger);
-    const [endDate, endTime] = dateToPickerFormat(inAWeek);
+      logger.warn(err);
+    }
+    const startDateTime = parsedTrigger.startDateTime ?? new Date();
+    const endDateTime =
+      parsedTrigger.endDateTime ??
+      new Date(
+        startDateTime.getFullYear(),
+        startDateTime.getMonth(),
+        startDateTime.getDate() + 7,
+        startDateTime.getHours(),
+        startDateTime.getMinutes(),
+      );
+    const [startDate, startTime] = dateToPickerFormat(startDateTime);
+    const [endDate, endTime] = dateToPickerFormat(endDateTime);
 
     return {
-      catchup: true,
+      catchup: catchup ?? true,
-      cron: '',
+      maxConcurrentRuns: maxConcurrentRuns || '10',
-      editCron: false,
+      hasEndDate: !!parsedTrigger?.endDateTime,
       endDate,
       endTime,
-      hasEndDate: false,
+      hasStartDate: !!parsedTrigger?.startDateTime,
-      hasStartDate: false,
-      intervalCategory: PeriodicInterval.MINUTE,
-      intervalValue: 1,
-      maxConcurrentRuns: '10',
-      selectedDays: new Array(7).fill(true),
       startDate,
       startTime,
-      type: TriggerType.INTERVALED,
+      selectedDays: new Array(7).fill(true),
+      type: parsedTrigger.type ?? TriggerType.INTERVALED,
+      // cron state
+      editCron: parsedTrigger.type === TriggerType.CRON,
+      cron: parsedTrigger.cron || '',
+      // interval state
+      intervalCategory: parsedTrigger.intervalCategory ?? PeriodicInterval.MINUTE,
+      intervalValue: parsedTrigger.intervalValue ?? 1,
     };
   })();
 

frontend/src/components/__snapshots__/Trigger.test.tsx.snap

@@ -118,7 +118,7 @@ exports[`Trigger enables a single day on click 1`] = `
           }
         }
         type="date"
-        value="2018-12-21"
+        value="2018-12-28"
         variant="outlined"
         width={160}
       />
@@ -459,7 +459,7 @@ exports[`Trigger renders all week days enabled 1`] = `
           }
         }
         type="date"
-        value="2018-12-21"
+        value="2018-12-28"
         variant="outlined"
         width={160}
       />
@@ -800,7 +800,7 @@ exports[`Trigger renders periodic schedule controls for initial render 1`] = `
           }
         }
         type="date"
-        value="2018-12-21"
+        value="2018-12-28"
         variant="outlined"
         width={160}
       />
@@ -1039,7 +1039,7 @@ exports[`Trigger renders periodic schedule controls if the trigger type is CRON
           }
         }
         type="date"
-        value="2018-12-21"
+        value="2018-12-28"
         variant="outlined"
         width={160}
       />
@@ -1301,7 +1301,7 @@ exports[`Trigger renders week days if the trigger type is CRON and interval is w
           }
         }
         type="date"
-        value="2018-12-21"
+        value="2018-12-28"
         variant="outlined"
         width={160}
       />

frontend/src/lib/TriggerUtils.test.ts

@@ -23,6 +23,7 @@ import {
   TriggerType,
   dateToPickerFormat,
   triggerDisplayString,
+  parseTrigger,
 } from './TriggerUtils';
 import { ApiTrigger } from '../apis/job';
 
@@ -236,6 +237,45 @@ describe('TriggerUtils', () => {
     });
   });
 
+  describe('parseTrigger', () => {
+    it('throws on invalid trigger', () => {
+      expect(() => parseTrigger({})).toThrow('Invalid trigger: {}');
+    });
+
+    it('parses periodic schedule', () => {
+      const startTime = new Date(1234);
+      const parsedTrigger = parseTrigger({
+        periodic_schedule: {
+          start_time: startTime,
+          interval_second: '120',
+        },
+      });
+      expect(parsedTrigger).toEqual({
+        type: TriggerType.INTERVALED,
+        startDateTime: startTime,
+        endDateTime: undefined,
+        intervalCategory: PeriodicInterval.MINUTE,
+        intervalValue: 2,
+      });
+    });
+
+    it('parses cron schedule', () => {
+      const endTime = new Date(12345);
+      const parsedTrigger = parseTrigger({
+        cron_schedule: {
+          end_time: endTime,
+          cron: '0 0 0 ? * 0,6',
+        },
+      });
+      expect(parsedTrigger).toEqual({
+        type: TriggerType.CRON,
+        cron: '0 0 0 ? * 0,6',
+        startDateTime: undefined,
+        endDateTime: endTime,
+      });
+    });
+  });
+
   describe('dateToPickerFormat', () => {
     it('converts date to picker format date and time', () => {
       const testDate = new Date(2018, 11, 13, 11, 33);

frontend/src/lib/TriggerUtils.ts

@@ -28,6 +28,20 @@ export enum PeriodicInterval {
   WEEK = 'Week',
   MONTH = 'Month',
 }
+const INTERVAL_SECONDS = {
+  [PeriodicInterval.MINUTE]: 60,
+  [PeriodicInterval.HOUR]: 60 * 60,
+  [PeriodicInterval.DAY]: 60 * 60 * 24,
+  [PeriodicInterval.WEEK]: 60 * 60 * 24 * 7,
+  [PeriodicInterval.MONTH]: 60 * 60 * 24 * 30,
+};
+const PERIODIC_INTERVAL_DESCENDING = [
+  PeriodicInterval.MONTH,
+  PeriodicInterval.WEEK,
+  PeriodicInterval.DAY,
+  PeriodicInterval.HOUR,
+  PeriodicInterval.MINUTE,
+];
 
 export const triggers = new Map<TriggerType, { displayName: string }>([
   [TriggerType.INTERVALED, { displayName: 'Periodic' }],
@@ -35,28 +49,26 @@ export const triggers = new Map<TriggerType, { displayName: string }>([
 ]);
 
 export function getPeriodInSeconds(interval: PeriodicInterval, count: number): number {
-  let intervalSeconds = 0;
+  const intervalSeconds = INTERVAL_SECONDS[interval];
-  switch (interval) {
+  if (!intervalSeconds) {
-    case PeriodicInterval.MINUTE:
+    throw new Error('Invalid interval category: ' + interval);
-      intervalSeconds = 60;
-      break;
-    case PeriodicInterval.HOUR:
-      intervalSeconds = 60 * 60;
-      break;
-    case PeriodicInterval.DAY:
-      intervalSeconds = 60 * 60 * 24;
-      break;
-    case PeriodicInterval.WEEK:
-      intervalSeconds = 60 * 60 * 24 * 7;
-      break;
-    case PeriodicInterval.MONTH:
-      intervalSeconds = 60 * 60 * 24 * 30;
-      break;
-    default:
-      throw new Error('Invalid interval category: ' + interval);
   }
   return intervalSeconds * count;
 }
+export function parsePeriodFromSeconds(
+  seconds: number,
+): { interval: PeriodicInterval; count: number } {
+  for (const interval of PERIODIC_INTERVAL_DESCENDING) {
+    const intervalSeconds = INTERVAL_SECONDS[interval];
+    if (seconds % intervalSeconds === 0) {
+      return {
+        interval,
+        count: seconds / intervalSeconds,
+      };
+    }
+  }
+  throw new Error('Invalid seconds: ' + seconds);
+}
 
 export function buildCron(
   startDateTime: Date | undefined,
@@ -174,6 +186,66 @@ export function buildTrigger(
   return trigger;
 }
 
+export type ParsedTrigger =
+  | {
+      type: TriggerType.INTERVALED;
+      intervalCategory: PeriodicInterval;
+      intervalValue: number;
+      startDateTime?: Date;
+      endDateTime?: Date;
+      cron?: undefined;
+    }
+  | {
+      type: TriggerType.CRON;
+      intervalCategory?: undefined;
+      intervalValue?: undefined;
+      startDateTime?: Date;
+      endDateTime?: Date;
+      cron: string;
+    };
+
+export function parseTrigger(trigger: ApiTrigger): ParsedTrigger {
+  if (trigger.periodic_schedule) {
+    const periodicSchedule = trigger.periodic_schedule;
+    const intervalSeconds = parseInt(periodicSchedule.interval_second || '', 10);
+    if (Number.isNaN(intervalSeconds)) {
+      throw new Error(
+        `Interval seconds is NaN: ${periodicSchedule.interval_second} for ${JSON.stringify(
+          trigger,
+        )}`,
+      );
+    }
+    const { interval: intervalCategory, count: intervalValue } = parsePeriodFromSeconds(
+      intervalSeconds,
+    );
+    return {
+      type: TriggerType.INTERVALED,
+      intervalCategory,
+      intervalValue,
+      // Generated client has a bug the fields will be string here instead, so
+      // we use new Date() to convert them to Date.
+      startDateTime: periodicSchedule.start_time
+        ? new Date(periodicSchedule.start_time as any)
+        : undefined,
+      endDateTime: periodicSchedule.end_time
+        ? new Date(periodicSchedule.end_time as any)
+        : undefined,
+    };
+  }
+  if (trigger.cron_schedule) {
+    const { cron, start_time: startTime, end_time: endTime } = trigger.cron_schedule;
+    return {
+      type: TriggerType.CRON,
+      cron: cron || '',
+      // Generated client has a bug the fields will be string here instead, so
+      // we use new Date() to convert them to Date.
+      startDateTime: startTime ? new Date(startTime as any) : undefined,
+      endDateTime: endTime ? new Date(endTime as any) : undefined,
+    };
+  }
+  throw new Error(`Invalid trigger: ${JSON.stringify(trigger)}`);
+}
+
 export function dateToPickerFormat(d: Date): [string, string] {
   const year = d.getFullYear();
   const month = ('0' + (d.getMonth() + 1)).slice(-2);

frontend/src/pages/NewRun.test.tsx

@@ -29,6 +29,7 @@ import { logger } from '../lib/Utils';
 import { NamespaceContext } from '../lib/KubeflowClient';
 import { ApiFilter, PredicateOp } from '../apis/filter';
 import { ExperimentStorageState } from '../apis/experiment';
+import { ApiJob } from 'src/apis/job';
 
 class TestNewRun extends NewRun {
   public _experimentSelectorClosed = super._experimentSelectorClosed;
@@ -55,6 +56,7 @@ describe('NewRun', () => {
   const getPipelineSpy = jest.spyOn(Apis.pipelineServiceApi, 'getPipeline');
   const getPipelineVersionSpy = jest.spyOn(Apis.pipelineServiceApi, 'getPipelineVersion');
   const getRunSpy = jest.spyOn(Apis.runServiceApi, 'getRun');
+  const getJobSpy = jest.spyOn(Apis.jobServiceApi, 'getJob');
   const loggerErrorSpy = jest.spyOn(logger, 'error');
   const historyPushSpy = jest.fn();
   const historyReplaceSpy = jest.fn();
@@ -139,6 +141,23 @@ describe('NewRun', () => {
     };
   }
 
+  function newMockJob(): ApiJob {
+    return {
+      id: 'job-id1',
+      name: 'some mock job name',
+      service_account: 'pipeline-runner',
+      pipeline_spec: {
+        pipeline_id: 'original-run-pipeline-id',
+        workflow_manifest: '{}',
+      },
+      trigger: {
+        periodic_schedule: {
+          interval_second: '60',
+        },
+      },
+    };
+  }
+
   function newMockRunWithEmbeddedPipeline(): ApiRunDetail {
     const runDetail = newMockRunDetail();
     delete runDetail.run!.pipeline_spec!.pipeline_id;
@@ -1064,6 +1083,36 @@ describe('NewRun', () => {
     });
   });
 
+  // TODO: test other attributes and scenarios
+  describe('cloning from a recurring run', () => {
+    it('clones trigger schedule', async () => {
+      const jobDetail = newMockJob();
+      const startTime = new Date(1234);
+      jobDetail.name = 'job1';
+      jobDetail.trigger = {
+        periodic_schedule: {
+          interval_second: '360',
+          start_time: startTime.toISOString() as any,
+        },
+      };
+      const props = generateProps();
+      props.location.search = `?${QUERY_PARAMS.cloneFromRecurringRun}=${jobDetail.id}`;
+
+      getJobSpy.mockImplementation(() => jobDetail);
+
+      tree = shallow(<TestNewRun {...props} />);
+      await TestUtils.flushPromises();
+
+      expect(tree.state('runName')).toBe('Clone of job1');
+      expect(tree.state('trigger')).toEqual({
+        periodic_schedule: {
+          interval_second: '360',
+          start_time: '1970-01-01T00:00:01.234Z',
+        },
+      });
+    });
+  });
+
   describe('arriving from pipeline details page', () => {
     let mockEmbeddedPipelineProps: PageProps;
     beforeEach(() => {

frontend/src/pages/NewRun.tsx

@@ -559,6 +559,11 @@ export class NewRun extends Page<{ namespace?: string }, NewRunState> {
               <div>Choose a method by which new runs will be triggered</div>
 
               <Trigger
+                initialProps={{
+                  trigger: this.state.trigger,
+                  maxConcurrentRuns: this.state.maxConcurrentRuns,
+                  catchup: this.state.catchup,
+                }}
                 onChange={({ trigger, maxConcurrentRuns, catchup }) =>
                   this.setStateSafe(
                     {
@@ -658,10 +663,15 @@ export class NewRun extends Page<{ namespace?: string }, NewRunState> {
     } else if (originalRecurringRunId) {
       // If we are cloning a recurring run, fetch the original
       try {
-        const originalRun = await Apis.jobServiceApi.getJob(originalRecurringRunId);
+        const originalJob = await Apis.jobServiceApi.getJob(originalRecurringRunId);
-        await this._prepareFormFromClone(originalRun);
+        await this._prepareFormFromClone(originalJob);
+        this.setStateSafe({
+          trigger: originalJob.trigger,
+          maxConcurrentRuns: originalJob.max_concurrency,
+          catchup: !originalJob.no_catchup,
+        });
         if (!experimentId) {
-          experimentId = RunUtils.getFirstExperimentReferenceId(originalRun);
+          experimentId = RunUtils.getFirstExperimentReferenceId(originalJob);
         }
       } catch (err) {
         await this.showPageError(

frontend/src/pages/__snapshots__/NewRun.test.tsx.snap

@@ -1482,6 +1482,13 @@ exports[`NewRun changes title and form if the new run will recur, based on the r
       Choose a method by which new runs will be triggered
     </div>
     <Trigger
+      initialProps={
+        Object {
+          "catchup": true,
+          "maxConcurrentRuns": undefined,
+          "trigger": undefined,
+        }
+      }
       onChange={[Function]}
     />
     <NewRunParameters
@@ -3575,6 +3582,13 @@ exports[`NewRun starting a new recurring run includes additional trigger input f
       Choose a method by which new runs will be triggered
     </div>
     <Trigger
+      initialProps={
+        Object {
+          "catchup": true,
+          "maxConcurrentRuns": undefined,
+          "trigger": undefined,
+        }
+      }
       onChange={[Function]}
     />
     <NewRunParameters