Feature #16006

Include attachments in forum post notifications

Added by T. Hauptman about 6 years ago. Updated 16 days ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Email notifications
Target version:4.2.0
Resolution:Fixed

Description

The send_notification is called on message creation, before attachments are added. In my case, I need links to attachments to be sent in the notification email.

My app/views/mailer/message_posted.text.erb do do this is:

<%= @message.content %>

<% if @message.attachments.any? -%>
---<%= l(:label_attachment_plural).ljust(37, '-') %>
<% @message.attachments.each do |attachment| -%>
<%=attachment.filename%>: <%=send(:named_attachment_path, attachment, attachment.filename,:only_path=>false) %> (<%= number_to_human_size(attachment.filesize) %>)
<% end -%>
<% end -%>
<%= @author.firstname + " " + @author.lastname + " - "+ @author.mail %>

If the forum message is created by an incoming email (with attachments) then no attachements are shown in outgoing notifications. If the forum message is created via the web form, the attachments are shown.

I fixed this by changing the send_notification trigger to after_commit. Patch attached.

notify.patch Magnifier (1.04 KB) T. Hauptman, 2014-02-01 20:37

forum_notification_include_attachments.png (200 KB) Yuichi HARADA, 2019-10-29 06:30

16006_forum_notification_include_attachments.patch Magnifier (3.08 KB) Yuichi HARADA, 2019-10-29 06:35

no-attachment-in-reply@2x.png (91 KB) Go MAEDA, 2019-12-19 13:06

attachment-in-reply.png (370 KB) Yuichi HARADA, 2019-12-26 01:55

16006_forum_notification_include_attachments_v2.patch Magnifier (4.76 KB) Yuichi HARADA, 2020-01-23 06:07


Related issues

Related to Redmine - Patch #1616: Allow email to create and reply to forum messages New 2008-07-10
Related to Redmine - Feature #33002: Include attachments in news post notifications Closed

Associated revisions

Revision 19506
Added by Go MAEDA 16 days ago

Include attachments in forum post notifications (#16006).

Patch by Yuichi HARADA.

History

#1 Updated by Toshi MARUYAMA almost 6 years ago

  • Category set to Email notifications

#2 Updated by Toshi MARUYAMA almost 6 years ago

  • Tracker changed from Patch to Defect

#3 Updated by Toshi MARUYAMA almost 6 years ago

  • Tracker changed from Defect to Patch

#4 Updated by Toshi MARUYAMA almost 6 years ago

  • Related to Patch #1616: Allow email to create and reply to forum messages added

#5 Updated by Yuichi HARADA 4 months ago

T. Hauptman wrote:

I fixed this by changing the send_notification trigger to after_commit.

Already improved by source:trunk/app/models/message.rb@17588#L48 . However, the attachments are not included in forum post notification.
I created a patch to include attachments in forum post notification.

#6 Updated by Go MAEDA 4 months ago

  • Target version set to Candidate for next major release

#7 Updated by Go MAEDA 2 months ago

Thank you for posting the patch. I tried out the patch but I am experiencing a problem that attachments are included only when a topic is created. No attachments information is in a notification when a reply is added.

#8 Updated by Yuichi HARADA 2 months ago

Go MAEDA wrote:

Thank you for posting the patch. I tried out the patch but I am experiencing a problem that attachments are included only when a topic is created. No attachments information is in a notification when a reply is added.

It cannot be reproduced. Attachments information is in a notification when a reply is added.

#9 Updated by Go MAEDA about 1 month ago

I still see the problem when adding a reply.

I found that container_type and container_id have not been set to the attachment when sending notifications. Those values are set after the delivery is finished. So, @message.attachments.any? in app/views/mailer/message_posted.html.erb returns false when generating notification emails.

[ActiveJob] Enqueued ActionMailer::DeliveryJob (Job ID: ce613aa3-4241-4bc4-b59d-009475839967) to Inline(mailers) with arguments: "Mailer", "message_posted", "deliver_now", #<GlobalID:0x00007ff1b32ac090 @uri=#<URI::GID gid://redmine-app/User/1>>, #<GlobalID:0x00007ff1b0f1b680 @uri=#<URI::GID gid://redmine-app/Message/9>>
  Attachment Load (0.3ms)  SELECT  "attachments".* FROM "attachments" WHERE "attachments"."id" = ? AND "attachments"."digest" = ? LIMIT ?  [["id", 29], ["digest", "632a392af1de2e3de4d8b5ad62da7275550ee0004cdf9f54fa3de05cd28d0aac"], ["LIMIT", 1]]
   (1.0ms)  begin transaction
  CACHE User Load (0.0ms)  SELECT  "users".* FROM "users" WHERE "users"."type" IN ('User', 'AnonymousUser') AND "users"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
  Attachment Update (1.1ms)  UPDATE "attachments" SET "container_id" = ?, "description" = ?, "container_type" = ? WHERE "attachments"."id" = ?  [["container_id", 9], ["description", ""], ["container_type", "Message"], ["id", 29]]
   (1.2ms)  commit transaction

#10 Updated by Yuichi HARADA about 1 month ago

Go MAEDA wrote:

I still see the problem when adding a reply.

Perhaps I think that Message's callback method after_create_commit ( Message#send_notification ) fired before saving the attachments.
Add the following patch to 16006_forum_notification_include_attachments.patch .

diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb
index bbc6a35d4..2a0e341b6 100644
--- a/app/controllers/messages_controller.rb
+++ b/app/controllers/messages_controller.rb
@@ -77,10 +77,10 @@ class MessagesController < ApplicationController
     @reply.author = User.current
     @reply.board = @board
     @reply.safe_attributes = params[:reply]
+    @reply.save_attachments(params[:attachments])
     @topic.children << @reply
     if !@reply.new_record?
       call_hook(:controller_messages_reply_after_save, { :params => params, :message => @reply})
-      attachments = Attachment.attach_files(@reply, params[:attachments])
       render_attachment_warning_if_needed(@reply)
     end
     flash[:notice] = l(:notice_successful_update)
@@ -91,12 +91,14 @@ class MessagesController < ApplicationController
   def edit
     (render_403; return false) unless @message.editable_by?(User.current)
     @message.safe_attributes = params[:message]
-    if request.post? && @message.save
-      attachments = Attachment.attach_files(@message, params[:attachments])
-      render_attachment_warning_if_needed(@message)
-      flash[:notice] = l(:notice_successful_update)
-      @message.reload
-      redirect_to board_message_path(@message.board, @message.root, :r => (@message.parent_id && @message.id))
+    if request.post?
+      @message.save_attachments(params[:attachments])
+      if @message.save
+        render_attachment_warning_if_needed(@message)
+        flash[:notice] = l(:notice_successful_update)
+        @message.reload
+        redirect_to board_message_path(@message.board, @message.root, :r => (@message.parent_id && @message.id))
+      end
     end
   end

#11 Updated by Go MAEDA 28 days ago

Yuichi HARADA wrote:

Perhaps I think that Message's callback method after_create_commit ( Message#send_notification ) fired before saving the attachments.
Add the following patch to 16006_forum_notification_include_attachments.patch .

Thank you for updating the patch. The latest patch works fine.

However, it may insert an orphaned row in the attachments table if Redmine fails to save a Message object for some reason. A possible cause is validation error when saving the Message object.

The following is an example of an orphaned row. Since Redmine failed to save a Message object, container_id and container_type of the Attachment object is nil.

[2] pry(main)> Attachment.last
  Attachment Load (0.3ms)  SELECT  "attachments".* FROM "attachments" ORDER BY "attachments"."id" DESC LIMIT ?  [["LIMIT", 1]]
=> #<Attachment:0x00007ffa37fa58f0
 id: 28,
 container_id: nil,
 container_type: nil,
 filename: "lenna.jpg",
 disk_filename: "200130122359_lenna.jpg",
 filesize: 20401,
 content_type: "image/jpeg",
 digest: "632a392af1de2e3de4d8b5ad62da7275550ee0004cdf9f54fa3de05cd28d0aac",
 downloads: 0,
 author_id: 1,
 created_on: Thu, 30 Jan 2020 08:06:16 UTC +00:00,
 description: nil,
 disk_directory: "2020/01">

#12 Updated by Yuichi HARADA 21 days ago

Go MAEDA wrote:

However, it may insert an orphaned row in the attachments table if Redmine fails to save a Message object for some reason. A possible cause is validation error when saving the Message object.

The following is an example of an orphaned row. Since Redmine failed to save a Message object, container_id and container_type of the Attachment object is nil.

[...]

Thank you for pointing out.
However, I think running the rake task redmine:attachments:prune on a regular basis should solve it.

#13 Updated by Go MAEDA 21 days ago

  • Target version changed from Candidate for next major release to 4.2.0

Yuichi HARADA wrote:

Go MAEDA wrote:

However, it may insert an orphaned row in the attachments table if Redmine fails to save a Message object for some reason. A possible cause is validation error when saving the Message object.

The following is an example of an orphaned row. Since Redmine failed to save a Message object, container_id and container_type of the Attachment object is nil.

[...]

Thank you for pointing out.
However, I think running the rake task redmine:attachments:prune on a regular basis should solve it.

You are right and I was wrong. Such orphaned attachments may also be created when attaching files to issues and the orphaned files are treated by rake redmine:attachments:prune. So, the patch doesn't have to consider the case and your patch 16006_forum_notification_include_attachments_v2.patch can be merged into the trunk as is.

Now I am setting the target version to 4.2.0.

#14 Updated by Go MAEDA 16 days ago

  • Tracker changed from Patch to Feature
  • Subject changed from Forum post notification does not include attachments to Include attachments in forum post notifications
  • Assignee set to Go MAEDA
  • Resolution set to Fixed

Committed the patch. Thank you for your contribution.

#15 Updated by Go MAEDA 16 days ago

  • Status changed from New to Closed

#16 Updated by Go MAEDA 13 days ago

  • Related to Feature #33002: Include attachments in news post notifications added

Also available in: Atom PDF