Daily Refactor #30: Move Method into Issue

The Refactoring

Today is the day that I finally get to finish the set of refactorings I started back on the 24th. My goal was to start converting the IssuesController into a skinny controller by moving code into the models. Today, I finally was able to cleanly move #issue_update from the controller into the Issue model.

Before

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
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62

# app/controllers/issues_controller.rb
  def edit
    update_issue_from_params
 
    @journal = @issue.current_journal
 
    respond_to do |format|
      format.html { }
      format.xml  { }
    end
  end
 
  def update
    update_issue_from_params
 
    if issue_update
      respond_to do |format|
        format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
        format.xml  { head :ok }
      end
    else
      @journal = @issue.current_journal
      respond_to do |format|
        format.html { render :action => 'edit' }
        format.xml  { render :xml => @issue.errors, :status => :unprocessable_entity }
      end
    end
 
  rescue ActiveRecord::StaleObjectError
    # Optimistic locking exception
    flash.now[:error] = l(:notice_locking_conflict)
    # Remove the previously added attachments if issue was not updated
    attachments[:files].each(&:destroy) if attachments[:files]
  end
 
  def issue_update
    if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project)
      @time_entry = TimeEntry.new(:project => @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 = Attachment.attach_files(@issue, params[:attachments])
      render_attachment_warning_if_needed(@issue)
 
      attachments[:files].each {|a| @issue.current_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 => @issue.current_journal})
      if @issue.save
        if !@issue.current_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 => @issue.current_journal})
        return true
      end
    end
    # failure, returns false
 
  end
 
end
 
1
2
3
4

# app/models/issue.rb
class Issue < ActiveRecord::Base
  # ...
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

# app/controllers/issues_controller.rb
class IssuesController  'show', :id => @issue}) }
        format.xml  { head :ok }
      end
    else
      render_attachment_warning_if_needed(@issue)
      if !@issue.current_journal.new_record?
        # Only send notification if something was actually changed
        flash[:notice] = l(:notice_successful_update)
      end
      @journal = @issue.current_journal
 
      respond_to do |format|
        format.html { render :action => 'edit' }
        format.xml  { render :xml => @issue.errors, :status => :unprocessable_entity }
      end
    end
 
  rescue ActiveRecord::StaleObjectError
    # Optimistic locking exception
    flash.now[:error] = l(:notice_locking_conflict)
    # Remove the previously added attachments if issue was not updated
    attachments[:files].each(&:destroy) if attachments[:files]
  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
26
27
28
29
30

# app/models/issue.rb
class Issue < ActiveRecord::Base
  # Saves an issue, time_entry, attachments, and a journal from the parameters
  def save_issue_with_child_records(params, existing_time_entry=nil)
    if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, project)
      @time_entry = existing_time_entry || TimeEntry.new
      @time_entry.project = project
      @time_entry.issue = self
      @time_entry.user = User.current
      @time_entry.spent_on = Date.today
      @time_entry.attributes = params[:time_entry]
      self.time_entries << @time_entry
    end
 
    if valid?
      attachments = Attachment.attach_files(self, params[:attachments])
 
      attachments[:files].each {|a| @current_journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
      # TODO: Rename hook
      Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
      if save
        # TODO: Rename hook
        Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
        return true
      end
    end
    # failure, returns false
 
  end
end

Review

In order to complete this move I had move some of the code for setting the flash messages into the action, but that’s fine by me. The resulting code is a bit bigger than when it was in the controller, but I feel that it’s in a place where it can be better used now.

The other pending problem is that with this move, the plugin hooks are now misnamed (Redmine::Hook.call_hook). I know there are plugins using them so I don’t want to just rename them and break the code. I’m thinking about adding a new Hook method for deprecated hooks that will let them continue to function but warn the plugin developers that they will be removed later. Has anyone else working with deprecating an API? How did you do it?

Reference commit