Trailblazer tutorial: updating old fat controller - part 4.

Since we prepared and tested Operation and Contract to create Proposal with Trailblazer way, it is time to clean the mess in the controller. Let's remind how our controller #create action looks like right now:

def create  
  if @event.closed? && @event.closes_at < 1.hour.ago  
    redirect_to event_path (@event)  
    flash[:danger] = "The CFP is closed for proposal submissions."  
  return  
 end  @proposal = @event.proposals.new(proposal_params)  
  speaker = @proposal.speakers[0]  
  speaker.user_id = current_user.id  
  speaker.event_id = @event.id  

  if @proposal.save  
    current_user.update_bio  
    flash[:info] = setup_flash_message  
    redirect_to event_proposal_url(event_slug: @event.slug, uuid: @proposal)  
  else  
  flash[:danger] = "There was a problem saving your proposal."  
  render :new  
  end  
end

Since we have all business logic moved to Operation, and all context-validation rules applied into the controller let's use it. But before, since we are refactoring our controller, let us improve test-coverage for all cases in this action, so we will be able to sleep well after deploying refactored code to production:

# spec/controllers/proposals_controller_spec.rb
require 'rails_helper'  

describe ProposalsController, type: :controller do  
  let(:event) { FactoryBot.create(:event, state: 'open') }
  # ...
  describe 'POST #create' do  
    let(:proposal) { build(:proposal, uuid: 'abc123') }  
    let(:user) { create(:user) }  
    let(:params) {  
      {  
        event_slug: event.slug,  
        proposal: {  
          title: proposal.title,  
          abstract: proposal.abstract,  
          details: proposal.details,  
          pitch: proposal.pitch,  
          session_format_id: proposal.session_format.id,  
          speakers: [  
            {  
              bio: 'my bio'  
            }  
          ]  
        }  
      }  
    }  

    before { allow(controller).to receive(:current_user).and_return(user) }  

    it "create Proposal" do  
      expect{  
        post :create, params: params  
      }.to change{Proposal.count}.by(1)  
    end

    it "sets the user's bio if not is present" do  
      user.bio = nil  
      post :create, params: params  
      expect(user.bio).to eq('my bio')  
    end  

    context "event closed" do  
      let!(:event) { FactoryBot.create(:event, state: "closed") }  
      it "redirects to event show page with 'The CFP is closed for proposal submissions.'  message" do  
        post :create, params: params  
        expect(response).to redirect_to(event_path(event))  
        expect(flash[:danger]).to match(/The CFP is closed for proposal submissions.*/)  
      end
    end  

    context "wrong slug passed" do  
      let(:wrong_params) { params.merge(event_slug: 'Non-existing-slug')}  
        it "re-render new form with 'Your event could not be found, please check the url.' message" do  
        post :create, params: wrong_params  
        expect(response).to redirect_to(events_path)  
        expect(flash[:danger]).to match(/Your event could not be found, please check the url.*/)  
      end  
    end
  end
end

Before we will start refactoring controller code, let's take a look for what as Rails developers we love so much:

class ProposalsController < ApplicationController  
  before_action :require_event, except: :index  
  before_action :require_user  
  before_action :require_proposal, except: [ :index, :create, :new, :parse_edit_field ]  
  before_action :require_invite_or_speaker, only: [:show]  
  before_action :require_speaker, except: [ :index, :create, :new, :parse_edit_field ]
  #a lot of actions

  private  
  def proposal_params  
    params.require(:proposal).permit(:title, {tags: []}, :session_format_id,
      :track_id, :abstract, :details, :pitch, custom_fields: @event.custom_fields,  
      comments_attributes: [:body, :proposal_id, :user_id],  
      speakers_attributes: [:bio, :id])  
  end
end

That is not a tutorial which goal is to convince you that having a lot of before_action callback will bite you sooner than later. I also don't want to try to list all OOP rules that this approach breaks. I just strongly believe that you understand why we will try to avoid using callbacks, and if you don't - then you can google it :) Meantime we will focus on our step-by-step tutorial.

So what we are planning to do with our controller is:

replace require_event as a callback into subprocess called from Operations that needs event (what we already have in our Operation),
change setup_flash_message to expect receive event argument, instead of using instance variable inside,
stop using strong params since contract using reform are taking care of it better than whole controller ( which doesn't know the context of each action, if it has one strong params method for the whole set of actions ),
use Proposal::Operation::Create and remove all business logic from controller,
ensure that our controller is only responsible for rendering proper response.

So how our new action would look like after refactor:

def create  
  result = Proposal::Operation::Create.call(params: params, current_user: current_user)  
  if result.success?  
   flash[:info] = setup_flash_message(result[:model].event)  
   redirect_to event_proposal_url(event_slug: result[:model].event.slug, uuid: result[:model])  
  elsif result[:errors] == "Event not found"  
    flash[:danger] = "Your event could not be found, please check the url."  
    redirect_to events_path  
  elsif result[:errors] == "Event is closed"  
    flash[:danger] = "The CFP is closed for proposal submissions."  
    redirect_to event_path(slug: result[:model].event.slug)  
  end
end

We also can exclude our action from before_action:

before_action :require_event, except: [:index, :create]

After we run all tests for #create context we get:

Run options: include {:locations=>{"./spec/controllers/proposals_controller_spec.rb"=>[35]}}
....

Finished in 1.37 seconds (files took 8.54 seconds to load)
4 examples, 0 failures

So that was it. Refactoring of #create action is finished. So let's slow down a bit, and think about what we have done, and what we achieved: we created 2 new abstract layers (Operation and Contract) that are more explicit and readable - they also have their own responsibilities. Both Operation and Contract are easier to test, they are isolated and easier to reuse.

Operation:
> It’s a simple service object that encapsulates and orchestrates all the business logic necessary to accomplish a certain task, such as creating a blog post or updating a user.

Contract:
> A contract is an abstraction to handle the validation of arbitrary data or object state. It is a fully self-contained object that is orchestrated by the operation.

We cleared controller so:

it is not an example of how to break all SOLID rules at one time,
it is easier to test - we can mock the whole Operation result instead of running integration test if we care about speed and isolation,
we don't use before_action callback anymore so it is more explicit what is happening there and when,
we get rid of strong params, so we can validate params that we receive on a contract layer in the context of a given domain process.

This was the first step in refactoring. Meantime we added few #ToDos, so now - we will focus on moving forward and clearing those parts. Let us know if you have any questions/feedback in comments or just join the Trailblazer community on gitter ( https://gitter.im/trailblazer/chat ).

If you still want to see some code, there is PR with our changes:
https://github.com/panSarin/cfp-app/pull/1/files.

Blog

Trailblazer tutorial: updating old fat controller - part 4.

Adam Piotrowski

Join Our Newsletter. No Spam, Only the good stuff.

Related