ChiliProject is not maintained anymore. Please be advised that there will be no more updates.

We do not recommend that you setup new ChiliProject instances and we urge all existing users to migrate their data to a maintained system, e.g. Redmine. We will provide a migration script later. In the meantime, you can use the instructions by Christian Daehn.

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


Added by Chris Serio at 2011-11-27 06:54 pm. Updated at 2011-11-30 05:04 pm.


Status:Closed Start date:2011-11-27
Priority:Normal Due date:
Assignee:Felix Schäfer % Done:

0%

Category:Mail Sending
Target version:2.5.0
Remote issue URL: Affected version:2.4.0

Description

I have ChiliProject 2.4.0 installed on an Ubuntu server with Apache2. I'm using Ruby 1.9.2. When i create a user with an email address with a dash such as I get a 500 internal error with this stack:

Notice the "y-d".

ArgumentError (invalid range "y-d" in string transliteration):
app/models/mailer.rb:399:in `delete'
app/models/mailer.rb:399:in `create_mail'
app/controllers/users_controller.rb:106:in `create'
<internal:prelude>:10:in `synchronize'
passenger (3.0.9) lib/phusion_passenger/rack/request_handler.rb:96:in `process_request'
passenger (3.0.9) lib/phusion_passenger/abstract_request_handler.rb:513:in `accept_and_process_next_request'
passenger (3.0.9) lib/phusion_passenger/abstract_request_handler.rb:274:in `main_loop'
passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:321:in `start_request_handler'
passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:275:in `block in handle_spawn_application'
passenger (3.0.9) lib/phusion_passenger/utils.rb:479:in `safe_fork'
passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:270:in `handle_spawn_application'
passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `server_main_loop'
passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:206:in `start_synchronously'
passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:180:in `start'
passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:149:in `start'
passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:219:in `block (2 levels) in spawn_rails_application'
passenger (3.0.9) lib/phusion_passenger/abstract_server_collection.rb:132:in `lookup_or_add'
passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:214:in `block in spawn_rails_application'
passenger (3.0.9) lib/phusion_passenger/abstract_server_collection.rb:82:in `block in synchronize'
<internal:prelude>:10:in `synchronize'
passenger (3.0.9) lib/phusion_passenger/abstract_server_collection.rb:79:in `synchronize'
passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:213:in `spawn_rails_application'
passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:132:in `spawn_application'
passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:275:in `handle_spawn_application'
passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `server_main_loop'
passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:206:in `start_synchronously'
passenger (3.0.9) helper-scripts/passenger-spawn-server:99:in `<main>'

Associated revisions

Revision f333f43a
Added by Felix Schäfer at 2011-11-30 06:04 pm

[#736] force recipients/cc to arrays in the Mailer

String#delete might break on 1.9 with dashes in the author's email address. Furthermore, String#delete doesn't do what the original author thought it does.

History

Updated by Chris Serio at 2011-11-27 06:54 pm

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

Updated by Felix Schäfer at 2011-11-27 06:55 pm

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

Updated by Felix Schäfer at 2011-11-27 07:03 pm

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.

Updated by Chris Serio at 2011-11-27 07:10 pm

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

Updated by Felix Schäfer at 2011-11-28 06:55 am

Chris Serio wrote:

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.

That would probably work, the problem with your solution is that you call recipients = and cc =, in that context, the recipients and cc you want to call are methods, i.e. something like cc(cc.gsub(@author.mail, '')) if cc would have worked. (and yes, you can have a method and a variable with the same name in the same scope…)

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?

 1diff --git a/app/models/mailer.rb b/app/models/mailer.rb
 2index 2be9de7..cfeab4e 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((recipients.is_a?(Array) ? recipients : [recipients]) - [@author.mail]) if recipients
12+      cc((cc.is_a?(Array) ? cc : [cc]) - [@author.mail]) if cc
13     end
14
15     notified_users = [recipients, cc].flatten.compact.uniq
  • Target version set to 2.5.0
  • (deleted custom field) set to 2.4.0
  • Status changed from Open to Ready for review

Updated by Chris Serio at 2011-11-29 10:45 am

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?

Updated by Felix Schäfer at 2011-11-30 05:04 pm

Pushed in f333f43, thanks for the report and your help :-)

  • Status changed from Ready for review to Closed

Also available in: Atom PDF