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
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
# 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_entryto better describe it's behavior. - I replaced the call to
save_time_entry_from_paramstoTimeEntry.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_entrydidn'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.
