From 1a395a3dd93825ed220aef3c12576c6b7124367c Mon Sep 17 00:00:00 2001
From: Rhys Arkins <rhys@keylocation.sg>
Date: Fri, 1 Sep 2017 11:27:54 +0200
Subject: [PATCH] fix: fix and improve schedule migrations (#761)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix: migrate “every xday” to “on xday”

* fix: do not migrate before and after if before is after after

e.g. do not migrate “after 1am and before 5am”
---
 lib/config/migration.js                       | 42 +++++++++++++------
 .../__snapshots__/migration.spec.js.snap      |  7 ++++
 test/config/migration.spec.js                 | 37 ++++++++++++++++
 test/workers/branch/schedule.spec.js          |  3 ++
 4 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/lib/config/migration.js b/lib/config/migration.js
index ad5131e026..74dea3413b 100644
--- a/lib/config/migration.js
+++ b/lib/config/migration.js
@@ -1,3 +1,4 @@
+const later = require('later');
 const options = require('./definitions').getOptions();
 
 const optionTypes = {};
@@ -89,22 +90,29 @@ function migrateConfig(config, parentConfig) {
           schedules[i].includes('before ') &&
           schedules[i].includes('after ')
         ) {
-          isMigrated = true;
-          const toSplit = schedules[i];
-          schedules[i] = toSplit
-            .replace(
-              /^(after|before) (.*?) and (after|before) (.*?)( |$)(.*)/,
-              '$1 $2 $6'
-            )
-            .trim();
-          schedules.push(
-            toSplit
+          const parsedSchedule = later.parse.text(
+            // We need to massage short hours first before we can parse it
+            schedules[i].replace(/( \d?\d)((a|p)m)/g, '$1:00$2')
+          ).schedules[0];
+          // Only migrate if the after time is greater than before, e.g. "after 10pm and before 5am"
+          if (parsedSchedule && parsedSchedule.t_a[0] > parsedSchedule.t_b[0]) {
+            isMigrated = true;
+            const toSplit = schedules[i];
+            schedules[i] = toSplit
               .replace(
                 /^(after|before) (.*?) and (after|before) (.*?)( |$)(.*)/,
-                '$3 $4 $6'
+                '$1 $2 $6'
               )
-              .trim()
-          );
+              .trim();
+            schedules.push(
+              toSplit
+                .replace(
+                  /^(after|before) (.*?) and (after|before) (.*?)( |$)(.*)/,
+                  '$3 $4 $6'
+                )
+                .trim()
+            );
+          }
         }
       }
       for (let i = 0; i < schedules.length; i += 1) {
@@ -126,6 +134,14 @@ function migrateConfig(config, parentConfig) {
           isMigrated = true;
           schedules[i] = schedules[i].replace('days', 'day');
         }
+        if (
+          schedules[i].indexOf('every ') === 0 &&
+          schedules[i].endsWith('day') &&
+          !schedules[i].endsWith('weekday')
+        ) {
+          isMigrated = true;
+          schedules[i] = schedules[i].replace('every ', 'on ');
+        }
       }
       if (isMigrated) {
         if (typeof val === 'string' && schedules.length === 1) {
diff --git a/test/config/__snapshots__/migration.spec.js.snap b/test/config/__snapshots__/migration.spec.js.snap
index 601ded54de..b9de284329 100644
--- a/test/config/__snapshots__/migration.spec.js.snap
+++ b/test/config/__snapshots__/migration.spec.js.snap
@@ -126,3 +126,10 @@ Object {
   },
 }
 `;
+
+exports[`config/migration migrateConfig(config, parentConfig) migrates before and after schedules 2`] = `
+Array [
+  "after 10pm every weekday",
+  "before 7am every weekday",
+]
+`;
diff --git a/test/config/migration.spec.js b/test/config/migration.spec.js
index a1c5f81672..05e409cbd1 100644
--- a/test/config/migration.spec.js
+++ b/test/config/migration.spec.js
@@ -77,6 +77,7 @@ describe('config/migration', () => {
       expect(migratedConfig.dependencies.schedule.length).toBe(2);
       expect(migratedConfig.dependencies.schedule[0]).toEqual('after 10pm');
       expect(migratedConfig.dependencies.schedule[1]).toEqual('before 7am');
+      expect(migratedConfig.devDependencies.schedule).toMatchSnapshot();
       expect(migratedConfig.devDependencies.schedule.length).toBe(2);
       expect(migratedConfig.devDependencies.schedule[0]).toEqual(
         'after 10pm every weekday'
@@ -85,6 +86,30 @@ describe('config/migration', () => {
         'before 7am every weekday'
       );
     });
+    it('migrates every friday', () => {
+      const config = {
+        schedule: 'every friday',
+      };
+      const parentConfig = { ...defaultConfig };
+      const { isMigrated, migratedConfig } = configMigration.migrateConfig(
+        config,
+        parentConfig
+      );
+      expect(isMigrated).toBe(true);
+      expect(migratedConfig.schedule).toEqual('on friday');
+    });
+    it('does not migrate every weekday', () => {
+      const config = {
+        schedule: 'every weekday',
+      };
+      const parentConfig = { ...defaultConfig };
+      const { isMigrated, migratedConfig } = configMigration.migrateConfig(
+        config,
+        parentConfig
+      );
+      expect(isMigrated).toBe(false);
+      expect(migratedConfig.schedule).toEqual(config.schedule);
+    });
     it('does not migrate multi days', () => {
       const config = {
         schedule: 'after 5:00pm on wednesday and thursday',
@@ -98,6 +123,18 @@ describe('config/migration', () => {
       expect(isMigrated).toBe(false);
       expect(migratedConfig.schedule).toEqual(config.schedule);
     });
+    it('does not migrate hour range', () => {
+      const config = {
+        schedule: 'after 1:00pm and before 5:00pm',
+      };
+      const parentConfig = { ...defaultConfig };
+      const { isMigrated, migratedConfig } = configMigration.migrateConfig(
+        config,
+        parentConfig
+      );
+      expect(migratedConfig.schedule).toEqual(config.schedule);
+      expect(isMigrated).toBe(false);
+    });
     it('it migrates packages', () => {
       const config = {
         packages: [
diff --git a/test/workers/branch/schedule.spec.js b/test/workers/branch/schedule.spec.js
index 3031d45db8..502854b69f 100644
--- a/test/workers/branch/schedule.spec.js
+++ b/test/workers/branch/schedule.spec.js
@@ -26,6 +26,9 @@ describe('workers/branch/schedule', () => {
         false
       );
     });
+    it('returns false for every xday', () => {
+      expect(schedule.hasValidSchedule(['every friday'])[0]).toBe(false);
+    });
     it('returns true if schedule has days of week', () => {
       expect(schedule.hasValidSchedule(['on friday and saturday'])[0]).toBe(
         true
-- 
GitLab