Trailblazer tutorial: refactoring fat controller - part 3.

Before we move our business logic to Operation class we still need to handle:

• Setting the speaker data,
• Updating the bio of current user based on the speaker from proposal bio.

(Iteration 6) Describe handling speaker data by tests.

context "with open event" do
  let(:current_user) { FactoryBot.create(:user) }
  let!(:event) {
    FactoryBot.create(:event, state: 'open', slug: 'Lorem')
  }
  let!(:session_format) { SessionFormat.create(name: 'FooBar', event: event)}
  it 'creates a proposal assigned to event identified by slug' do
    expect(result).to be_truthy
    expect(result[:model].title).to eq('Foo')
    expect(result[:model].persisted?).to be_truthy
    expect(result[:model].event).to eq(event)
  end

  it 'assign current user as a speaker assigned to current event and filled with passed bio' do
    expect(result[:model].speakers[0].user).to eq(current_user)
    expect(result[:model].speakers[0].bio).to eq('my bio')
    expect(result[:model].speakers[0].event).to eq(event)
  end

  it 'update speaker bio from passed params' do
    expect(result[:model].speakers.first.bio).to eq("my bio")
  end
end

Since we have the whole expected behavior described by tests, we can investigate how current code works and figure out about what we want to re-use and what we want to avoid.

As we can see in controller #create,

@proposal = @event.proposals.new(proposal_params)
speaker = @proposal.speakers[0]
speaker.user_id = current_user.id
speaker.event_id = @event.id

our current code is using nested_attributes_for to handle initializing data for speakers. So part of "initializing speakers data" is handled by rails magic, and second part like assigning user and event is handled by the controller, which (as we already know) should be responsible for that.

Since our logic is more complex than just assigning nested data from the form, let's avoid using nested_attributes and try to avoid handling this kind of logic in the controller action altogether. We shall stick to the rules of SOLID and OOP and write as much explicit code as we can.

Also, we can investigate the whole codebase to check if we use

accepts_nested_attributes_for :speakers

from proposal.rb model anywhere else except #create action in our controller. Since it is used in proposals/_form.html.haml partial which is also used in edit action, we will not remove it from model yet as we don’t want to break the edit/update actions. But it would be great to comment out this code in the model to remember we need to remove it after refactoring the edit action.

#toDo remove when refactor #edit proposal 
accepts_nested_attributes_for :speakers

Let's focus on handling that data the trailblazer way. Trailblazer contracts (which are using reform under the hood) give us a really simple way to handle that. As we can see in the documentation, to handle nested resources inside the contract we can use collection, which enables us to define collection of associated resources we will initialize as we fill the contract with data.

collection :speakers, populate_if_empty: Speaker do
  property :bio
end

The above code will search for speakers key in params we received and use them to populate all speakers, passing bio attribute to each new instance of Speaker.

After we added this code, let's run all tests and check whether they fail or pass. And as we can see, the tests within "with open event", including the ones that had previously passed, all have failed.
What is the reason for that and how to debug it? The reason is quite obvious, but also quite hard to track before we understand how each step works. Now we can slow down a bit, and discuss each TRB Macro step:

• initialize our proposal without filling it with data

step Model(Proposal, :new) 

• that initialized contract will be used to validate data that we want to pass to our model through Contract rules. In this step, we already filled

step Contract::Build(constant: Proposal::Contract::Create) 

we can also call it by:

step :build_contract

def build_contract(ctx, model:, **)  
  ctx["result.contract.default"] = User::Contract::Create.new(model)  
end

• that step redirects the flow to the Present operation. After finishing the flow returns back to the outer operation (Proposal::Create in our case)

step Nested(Present)

• IMPORTANT: First it fills the model with data from params based on contract (and members/collection if there are some) and only then it checks if data in [:proposal] hash from params have proper values and format based on initialized Contract

step Contract::Validate(key: :proposal)

we can also call it by:

step :validate
def validate(ctx, params:, **)  
  ctx["result.contract.default"].validate(params)  
end

• this step call .save method on our object

step Contract::Persist(method: :save) 

we can also call it by:

def persist(ctx, model:, **)  
  ctx["result.contract.default"].save  
end

Since our Proposal class is AR class, and we didn’t refactor it yet, we still have one of the biggest issues with standard MVC approach there - we have all validations in the model that doesn’t care about the context of where we try to save the object. Because of that if we call ctx[:model].errors after this step, we will receive:

step Contract::Persist(method: :save)
fail ->(ctx, **) { binding.pry; true}

pry(Proposal::Operation::Create)> ctx[:model].errors
=> #<ActiveModel::Errors:0x007ff9c50cfe38
@base=
#<Proposal:0x007ff9c2799de8
id: nil,
event_id: 1,
state: "submitted",
uuid: nil,
title: "Foo",
session_format_id: 1,
track_id: nil,
abstract: "Abstract bar",
details: "About proposal",
pitch: "Elevator",
last_change: nil,
confirmation_notes: nil,
proposal_data: {},
updated_by_speaker_at: nil,
confirmed_at: nil,
created_at: nil,
updated_at: nil>,
@details=
{:"speakers.event"=>[{:error=>:blank}], :"speakers.name"=>[{:error=>:blank}], :"speakers.email"=>[{:error=>:blank}, {:error=>:invalid, :value=>nil}]},
@messages={:"speakers.event"=>["can't be blank"], :"speakers.name"=>["can't be blank"], :"speakers.email"=>["can't be blank", "is invalid"]}>

Since we know about those validations, before we focus on refactoring Speaker model, we should check if other places are using those validations.

If we comment on all AR validation and run specs one will fail:

rspec ./spec/features/staff/speakers_spec.rb:101 
# Organizers can manage speakers for Program Sessions 
# An organizer Can't update a speaker with a bad email .

In this case lets comment validation, add ToDo comment that will indicate that we need to handle that validation for other actions & contexts in our application and go back to our current code.

class Speaker < ApplicationRecord
...
#ToDo: move all validations to proper contracts, that will distinguish which validation is necessary when 
# validates :event, presence: true
# validates :bio, length: {maximum: 500}
# validates :name, :email, presence: true, unless: :skip_name_email_validation
# validates_format_of :email, with: Devise.email_regexp

So let's move validation to contract, and handle validation errors from the contract in operation (like a gentleman, not a barbarian which uses one set of AR validations definition disregarding the context).

module Proposal::Contract
  class Create < Reform::Form
    property :event_id
    ...
    collection :speakers, populate_if_empty: Speaker do
      property :bio  
      property :event_id  
      property :user_id  

      validates :event_id, presence: true  
      validates :user_id, presence: true  
      validates :bio, length: {maximum: 500}  
    end
  end
end

Then let's update our Operation to handle validation errors,

  step  Contract::Validate(key: :proposal)
  fail :validation_failed, fail_fast: true

and run our "with open event" tests which will give us:

Failure/Error: expect(result).to be_success

expected `#<Trailblazer::Operation::Trace::Result(<Result:false #<Trailblazer::Context:0x007fcb32946498 @wrappe..., 
:errors=>{:"speakers.event_id"=>["can't be blank"],
:"speakers.user_id"=>["can't be blank"]}}> >)>.success?`
to return true, got false

Since we don’t pass event_id or user_id we shouldn’t be surprised. Starting with doing that, user_id will be taken from current_user, so we need to pass it to contract (http://trailblazer.to/gems/operation/2.0/contract.html#manual-build).

module Proposal::Contract
  class Create < Reform::Form
    property :title
    property :abstract
    property :details
    property :pitch
    property :session_format_id
    property :event_id
    property :current_user, virtual: true
    validates :current_user, presence: true

    collection :speakers, populate_if_empty: Speaker do
      property :bio
      property :event_id
      property :user_id

      validates :event_id, presence: true
      validates :user_id, presence: true
      validates :bio, length: {maximum: 500}
    end
  end
end

That is how we prepared our contract to receive current_user which is not a field of Proposal.

We shall focus on how to pass event_id (which we have in Contract object), and how to pass user_id ( taken from current_user) to initialized speaker. Since the contract is filled by data during building it, we are supposed to have an event initialized to have it in the contract. So we have to change the operation steps order*(in the sake of visibility, for now, we skip using Nested Present class)*.

module Proposal::Operation
  class Create < Trailblazer::Operation      
    step :event
    fail :event_not_found, fail_fast: true
    step Model(Proposal, :new)
    step :assign_event
    step Contract::Build(constant: Proposal::Contract::Create,
      builder: -> (ctx, constant:, model:, **){
        constant.new(model, current_user: ctx[:current_user])
      }
    )

    step :event_open?
    fail :event_not_open_error, fail_fast: true
    step Contract::Validate(key: :proposal)
    fail :validation_failed, fail_fast: true
    step Contract::Persist(method: :save)
    #our definitions ...    
  end  
end

As we can see we moved the event step to the beginning of the operation since event object will be useful during the next steps like building contract and eventually validating it. Since we have our event, the second issue was to pass current_user. We did by using builder which is an indication of how we will initialize Proposal::Contract::Create. “constant” there is just Proposal::Contract::Create (we could replace constant with Proposal::Contract::Create, but it will be longer and less readable). Thanks to using builder, we can not only pass model but also additional arguments, like current_user.

Operation initializes the contract with all the necessary data, so let's focus on how to initialize speakers based on passed data in our contract:

collection :speakers, populator: ->(model:, index:, **args) {    
  model.insert(index,
    Speaker.find_or_initialize_by(user_id: current_user.id, event_id: self.event_id) )
  } do
      property :bio  
      property :event_id  
      property :user_id  

      validates :event_id, presence: true  
      validates :user_id, presence: true  
      validates :bio, length: {maximum: 500}  
    end

We can check our tests:

1) Proposal::Operation::Create with valid params with open event
 update speaker user bio from passed params
Failure/Error: expect(result[:model].speakers[0].user.bio).to eq("my bio")

expected: "my bio"
got: "A great Bio"

(compared using ==)
#./spec/concepts/proposal/operation/create_spec.rb:48:in `block (4 levels) in <top (required)>'


Finished in 1.37 seconds (files took 7.3 seconds to load)
6 examples, 1 failure

This is not surprising since we didn’t implement updating user bio-based on a new speaker bio. All other tests are green, so that is cool, and we can focus on making the last one to pass. Updating user bio-based on bio from a saved speaker is business logic of the whole operation, so we just add another step:

  step Contract::Persist(method: :save)  
  step :update_user_bio  

And a step definition:

def update_user_bio(ctx, **)    
  ctx[:current_user].update(bio: ctx[:model].speakers[0][:bio])  
end

And that's it. The test is passing, everyone is happy. Of course, we should also test and handle all invalid scenarios, like validations in the user model, or errors during saving our contract - and I suggest you to try to implement it on your own.