From 7f2f87bf5929f7425d0f3736386202373d9d25ac Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Thu, 10 Sep 2020 13:41:46 +1000
Subject: [PATCH] DEV: Review fixes (#10641)

See comments in https://review.discourse.org/t/dev-imap-log-to-database-10435/14337/6 for context.
---
 app/models/imap_sync_log.rb                   |  9 ++------
 ...020909_make_imap_sync_log_cols_not_null.rb |  8 +++++++
 lib/demon/email_sync.rb                       | 23 +++++++++++--------
 spec/jobs/cleanup_imap_sync_log_spec.rb       |  4 ++--
 4 files changed, 25 insertions(+), 19 deletions(-)
 create mode 100644 db/migrate/20200910020909_make_imap_sync_log_cols_not_null.rb

diff --git a/app/models/imap_sync_log.rb b/app/models/imap_sync_log.rb
index 7abf098647d..71b285c6b81 100644
--- a/app/models/imap_sync_log.rb
+++ b/app/models/imap_sync_log.rb
@@ -1,17 +1,12 @@
 # frozen_string_literal: true
 
 class ImapSyncLog < ActiveRecord::Base
-  RETAIN_LOGS_DAYS = 5
+  RETAIN_LOGS_DAYS ||= 5
 
   belongs_to :group
 
   def self.levels
-    @levels ||= Enum.new(
-      debug: 1,
-      info: 2,
-      warn: 3,
-      error: 4
-    )
+    @levels ||= Enum.new(:debug, :info, :warn, :error)
   end
 
   def self.log(message, level, group_id = nil)
diff --git a/db/migrate/20200910020909_make_imap_sync_log_cols_not_null.rb b/db/migrate/20200910020909_make_imap_sync_log_cols_not_null.rb
new file mode 100644
index 00000000000..fa21b7062f8
--- /dev/null
+++ b/db/migrate/20200910020909_make_imap_sync_log_cols_not_null.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class MakeImapSyncLogColsNotNull < ActiveRecord::Migration[6.0]
+  def change
+    change_column_null :imap_sync_logs, :message, false
+    change_column_null :imap_sync_logs, :level, false
+  end
+end
diff --git a/lib/demon/email_sync.rb b/lib/demon/email_sync.rb
index f548e987653..8777b3e5e15 100644
--- a/lib/demon/email_sync.rb
+++ b/lib/demon/email_sync.rb
@@ -68,9 +68,7 @@ class Demon::EmailSync < ::Demon::Base
 
     @sync_data.each do |db, sync_data|
       sync_data.each do |_, data|
-        data[:thread].kill
-        data[:thread].join
-        data[:syncer]&.disconnect! rescue nil
+        kill_and_disconnect!(data)
       end
     end
 
@@ -104,9 +102,7 @@ class Demon::EmailSync < ::Demon::Base
         next true if all_dbs.include?(db)
 
         sync_data.each do |_, data|
-          data[:thread].kill
-          data[:thread].join
-          data[:syncer]&.disconnect!
+          kill_and_disconnect!(data)
         end
 
         false
@@ -130,10 +126,7 @@ class Demon::EmailSync < ::Demon::Base
               ImapSyncLog.warn("Thread for group is dead", group_id)
             end
 
-            data[:thread].kill
-            data[:thread].join
-            data[:syncer]&.disconnect!
-
+            kill_and_disconnect!(data)
             false
           end
 
@@ -166,4 +159,14 @@ class Demon::EmailSync < ::Demon::Base
     STDERR.puts e.backtrace.join("\n")
     exit 1
   end
+
+  def kill_and_disconnect!(data)
+    data[:thread].kill
+    data[:thread].join
+    begin
+      data[:syncer]&.disconnect!
+    rescue Net::IMAP::ResponseError => err
+      puts "[EmailSync] Encountered a response error when disconnecting: #{err.to_s}"
+    end
+  end
 end
diff --git a/spec/jobs/cleanup_imap_sync_log_spec.rb b/spec/jobs/cleanup_imap_sync_log_spec.rb
index e2a8091b795..8b52141ac27 100644
--- a/spec/jobs/cleanup_imap_sync_log_spec.rb
+++ b/spec/jobs/cleanup_imap_sync_log_spec.rb
@@ -10,8 +10,8 @@ describe Jobs::CleanupImapSyncLog do
     log2 = ImapSyncLog.log("Test log 2", :debug)
     log3 = ImapSyncLog.log("Test log 3", :debug)
 
-    log2.update(created_at: Time.now - 6.days)
-    log3.update(created_at: Time.now - 7.days)
+    log2.update(created_at: 6.days.ago)
+    log3.update(created_at: 7.days.ago)
 
     job_class.execute({})