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.
Top comments (0)