Fixed IMAP support, added specs and functional test, refactored code. #46

Merged
merged 14 commits into from Mar 17, 2012

Conversation

Projects
None yet
2 participants
Contributor

ndbroadbent commented Mar 17, 2012

Hey,

I've tidied up the IMAP pull request (#18):

  • added mocks, specs, and functional test
  • refactored the code that POP3 and IMAP share
  • organized spec_helper a bit, using a spec/support directory

All specs are passing

Cheers :)

@titanous titanous and 1 other commented on an outdated diff Mar 17, 2012

lib/mailman/receiver/imap.rb
+
+ # @param [Hash] options the receiver options
+ # @option options [MessageProcessor] :processor the processor to pass new
+ # messages to
+ # @option options [String] :server the server to connect to
+ # @option options [Integer] :port the port to connect to
+ # @option options [String] :username the username to authenticate with
+ # @option options [String] :password the password to authenticate with
+ def initialize(options)
+ @processor, @username, @password, @server, @filter, @port = nil, nil, nil, nil, ["NEW"], 143
+
+ @processor = options[:processor] if options.has_key? :processor
+ @username = options[:username] if options.has_key? :username
+ @password = options[:password] if options.has_key? :password
+ @filter = options[:filter] if options.has_key? :filter
+ @port = options[:port] if options.has_key? :port
@titanous

titanous Mar 17, 2012

Contributor

Lines 19-25 can be refactored significantly:

 @processor = options[:processor]
 @username  = options[:username]
 @password  = options[:password]
 @filter    = options[:filter] || ['NEW']
 @port      = options[:port] || 143
@ndbroadbent

ndbroadbent Mar 17, 2012

Contributor

Good point, I think he might have copied an older version of lib/mailman/receiver/pop3.rb. Will do that

@titanous titanous and 1 other commented on an outdated diff Mar 17, 2012

lib/mailman/receiver/imap.rb
+ def connect
+ @connection.login(@username, @password)
+ @connection.examine("INBOX")
+ end
+
+ # Disconnects from the IMAP server.
+ def disconnect
+ @connection.logout
+ @connection.disconnected? ? true : @connection.disconnect rescue nil
+ end
+
+ # Iterates through new messages, passing them to the processor, and
+ # deleting them.
+ def get_messages
+ @connection.search(@filter).each do |message|
+ puts "PROCESSING MESSAGE #{message}"
@titanous

titanous Mar 17, 2012

Contributor

This should use the logger, using sentence case.

@ndbroadbent

ndbroadbent Mar 17, 2012

Contributor

Yep, I'll correct this

@titanous titanous commented on the diff Mar 17, 2012

lib/mailman/receiver/imap.rb
+ def disconnect
+ @connection.logout
+ @connection.disconnected? ? true : @connection.disconnect rescue nil
+ end
+
+ # Iterates through new messages, passing them to the processor, and
+ # deleting them.
+ def get_messages
+ @connection.search(@filter).each do |message|
+ puts "PROCESSING MESSAGE #{message}"
+ body = @connection.fetch(message,"RFC822")[0].attr["RFC822"]
+ @processor.process(body)
+ @connection.store(message,"+FLAGS",[Net::IMAP::DELETED])
+ end
+ # Clears messages that have the Deleted flag set
+ @connection.expunge
@titanous

titanous Mar 17, 2012

Contributor

What do you think about moving messages to a different folder instead of deleting them?

@ndbroadbent

ndbroadbent Mar 17, 2012

Contributor

Yep, that's going to be the next feature. I'm looking to replace our existing IMAP processor with Mailman, which currently moves processed emails to a new folder.

@ndbroadbent

ndbroadbent Mar 17, 2012

Contributor

Here's what I would like to implement: https://github.com/fatfreecrm/fat_free_crm/blob/master/lib/fat_free_crm/dropbox.rb#L146-164

I'm not sure if / how Mailman handles unmatched emails? We use a discard method, which either deletes the message or moves it to an invalid folder.

@titanous titanous commented on the diff Mar 17, 2012

lib/mailman/receiver/imap.rb
+ @password = options[:password] if options.has_key? :password
+ @filter = options[:filter] if options.has_key? :filter
+ @port = options[:port] if options.has_key? :port
+ @connection = Net::IMAP.new(options[:server], @port)
+ end
+
+ # Connects to the IMAP server.
+ def connect
+ @connection.login(@username, @password)
+ @connection.examine("INBOX")
+ end
+
+ # Disconnects from the IMAP server.
+ def disconnect
+ @connection.logout
+ @connection.disconnected? ? true : @connection.disconnect rescue nil
@titanous

titanous Mar 17, 2012

Contributor

How about this?

@connection.disconnect unless @connection.disconnected?
@ndbroadbent

ndbroadbent Mar 17, 2012

Contributor

Just copying the specs for POP3, I need to return true for this line https://github.com/fatfreecrm/mailman/blob/imap/spec/mailman/receiver/imap_spec.rb#L22

@connection.disconnect unless @connection.disconnected? will return nil if @connection.disconnected? is true, failing the spec

@titanous

titanous Mar 17, 2012

Contributor

Ah, that's fine then.

Contributor

titanous commented Mar 17, 2012

Looks great, I'll merge it once my comments have been addressed. This will need some documentation at some point as well.

titanous referenced this pull request Mar 17, 2012

Closed

added in IMAP support #18

Contributor

ndbroadbent commented Mar 17, 2012

Here's those changes. I removed that PROCESSING MESSAGE line, since the POP3 receiver doesn't log anything in #get_messages

titanous merged commit e3c32b3 into mailman:master Mar 17, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment