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