Now I’m starting to refactor Redmine’s IssuesController
again in order to have it match the standard Rails REST controllers. This time I’m working on the new/create
action pair.
The Refactoring
This refactoring splits the single #new
action into the two separate actions.
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 |
# app/controllers/issues_controller.rb class IssuesController [:new, :update_form, :preview, :auto_complete] def new @issue = Issue.new @issue.copy_from(params[:copy_from]) if params[:copy_from] @issue.project = @project # Tracker must be set before custom field values @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first) if @issue.tracker.nil? render_error l(:error_no_tracker_in_project) return end if @issue.status.nil? render_error l(:error_no_default_issue_status) return end if params[:issue].is_a?(Hash) @issue.safe_attributes = params[:issue] @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project) end @issue.author = User.current if request.get? || request.xhr? @issue.start_date ||= Date.today else call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue }) if @issue.save attachments = Attachment.attach_files(@issue, params[:attachments]) render_attachment_warning_if_needed(@issue) flash[:notice] = l(:notice_successful_create) call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue}) respond_to do |format| format.html { redirect_to(params[:continue] ? { :action => 'new', :issue => {:tracker_id => @issue.tracker, :parent_issue_id => @issue.parent_issue_id}.reject {|k,v| v.nil?} } : { :action => 'show', :id => @issue }) } format.xml { render :action => 'show', :status => :created, :location => url_for(:controller => 'issues', :action => 'show', :id => @issue) } end return else respond_to do |format| format.html { } format.xml { render(:xml => @issue.errors, :status => :unprocessable_entity); return } end end end @priorities = IssuePriority.all @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true) render :layout => !request.xhr? end 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 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 63 64 65 66 67 68 69 70 71 72 73 74 75 76 |
# app/controllers/issues_controller.rb class IssuesController [:new, :create, :update_form, :preview, :auto_complete] verify :method => :post, :only => :create, :render => {:nothing => true, :status => :method_not_allowed } def new @issue = Issue.new @issue.copy_from(params[:copy_from]) if params[:copy_from] @issue.project = @project # Tracker must be set before custom field values @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first) if @issue.tracker.nil? render_error l(:error_no_tracker_in_project) return end if @issue.status.nil? render_error l(:error_no_default_issue_status) return end if params[:issue].is_a?(Hash) @issue.safe_attributes = params[:issue] @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project) end @issue.author = User.current @issue.start_date ||= Date.today @priorities = IssuePriority.all @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true) render :action => 'new', :layout => !request.xhr? end def create @issue = Issue.new @issue.copy_from(params[:copy_from]) if params[:copy_from] @issue.project = @project # Tracker must be set before custom field values @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first) if @issue.tracker.nil? render_error l(:error_no_tracker_in_project) return end if @issue.status.nil? render_error l(:error_no_default_issue_status) return end if params[:issue].is_a?(Hash) @issue.safe_attributes = params[:issue] @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project) end @issue.author = User.current @priorities = IssuePriority.all @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true) call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue }) if @issue.save attachments = Attachment.attach_files(@issue, params[:attachments]) render_attachment_warning_if_needed(@issue) flash[:notice] = l(:notice_successful_create) call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue}) respond_to do |format| format.html { redirect_to(params[:continue] ? { :action => 'new', :issue => {:tracker_id => @issue.tracker, :parent_issue_id => @issue.parent_issue_id}.reject {|k,v| v.nil?} } : { :action => 'show', :id => @issue }) } format.xml { render :action => 'show', :status => :created, :location => url_for(:controller => 'issues', :action => 'show', :id => @issue) } end return else respond_to do |format| format.html { render :action => 'new' } format.xml { render(:xml => @issue.errors, :status => :unprocessable_entity); return } end end end end |
Review
This helped #new
because it’s now not concerned with creating a new Issue
, it only builds an Issue
object for the view. Now #create
will only need to be concerned with saving the Issue
.
This refactoring did introduce a lot of duplication, which I’ll be resolving over the next few days. Something taking on more code debt, in the form of duplication, is required over the short term in order to reach your goal.