Daily Refactor #27: Move Method to the Attachment Model

The Refactoring

Today I used move method to refactor part of Redmine that had a TODO comment since 2007.

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

# app/controllers/application_controller.rb
class ApplicationController  0
        a = Attachment.create(:container => obj, 
                              :file => file,
                              :description => attachment['description'].to_s.strip,
                              :author => User.current)
        a.new_record? ? (unsaved << a) : (attached << a)
      end
      if unsaved.any?
        flash[:warning] = l(:warning_attachments_not_saved, unsaved.size)
      end
    end
    attached
  end
end
 
1
2
3
4

# app/models/attachment.rb
class Attachment < ActiveRecord::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

# app/controllers/issues_controller.rb
class IssuesController  @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
      @time_entry.attributes = params[:time_entry]
      @issue.time_entries << @time_entry
    end
 
    if @issue.valid?
      attachments = attach_files(@issue, params[:attachments])
      attachments.each {|a| @journal.details < '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
        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 => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
        return true
      end
    end
    # failure, returns false
 
  end
end

After

1
2
3
4

# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
  # ...
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
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

Review

I’m not completely happy with the results of this refactoring. The previous ApplicationController#attach_files accessed the flash directly so I had to do some tricks to keep the flash messages in the model. This is why the callers have to check attachments[:flash] themselves. It would be ideal if the model could handle this directly so no one forgets to check the flash value, but that will have to wait until later.

Reference commit