written by Eric Davis on February 26, 2010
0 Comments
The Refactoring
This refactoring is what I call the magic trick refactor; it looks like a lot is going on but in reality it's just smoke and mirrors!
Before
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def update
update_issue_from_params
if request.get?
# nop
else
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| @journal.details << JournalDetail.new(:property => '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
# Log spend time
if User.current.allowed_to?(:log_time, @project)
@time_entry.save
end
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})
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
return
end
end
# failure
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.each(&:destroy)
end
end
end
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
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
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.each(&:destroy)
end
# ...
# TODO: Temporary utility method for #update. Should be split off
# and moved to the Issue model (accepts_nested_attributes_for maybe?)
# TODO: move attach_files to the model so this can be moved to the
# model also
def issue_update
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| @journal.details << JournalDetail.new(:property => '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
# Log spend time
if User.current.allowed_to?(:log_time, @project)
@time_entry.save
end
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
Review
Truthfully, this refactor isn't just smoke and mirrors. The behavior stayed the same but now #update is easier to read and is working at a higher level of abstraction (e.g. only handling the response). The #issue_update method is still pretty smelly but it's just about ripe for some refactoring.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on February 25, 2010
0 Comments
I tackled two refactorings today in order to clean up IssuesController#edit.
The Refactoring - #1
This first refactoring moves most of the #edit method's body into #update, where it belongs. This did cause a some duplication though.
Before
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def edit
@allowed_statuses = @issue.new_statuses_allowed_to(User.current)
@priorities = IssuePriority.all
@edit_allowed = User.current.allowed_to?(:edit_issues, @project)
@time_entry = TimeEntry.new
@notes = params[:notes]
journal = @issue.init_journal(User.current, @notes)
# User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
attrs = params[:issue].dup
attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
@issue.safe_attributes = attrs
end
if request.get?
# nop
else
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| journal.details << JournalDetail.new(:property => '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
# Log spend time
if User.current.allowed_to?(:log_time, @project)
@time_entry.save
end
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})
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
return
end
end
# failure
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.each(&:destroy)
end
def update
edit
end
end
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def edit
@allowed_statuses = @issue.new_statuses_allowed_to(User.current)
@priorities = IssuePriority.all
@edit_allowed = User.current.allowed_to?(:edit_issues, @project)
@time_entry = TimeEntry.new
@notes = params[:notes]
journal = @issue.init_journal(User.current, @notes)
# User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
attrs = params[:issue].dup
attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
@issue.safe_attributes = attrs
end
respond_to do |format|
format.html { }
format.xml { }
end
end
def update
@allowed_statuses = @issue.new_statuses_allowed_to(User.current)
@priorities = IssuePriority.all
@edit_allowed = User.current.allowed_to?(:edit_issues, @project)
@time_entry = TimeEntry.new
@notes = params[:notes]
journal = @issue.init_journal(User.current, @notes)
# User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
attrs = params[:issue].dup
attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
@issue.safe_attributes = attrs
end
if request.get?
# nop
else
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| journal.details << JournalDetail.new(:property => '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
# Log spend time
if User.current.allowed_to?(:log_time, @project)
@time_entry.save
end
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})
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
return
end
end
# failure
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.each(&:destroy)
end
end
The Refactoring - #2
This refactoring used extract method to pull the duplication out of #edit and #update into a utility method.
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def edit
update_issue_from_params
respond_to do |format|
format.html { }
format.xml { }
end
end
def update
update_issue_from_params
if request.get?
# nop
else
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
# ... saving code
end
# failure
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.each(&:destroy)
end
#...
def update_issue_from_params
@allowed_statuses = @issue.new_statuses_allowed_to(User.current)
@priorities = IssuePriority.all
@edit_allowed = User.current.allowed_to?(:edit_issues, @project)
@time_entry = TimeEntry.new
@notes = params[:notes]
@journal = @issue.init_journal(User.current, @notes)
# User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
attrs = params[:issue].dup
attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
@issue.safe_attributes = attrs
end
end
end
Review
Now that IssuesController is following the REST design for it's edit and update actions, the next refactorings on them will be to clean up their internal implementation. #edit looks really well factored already, but #update can use some more, especially inside of it's main if conditional. And #update_issue_from_params is doing quite a bit of work, especially for the read-only #edit action.
Reference commits
Tagged: ruby redmine refactoring extract-method-body
written by Eric Davis on February 25, 2010
0 Comments
The 0.5.0 release of the Redmine Bulk Time Entry plugin has just been uploaded to my Redmine, Github, and gemcutter. This is a bug fix release for Redmine 0.9 only.
Changes
Help
If you need help, my Redmine bug tracker is open to the public and you are welcome to ask for help there.
Eric
Tagged: open source redmine redmine plugins
written by Eric Davis on February 24, 2010
0 Comments
After yesterday's refactor of the #bulk_edit method, Jean-Philippe Lang finished the refactoring by merging the temporary method I created in the model (Issue#bulk_edit) and simplifying the parameter attributes.
Back to the drawing board, it looks like the method with the next highest flog score is #edit. #edit is a pretty complex action in Redmine, since it serves as both rendering the edit form and saving the edits of issues and time entries. It flows like this:
def edit
if allowed to change issue
set issue attribues
end
if GET request
render form
else
if time entry is valid
attach file uploads
if issue saved
save time entry
redirect to issue
end
end
rerender form
end
rescue stale object updates
render form
end
end
Just from that psuedocode, it's easy to see how many different responsibilities that action has. To follow the standard REST design it should be two actions; one for rendering the form (#edit) and one for processing the update (#update).
The Refactoring
This refactoring is the first step towards decouping #edit into two actions.
Before
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def edit
@allowed_statuses = @issue.new_statuses_allowed_to(User.current)
@priorities = IssuePriority.all
@edit_allowed = User.current.allowed_to?(:edit_issues, @project)
@time_entry = TimeEntry.new
@notes = params[:notes]
journal = @issue.init_journal(User.current, @notes)
# User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
attrs = params[:issue].dup
attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
@issue.safe_attributes = attrs
end
if request.get?
# nop
else
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| journal.details << JournalDetail.new(:property => '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
# Log spend time
if User.current.allowed_to?(:log_time, @project)
@time_entry.save
end
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})
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
return
end
end
# failure
respond_to do |format|
format.html { }
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.each(&:destroy)
end
end
# app/views/issues/_edit.rhtml
<% labelled_tabular_form_for :issue, @issue,
:url => {:action => 'edit', :id => @issue},
:html => {:id => 'issue-form',
:class => nil,
:multipart => true} do |f| %>
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
def edit
@allowed_statuses = @issue.new_statuses_allowed_to(User.current)
@priorities = IssuePriority.all
@edit_allowed = User.current.allowed_to?(:edit_issues, @project)
@time_entry = TimeEntry.new
@notes = params[:notes]
journal = @issue.init_journal(User.current, @notes)
# User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
attrs = params[:issue].dup
attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
@issue.safe_attributes = attrs
end
if request.get?
# nop
else
@time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| journal.details << JournalDetail.new(:property => '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
# Log spend time
if User.current.allowed_to?(:log_time, @project)
@time_entry.save
end
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})
respond_to do |format|
format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
format.xml { head :ok }
end
return
end
end
# failure
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.each(&:destroy)
end
#--
# Start converting to the Rails REST controllers
#++
def update
edit
end
end
# app/views/issues/_edit.rhtml
<% labelled_tabular_form_for :issue, @issue,
:url => {:action => update, :id => @issue},
:html => {:id => 'issue-form',
:class => nil,
:method => :put,
:multipart => true} do |f| %>
Review
Instead of trying to dig into #edit and refactor everything at once, I put a little shim in place so #edit was mostly unmodified. Instead, I modified the issue forms so they are already using the new #update method. This will make it easier to move code over piece by piece. It will take a few refactorings but with the good test suite included in Redmine, it shouldn't be difficult.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on February 23, 2010
0 Comments
I started to refactor the IssuesController today. Like I said yesterday, this class is full of bad smells and could really use some major rework. Picking a good place to start is overwhelming so I turned to a useful tool called flog. Flog is a tool that I use to find out how complex a class or method is. It checks for assignments (abc = 42), branches (if), and calls (@foo.bar) and gives each method a total score.
Using the rough numbers from flog, I can see which methods in IssuesController need the most work:
flog app/controllers/issues_controller.rb
1423.2: flog total
67.8: flog/method average
194.9: IssuesController#bulk_edit
145.6: IssuesController#edit
136.4: IssuesController#retrieve_query
131.6: IssuesController#move
127.3: IssuesController#new
98.3: IssuesController#index
84.8: IssuesController#context_menu
The #bulk_edit method looks the worst according to flog so that's a good place to start.
The Refactoring
The easiest way to reduce the flog score for one method is to move a similar chunk of code to a new method. This won't affect the overall score since the complexity is just moved to the new method, but it can make it easier to refactor and reuse the code in other methods. For this refactoring, I used both extract method and move method to move a chunk of code from #bulk_edit to the Issue objects themselves.
Before
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
# Bulk edit a set of issues
def bulk_edit
if request.post?
tracker = params[:tracker_id].blank? ? nil : @project.trackers.find_by_id(params[:tracker_id])
status = params[:status_id].blank? ? nil : IssueStatus.find_by_id(params[:status_id])
priority = params[:priority_id].blank? ? nil : IssuePriority.find_by_id(params[:priority_id])
assigned_to = (params[:assigned_to_id].blank? || params[:assigned_to_id] == 'none') ? nil : User.find_by_id(params[:assigned_to_id])
category = (params[:category_id].blank? || params[:category_id] == 'none') ? nil : @project.issue_categories.find_by_id(params[:category_id])
fixed_version = (params[:fixed_version_id].blank? || params[:fixed_version_id] == 'none') ? nil : @project.shared_versions.find_by_id(params[:fixed_version_id])
custom_field_values = params[:custom_field_values] ? params[:custom_field_values].reject {|k,v| v.blank?} : nil
unsaved_issue_ids = []
@issues.each do |issue|
journal = issue.init_journal(User.current, params[:notes])
issue.tracker = tracker if tracker
issue.priority = priority if priority
issue.assigned_to = assigned_to if assigned_to || params[:assigned_to_id] == 'none'
issue.category = category if category || params[:category_id] == 'none'
issue.fixed_version = fixed_version if fixed_version || params[:fixed_version_id] == 'none'
issue.start_date = params[:start_date] unless params[:start_date].blank?
issue.due_date = params[:due_date] unless params[:due_date].blank?
issue.done_ratio = params[:done_ratio] unless params[:done_ratio].blank?
issue.custom_field_values = custom_field_values if custom_field_values && !custom_field_values.empty?
call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
# Don't save any change to the issue if the user is not authorized to apply the requested status
unless (status.nil? || (issue.new_statuses_allowed_to(User.current).include?(status) && issue.status = status)) && issue.save
# Keep unsaved issue ids to display them in flash error
unsaved_issue_ids << issue.id
end
end
if unsaved_issue_ids.empty?
flash[:notice] = l(:notice_successful_update) unless @issues.empty?
else
flash[:error] = l(:notice_failed_to_save_issues, :count => unsaved_issue_ids.size,
:total => @issues.size,
:ids => '#' + unsaved_issue_ids.join(', #'))
end
redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
return
end
@available_statuses = Workflow.available_statuses(@project)
@custom_fields = @project.all_issue_custom_fields
end
#...
end
# app/models/issue.rb
class Issue < ActiveRecord::Base
#...
end
After
# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
# Bulk edit a set of issues
def bulk_edit
if request.post?
tracker = params[:tracker_id].blank? ? nil : @project.trackers.find_by_id(params[:tracker_id])
status = params[:status_id].blank? ? nil : IssueStatus.find_by_id(params[:status_id])
priority = params[:priority_id].blank? ? nil : IssuePriority.find_by_id(params[:priority_id])
assigned_to = (params[:assigned_to_id].blank? || params[:assigned_to_id] == 'none') ? nil : User.find_by_id(params[:assigned_to_id])
category = (params[:category_id].blank? || params[:category_id] == 'none') ? nil : @project.issue_categories.find_by_id(params[:category_id])
fixed_version = (params[:fixed_version_id].blank? || params[:fixed_version_id] == 'none') ? nil : @project.shared_versions.find_by_id(params[:fixed_version_id])
custom_field_values = params[:custom_field_values] ? params[:custom_field_values].reject {|k,v| v.blank?} : nil
# Need to merge in the records found above for Issue#bulk_edit.
# Assuming this is done so the associations are only looked up once.
merged_params = params.merge({
:tracker => tracker,
:status => status,
:priority => priority,
:assigned_to => assigned_to,
:category => category,
:fixed_version => fixed_version,
:custom_field_values => custom_field_values
})
unsaved_issue_ids = []
@issues.each do |issue|
unless issue.bulk_edit(merged_params)
# Keep unsaved issue ids to display them in flash error
unsaved_issue_ids << issue.id
end
end
if unsaved_issue_ids.empty?
flash[:notice] = l(:notice_successful_update) unless @issues.empty?
else
flash[:error] = l(:notice_failed_to_save_issues, :count => unsaved_issue_ids.size,
:total => @issues.size,
:ids => '#' + unsaved_issue_ids.join(', #'))
end
redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
return
end
@available_statuses = Workflow.available_statuses(@project)
@custom_fields = @project.all_issue_custom_fields
end
# ...
end
# app/models/issue.rb
class Issue < ActiveRecord::Base
def bulk_edit(params)
journal = init_journal(User.current, params[:notes])
self.tracker = params[:tracker] if params[:tracker]
self.priority = params[:priority] if params[:priority]
self.assigned_to = params[:assigned_to] if params[:assigned_to] || params[:assigned_to_id] == 'none'
self.category = params[:category] if params[:category] || params[:category_id] == 'none'
self.fixed_version = params[:fixed_version] if params[:fixed_version] || params[:fixed_version_id] == 'none'
self.start_date = params[:start_date] unless params[:start_date].blank?
self.due_date = params[:due_date] unless params[:due_date].blank?
self.done_ratio = params[:done_ratio] unless params[:done_ratio].blank?
self.custom_field_values = params[:custom_field_values] if params[:custom_field_values] && !params[:custom_field_values].empty?
# TODO: Edit hook name
Redmine::Hook.call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => self })
# Don't save any change to the issue if the user is not authorized to apply the requested status
return (params[:status].nil? || (new_statuses_allowed_to(User.current).include?(params[:status]) && self.status = params[:status])) && save
end
#...
end
Review
I had to put a small shim into #bulk_edit in order for the extraction to work without changing how the code works. This is shown by the merged_params variable. Like I've said previously, an inline comment is an easy way to spot a new refactoring. This time, I added a comment to myself to remind me later.
Now that the #bulk_edit method has been moved to the Issue class, the flog score for IssuesController is looking a lot better now.
flog app/controllers/issues_controller.rb
1354.9: flog total
64.5: flog/method average
145.6: IssuesController#edit
136.4: IssuesController#retrieve_query
131.6: IssuesController#move
127.3: IssuesController#new
126.6: IssuesController#bulk_edit
98.3: IssuesController#index
84.8: IssuesController#context_menu
If I run flog on the Issue model, you can still see the 60 points. This is because the complexity was just moved around, not actually removed.
flog app/models/issue.rb
788.5: flog total
15.8: flog/method average
...
61.1: Issue#bulk_edit
...
Now there are a few things I can refactor in the IssuesController and Issue classes. I think I'll stick with the controller for now, since it's in worse condition.
Reference commit
Tagged: ruby redmine refactoring move-method extract-method
written by Eric Davis on February 22, 2010
0 Comments
I tried to start this week's refactorings with some major changes to one of the worst areas of Redmine, the IssuesController. Weighing in at over 576 lines and 20 actions, it is full of really bad smells and hasn't getting any better over time. But after spending two hours on it, I was getting nowhere and decided to start over. So instead of going for the big impact changes, I'm going to tackle the little changes one by one.
The Refactoring
Today's refactoring involves moving some of the IssuesController's routing tests from the functional tests into the integration tests.
Before
# test/functional/issues_controller_test.rb
class IssuesControllerTest < ActionController::TestCase
def test_index_routing
assert_routing(
{:method => :get, :path => '/issues'},
:controller => 'issues', :action => 'index'
)
end
# ...
def test_index_with_project_routing
assert_routing(
{:method => :get, :path => '/projects/23/issues'},
:controller => 'issues', :action => 'index', :project_id => '23'
)
end
# ...
def test_index_with_project_routing
assert_routing(
{:method => :get, :path => 'projects/23/issues'},
:controller => 'issues', :action => 'index', :project_id => '23'
)
end
# ...
def test_index_with_project_routing_formatted
assert_routing(
{:method => :get, :path => 'projects/23/issues.pdf'},
:controller => 'issues', :action => 'index', :project_id => '23', :format => 'pdf'
)
assert_routing(
{:method => :get, :path => 'projects/23/issues.atom'},
:controller => 'issues', :action => 'index', :project_id => '23', :format => 'atom'
)
end
# ...
def test_index_formatted
assert_routing(
{:method => :get, :path => 'issues.pdf'},
:controller => 'issues', :action => 'index', :format => 'pdf'
)
assert_routing(
{:method => :get, :path => 'issues.atom'},
:controller => 'issues', :action => 'index', :format => 'atom'
)
end
# ...
def test_show_routing
assert_routing(
{:method => :get, :path => '/issues/64'},
:controller => 'issues', :action => 'show', :id => '64'
)
end
def test_show_routing_formatted
assert_routing(
{:method => :get, :path => '/issues/2332.pdf'},
:controller => 'issues', :action => 'show', :id => '2332', :format => 'pdf'
)
assert_routing(
{:method => :get, :path => '/issues/23123.atom'},
:controller => 'issues', :action => 'show', :id => '23123', :format => 'atom'
)
end
# ...
def test_new_routing
assert_routing(
{:method => :get, :path => '/projects/1/issues/new'},
:controller => 'issues', :action => 'new', :project_id => '1'
)
assert_recognizes(
{:controller => 'issues', :action => 'new', :project_id => '1'},
{:method => :post, :path => '/projects/1/issues'}
)
end
# ...
def test_copy_routing
assert_routing(
{:method => :get, :path => '/projects/world_domination/issues/567/copy'},
:controller => 'issues', :action => 'new', :project_id => 'world_domination', :copy_from => '567'
)
end
# ...
def test_edit_routing
assert_routing(
{:method => :get, :path => '/issues/1/edit'},
:controller => 'issues', :action => 'edit', :id => '1'
)
assert_recognizes( #TODO: use a PUT on the issue URI isntead, need to adjust form
{:controller => 'issues', :action => 'edit', :id => '1'},
{:method => :post, :path => '/issues/1/edit'}
)
end
# ...
def test_reply_routing
assert_routing(
{:method => :post, :path => '/issues/1/quoted'},
:controller => 'issues', :action => 'reply', :id => '1'
)
end
# ...
def test_move_routing
assert_routing(
{:method => :get, :path => '/issues/1/move'},
:controller => 'issues', :action => 'move', :id => '1'
)
assert_recognizes(
{:controller => 'issues', :action => 'move', :id => '1'},
{:method => :post, :path => '/issues/1/move'}
)
end
# ...
def test_destroy_routing
assert_recognizes( #TODO: use DELETE on issue URI (need to change forms)
{:controller => 'issues', :action => 'destroy', :id => '1'},
{:method => :post, :path => '/issues/1/destroy'}
)
end
end
# test/integration/routing_test.rb
class RoutingTest < ActionController::IntegrationTest
end
After
# test/functional/issues_controller_test.rb
class IssuesControllerTest < ActionController::TestCase
# ...
end
# test/integration/routing_test.rb
class RoutingTest < ActionController::IntegrationTest
context "issues" do
# REST actions
should_route :get, "/issues", :controller => 'issues', :action => 'index'
should_route :get, "/issues.pdf", :controller => 'issues', :action => 'index', :format => 'pdf'
should_route :get, "/issues.atom", :controller => 'issues', :action => 'index', :format => 'atom'
should_route :get, "/issues.xml", :controller => 'issues', :action => 'index', :format => 'xml'
should_route :get, "/projects/23/issues", :controller => 'issues', :action => 'index', :project_id => '23'
should_route :get, "/projects/23/issues.pdf", :controller => 'issues', :action => 'index', :project_id => '23', :format => 'pdf'
should_route :get, "/projects/23/issues.atom", :controller => 'issues', :action => 'index', :project_id => '23', :format => 'atom'
should_route :get, "/projects/23/issues.xml", :controller => 'issues', :action => 'index', :project_id => '23', :format => 'xml'
should_route :get, "/issues/64", :controller => 'issues', :action => 'show', :id => '64'
should_route :get, "/issues/64.pdf", :controller => 'issues', :action => 'show', :id => '64', :format => 'pdf'
should_route :get, "/issues/64.atom", :controller => 'issues', :action => 'show', :id => '64', :format => 'atom'
should_route :get, "/issues/64.xml", :controller => 'issues', :action => 'show', :id => '64', :format => 'xml'
should_route :get, "/projects/23/issues/new", :controller => 'issues', :action => 'new', :project_id => '23'
should_route :get, "/issues/64/edit", :controller => 'issues', :action => 'edit', :id => '64'
# TODO: Should use PUT
should_route :post, "/issues/64/edit", :controller => 'issues', :action => 'edit', :id => '64'
# TODO: Should use DELETE
should_route :post, "/issues/64/destroy", :controller => 'issues', :action => 'destroy', :id => '64'
# Extra actions
should_route :get, "/projects/23/issues/64/copy", :controller => 'issues', :action => 'new', :project_id => '23', :copy_from => '64'
should_route :get, "/issues/1/move", :controller => 'issues', :action => 'move', :id => '1'
should_route :post, "/issues/1/move", :controller => 'issues', :action => 'move', :id => '1'
should_route :post, "/issues/1/quoted", :controller => 'issues', :action => 'reply', :id => '1'
should_route :get, "/issues/calendar", :controller => 'issues', :action => 'calendar'
should_route :post, "/issues/calendar", :controller => 'issues', :action => 'calendar'
should_route :get, "/projects/project-name/issues/calendar", :controller => 'issues', :action => 'calendar', :project_id => 'project-name'
should_route :post, "/projects/project-name/issues/calendar", :controller => 'issues', :action => 'calendar', :project_id => 'project-name'
should_route :get, "/issues/gantt", :controller => 'issues', :action => 'gantt'
should_route :post, "/issues/gantt", :controller => 'issues', :action => 'gantt'
should_route :get, "/projects/project-name/issues/gantt", :controller => 'issues', :action => 'gantt', :project_id => 'project-name'
should_route :post, "/projects/project-name/issues/gantt", :controller => 'issues', :action => 'gantt', :project_id => 'project-name'
end
end
Review
It's not really apparent here, but if you look at the diff for this refactoring you can see that all of the routing tests were interspaced in the functional test. This made it extremely difficult to find how the routes should behave for the IssuesController, short of reading the routes.rb. Now with the integration test using shoulda's should_route it's really easy to see how the HTTP verbs and urls map to the controllers and actions.
Reference commit
Tagged: ruby redmine refactoring testing move-method
written by Eric Davis on February 20, 2010
0 Comments
Since I'm working so much with Redmine and Open Source, I wanted to take a few minutes and post an update about some of the major things I've been working on recently.
Redmine LDAP integration
I've been working with a client on several new features for Redmine's LDAP integration. He's wanting to add a lot more automation for the user management in Redmine so we've been doing a lot of integration with LDAP over the past two weeks.
We're planning to submit these new features back to Redmine in a couple of weeks but I've already started on a new plugin that has some of the automation:
- Rake Task: Add all of the users found in LDAP to Redmine, optionally adding them to a Redmine group.
- Rake Task: Lock (or unlock) any account registered in Redmine with an LDAP connection but not found. (e.g. if a user was removed from LDAP, this will lock their account in Redmine)
- A web user interface to "Sync LDAP" which will run both of the above commands
- Rake Task: Add the existing Redmine users to a default group (e.g. Everyone should be in the Reporter group)
Some of the Redmine core features we want to submit are:
- Automatically adding new users to specific Redmine groups based on their LDAP connection.
- Extending the incoming mail handler to check for accounts in LDAP and create them on the fly.
- Adding a custom filter for each LDAP connection to refine the user search (e.g. getting all the members of the "Developers" group out of LDAP:
(&(objectcategory=group)(member="Developers"))
Redmine Bulk Time Entry plugin
I'm just about done with a new release for the Redmine Bulk Time Entry plugin. I've been frantically trying to get all 70+ of my Redmine plugins updated for the 0.9 Redmine release last month. I'm going to be asking for some help to maintain them once I get my first big project of 2010 launched...
See Project Run
I've been working hard to get the first of my 12 businesses in 2010 launched, SeeProjectRun. I'm not quite ready to open it up for sign ups yet but I've added a form where you can register for a private beta invite. If you're a freelancer or small technology business, I'll appreciate it if you could sign up for the beta.
That's what I've been working on lately. It's stressful and overwhelming at times but a lot of good things are coming out from it.
Eric Davis
Tagged: redmine ldap business seeprojectrun
written by Eric Davis on February 19, 2010
0 Comments
Right after publishing the final refactoring for AuthSourceLdap, another really good one jumped out at me and I couldn't wait until next week to fix it.
The Refactoring
Before
# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = []
# get user's DN
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
dn = String.new
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
:attributes=> search_attributes) do |entry|
dn = entry.dn
attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
end
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
end
After
# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = get_user_dn(login)
if attrs.first && attrs.first[:dn] && authenticate_dn(attrs.first[:dn], password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Get the user's dn and any attributes for them, given their login
def get_user_dn(login)
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
attrs = []
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
:attributes=> search_attributes) do |entry|
if onthefly_register?
attrs = get_user_attributes_from_ldap_entry(entry)
else
attrs = [:dn => entry.dn]
end
logger.debug "DN found for #{login}: #{attrs.first[:dn]}" if logger && logger.debug?
end
attrs
end
end
Review
This refactor moves a massive block of code out of AuthSourceLdap#authenticate. Now #authenticate works at a higher level of abstraction and relies on #get_user_dn and #authenticate_dn to handle the actual LDAP queries.
The only reason I noticed this reafactoring was with the single inline comment remaining in #authenticate. Whenever I see an inline comment, I try to stop and see if that code can be isolated and moved to another method.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on February 19, 2010
0 Comments
The Refactoring
I mentioned yesterday that I wanted to refactor the ternary operation in the ldap_con#search call so today I used extract method to pull it out.
Before
# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = []
# get user's DN
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
dn = String.new
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
# only ask for the DN if on-the-fly registration is disabled
:attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
dn = entry.dn
attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
end
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
end
After
# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = []
# get user's DN
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
dn = String.new
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
:attributes=> search_attributes) do |entry|
dn = entry.dn
attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
end
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Return the attributes needed for the LDAP search. It will only
# include the user attributes if on-the-fly registration is enabled
def search_attributes
if onthefly_register?
['dn', self.attr_firstname, self.attr_lastname, self.attr_mail]
else
['dn']
end
end
end
Review
With this refactor, I'm ready to conclude working on AuthSourceLdap for now. With it cleaned up and well factored, it is now easier to implement new features that I have in progress. Like in cooking, it's easier to create good things when the kitchen is clean.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on February 18, 2010
0 Comments
With the help of Nate Sutton I've done two small refactorings to AuthSourceLdap#authenticate and #authenticate_dn.
The Refactoring - One
This refactoring moved the check for an empty DN into the #authenticate_dn method. It makes sense to have all of the business logic and data validations for authenticating in one method.
Before
# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = []
# get user's DN
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
dn = String.new
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
# only ask for the DN if on-the-fly registration is disabled
:attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
dn = entry.dn
attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?
end
return nil if dn.empty?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
else
return nil
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Check if a DN (user record) authenticates with the password
def authenticate_dn(dn, password)
ldap_con = initialize_ldap_con(dn, password)
return ldap_con.bind
end
end
After
# app/models/auth_source_ldap.rb
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = []
# get user's DN
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
dn = String.new
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
# only ask for the DN if on-the-fly registration is disabled
:attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
dn = entry.dn
attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
end
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
else
return nil
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Check if a DN (user record) authenticates with the password
def authenticate_dn(dn, password)
return nil if dn.empty?
ldap_con = initialize_ldap_con(dn, password)
return ldap_con.bind
end
end
The Refactoring - Two
This refactoring cleans up the internals of authenticate_dn by using Ruby's ability to implicitly return the last value in a method.
Before
# app/models/auth_source_ldap.rb
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = []
# get user's DN
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
dn = String.new
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
# only ask for the DN if on-the-fly registration is disabled
:attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
dn = entry.dn
attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
end
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
else
return nil
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Check if a DN (user record) authenticates with the password
def authenticate_dn(dn, password)
return nil if dn.empty?
ldap_con = initialize_ldap_con(dn, password)
return ldap_con.bind
end
end
After
# app/models/auth_source_ldap.rb
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = []
# get user's DN
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
dn = String.new
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
# only ask for the DN if on-the-fly registration is disabled
:attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
dn = entry.dn
attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
end
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Check if a DN (user record) authenticates with the password
def authenticate_dn(dn, password)
if dn.present? && password.present?
initialize_ldap_con(dn, password).bind
end
end
end
Review
With these two changes, what #authenticate is doing has become a lot clearer for each the different return paths (only 3 now). I think there is only one more refactoring I want to make to it before I move on; the ternary operation in the #search call. Once that's refactored, I'll be able to easily port over a new feature from one of my Redmine clients.
Reference commits
Tagged: ruby redmine refactoring
