Refactor: Decouple failed attachments and the flash messages

Attachment#attach_files will no longer need to return a flash message,
instead it will put unsaved attachments into object#unsaved_attachments
where the calling object can access them.

A utility method #render_attachment_warning_if_needed is included for setting
the standard flash warning.

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@3528 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/controllers/application_controller.rb

@@ -301,4 +301,9 @@ class ApplicationController < ActionController::Base
   def api_request?
     %w(xml json).include? params[:format]
   end
+
+  # Renders a warning flash if obj has unsaved attachments
+  def render_attachment_warning_if_needed(obj)
+    flash[:warning] = l(:warning_attachments_not_saved, obj.unsaved_attachments.size) if obj.unsaved_attachments.present?
+  end
 end

app/controllers/documents_controller.rb

@@ -48,7 +48,7 @@ class DocumentsController < ApplicationController
     @document = @project.documents.build(params[:document])    
     if request.post? and @document.save	
       attachments = Attachment.attach_files(@document, params[:attachments])
-      flash[:warning] = attachments[:flash] if attachments[:flash]
+      render_attachment_warning_if_needed(@document)
       flash[:notice] = l(:notice_successful_create)
       redirect_to :action => 'index', :project_id => @project
     end
@@ -69,7 +69,7 @@ class DocumentsController < ApplicationController
   
   def add_attachment
     attachments = Attachment.attach_files(@document, params[:attachments])
-    flash[:warning] = attachments[:flash] if attachments[:flash]
+    render_attachment_warning_if_needed(@document)
 
     Mailer.deliver_attachments_added(attachments[:files]) if attachments.present? && attachments[:files].present? && Setting.notified_events.include?('document_added')
     redirect_to :action => 'show', :id => @document

app/controllers/issues_controller.rb

@@ -159,7 +159,7 @@ class IssuesController < ApplicationController
       call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue })
       if @issue.save
         attachments = Attachment.attach_files(@issue, params[:attachments])
-        flash[:warning] = attachments[:flash] if attachments[:flash]
+        render_attachment_warning_if_needed(@issue)
         flash[:notice] = l(:notice_successful_create)
         call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
         respond_to do |format|
@@ -571,7 +571,8 @@ private
 
     if @issue.valid?
       attachments = Attachment.attach_files(@issue, params[:attachments])
-      flash[:warning] = attachments[:flash] if attachments[:flash]
+      render_attachment_warning_if_needed(@issue)
+
       attachments[:files].each {|a| @journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
       call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
       if @issue.save

app/controllers/messages_controller.rb

@@ -63,7 +63,7 @@ class MessagesController < ApplicationController
     if request.post? && @message.save
       call_hook(:controller_messages_new_after_save, { :params => params, :message => @message})
       attachments = Attachment.attach_files(@message, params[:attachments])
-      flash[:warning] = attachments[:flash] if attachments[:flash]
+      render_attachment_warning_if_needed(@message)
       redirect_to :action => 'show', :id => @message
     end
   end
@@ -77,7 +77,7 @@ class MessagesController < ApplicationController
     if !@reply.new_record?
       call_hook(:controller_messages_reply_after_save, { :params => params, :message => @reply})
       attachments = Attachment.attach_files(@reply, params[:attachments])
-      flash[:warning] = attachments[:flash] if attachments[:flash]
+      render_attachment_warning_if_needed(@reply)
     end
     redirect_to :action => 'show', :id => @topic, :r => @reply
   end
@@ -91,7 +91,7 @@ class MessagesController < ApplicationController
     end
     if request.post? && @message.update_attributes(params[:message])
       attachments = Attachment.attach_files(@message, params[:attachments])
-      flash[:warning] = attachments[:flash] if attachments[:flash]
+      render_attachment_warning_if_needed(@message)
       flash[:notice] = l(:notice_successful_update)
       @message.reload
       redirect_to :action => 'show', :board_id => @message.board, :id => @message.root, :r => (@message.parent_id && @message.id)

app/controllers/projects_controller.rb

@@ -304,7 +304,7 @@ class ProjectsController < ApplicationController
     if request.post?
       container = (params[:version_id].blank? ? @project : @project.versions.find_by_id(params[:version_id]))
       attachments = Attachment.attach_files(container, params[:attachments])
-      flash[:warning] = attachments[:flash] if attachments[:flash]
+      render_attachment_warning_if_needed(container)
 
       if !attachments.empty? && Setting.notified_events.include?('file_added')
         Mailer.deliver_attachments_added(attachments[:files])

app/controllers/wiki_controller.rb

@@ -77,7 +77,7 @@ class WikiController < ApplicationController
     else
       if !@page.new_record? && @content.text == params[:content][:text]
         attachments = Attachment.attach_files(@page, params[:attachments])
-        flash[:warning] = attachments[:flash] if attachments[:flash]
+        render_attachment_warning_if_needed(@page)
         # don't save if text wasn't changed
         redirect_to :action => 'index', :id => @project, :page => @page.title
         return
@@ -89,7 +89,7 @@ class WikiController < ApplicationController
       # if page is new @page.save will also save content, but not if page isn't a new record
       if (@page.new_record? ? @page.save : @content.save)
         attachments = Attachment.attach_files(@page, params[:attachments])
-        flash[:warning] = attachments[:flash] if attachments[:flash]
+        render_attachment_warning_if_needed(@page)
         call_hook(:controller_wiki_edit_after_save, { :params => params, :page => @page})
         redirect_to :action => 'index', :id => @project, :page => @page.title
       end
@@ -214,7 +214,7 @@ class WikiController < ApplicationController
   def add_attachment
     return render_403 unless editable?
     attachments = Attachment.attach_files(@page, params[:attachments])
-    flash[:warning] = attachments[:flash] if attachments[:flash]
+    render_attachment_warning_if_needed(@page)
     redirect_to :action => 'index', :page => @page.title
   end
 

app/models/attachment.rb

@@ -139,11 +139,9 @@ class Attachment < ActiveRecord::Base
   # Returns a Hash of the results:
   # :files => array of the attached files
   # :unsaved => array of the files that could not be attached
-  # :flash => warning message
   def self.attach_files(obj, attachments)
     attached = []
     unsaved = []
-    flash = nil
     if attachments && attachments.is_a?(Hash)
       attachments.each_value do |attachment|
         file = attachment['file']
@@ -152,13 +150,10 @@ class Attachment < ActiveRecord::Base
                               :file => file,
                               :description => attachment['description'].to_s.strip,
                               :author => User.current)
-        a.new_record? ? (unsaved << a) : (attached << a)
+        a.new_record? ? (obj.unsaved_attachments << a) : (attached << a)
-      end
-      if unsaved.any?
-        flash = l(:warning_attachments_not_saved, unsaved.size)
       end
     end
-    {:files => attached, :flash => flash, :unsaved => unsaved}
+    {:files => attached, :unsaved => obj.unsaved_attachments}
   end
   
 private

vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb

@@ -32,6 +32,8 @@ module Redmine
           has_many :attachments, options.merge(:as => :container,
                                                :order => "#{Attachment.table_name}.created_on",
                                                :dependent => :destroy)
+          attr_accessor :unsaved_attachments
+          after_initialize :initialize_unsaved_attachments
           send :include, Redmine::Acts::Attachable::InstanceMethods
         end
       end
@@ -48,7 +50,11 @@ module Redmine
         def attachments_deletable?(user=User.current)
           user.allowed_to?(self.class.attachable_options[:delete_permission], self.project)
         end
-        
+
+        def initialize_unsaved_attachments
+          @unsaved_attachments ||= []
+        end
+
         module ClassMethods
         end
       end