Redmine

+1

After applying the patch and edit: /config/initializers/10-patches.rb

module ActionMailer
  class MessageDelivery < Delegator
    def deliver
      deliver_later
    end
  end
end

I get an error when updating an issue.

Started PATCH "/issues/2" for 127.0.0.1 at 2018-05-23 04:38:23 +0000
Processing by IssuesController#update as HTML
  Parameters: {"utf8"=>"✓",...}
...

[ActiveJob] [ActionMailer::DeliveryJob] [a5d3c247-2e7a-4879-aaad-d0590de9026b] Performing ActionMailer::DeliveryJob (Job ID: a5d3c247-2e7a-4879-aaad-d0590de9026b) from Async(mailers) with arguments: "Mailer", "issue_edit", "deliver_now", #<GlobalID:0x00007fa2d053b888 @uri=#<URI::GID gid://redmine-app/User/5>>, #<GlobalID:0x00007fa2d053af28 @uri=#<URI::GID gid://redmine-app/Journal/8>>

[ActiveJob] [ActionMailer::DeliveryJob] [a5d3c247-2e7a-4879-aaad-d0590de9026b] Error performing ActionMailer::DeliveryJob (Job ID: a5d3c247-2e7a-4879-aaad-d0590de9026b) from Async(mailers) in 0.15ms: ArgumentError (wrong number of arguments (given 2, expected 1)):

.../app/models/mailer.rb:246:in `issue_edit'

Is there a plan to get this patch merged into the core code base?

Ken Zalewski wrote:

Is there a plan to get this patch merged into the core code base?

It's on the roadmap for 4.0.0 so I think there is a plan. ;-)

Because of r17269, the patch does not apply cleanly. Could you update it so we can commit this change? Thanks.

Thank you Marius, but still I get many conflicts when trying to apply your patch on current trunk.

Jean-Philippe Lang wrote:

Thank you Marius, but still I get many conflicts when trying to apply your patch on current trunk.

Let me take a look, maybe I missed something.

Thanks Marius, your patch applies cleanly but I get a few test failures: IP address is blank in security notifications.

Jean-Philippe Lang wrote:

Thanks Marius, your patch applies cleanly but I get a few test failures: IP address is blank in security notifications.

Sorry Jean-Philippe, I'm not sure how to fix the failing test and I need more time to investigate the problem by myself. I propose to postpone this ticket to 4.1.0 if Holger can't update the patch in a proper time. IMHO, I consider the release of 4.0.0 top priority.

I looked again in the code and a fix that makes sense for me is the following one:

diff --git a/app/models/mailer.rb b/app/models/mailer.rb
index 717b000..fad1939 100644
--- a/app/models/mailer.rb
+++ b/app/models/mailer.rb
@@ -604,7 +604,7 @@ class Mailer < ActionMailer::Base
       value: options[:value]
     )
     @title = options[:title] && l(options[:title])
-    @originator = options[:originator] || User.current
+    @originator = options[:originator] || sender
     @remote_ip = options[:remote_ip] || @originator.remote_ip
     @url = options[:url] && (options[:url].is_a?(Hash) ? url_for(options[:url]) : options[:url])
     redmine_headers 'Sender' => @originator.login

diff --git a/app/models/mailer.rb b/app/models/mailer.rb
index fad1939ce..0e3f01483 100644
--- a/app/models/mailer.rb
+++ b/app/models/mailer.rb
@@ -598,7 +598,6 @@ class Mailer < ActionMailer::Base
   def security_notification(sender, options={})
     @sender = sender
     redmine_headers 'Sender' => sender.login
-    @user = User.current
     @message = l(options[:message],
       field: (options[:field] && l(options[:field])),
       value: options[:value]

The test MailerTest#test_security_notification (IP address is blank in security notifications.) passes now and the results for the entire tests suite look good on my environment (with the updated patch and the above two proposed fixes applied):

View test resultsView test results

Run options: --seed 34100

# Running:

.................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................F

Failure:
CalendarsControllerTest#test_show [/work/test/functional/calendars_controller_test.rb:56]:
Expected at least 1 element matching "img[class="gravatar"]", found 0..
Expected 0 to be >= 1.

bin/rails test test/functional/calendars_controller_test.rb:35

Error:
GanttsControllerTest#test_gantt_should_export_to_png:
Magick::ImageMagickError: non-conforming drawing primitive definition `text' @ error/draw.c/DrawImage/3265
    lib/redmine/helpers/gantt.rb:439:in `draw'
    lib/redmine/helpers/gantt.rb:439:in `to_image'
    app/controllers/gantts_controller.rb:42:in `block (2 levels) in show'
    app/controllers/gantts_controller.rb:40:in `show'
    lib/redmine/sudo_mode.rb:63:in `sudo_mode'
    test/functional/gantts_controller_test.rb:153:in `test_gantt_should_export_to_png'

bin/rails test test/functional/gantts_controller_test.rb:152

Finished in 451.992636s, 9.5909 runs/s, 63.0984 assertions/s.
4335 runs, 28520 assertions, 1 failures, 1 errors, 2 skips

You have skipped tests. Run with --verbose for details.

1. CalendarsControllerTest#test_show [/work/test/functional/calendars_controller_test.rb:56] fails also on the current trunk (https://redmine.org/builds/logs/build_trunk_mysql_ruby-2.5_384.html)
2. "GanttsControllerTest#test_gantt_should_export_to_png" is caused by an issue on my local environment.

Marius BALTEANU wrote:

[...]
where:

1. CalendarsControllerTest#test_show [/work/test/functional/calendars_controller_test.rb:56] fails also on the current trunk (https://redmine.org/builds/logs/build_trunk_mysql_ruby-2.5_384.html)

I think that was an external problem because now the test passes on redmine.org and on my environment as well.

Thank you all for working on the patch and considering its inclusion!

I'll try to get to this tomorrow to fix any remaining test failures. Unfortunately, it's been a crazy week so far and I hadn't had much time so far...

Holger Just wrote:

If the general direction of this patch is accepted, it is probably desireable to move the mail sending to ActiveJob completely. This could replace the async_* delivery methods currently configurable in configuration.yml. This change can either be hardcoded in source:trunk/config/initializers/10-patches.rb#L160 or made configurable with a setting. I'll be glad to provide a patch for this. It would only be a few lines of code.

Yes, we should now use to ActiveJob completely. I had to revert patch 0003 because it was raising an error when trying to send the test_email with #deliver_later (see the test added in r17587), something wrong with the serialization:

Error:
MailerTest#test_test_later:
ArgumentError: First argument has to be a user, was "--- !ruby/object:User\nconcise_attributes:\n- 
!ruby/object:ActiveModel::Attribute::FromDatabase\n  name: id\n  value_before_type_cast: 1\n- 
!ruby/object:ActiveModel::Attribute::FromDatabase\n  name: login\n  value_before_type_cast: admin\n- 
!ruby/object:ActiveModel::Attribute::FromDatabase\n  name: hashed_password\n  value_before_type_cast: 
b5b6ff9543bf1387374cdfa27a54c96d236a7150\n- !ruby/object:ActiveModel::Attribute::FromDatabase\n  name: firstname\n  
value_before_type_cast: Redmine\n- !ruby/object:ActiveModel::Attribute::FromDatabase\n  
...

holger mareck, have you an implementation of MultiMessage that is compatible with Async delivery?

Current trunk seems to be far from being compatible. I've spent a few hours on it and it just seems to be broken. For example, I tried to send settings_updated notification asynchronously (see patch below), and only the current user is getting an email. Mailer.settings_updated is called the first time with the array of admins, and then when the "real" notification is processed in the background, it's called with the current user as the first argument.

Please let me know if you have worked on it before I go on.

Index: app/models/mailer.rb
===================================================================
--- app/models/mailer.rb    (revision 17587)
+++ app/models/mailer.rb    (working copy)
@@ -657,7 +657,7 @@
     return unless changes.present?

     users = User.active.where(admin: true).to_a
-    settings_updated(users, changes).deliver
+    settings_updated(users, changes).deliver_later
   end

   # Build a Mail::Message object with a test email for the current user
Index: app/models/setting.rb
===================================================================
--- app/models/setting.rb    (revision 17581)
+++ app/models/setting.rb    (working copy)
@@ -131,7 +131,7 @@
       previous_value = Setting[name]
       set_from_params name, value
       if available_settings[name.to_s]['security_notifications'] && Setting[name] != previous_value
-        changes << name
+        changes << name.to_s
       end
     end
     if changes.any?

One way to work around this would be to introduce deliver_* methods (like the now obsolete Mailer.deliver_issue_add method). However doing so would have changed the intended external interface of our Mailer class and would likely lead to integration pain with all existing Redmine plugins sending mail.

I understand that it would change the external interface of the Mailer but I think it's a better way to go than the trickier proposed solution. Maybe we should revert all the changes done here and postpone this feature in order not to delay 4.0 release.

Jean-Philippe Lang wrote:

One way to work around this would be to introduce deliver_* methods (like the now obsolete Mailer.deliver_issue_add method). However doing so would have changed the intended external interface of our Mailer class and would likely lead to integration pain with all existing Redmine plugins sending mail.

I understand that it would change the external interface of the Mailer but I think it's a better way to go than the trickier proposed solution. Maybe we should revert all the changes done here and postpone this feature in order not to delay 4.0 release.

In my opinion, the safest option now (from many reasons) is to revert all the changes done and postpone this feature for 4.1.0.

Sorry for the long silence from my part. It was rather busy at work recently and I had no further energy to work on this issue during the evenings. With that being said, thank you for working on this patch Marius and Jean=-Philippe. I sincerely appreciate it.

In general, I tried to keep the external interface of the mailer (i.e. the interface used by all the rest of Redmine's code) as stable as possible. This resulted in some complications which I tried to solve with the MultiMailer. With my initial tests, I checked that it the MultiMailer compatible with redmine's existing async-* delivery methods. I had not initially checked whether this works with ActiveJob. I do appreciate your work on moving over to it since it will help reduce special homegrown code in favor of upstream-maintained mechanisms.

Personally, I'm very fine with adding the deliver_* methods to the mailer. Since the methods signatures of the previously used Mailer methods change (because if the added user parameter), it should be apparent when some plugin is not yet upgraded since it will result in a load ArgumentError instead of silently swallowing notifications. Using the deliver_* class methods makes the whole implementation of the mailer much simpler.

Some comments to your last patch, Jean-Philippe:

• Do we still need Mailer#method_missing? Since we are reverting on the deprecation, we should probably get rid of the fallback to avoid confusion.
• On some places in the Mailer class, you still directly call deliver. This seems to still work as an alias to deliver_now but I have no idea why since you removed the patch in config/initializers/10-patches.rb. In any case, shouldn't the calls also be to deliver_later?

As for my third patch to adapt the serialization of the references objects for ActiveJob, this was more or less a proof-of concept to ensure that the full state of the notified object is encoded into the job. This would have caused side-effects if you are not very careful since it could also serialize nested ActiveRecord objects which might cause strange consistency issues if they differ from the state in the database.

I have attempted to test this when I wrote the patch and found no further issues, but I did not primarily test it to use ActiveJob's deliver_later function yet. This was something I intended to further investigate once the general idea of the patch was accepted.

In general, I think it is desirable to not mess with the serialization at all and not use the third patch (which you have now reverted in r17587 already). Instead, we might try to make the objects used for rendering the mails (mostly) immutable over time. For issues an option could be to use "representer" objects instead of actual issues. These would be Structs (or similar) which could be serialized into the job and contain all necessary information to render the mail without having to rely in the current state of the issue object in the database.

What do you think?

Marius BALTEANU wrote:

In my opinion, the safest option now (from many reasons) is to revert all the changes done and postpone this feature for 4.1.0.

That was the idea but I had some time to work on this and I finally decided to commit the changes.

Holger Just wrote:

• Do we still need Mailer#method_missing? Since we are reverting on the deprecation, we should probably get rid of the fallback to avoid confusion.
• On some places in the Mailer class, you still directly call deliver. This seems to still work as an alias to deliver_now but I have no idea why since you removed the patch in config/initializers/10-patches.rb. In any case, shouldn't the calls also be to deliver_later?

Thanks, this is fixed.

In general, I think it is desirable to not mess with the serialization at all and not use the third patch (which you have now reverted in r17587 already). Instead, we might try to make the objects used for rendering the mails (mostly) immutable over time. For issues an option could be to use "representer" objects instead of actual issues. These would be Structs (or similar) which could be serialized into the job and contain all necessary information to render the mail without having to rely in the current state of the issue object in the database.

That would be nice but we don't just need the issue and its attributes. Rendering the emails relies on many associations like attachments, custom fields. I'm afraid this solution would lead to different kind of problems. I'd happy to get some feedback anyway if you want to give it a try. Thanks.

Holger Just wrote:

BCC recipients¶

Since each user gets their own mail, we can probably remove the setting to send mails with BCC completely. If plugins want to send mails to non-user recipients, we would have to make sure to either set the recipients manually on BCC or group them appropriately. It is not a required setting in Redmine core anymore though.

I've attached a patch to #30820 to drop the bcc setting.