forward email with attachments

Question

I would like to do this without downloading the attachments and then re/attaching to the new email.
This is what i have tried:

$emailslist.each do |e|
  Mail.deliver do
    from   fromstr
    to "mailman@somedomain.com"
    subject "[Events] #{subjectstr}"

    if e.attachments.length>0
      e.attachments.each do |a| 
       add_file a
     end
   end
 end
end

#error in 'e.attachments.each'=>undefined method `attachments' for
#<TypeError: can't convert nil into String> 

EDIT I have been using this code for months and it worked fine.

The new stuff i have introduced now is the code above.

Anyways I'm pasting the whole code upon request.

require 'mail'

$subscribers=[]

File.new("C:/Users/j.de_miguel/Desktop/mailman.forma/subscribers2.txt",'r').each do |line| 
  line=line.sub("\n","")
  $subscribers.push(line) if line =~ /@/
end

puts $subscribers

$errorfile=File.new("C:/Users/j.de_miguel/Desktop/mailman.forma/error_log2.txt",'a+')
$errorfile.write("#{Time.now}\n")
$errorfile.flush

def deleteSubjectRecursion(subjstr)

  if subjstr =~ /(.\[FORMA 2013\])+/
    subjstr.gsub!(/.\[FORMA 2013\]/,"")
  end

  if subjstr =~ /((?i)Re: ){2,}/
    subjstr.gsub!(/((?i)Re: ){2,}/,"Re: ")
  end

  return subjstr
end

def UserIsRegistered(mailaddr)

  registered = false
  $subscribers.each{|s| registered = true if mailaddr==s}
  if registered == false
    $errorfile.write("#{Time.now} : user #{mailaddr} attempted to mailman\n")
    $errorfile.flush
  end

  return registered

end


Mail.defaults do
  retriever_method :imap, { :address    => "imap.1and1.es",
                            :port       => 143, 
                            :user_name  => "mailman@somedomain.com",
                            :password   => "xxxxxxxx",
                            :enable_ssl => false }

  delivery_method :smtp, { :address              => "smtp.1and1.es",
                           :port                 => 587,
                           :domain               => '1and1.es',
                           :user_name            => 'mailman@somaedomain.com',
                           :password             => 'xxxxxxxxxxxx',
                           :authentication       => 'plain',
                           :enable_starttls_auto => true  }
end

#$emailslist=Mail.find(keys: ['NOT','SEEN'])
$emailslist=[Mail.last]

$emailslist.each do |e|

  eplain_part = e.text_part ? e.text_part.body.decoded : nil
  ehtml_part = e.html_part ? e.html_part.body.decoded : nil

  type=e.charset
  type_plain=eplain_part ? e.text_part.charset.to_s : nil
  type_html=ehtml_part ? e.html_part.charset.to_s : nil

  bodystr= type ? e.body.decoded.to_s.force_encoding(type) : nil

  type=type ? type.to_s : type_plain
  puts type.inspect

  subjectstr=e.subject.to_s.encode(type)
  fromstr=e.from.first.to_s.encode(type)
  puts fromstr

  bodystr_plain=eplain_part ? eplain_part.force_encoding(type_plain) : nil
  bodystr_html=ehtml_part ? ehtml_part.force_encoding(type_html) : nil

  $subscribers.each do |tostr|

    puts tostr.inspect

    if (not subjectstr =~ /^\[FORMA 2013\]/  ) && (UserIsRegistered(fromstr) == true)
      subjectstr=deleteSubjectRecursion(subjectstr)

      begin
        Mail.deliver do

          from   fromstr
          to      "mailman@somedomain.com"
          bcc  tostr
          subject "[FORMA 2013] #{subjectstr}"

          if ehtml_part != nil
            html_part do
              content_type("text/html; charset=#  {type_html}")
              #content_transfer_encoding("7bit")
              body "# {bodystr_html}\nmailman@forma.culturadigital.cc para darte de baja escribe \"baja\" a info@culturadigital.cc"
            end
          end

          if eplain_part != nil
            text_part do
              content_type("text/plain; charset=# {type_plain}")
              #content_transfer_encoding("7bit")
              body "#{bodystr_plain}\nmailman@forma.culturadigital.cc para darte de baja escribe \"baja\" a info@culturadigital.cc"
            end
          end

          if eplain_part == nil && ehtml_part == nil
            body "#{bodystr}\nmailman@forma.culturadigital.cc para darte de baja escribe \"baja\" a info@culturadigital.cc" 
            charset=type
          end
          #puts e.attachments.inspect
          if e.attachments.length>0
            e.attachments.each do |a| 
              add_file a.encoded
            end
          end



        end
        puts "1 email sent"
      rescue => e
        puts "error: #{e}"
        $errorfile.write("#{Time.now}\nerror sending to #{tostr}: #{e},\nemail subject: #{subjectstr}\n\n")
        $errorfile.flush()
      end
    end
  end
end

$errorfile.close()

Answer 1

The Tin Mans answer mostly works. I change how attachments were added since his version was not working for me.

e.attachments.each { |a| attachments[a.filename] = a.decoded } if e.attachments.length > 0

Answer 2

This is untested, and isn't really an attempt to find or fix the bug. It's to show how your code should look, written in more idiomatic Ruby code. And, as a result, it might fix the problem you're seeing. If not, at least you'll have a better idea how you should be writing your code:

require 'mail'

• Define some constants for literal strings that get reused. Do this at the top so you don't have to search through the code to change things in multiple places, making it likely you'll miss one of them.

  PATH_TO_FILES = "C:/Users/j.de_miguel/Desktop/mailman.forma"
  BODY_BOILERPLATE_FORMAT = "%s\nmailman@forma.culturadigital.cc para darte de baja escribe \"baja\" a info@culturadigital.cc"
  

• Group your methods toward the top of the file, after constants.

• We open using 'a', not 'a+'. We don't need read/write, we only need write.
• This opens and closes the file as necessary.
• Closing the file automatically does a flush.
• If you're calling the log method often then there are better ways to do this, but this isn't a heavyweight script.
• I'm using File.join to build the filename based on the path. File.join is aware of the path separators and does the right thing automatically.

• String.% makes it easy to create a standard output format.

  def log(text)
  
    File.open(File.join(PATH_TO_FILES, "error_log2.txt"), 'a') do |log_file|
      log_file.puts "%s : %s" % [Time.now, text]
    end
  
  end
  

• Method names in Ruby are snake_case, not CamelCase.

• There's no reason to have multiple gsub! nor are the conditional tests necessary. If the sub-string you want to purge exists in the string gsub will do it, otherwise it moves on. Chaining the gsub methods reduces the code to one line.
• gsub could/should probably be sub unless you know there could be multiple hits to be substituted in the string.

• return is redundant so we don't use it unless we're explicitly returning a value to leave a block prematurely.

  def delete_subject_recursion(subjstr)
  
    subjstr.gsub(/.\[FORMA 2013\]/,"").gsub(/((?i)Re: ){2,}/, "Re: ")
  
  end
  

• Since registered is supposed to be a boolean, use any? to do the test. If any matches are found any? bails out and returns true.

  def user_is_registered(mailaddr)
  
    registered = subscribers.any?{ |s| mailaddr == s }
    log("user #{ mailaddr } attempted to mailman") unless registered
  
    registered
  
  end
  

• Use foreach to iterate over the lines of a file.

  subscribers = []
  File.foreach(File.join(PATH_TO_FILES, "subscribers2.txt")) do |line| 
    subscribers << line.chomp if line['@']
  end
  
  puts subscribers
  
  log('')
  
  Mail.defaults do
  
    retriever_method(
      :imap,
      {
        :address    => "imap.1and1.es",
        :port       => 143, 
        :user_name  => "mailman@somedomain.com",
        :password   => "xxxxxxxx",
        :enable_ssl => false 
      }
    )
  
    delivery_method(
      :smtp,
      {
        :address              => "smtp.1and1.es",
        :port                 => 587,
        :domain               => '1and1.es',
        :user_name            => 'mailman@somaedomain.com',
        :password             => 'xxxxxxxxxxxx',
        :authentication       => 'plain',
        :enable_starttls_auto => true  
      }
    )
  
  end
  
  #emailslist=Mail.find(keys: ['NOT','SEEN'])
  emailslist = [Mail.last]
  
  emailslist.each do |e|
  

• This use of ternary statements here is probably not desirable but I left it.

• Formatting into columns makes it easier to read.

• Organize your assignments and uses so they're not strewn all through the file.

    eplain_part = e.text_part ? e.text_part.body.decoded : nil
    type_plain  = eplain_part ? e.text_part.charset.to_s : nil
    ehtml_part  = e.html_part ? e.html_part.body.decoded : nil
    type_html   = ehtml_part  ? e.html_part.charset.to_s : nil
  
    e_charset = e.charset
    body_str  = e_charset ? e.body.decoded.to_s.force_encoding(e_charset) : nil
    e_charset = e_charset ? e_charset.to_s : type_plain
    puts e_charset.inspect
  
    subjectstr = e.subject.to_s.encode(e_charset)
    fromstr    = e.from.first.to_s.encode(e_charset)
    puts fromstr
  
    bodystr_plain = eplain_part ? eplain_part.force_encoding(type_plain) : nil
    bodystr_html  = ehtml_part  ? ehtml_part.force_encoding(type_html)   : nil
  
    subscribers.each do |subscriber|
  
      puts subscriber.inspect
  
      if !subjectstr[/^\[FORMA 2013\]/] && user_is_registered(fromstr)
  
        subjectstr = delete_subject_recursion(subjectstr)
  
        begin
  
          Mail.deliver do
  
            from   fromstr
            to      "mailman@somedomain.com"
            bcc  subscriber
            subject "[FORMA 2013] #{ subjectstr }"
  
            if ehtml_part
              html_part do
                content_type("text/html; charset=#{ type_html }")
                #content_transfer_encoding("7bit")
                body BODY_BOILERPLATE_FORMAT % bodystr_html
              end
            end
  
            if eplain_part
              text_part do
                content_type("text/plain; charset=#{ type_plain }")
                #content_transfer_encoding("7bit")
                body BODY_BOILERPLATE_FORMAT % bodystr_plain
              end
            end
  
            if !eplain_part && !ehtml_part
              body BODY_BOILERPLATE_FORMAT % body_str
              charset = e_charset
            end
  
            #puts e.attachments.inspect
            e.attachments.each { |a| add_file a.encoded } if e.attachments.length > 0
          end
  
          puts "1 email sent"
  
        rescue => e
  
          puts "error: #{ e }"
          log("error sending to #{ subscriber }: #{ e },\nemail subject: #{ subjectstr }")
  
        end
      end
    end
  end
  

---

if e.attachments.length>0
  e.attachments.each do |a| 
    add_file a
  end
end

That can be refactored into a simple, single-line using a trailing conditional if test:

e.attachments.each { |a| add_file a.encoded } if e.attachments.length > 0

Using a single line like this is OK when you're doing something simple. Don't use them for more complex code because you'll induce visual noise, which makes it hard to understand and read your code.

But let's look at what the code above is actually doing. e.attachments in this context appears to be returning an array, or some sort of enumerable collection, otherwise each wouldn't work. length will tell us how many elements exist in the "array" (or whatever it is) that is returned by attachments.

If length is zero, then we don't want to do anything, so we could say:

e.attachments.each { |a| add_file a.encoded } unless e.attachments.empty?

(Assuming attachments implements an empty? method.)

That's kind of redundant too though. If e.attachments is empty already, what will each do? It would check to see if attachments returned an array containing any elements and if it's empty it'd skip its block entirely, effectively acting just like the trailing if condition was triggered. SOOOooo, we can use this instead:

e.attachments.each { |a| add_file a.encoded }

---

Ruby Style guides:

• https://github.com/bbatsov/ruby-style-guide
• https://github.com/styleguide/ruby

The second is based on the first.