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 => @time_entry.hours, :target => 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 <= 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 && @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 => @time_entry.hours, :target => 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 <= 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:
- I renamed the method to
create_bulk_time_entry
to better describe it’s behavior. - I replaced the call to
save_time_entry_from_params
toTimeEntry.create_bulk_time_entry
. - I changed the method body to return false if the user isn’t allowed to log time to the project.
- 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.