Daily Refactor #28: Decouple flash messages from Attachment#attach_files

The Refactoring

After thinking about yesterday’s refactor, I noticed that setting the flash message is going to cause problems for future refactorings since it’s trying to break the MVC constraints. Today I found a way to decouple tracking the failed saves while keeping the flash messages up in the controllers.

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21

# vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb
module Redmine
  module Acts
    module Attachable
      def self.included(base)
        base.extend ClassMethods
      end
 
      module ClassMethods
        def acts_as_attachable(options = {})
          # ...
          send :include, Redmine::Acts::Attachable::InstanceMethods
        end
      end
 
      module InstanceMethods
        # ...
      end
    end
  end
end
 
1
2
3
4
5
6
7
8
9

# app/models/attachment.rb
class Attachment  0
        a = Attachment.create(:container => obj, 
                              :file => file,
                              :description => attachment['description'].to_s.strip,
                              :author => User.current)
        a.new_record? ? (unsaved &lt;&lt; a) : (attached &lt;<a> attached, :flash =&gt; flash, :unsaved =&gt; unsaved}
  end
end
 
1
2
3
4

# app/controllers/application_controller.rb
class ApplicationController &lt; ActionController::Base
  # ...
end
 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24

# app/controllers/issues_controller.rb
class IssuesController  @project, :issue =&gt; @issue, :user =&gt; User.current, :spent_on =&gt; Date.today)
      @time_entry.attributes = params[:time_entry]
      @issue.time_entries &lt;&lt; @time_entry
    end
 
    if @issue.valid?
      attachments = Attachment.attach_files(@issue, params[:attachments])
      flash[:warning] = attachments[:flash] if attachments[:flash]
      attachments[:files].each {|a| @journal.details &lt; 'attachment', :prop_key =&gt; a.id, :value =&gt; a.filename)}
      call_hook(:controller_issues_edit_before_save, { :params =&gt; params, :issue =&gt; @issue, :time_entry =&gt; @time_entry, :journal =&gt; @journal})
      if @issue.save
        if !@journal.new_record?
          # Only send notification if something was actually changed
          flash[:notice] = l(:notice_successful_update)
        end
        call_hook(:controller_issues_edit_after_save, { :params =&gt; params, :issue =&gt; @issue, :time_entry =&gt; @time_entry, :journal =&gt; @journal})
        return true
      end
    end
    # failure, returns false
 
  end
end

After

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28

# vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb
module Redmine
  module Acts
    module Attachable
      def self.included(base)
        base.extend ClassMethods
      end
 
      module ClassMethods
        def acts_as_attachable(options = {})
          # ...
          attr_accessor :unsaved_attachments
          after_initialize :initialize_unsaved_attachments
          send :include, Redmine::Acts::Attachable::InstanceMethods
        end
      end
 
      module InstanceMethods
        # ...
 
        def initialize_unsaved_attachments
          @unsaved_attachments ||= []
        end
 
      end
    end
  end
end
 
1
2
3
4
5
6
7
8
9

# app/models/attachment.rb
class Attachment  0
        a = Attachment.create(:container =&gt; obj, 
                              :file =&gt; file,
                              :description =&gt; attachment['description'].to_s.strip,
                              :author =&gt; User.current)
        a.new_record? ? (obj.unsaved_attachments &lt;&lt; a) : (attached &lt;<a> attached, :unsaved =&gt; obj.unsaved_attachments}
  end
end
 
1
2
3
4
5
6
7

# app/controllers/application_controller.rb
class ApplicationController &lt; ActionController::Base
  # 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
 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25

# app/controllers/issues_controller.rb
class IssuesController  @project, :issue =&gt; @issue, :user =&gt; User.current, :spent_on =&gt; Date.today)
      @time_entry.attributes = params[:time_entry]
      @issue.time_entries &lt;&lt; @time_entry
    end
 
    if @issue.valid?
      attachments = Attachment.attach_files(@issue, params[:attachments])
      render_attachment_warning_if_needed(@issue)
 
      attachments[:files].each {|a| @journal.details &lt; 'attachment', :prop_key =&gt; a.id, :value =&gt; a.filename)}
      call_hook(:controller_issues_edit_before_save, { :params =&gt; params, :issue =&gt; @issue, :time_entry =&gt; @time_entry, :journal =&gt; @journal})
      if @issue.save
        if !@journal.new_record?
          # Only send notification if something was actually changed
          flash[:notice] = l(:notice_successful_update)
        end
        call_hook(:controller_issues_edit_after_save, { :params =&gt; params, :issue =&gt; @issue, :time_entry =&gt; @time_entry, :journal =&gt; @journal})
        return true
      end
    end
    # failure, returns false
 
  end
end

Review

There’s a lot going on here, so I’m going to try to break it down into smaller chunks.

  • acts_as_attachable.rb had a new accessor added, an array to track unsaved_attachments.
  • Attachment#attach_files had all of it’s flash tracking removed and in it’s place it will store attachments that didn’t save to the object’s unsaved_attachments.
  • In the controllers (IssuesController): instead of checking the response hash from Attachment#attach_files for a flash message, it’s checking if there is anything in object#unsaved_attachments and displaying the flash.
  • ApplicationController had a utility method #render_attachment_warning_if_needed to abstract the “display a flash” logic. This was needed because there are 5 controllers using Attachment#attach_files and I didn’t want to duplicate the code in each one.

Now I think I’ll be able to finish refactoring IssuesController#issue_update by pushing the logic down to the model.

Reference commit