Adding users with a dash "-" in email address is broken sometimes (Bug #736)

I fixed it locally but my ruby knowledge is terrible so someone else should definitely look into this..

Go to app/models/mailer.rb around line 399. You'll see the code below.

Look at the variable "recipients". Sometimes recipients is an array and then calling "delete" on the array removes any elements that contain the email address that is marked as "don't self notify". This works fine...HOWEVER, sometimes recipients is NOT an array and just contains a single string. In this case, the "delete" method being called is NOT on an array object but on a STRING object which interprets a dash as a range of values and that's why it crashes.

  # Overrides the create_mail method
  def create_mail
    # Removes the current user from the recipients and cc
    # if he doesn't want to receive notifications about what he does
    @author ||= User.current
    if @author.pref[:no_self_notified]
      recipients.delete(@author.mail) if recipients
      cc.delete(@author.mail) if cc
    end

    notified_users = [recipients, cc].flatten.compact.uniq
    # Rails would log recipients only, not cc and bcc
    mylogger.info "Sending email notification to: #{notified_users.join(', ')}" if mylogger

    # Blind carbon copy recipients
    if Setting.bcc_recipients?
      bcc(notified_users)
      recipients []
      cc []
    end
    super
  end

Thanks, looking at the best way to handle this, if you're lucky it might make it into 2.5.0 :-)

Mmh, I've tried the following:

 1diff --git a/app/models/mailer.rb b/app/models/mailer.rb
 2index 2be9de7..7a004c4 100644
 3--- a/app/models/mailer.rb
 4+++ b/app/models/mailer.rb
 5@@ -396,8 +396,8 @@ class Mailer < ActionMailer::Base
 6     # if he doesn't want to receive notifications about what he does
 7     @author ||= User.current
 8     if @author.pref[:no_self_notified]
 9-      recipients.delete(@author.mail) if recipients
10-      cc.delete(@author.mail) if cc
11+      recipients(Array.new(recipients).delete(@author.mail)) if recipients
12+      cc(Array.new(cc).delete(@author.mail)) if cc
13     end
14
15     notified_users = [recipients, cc].flatten.compact.uniq

But there's some failing tests with that, and I don't have time to look today anymore. Care to post your solution so I can have a look? Thanks.

I'm a C++ dev so I know nothing about Ruby but here's what I did...There are two parts, first, I test if recipients/cc are arrays. If they are, I call delete, otherwise I call gsub. The second part of the fix is in the BCC block of code where I just test "if recipients" and "if cc" before nuking the arrays (if that's what the empty [] brackets even do).

  # Overrides the create_mail method
  def create_mail
    # Removes the current user from the recipients and cc
    # if he doesn't want to receive notifications about what he does
    @author ||= User.current
    if @author.pref[:no_self_notified]
        if recipients.kind_of?(Array)
            recipients.delete(@author.mail) if recipients
        else
            recipients = recipients.gsub(@author.mail, '') if recipients
        end
        if cc.kind_of?(Array)
            cc.delete(@author.mail) if cc
        else
            cc = cc.gsub(@author.mail, '') if cc
        end
    end

    notified_users = [recipients, cc].flatten.compact.uniq
    # Rails would log recipients only, not cc and bcc
    mylogger.info "Sending email notification to: #{notified_users.join(', ')}" if mylogger

    # Blind carbon copy recipients
    if Setting.bcc_recipients?
      bcc(notified_users)
      recipients [] if recipients
      cc [] if cc
    end
    super
  end

Felix Schäfer wrote:

Chris Serio wrote:

The second part of the fix is in the BCC block of code where I just test "if recipients" and "if cc" before nuking the arrays (if that's what the empty [] brackets even do).

recipients [] resets the recipients to an empty array indeed, but no need to test if there are recipients or not.

Anyway, my proposed solution is to force the recipients and the cc to an array before removing the author email. The tests pass on 1.8.7 and 1.9.2 with the following, could you try it on your system?

[...]

Hi Felix, I have not had time to test this on my system yet but in my fix, I definitely needed to test whether there were recipients/cc or not because it crashed when I didn't. I kept getting "NoMethodError (undefined method `[]' for nil:NilClass)". Perhaps this is caused by my fix doing something wrong that yours does not?