Daily Refactor #2: Move Method in the BulkTimeEntriesController – saving a new TimeEntry

The Refactoring

Following up to yesterday’s refactoring, today I kept refactoring the Bulk Time Entry plugin’s controller. The save_time_entry_from_params method I extracted yesterday only uses the TimeEntry class, so it would be perfect to move the method to the TimeEntry class. Since the TimeEntry class is defined by Redmine, I had to monkey-patch that class. But the main content of this refactor is below:

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

  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          @time_entry = save_time_entry_from_params(entry)
          unless @time_entry.save
            page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
          else
            time_entry_target = if @time_entry.issue
                                  "#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
                                else
                                  "#{h(@time_entry.project.name)}"
                                end
            page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count =&gt; @time_entry.hours, :target =&gt; time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
          end
        end
      end
    end    
  end
 
# ...
 
  def save_time_entry_from_params(entry)
    next unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
    time_entry = TimeEntry.new(entry)
    time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours &lt;= 0
    time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
    time_entry.user = User.current
    time_entry
  end
  helper_method :save_time_entry_from_params

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
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47

# bulk_time_entries_controller.rb
  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          @time_entry = TimeEntry.create_bulk_time_entry(entry)
          unless @time_entry &amp;&amp; @time_entry.save
            page.replace "entry_#{html_id}", :partial =&gt; 'time_entry', :object =&gt; @time_entry
          else
            time_entry_target = if @time_entry.issue
                                  "#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
                                else
                                  "#{h(@time_entry.project.name)}"
                                end
            page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count =&gt; @time_entry.hours, :target =&gt; time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
          end
        end
      end
    end    
  end
 
# time_entry_patch.rb
module BulkTimeEntryPlugin
  module Patches
    module TimeEntryPatch
      def self.included(base)
        base.extend ClassMethods
      end
 
      module ClassMethods
 
        def create_bulk_time_entry(entry)
          return false unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
          time_entry = TimeEntry.new(entry)
          time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours &lt;= 0
          time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
          time_entry.user = User.current
          time_entry
        end
 
      end
 
    end
  end
end

Few things to note in this refactoring:

  1. I renamed the method to create_bulk_time_entry to better describe it’s behavior.
  2. I replaced the call to save_time_entry_from_params to TimeEntry.create_bulk_time_entry.
  3. I changed the method body to return false if the user isn’t allowed to log time to the project.
  4. I changed the controller to make sure the create_bulk_time_entry didn’t return false (user not allowed to access the project).

Review

This refactoring really cleaned up the model/controller interaction. There are still a few refactoring that can be done for the controller/view interaction now. But the TimeEntry patch could still use some work first, it’s very procedural and has it’s own code smells already.

In the reference commit I also included some unit tests for the refactoring and even happened to fix two small bugs.