Merge pull request #162 from thesis/fix-schedule-sorting-bug

Fix schedule sorting bug and clean up a couple things

This fixes a bug in sorting datetime jobs for reminder list and schedule
list commands.

The bug was introduced when refactoring the schedule script to pull some
functionality out into a lib module. Previously, the job get, sort, and format
steps were all done within one function; the sort was performed on the job
object's keys array, and the string formatting happened on that array.

When these steps were broken out into separate functions, the sorted jobs
were returned as an object, then passed to the string formatter, but because
objects are inherently unordered this broke the sorting.

This changes the sort function's return type to an array and updates the string
formatting function to handle the change.

This PR also sweeps up a couple of crumbs leftover from previous PRs.

lib/schedule-util.js

@@ -252,12 +252,7 @@ function syncSchedules(robot, storageKey, jobsInMemory) {
     jobsInMemory,
   )
   for (id of Object.keys(nonCachedSchedules || {})) {
-    // Update legacy jobs in the brain that were created with a `threadId` param
-    // instead of the currently-saved `metadata`.
-    // TODO: Remove `ensureUpgradedJob` call after this sync has run on prod.
-    job = ensureUpgradedJob(nonCachedSchedules[id])
-    nonCachedSchedules[id] = job
-
+    job = nonCachedSchedules[id]
     scheduleFromBrain(robot, jobsInMemory, storageKey, id, ...job)
 
     logSerializedJobDetails(
@@ -373,14 +368,14 @@ function isRestrictedRoom(targetRoom, robot, msg) {
 
 /**
  * Given an object containing scheduled jobs currently in memory, an array of
- * room (ids? names?), and optionally a usedId, returns an array containing two
- * objects, the datetime jobs and the cron jobs scheduled for the given rooms
- * and visible to the given user.
+ * room ids, and optionally a usedId, returns:
+ * an array containing two arrays: the datetime jobs and the cron jobs
+ * scheduled for the given rooms, and visible to the given user.
  */
 function getScheduledJobList(jobsInMemory, rooms, userIdForDMs = null) {
   // split jobs into date and cron pattern jobs
-  const dateJobs = {}
-  const cronJobs = {}
+  const dateJobs = []
+  const cronJobs = []
 
   for (let id in jobsInMemory) {
     let job = jobsInMemory[id]
@@ -391,9 +386,9 @@ function getScheduledJobList(jobsInMemory, rooms, userIdForDMs = null) {
         continue
       }
       if (!isCronPattern(job.pattern)) {
-        dateJobs[id] = job
+        dateJobs.push(job)
       } else {
-        cronJobs[id] = job
+        cronJobs.push(job)
       }
     }
   }
@@ -470,11 +465,11 @@ function formatJobForMessage(
 }
 
 function sortJobsByDate(jobs) {
-  // sort by date in ascending order
-  for (let id of Object.keys(jobs).sort(
-    (a, b) => new Date(jobs[a].pattern) - new Date(jobs[b].pattern),
-  ))
-    return jobs
+  jobs.sort((a, b) => {
+    return new Date(a.pattern) - new Date(b.pattern)
+  })
+
+  return jobs
 }
 
 function formatJobsForListMessage(robotAdapter, jobs, isCron) {
@@ -483,9 +478,7 @@ function formatJobsForListMessage(robotAdapter, jobs, isCron) {
   if (!isCron) {
     jobs = sortJobsByDate(jobs)
   }
-  for (let id in jobs) {
-    let job = jobs[id]
-
+  for (let job of jobs) {
     output += formatJobForMessage(
       robotAdapter,
       job.pattern,
@@ -509,19 +502,6 @@ function logSerializedJobDetails(logger, serializedJob, messagePrefix, jobId) {
   )
 }
 
-// TODO: Remove this function after a sync with this has run on prod.
-// This is to ensure backwards-compatibility of jobs saved before
-// `metadata` replaced the `threadId` param.
-function ensureUpgradedJob(serializedJob) {
-  let [, , , metadata, remindInThread] = serializedJob
-
-  if (typeof metadata == "string") {
-    serializedJob[3] = { thread_id: metadata }
-  }
-
-  return serializedJob
-}
-
 class Job {
   constructor(
     id,

scripts/remind.js

@@ -62,7 +62,7 @@ module.exports = function(robot) {
 
     let who = msg.match[1]
     let message = whoToTag[who] || ""
-    let metadata = msg.message.metadata // TODO: strip to only needed metadata, or include all?
+    let metadata = msg.message.metadata
 
     try {
       let inputString = msg.match[2]
@@ -89,7 +89,7 @@ module.exports = function(robot) {
         REMINDER_JOBS,
         REMINDER_KEY,
         msg.message.user,
-        null, //targetRoomId || targetRoom,
+        null,
         date.date(),
         message,
         metadata,
@@ -166,13 +166,8 @@ module.exports = function(robot) {
     }
 
     try {
-      let [dateJobs, cronJobs] = getScheduledJobList(
-        REMINDER_JOBS,
-        rooms,
-        userIdForDMs,
-      )
+      let [dateJobs] = getScheduledJobList(REMINDER_JOBS, rooms, userIdForDMs)
       output = formatJobsForListMessage(robot.adapter, dateJobs, false)
-      output += formatJobsForListMessage(robot.adapter, cronJobs, true)
 
       if (!!output.length) {
         output = outputPrefix + "===\n" + output

scripts/schedule.js

@@ -60,7 +60,7 @@ module.exports = function(robot) {
       let pattern = _.trim(msg.match[2])
 
       // store the metadata, but do not use it to post the job
-      let metadata = msg.message.metadata // TODO: strip to only needed metadata, or include all?
+      let metadata = msg.message.metadata
 
       if (!isBlank(targetRoom)) {
         targetRoomId = getRoomIdFromName(robot.adapter, targetRoom)
@@ -78,14 +78,6 @@ module.exports = function(robot) {
         }
       }
 
-      // if (!isCronPattern(pattern)) {
-      //   return msg.send(`\"${pattern}\" is an invalid pattern.
-      //     See http://crontab.org/ or https://crontab.guru/ for cron-style format pattern.
-      //     If you're trying to schedule a one-time reminder, try using the \`remind\` command:
-      //     See \`help remind\` for more information.
-      //     `)
-      // }
-
       try {
         let resp = createScheduledJob(
           robot,