FIX: IMAP archive fix and group list mailbox code unification (#10355)

FIX: IMAP archive fix and group list mailbox code unification (#10355)

  • Fixed an issue I introduced in the last PR where I am just archiving everything regardless of whether it is actually archived in Discourse man_facepalming
  • Refactor group list_mailboxes IMAP code to use providers, add specs, and add provider code to get the correct prodivder
diff --git a/app/models/group.rb b/app/models/group.rb
index 7bbc04e..ded3acb 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -758,23 +758,24 @@ class Group < ActiveRecord::Base
   def imap_mailboxes
     return [] if self.imap_server.blank? ||
                  self.email_username.blank? ||
-                 self.email_password.blank?
+                 self.email_password.blank? ||
+                 !SiteSetting.enable_imap
 
     Discourse.cache.fetch("group_imap_mailboxes_#{self.id}", expires_in: 30.minutes) do
       Rails.logger.info("[IMAP] Refreshing mailboxes list for group #{self.name}")
       mailboxes = []
 
       begin
-        @imap = Net::IMAP.new(self.imap_server, self.imap_port, self.imap_ssl)
-        @imap.login(self.email_username, self.email_password)
-
-        @imap.list('', '*').each do |m|
-          next if m.attr.include?(:Noselect)
-          mailboxes << m.name
-        end
+        imap_provider = Imap::Providers::Detector.init_with_detected_provider(
+          self.imap_config
+        )
+        imap_provider.connect!
+        mailboxes = imap_provider.list_mailboxes
+        imap_provider.disconnect!
 
         update_columns(imap_last_error: nil)
       rescue => ex
+        Rails.logger.warn("[IMAP] Mailbox refresh failed for group #{self.name} with error: #{ex}")
         update_columns(imap_last_error: ex.message)
       end
 
@@ -782,6 +783,16 @@ class Group < ActiveRecord::Base
     end
   end
 
+  def imap_config
+    {
+      server: self.imap_server,
+      port: self.imap_port,
+      ssl: self.imap_ssl,
+      username: self.email_username,
+      password: self.email_password
+    }
+  end
+
   def email_username_regex
     user, domain = email_username.split('@')
     if user.present? && domain.present?
diff --git a/lib/demon/email_sync.rb b/lib/demon/email_sync.rb
index e4c7f15..dbf6d6f 100644
--- a/lib/demon/email_sync.rb
+++ b/lib/demon/email_sync.rb
@@ -25,7 +25,7 @@ class Demon::EmailSync < ::Demon::Base
       RailsMultisite::ConnectionManagement.with_connection(db) do
         puts "[EmailSync] Thread started for group #{group.name} (id = #{group.id}) in db #{db}"
         begin
-          obj = Imap::Sync.for_group(group)
+          obj = Imap::Sync.new(group)
         rescue Net::IMAP::NoResponseError => e
           group.update(imap_last_error: e.message)
           Thread.exit
diff --git a/lib/imap/providers/detector.rb b/lib/imap/providers/detector.rb
new file mode 100644
index 0000000..41f3565
--- /dev/null
+++ b/lib/imap/providers/detector.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+module Imap
+  module Providers
+    class Detector
+      def self.init_with_detected_provider(config)
+        if config[:server] == 'imap.gmail.com'
+          return Imap::Providers::Gmail.new(config[:server], config)
+        end
+        Imap::Providers::Generic.new(config[:server], config)
+      end
+    end
+  end
+end
diff --git a/lib/imap/providers/generic.rb b/lib/imap/providers/generic.rb
index c7bb682..0801c9d 100644
--- a/lib/imap/providers/generic.rb
+++ b/lib/imap/providers/generic.rb
@@ -4,11 +4,9 @@ require 'net/imap'
 
 module Imap
   module Providers
-
     class WriteDisabledError < StandardError; end
 
     class Generic
-
       def initialize(server, options = {})
         @server = server
         @port = options[:port] || 993
@@ -124,7 +122,10 @@ module Imap
       end
 
       def list_mailboxes
-        imap.list('', '*').map(&:name)
+        imap.list('', '*').map do |m|
+          next if m.attr.include?(:Noselect)
+          m.name
+        end
       end
 
       def archive(uid)
diff --git a/lib/imap/providers/gmail.rb b/lib/imap/providers/gmail.rb
index 347fb2d..82a9ecd 100644
--- a/lib/imap/providers/gmail.rb
+++ b/lib/imap/providers/gmail.rb
@@ -96,6 +96,10 @@ module Imap
 
           # Modified version of the original `msg_att` from here:
           # https://github.com/ruby/ruby/blob/1cc8ff001da217d0e98d13fe61fbc9f5547ef722/lib/net/imap.rb#L2346
+          #
+          # This is done so we can extract X-GM-LABELS, X-GM-MSGID, and
+          # X-GM-THRID, all Gmail extended attributes.
+          #
           # rubocop:disable Style/RedundantReturn
           def msg_att(n)
             match(T_LPAR)
@@ -127,6 +131,7 @@ module Imap
                 name, val = uid_data
               when /\A(?:MODSEQ)\z/ni
                 name, val = modseq_data
+
               # Adding support for GMail extended attributes.
               when /\A(?:X-GM-LABELS)\z/ni
                 name, val = label_data
@@ -134,6 +139,8 @@ module Imap
                 name, val = uid_data
               when /\A(?:X-GM-THRID)\z/ni
                 name, val = uid_data
+              # End custom support for Gmail.
+
               else
                 parse_error("unknown attribute `%s' for {%d}", token.value, n)
               end
diff --git a/lib/imap/sync.rb b/lib/imap/sync.rb
index 73593b0..7a29601 100644
--- a/lib/imap/sync.rb
+++ b/lib/imap/sync.rb
@@ -14,26 +14,9 @@ module Imap
       end
     end
 
-    def self.for_group(group, opts = {})
-      if group.imap_server == 'imap.gmail.com'
-        opts[:provider] ||= Imap::Providers::Gmail
-      end
-
-      Imap::Sync.new(group, opts)
-    end
-
     def initialize(group, opts = {})
       @group = group
-
-      provider_klass ||= opts[:provider] || Imap::Providers::Generic
-      @provider = provider_klass.new(
-        @group.imap_server,
-        port: @group.imap_port,
-        ssl: @group.imap_ssl,
-        username: @group.email_username,
-        password: @group.email_password
-      )
-
+      @provider = Imap::Providers::Detector.init_with_detected_provider(@group.imap_config)
       connect!
     end
 
@@ -190,6 +173,10 @@ module Imap
       emails = @provider.emails(new_uids, ['UID', 'FLAGS', 'LABELS', 'RFC822'])
       processed = 0
 
+      # TODO (maybe): We might need something here to exclusively handle
+      # the UID of the incoming email, so we don't end up with a race condition
+      # where the same UID is handled multiple times before the group imap_X
+      # columns are updated.
       emails.each do |email|
         # Synchronously process emails because the order of emails matter
         # (for example replies must be processed after the original email
@@ -310,15 +297,17 @@ module Imap
         new_labels << '\\Inbox'
       else
         Logger.log("[IMAP] (#{@group.name}) Archiving UID #{incoming_email.imap_uid}")
+
+        # some providers need special handling for archiving. this way we preserve
+        # any new tag-labels, and archive, even though it may cause extra requests
+        # to the IMAP server
+        @provider.archive(incoming_email.imap_uid)
       end
 
+      # regardless of whether the topic needs to be archived we still update
+      # the flags and the labels
       @provider.store(incoming_email.imap_uid, 'FLAGS', flags, new_flags)
       @provider.store(incoming_email.imap_uid, 'LABELS', labels, new_labels)
-
-      # some providers need special handling for archiving. this way we preserve
-      # any new tag-labels, and archive, even though it may cause extra requests
-      # to the IMAP server
-      @provider.archive(incoming_email.imap_uid)
     end
   end
 end
diff --git a/spec/components/imap/sync_spec.rb b/spec/components/imap/sync_spec.rb
index 0ead0f4..14378be 100644
--- a/spec/components/imap/sync_spec.rb
+++ b/spec/components/imap/sync_spec.rb
@@ -26,7 +26,20 @@ describe Imap::Sync do
     )
   end
 
-  let(:sync_handler) { Imap::Sync.new(group, provider: MockedImapProvider) }
+  let(:sync_handler) { Imap::Sync.new(group) }
+
+  before do
+    mocked_imap_provider = MockedImapProvider.new(
+      group.imap_server,
+      port: group.imap_port,
+      ssl: group.imap_ssl,
+      username: group.email_username,
+      password: group.email_password
+    )
+    Imap::Providers::Detector.stubs(:init_with_detected_provider).returns(

[... diff too long, it was truncated ...]

GitHub sha: 5a3494b1

1 Like

This commit appears in #10355 which was merged by martin.